[profiler] prevent duplicate id

With the switch to primitive ids make sure no profiler can be returned
with the same id.

Bug: chromium:1297283
Change-Id: I9cf944e9a472ea45679feb0f30137dba95a32ca6
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3582786
Auto-Submit: Corentin Pescheloche <cpescheloche@fb.com>
Reviewed-by: Camillo Bruni <cbruni@chromium.org>
Commit-Queue: Camillo Bruni <cbruni@chromium.org>
Cr-Commit-Position: refs/heads/main@{#79975}

src/profiler/profile-generator.cc

@@ -899,9 +899,20 @@ CpuProfilesCollection::CpuProfilesCollection(Isolate* isolate)
   USE(isolate_);
 }
 
+CpuProfilingResult CpuProfilesCollection::StartProfilingForTesting(
+    ProfilerId id) {
+  return StartProfiling(id);
+}
+
 CpuProfilingResult CpuProfilesCollection::StartProfiling(
     const char* title, CpuProfilingOptions options,
     std::unique_ptr<DiscardedSamplesDelegate> delegate) {
+  return StartProfiling(++last_id_, title, options, std::move(delegate));
+}
+
+CpuProfilingResult CpuProfilesCollection::StartProfiling(
+    ProfilerId id, const char* title, CpuProfilingOptions options,
+    std::unique_ptr<DiscardedSamplesDelegate> delegate) {
   current_profiles_semaphore_.Wait();
 
   if (static_cast<int>(current_profiles_.size()) >= kMaxSimultaneousProfiles) {
@@ -912,22 +923,22 @@ CpuProfilingResult CpuProfilesCollection::StartProfiling(
     };
   }
 
-  if (title != nullptr) {
-    for (const std::unique_ptr<CpuProfile>& profile : current_profiles_) {
-      if (profile->title() != nullptr && strcmp(profile->title(), title) == 0) {
-        // Ignore attempts to start profile with the same title...
-        current_profiles_semaphore_.Signal();
-        // ... though return kAlreadyStarted to force it collect a sample.
-        return {
-            profile->id(),
-            CpuProfilingStatus::kAlreadyStarted,
-        };
-      }
+  for (const std::unique_ptr<CpuProfile>& profile : current_profiles_) {
+    if ((profile->title() != nullptr && title != nullptr &&
+         strcmp(profile->title(), title) == 0) ||
+        profile->id() == id) {
+      // Ignore attempts to start profile with the same title or id
+      current_profiles_semaphore_.Signal();
+      // ... though return kAlreadyStarted to force it collect a sample.
+      return {
+          profile->id(),
+          CpuProfilingStatus::kAlreadyStarted,
+      };
     }
   }
 
-  CpuProfile* profile = new CpuProfile(profiler_, ++last_id_, title, options,
-                                       std::move(delegate));
+  CpuProfile* profile =
+      new CpuProfile(profiler_, id, title, options, std::move(delegate));
   current_profiles_.emplace_back(profile);
   current_profiles_semaphore_.Signal();
 

src/profiler/profile-generator.h

@@ -544,6 +544,8 @@ class V8_EXPORT_PRIVATE CpuProfilesCollection {
       const char* title = nullptr, CpuProfilingOptions options = {},
       std::unique_ptr<DiscardedSamplesDelegate> delegate = nullptr);
 
+  // This Method is only visible for testing
+  CpuProfilingResult StartProfilingForTesting(ProfilerId id);
   CpuProfile* StopProfiling(ProfilerId id);
   bool IsLastProfileLeft(ProfilerId id);
   CpuProfile* Lookup(const char* title);
@@ -574,6 +576,10 @@ class V8_EXPORT_PRIVATE CpuProfilesCollection {
   static const int kMaxSimultaneousProfiles = 100;
 
  private:
+  CpuProfilingResult StartProfiling(
+      ProfilerId id, const char* title = nullptr,
+      CpuProfilingOptions options = {},
+      std::unique_ptr<DiscardedSamplesDelegate> delegate = nullptr);
   StringsStorage resource_names_;
   std::vector<std::unique_ptr<CpuProfile>> finished_profiles_;
   CpuProfiler* profiler_;

test/cctest/test-profile-generator.cc

@@ -536,6 +536,32 @@ TEST(SampleIds_StopProfilingByProfilerId) {
   CHECK_NE(profile, nullptr);
 }
 
+TEST(CpuProfilesCollectionDuplicateId) {
+  CpuProfilesCollection collection(CcTest::i_isolate());
+  CpuProfiler profiler(CcTest::i_isolate());
+  collection.set_cpu_profiler(&profiler);
+
+  auto profile_result = collection.StartProfiling();
+  CHECK_EQ(CpuProfilingStatus::kStarted, profile_result.status);
+  CHECK_EQ(CpuProfilingStatus::kAlreadyStarted,
+           collection.StartProfilingForTesting(profile_result.id).status);
+
+  collection.StopProfiling(profile_result.id);
+}
+
+TEST(CpuProfilesCollectionDuplicateTitle) {
+  CpuProfilesCollection collection(CcTest::i_isolate());
+  CpuProfiler profiler(CcTest::i_isolate());
+  collection.set_cpu_profiler(&profiler);
+
+  auto profile_result = collection.StartProfiling("duplicate");
+  CHECK_EQ(CpuProfilingStatus::kStarted, profile_result.status);
+  CHECK_EQ(CpuProfilingStatus::kAlreadyStarted,
+           collection.StartProfiling("duplicate").status);
+
+  collection.StopProfiling(profile_result.id);
+}
+
 namespace {
 class DiscardedSamplesDelegateImpl : public v8::DiscardedSamplesDelegate {
  public:
@@ -749,7 +775,6 @@ TEST(Issue51919) {
     i::DeleteArray(titles[i]);
 }
 
-
 static const v8::CpuProfileNode* PickChild(const v8::CpuProfileNode* parent,
                                            const char* name) {
   for (int i = 0; i < parent->GetChildrenCount(); ++i) {