Remove context filtration argument for the JS Self-Profiling API

Since the JS Self-Profiling API will require COOP+COEP to ship, it's no longer
necessary to isolate by context. This is a step towards deprecating this
functionality in V8 entirely, as context scraping depends on reads of
potentially uninitialized memory.

Bug: 956688
Change-Id: I6fb8c2565f1979991f6ef8f86ceefe82ce5a5e54
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2387150
Commit-Queue: Andrew Comminos <acomminos@fb.com>
Reviewed-by: Nicolás Peña Moreno <npm@chromium.org>
Cr-Commit-Position: refs/heads/master@{#805974}

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

@@ -15,6 +15,7 @@
 #include "third_party/blink/renderer/platform/bindings/exception_state.h"
 #include "third_party/blink/renderer/platform/bindings/script_state.h"
 #include "third_party/blink/renderer/platform/runtime_enabled_features.h"
+#include "third_party/blink/renderer/platform/scheduler/public/thread_scheduler.h"
 #include "third_party/blink/renderer/platform/weborigin/security_origin.h"
 #include "third_party/blink/renderer/platform/wtf/functional.h"
 #include "third_party/blink/renderer/platform/wtf/vector.h"
@@ -83,11 +84,11 @@ Profiler* ProfilerGroup::CreateProfiler(ScriptState* script_state,
   DCHECK(cpu_profiler_);
 
   String profiler_id = NextProfilerId();
-  v8::CpuProfilingOptions options(
-      v8::kLeafNodeLineNumbers,
-      init_options.hasMaxBufferSize() ? init_options.maxBufferSize()
+  v8::CpuProfilingOptions options(v8::kLeafNodeLineNumbers,
+                                  init_options.hasMaxBufferSize()
+                                      ? init_options.maxBufferSize()
                                       : v8::CpuProfilingOptions::kNoSampleLimit,
-      static_cast<int>(sample_interval_us), script_state->GetContext());
+                                  static_cast<int>(sample_interval_us));
 
   cpu_profiler_->StartProfiling(V8String(isolate_, profiler_id), options);
 
@@ -199,14 +200,19 @@ void ProfilerGroup::CancelProfilerAsync(ScriptState* script_state,
   DCHECK(cpu_profiler_);
   DCHECK(!profiler->stopped());
   profilers_.erase(profiler);
-  ExecutionContext::From(script_state)
-      ->GetTaskRunner(TaskType::kInternalDefault)
-      ->PostTask(FROM_HERE,
-                 WTF::Bind(&ProfilerGroup::CancelProfilerImpl,
+
+  // Since it's possible for the profiler to get destructed along with its
+  // associated context, dispatch a task to cleanup context-independent isolate
+  // resources (rather than use the context's task runner).
+  ThreadScheduler::Current()->V8TaskRunner()->PostTask(
+      FROM_HERE, WTF::Bind(&ProfilerGroup::CancelProfilerImpl,
                            WrapPersistent(this), profiler->ProfilerId()));
 }
 
 void ProfilerGroup::CancelProfilerImpl(String profiler_id) {
+  if (!cpu_profiler_)
+    return;
+
   v8::HandleScope scope(isolate_);
   v8::Local<v8::String> v8_profiler_id = V8String(isolate_, profiler_id);
   auto* profile = cpu_profiler_->StopProfiling(v8_profiler_id);

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

@@ -9,6 +9,7 @@
 #include "third_party/blink/renderer/bindings/core/v8/v8_binding_for_testing.h"
 #include "third_party/blink/renderer/bindings/core/v8/v8_profiler_init_options.h"
 #include "third_party/blink/renderer/core/timing/profiler.h"
+#include "third_party/blink/renderer/platform/testing/unit_test_helpers.h"
 
 namespace blink {
 
@@ -207,4 +208,29 @@ TEST(ProfilerGroupTest, Bug1119865) {
   profiler->stop(scope.GetScriptState()).Then(function);
 }
 
+// Tests that a leaked profiler doesn't crash when disposed alongside its
+// context.
+TEST(ProfilerGroupTest, LeakProfilerWithContext) {
+  Profiler* profiler;
+  {
+    V8TestingScope scope;
+    ProfilerGroup* profiler_group = ProfilerGroup::From(scope.GetIsolate());
+
+    ProfilerInitOptions* init_options = ProfilerInitOptions::Create();
+    init_options->setSampleInterval(0);
+    init_options->setMaxBufferSize(0);
+    profiler = profiler_group->CreateProfiler(scope.GetScriptState(),
+                                              *init_options, base::TimeTicks(),
+                                              scope.GetExceptionState());
+
+    EXPECT_FALSE(profiler->stopped());
+  }
+
+  // Force a collection of the underlying Profiler and v8::Context, and ensure
+  // a crash doesn't occur.
+  profiler = nullptr;
+  ThreadState::Current()->CollectAllGarbageForTesting();
+  test::RunPendingTasks();
+}
+
 }  // namespace blink

third_party/blink/web_tests/http/tests/js-self-profiling/iframe-context-filtration.html

@@ -33,22 +33,16 @@
       assert_true(ProfileUtils.containsFrame(trace, { name: 'parentWrapper' }),
                   'function from own (parent) context included in trace');
 
-      assert_false(ProfileUtils.containsFrame(trace, { name: 'childCollectSample' }),
-                   'function from child context excluded in trace');
+      assert_true(ProfileUtils.containsFrame(trace, { name: 'childCollectSample' }),
+                   'function from child context included in trace');
 
       assert_true(ProfileUtils.containsFrame(trace, { name: 'parentCollectSample' }),
                   'sampling wrapper function from own (parent) context included in trace');
 
-      console.log(JSON.stringify(trace));
-      assert_true(ProfileUtils.containsSubstack(trace, [
-        { name: 'parentCollectSample' },
-        { name: 'parentWrapper' },
-      ]), 'intermediate cross-context frames are discarded');
-
-      assert_false(ProfileUtils.containsResource(trace,
+      assert_true(ProfileUtils.containsResource(trace,
         'http://127.0.0.1:8000/js-self-profiling/resources/child-frame.html',
-      ), 'child resources are not included');
-    }, 'functions from child frame are not included in profile created by parent frame');
+      ), 'child resources are included');
+    }, 'functions from child frame are included in profile created by parent frame');
 
     promise_test(async t => {
       const iframe = frames[0];
@@ -62,21 +56,21 @@
 
       const trace = await profiler.stop();
 
-      assert_false(ProfileUtils.containsFrame(trace, { name: 'parentWrapper' }),
-                   'function from parent context excluded from trace');
+      assert_true(ProfileUtils.containsFrame(trace, { name: 'parentWrapper' }),
+                  'function from parent context included in trace');
 
       assert_true(ProfileUtils.containsFrame(trace, { name: 'childCollectSample' }),
                   'function from own (child) context included in trace');
 
-      assert_false(ProfileUtils.containsResource(trace,
+      assert_true(ProfileUtils.containsResource(trace,
         window.location.href,
-      ), 'parent resource is not included');
+      ), 'parent resource is included');
 
       assert_true(ProfileUtils.containsResource(trace,
         iframe.location.href,
       ), 'child resource is included');
 
-    }, 'functions from parent context are not included in profile created by child frame');
+    }, 'functions from parent context are included in profile created by child frame');
   </script>
 </body>
 </html>