[blink] Throw exception when profiler->StartProfiling() returns error status

 - cpu_profiler_->StartProfiling() now returns a status enum, of either kStarted, kAlreadyStarted, or kErrorTooManyProfilers.
This change throws an exception and returns a nullptr when either kAlreadyStarted or kErrorTooManyProfilers is returned

R=acomminos@fb.com, npm@chromium.org, petermarshall@chromium.org, ulan@chromium.org

Change-Id: Ie9c6df3c836119064c30fffc54ee7f21f34d9eb1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2549465Reviewed-by: Nicolás Peña Moreno <npm@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Peter Marshall <petermarshall@chromium.org>
Commit-Queue: Nicolas Dubus <nicodubus@fb.com>
Cr-Commit-Position: refs/heads/master@{#833852}

third_party/blink/renderer/core/timing/profiler_group.cc

@@ -90,34 +90,50 @@ Profiler* ProfilerGroup::CreateProfiler(ScriptState* script_state,
                                       : v8::CpuProfilingOptions::kNoSampleLimit,
                                   static_cast<int>(sample_interval_us));
 
-  cpu_profiler_->StartProfiling(V8String(isolate_, profiler_id), options);
-
-  // Limit non-crossorigin script frames to the origin that started the
-  // profiler.
-  auto* execution_context = ExecutionContext::From(script_state);
-  scoped_refptr<const SecurityOrigin> source_origin(
-      execution_context->GetSecurityOrigin());
-
-  // The V8 CPU profiler ticks in multiples of the base sampling interval. This
-  // effectively means that we gather samples at the multiple of the base
-  // sampling interval that's greater than or equal to the requested interval.
-  int effective_sample_interval_ms =
-      static_cast<int>(sample_interval.InMilliseconds());
-  if (effective_sample_interval_ms % kBaseSampleIntervalMs != 0 ||
-      effective_sample_interval_ms == 0) {
-    effective_sample_interval_ms +=
-        (kBaseSampleIntervalMs -
-         effective_sample_interval_ms % kBaseSampleIntervalMs);
+  v8::CpuProfilingStatus status =
+      cpu_profiler_->StartProfiling(V8String(isolate_, profiler_id), options);
+
+  switch (status) {
+    case v8::CpuProfilingStatus::kErrorTooManyProfilers: {
+      exception_state.ThrowTypeError(
+          "Reached maximum concurrent amount of profilers");
+      return nullptr;
+    }
+    case v8::CpuProfilingStatus::kAlreadyStarted: {
+      // Since we increment the profiler id for every invocation of
+      // StartProfiling, we do not expect to hit kAlreadyStarted status
+      DCHECK(false);
+      return nullptr;
+    }
+    case v8::CpuProfilingStatus::kStarted: {
+      // Limit non-crossorigin script frames to the origin that started the
+      // profiler.
+      auto* execution_context = ExecutionContext::From(script_state);
+      scoped_refptr<const SecurityOrigin> source_origin(
+          execution_context->GetSecurityOrigin());
+
+      // The V8 CPU profiler ticks in multiples of the base sampling interval.
+      // This effectively means that we gather samples at the multiple of the
+      // base sampling interval that's greater than or equal to the requested
+      // interval.
+      int effective_sample_interval_ms =
+          static_cast<int>(sample_interval.InMilliseconds());
+      if (effective_sample_interval_ms % kBaseSampleIntervalMs != 0 ||
+          effective_sample_interval_ms == 0) {
+        effective_sample_interval_ms +=
+            (kBaseSampleIntervalMs -
+             effective_sample_interval_ms % kBaseSampleIntervalMs);
+      }
+
+      auto* profiler = MakeGarbageCollected<Profiler>(
+          this, script_state, profiler_id, effective_sample_interval_ms,
+          source_origin, time_origin);
+      profilers_.insert(profiler);
+
+      num_active_profilers_++;
+      return profiler;
+    }
   }
-
-  auto* profiler = MakeGarbageCollected<Profiler>(
-      this, script_state, profiler_id, effective_sample_interval_ms,
-      source_origin, time_origin);
-  profilers_.insert(profiler);
-
-  num_active_profilers_++;
-
-  return profiler;
 }
 
 ProfilerGroup::~ProfilerGroup() {

third_party/blink/renderer/core/timing/profiler_group.h

@@ -53,7 +53,7 @@ class CORE_EXPORT ProfilerGroup
 
   // Cancels a profiler, discarding its associated trace.
   void CancelProfiler(Profiler*);
-  // Asynchronously cancels a profiler. Invoked on Profiler descrution.
+  // Asynchronously cancels a profiler. Invoked on Profiler destruction.
   void CancelProfilerAsync(ScriptState*, Profiler*);
   // Internal implementation of cancel.
   void CancelProfilerImpl(String profiler_id);

third_party/blink/renderer/core/timing/profiler_group_test.cc

@@ -16,25 +16,10 @@ namespace blink {
 namespace {
 
 static constexpr int kLargeProfilerCount = 128;
+static constexpr int kMaxConcurrentProfilerCount = 100;
 
 }  // namespace
 
-// Tests that a leaked profiler doesn't crash the isolate on heap teardown.
-TEST(ProfilerGroupTest, LeakProfiler) {
-  V8TestingScope scope;
-
-  ProfilerGroup* profiler_group = ProfilerGroup::From(scope.GetIsolate());
-
-  ProfilerInitOptions* init_options = ProfilerInitOptions::Create();
-  init_options->setSampleInterval(0);
-  init_options->setMaxBufferSize(0);
-  Profiler* profiler = profiler_group->CreateProfiler(
-      scope.GetScriptState(), *init_options, base::TimeTicks(),
-      scope.GetExceptionState());
-
-  EXPECT_FALSE(profiler->stopped());
-}
-
 TEST(ProfilerGroupTest, StopProfiler) {
   V8TestingScope scope;
 
@@ -83,6 +68,9 @@ TEST(ProfilerGroupTest, CreateProfiler) {
 
   EXPECT_FALSE(profiler->stopped());
   EXPECT_FALSE(scope.GetExceptionState().HadException());
+
+  // clean up
+  profiler->stop(scope.GetScriptState());
 }
 
 TEST(ProfilerGroupTest, ClampedSamplingIntervalZero) {
@@ -102,6 +90,9 @@ TEST(ProfilerGroupTest, ClampedSamplingIntervalZero) {
   // interval.
   EXPECT_EQ(profiler->sampleInterval(),
             ProfilerGroup::GetBaseSampleInterval().InMilliseconds());
+
+  // clean up
+  profiler->stop(scope.GetScriptState());
 }
 
 TEST(ProfilerGroupTest, ClampedSamplingIntervalNext) {
@@ -123,6 +114,42 @@ TEST(ProfilerGroupTest, ClampedSamplingIntervalNext) {
   // interval.
   EXPECT_EQ(profiler->sampleInterval(),
             (ProfilerGroup::GetBaseSampleInterval() * 2).InMilliseconds());
+
+  // clean up
+  profiler->stop(scope.GetScriptState());
+}
+
+TEST(ProfilerGroupTest, V8ProfileLimitThrowsExceptionWhenMaxConcurrentReached) {
+  V8TestingScope scope;
+
+  HeapVector<Member<Profiler>> profilers;
+  ProfilerGroup* profiler_group = ProfilerGroup::From(scope.GetIsolate());
+  ProfilerInitOptions* init_options = ProfilerInitOptions::Create();
+
+  for (auto i = 0; i < kMaxConcurrentProfilerCount; i++) {
+    init_options->setSampleInterval(i);
+    profilers.push_back(profiler_group->CreateProfiler(
+        scope.GetScriptState(), *init_options, base::TimeTicks(),
+        scope.GetExceptionState()));
+    EXPECT_FALSE(scope.GetExceptionState().HadException());
+  }
+
+  // check kErrorTooManyProfilers
+  ProfilerGroup* extra_profiler_group = ProfilerGroup::From(scope.GetIsolate());
+  ProfilerInitOptions* extra_init_options = ProfilerInitOptions::Create();
+  extra_init_options->setSampleInterval(100);
+  for (auto i = kMaxConcurrentProfilerCount; i < kLargeProfilerCount; i++) {
+    extra_profiler_group->CreateProfiler(scope.GetScriptState(),
+                                         *extra_init_options, base::TimeTicks(),
+                                         scope.GetExceptionState());
+    EXPECT_TRUE(scope.GetExceptionState().HadException());
+    EXPECT_EQ(scope.GetExceptionState().Message(),
+              "Reached maximum concurrent amount of profilers");
+  }
+
+  for (auto profiler : profilers) {
+    profiler->stop(scope.GetScriptState());
+  }
 }
 
 TEST(ProfilerGroupTest, NegativeSamplingInterval) {
@@ -152,30 +179,6 @@ TEST(ProfilerGroupTest, OverflowSamplingInterval) {
   EXPECT_TRUE(scope.GetExceptionState().HadException());
 }
 
-// Tests behaviour when exceeding the maximum number of concurrent profiles
-// supported by the V8 profiling API (https://crbug.com/1052341).
-TEST(ProfilerGroupTest, V8ProfileLimit) {
-  V8TestingScope scope;
-
-  ProfilerGroup* profiler_group = ProfilerGroup::From(scope.GetIsolate());
-
-  ProfilerInitOptions* init_options = ProfilerInitOptions::Create();
-  init_options->setSampleInterval(0);
-
-  HeapVector<Member<Profiler>> profilers;
-  for (auto i = 0; i < kLargeProfilerCount; i++) {
-    // TODO(acomminos): The V8 public API should likely be changed to expose
-    // exceeding the profile limit during creation. This would enable
-    // instantiation of profiles to cause a promise rejection instead.
-    profilers.push_back(profiler_group->CreateProfiler(
-        scope.GetScriptState(), *init_options, base::TimeTicks(),
-        scope.GetExceptionState()));
-  }
-  for (auto profiler : profilers) {
-    profiler->stop(scope.GetScriptState());
-  }
-}
-
 TEST(ProfilerGroupTest, Bug1119865) {
   class ExpectNoCallFunction : public ScriptFunction {
    public:
@@ -208,6 +211,27 @@ TEST(ProfilerGroupTest, Bug1119865) {
   profiler->stop(scope.GetScriptState()).Then(function);
 }
 
+/*
+ *  LEAK TESTS - SHOULD RUN LAST
+ */
+
+// Tests that a leaked profiler doesn't crash the isolate on heap teardown.
+// These should run last
+TEST(ProfilerGroupTest, LeakProfiler) {
+  V8TestingScope scope;
+
+  ProfilerGroup* profiler_group = ProfilerGroup::From(scope.GetIsolate());
+
+  ProfilerInitOptions* init_options = ProfilerInitOptions::Create();
+  init_options->setSampleInterval(0);
+  init_options->setMaxBufferSize(0);
+  Profiler* profiler = profiler_group->CreateProfiler(
+      scope.GetScriptState(), *init_options, base::TimeTicks(),
+      scope.GetExceptionState());
+
+  EXPECT_FALSE(profiler->stopped());
+}
+
 // Tests that a leaked profiler doesn't crash when disposed alongside its
 // context.
 TEST(ProfilerGroupTest, LeakProfilerWithContext) {