Stabilize JavaScript Profiler in Cobalt (#2454)

b/323983545 A major refactor was needed in order to prevent the JavaScript Profiler from SEGFAULTING due to a garbage-collected nullptr. This refactor includes the use of a `ProfilerGroup` class. The life of a cobalt::js_profiler::Profiler is extended by adding scoped_refptrs to the ProfilerGroup; thus preventing GC. Profilers live as long as their ProfilerGroup, which lives as long as the Isolate and Web Agent. One `ProfilerGroup` is bound to one `web::Agent`, and contains one `v8::Isolate`, one `v8::CpuProfiler`, and many `cobalt::js_profiler::Profiler`s. The major advantage of this refactor is that it does not crash, and is more faithful to the [original Chromium Implementation](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/timing/profiler_group.cc;bpv=0;bpt=0). Test-On-Device: true --------- Co-authored-by: Ahmed Elzeiny <[email protected]> (cherry picked from commit 84c616f)
youtube · Mar 26, 2024 · f598afe · f598afe
1 parent 45d08c6
commit f598afe
Show file tree

Hide file tree

Showing 13 changed files with 413 additions and 90 deletions.
diff --git a/.gitignore b/.gitignore
@@ -9,3 +9,4 @@
 /venv/*
 _certs/
 .coverage
+compile_commands.json
diff --git a/cobalt/base/tokens.h b/cobalt/base/tokens.h
@@ -116,6 +116,7 @@ namespace base {
     MacroOpWithNameOnly(resourcetimingbufferfull)                    \
     MacroOpWithNameOnly(result)                                      \
     MacroOpWithNameOnly(resume)                                      \
+    MacroOpWithNameOnly(samplebufferfull)                            \
     MacroOpWithNameOnly(scroll)                                      \
     MacroOpWithNameOnly(securitypolicyviolation)                     \
     MacroOpWithNameOnly(seeked)                                      \

diff --git a/cobalt/js_profiler/BUILD.gn b/cobalt/js_profiler/BUILD.gn
@@ -16,6 +16,8 @@ static_library("js_profiler") {
   sources = [
     "profiler.cc",
     "profiler.h",
+    "profiler_group.cc",
+    "profiler_group.h",
     "profiler_trace_builder.cc",
     "profiler_trace_builder.h",
     "profiler_trace_wrapper.h",

diff --git a/cobalt/js_profiler/js_profiler_test.cc b/cobalt/js_profiler/js_profiler_test.cc
@@ -35,6 +35,10 @@ class ProfilerTest : public dom::testing::TestWithJavaScript {
  public:
   ProfilerTest() {}
 
+  void CollectGarbage() {
+    window_.web_context()->javascript_engine()->CollectGarbage();
+  }
+
  protected:
   dom::testing::StubWindow window_;
   StrictMock<script::testing::MockExceptionState> exception_state_;
@@ -111,5 +115,42 @@ TEST_F(ProfilerTest, ProfilerJSCode) {
   EXPECT_TRUE(EvaluateScript("Profiler", &result));
   EXPECT_EQ(result, "function Profiler() { [native code] }");
 }
+
+TEST_F(ProfilerTest, ProfilerGroupDisposesOfCpuProfiler) {
+  v8::HandleScope scope(web::get_isolate(window_.environment_settings()));
+  ProfilerInitOptions init_options;
+  init_options.set_sample_interval(10);
+  init_options.set_max_buffer_size(1);
+
+  auto profiler_group = ProfilerGroup::From(window_.environment_settings());
+  EXPECT_FALSE(profiler_group->active());
+  EXPECT_EQ(profiler_group->num_active_profilers(), 0);
+
+  scoped_refptr<Profiler> profiler_(new Profiler(
+      window_.environment_settings(), init_options, &exception_state_));
+  EXPECT_EQ(profiler_group->num_active_profilers(), 1);
+  EXPECT_TRUE(profiler_group->active());
+
+  auto promise = profiler_->Stop(window_.environment_settings());
+  EXPECT_EQ(profiler_->stopped(), true);
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_EQ(profiler_group->num_active_profilers(), 0);
+  EXPECT_FALSE(profiler_group->active());
+}
+
+TEST_F(ProfilerTest, ProfilerCanBeCancelled) {
+  v8::HandleScope scope(web::get_isolate(window_.environment_settings()));
+  ProfilerInitOptions init_options;
+  init_options.set_sample_interval(10);
+  init_options.set_max_buffer_size(1000);
+
+  scoped_refptr<Profiler> profiler_(new Profiler(
+      window_.environment_settings(), init_options, &exception_state_));
+
+  profiler_->Cancel();
+  EXPECT_EQ(profiler_->stopped(), true);
+}
+
 }  // namespace js_profiler
 }  // namespace cobalt
diff --git a/cobalt/js_profiler/profiler.cc b/cobalt/js_profiler/profiler.cc
@@ -14,45 +14,27 @@
 
 #include "cobalt/js_profiler/profiler.h"
 
-#include <iostream>
 #include <limits>
 #include <memory>
 #include <string>
 #include <utility>
 
-#include "cobalt/base/polymorphic_downcast.h"
-#include "cobalt/js_profiler/profiler_trace_builder.h"
+#include "base/logging.h"
 #include "cobalt/js_profiler/profiler_trace_wrapper.h"
 #include "cobalt/web/cache_utils.h"
 #include "cobalt/web/context.h"
 #include "cobalt/web/dom_exception.h"
-#include "cobalt/web/environment_settings.h"
 #include "cobalt/web/environment_settings_helper.h"
 
-namespace {
-v8::Local<v8::String> toV8String(v8::Isolate* isolate,
-                                 const std::string& string) {
-  if (string.empty()) return v8::String::Empty(isolate);
-  return v8::String::NewFromUtf8(isolate, string.c_str(),
-                                 v8::NewStringType::kNormal, string.length())
-      .ToLocalChecked();
-}
-}  // namespace
-
 namespace cobalt {
 namespace js_profiler {
 
-volatile uint32_t s_lastProfileId = 0;
-
-static constexpr int kBaseSampleIntervalMs = 10;
-
 Profiler::Profiler(script::EnvironmentSettings* settings,
                    ProfilerInitOptions options,
                    script::ExceptionState* exception_state)
-    : cobalt::web::EventTarget(settings),
-      stopped_(false),
-      time_origin_{base::TimeTicks::Now()} {
-  profiler_id_ = nextProfileId();
+    : stopped_(false), time_origin_{base::TimeTicks::Now()} {
+  profiler_group_ = ProfilerGroup::From(settings);
+  profiler_id_ = profiler_group_->NextProfilerId();
 
   const base::TimeDelta sample_interval =
       base::Milliseconds(options.sample_interval());
@@ -66,21 +48,19 @@ Profiler::Profiler(script::EnvironmentSettings* settings,
 
   int effective_sample_interval_ms =
       static_cast<int>(sample_interval.InMilliseconds());
-  if (effective_sample_interval_ms % kBaseSampleIntervalMs != 0 ||
+  if (effective_sample_interval_ms % Profiler::kBaseSampleIntervalMs != 0 ||
       effective_sample_interval_ms == 0) {
     effective_sample_interval_ms +=
-        (kBaseSampleIntervalMs -
-         effective_sample_interval_ms % kBaseSampleIntervalMs);
+        (Profiler::kBaseSampleIntervalMs -
+         effective_sample_interval_ms % Profiler::kBaseSampleIntervalMs);
   }
   sample_interval_ = effective_sample_interval_ms;
 
-  auto isolate = web::get_isolate(settings);
-
-  auto status = ImplProfilingStart(
-      profiler_id_,
+  SB_LOG(INFO) << "[PROFILER] START " + profiler_id_;
+  auto status = profiler_group_->ProfilerStart(
+      this, settings,
       v8::CpuProfilingOptions(v8::kLeafNodeLineNumbers,
-                              options.max_buffer_size(), sample_interval_us),
-      settings);
+                              options.max_buffer_size(), sample_interval_us));
 
   if (status == v8::CpuProfilingStatus::kAlreadyStarted) {
     web::DOMException::Raise(web::DOMException::kInvalidStateErr,
@@ -91,46 +71,31 @@ Profiler::Profiler(script::EnvironmentSettings* settings,
   }
 }
 
-Profiler::~Profiler() {
-  if (cpu_profiler_) {
-    cpu_profiler_->Dispose();
-    cpu_profiler_ = nullptr;
+void Profiler::AddEventListener(
+    script::EnvironmentSettings* environment_settings, const std::string& name,
+    const Profiler::SampleBufferFullCallbackHolder& holder) {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  if (name != base::Tokens::samplebufferfull()) {
+    return;
   }
+  auto* global_wrappable = web::get_global_wrappable(environment_settings);
+  SampleBufferFullCallbackReference* token_callback =
+      new SampleBufferFullCallbackReference(global_wrappable, holder);
+  listeners_.push_back(
+      std::unique_ptr<SampleBufferFullCallbackReference>(token_callback));
 }
 
-v8::CpuProfilingStatus Profiler::ImplProfilingStart(
-    std::string profiler_id, v8::CpuProfilingOptions options,
-    script::EnvironmentSettings* settings) {
-  auto isolate = web::get_isolate(settings);
-  cpu_profiler_ = v8::CpuProfiler::New(isolate);
-  cpu_profiler_->SetSamplingInterval(kBaseSampleIntervalMs *
-                                     base::Time::kMicrosecondsPerMillisecond);
-  return cpu_profiler_->StartProfiling(
-      toV8String(isolate, profiler_id), options,
-      std::make_unique<ProfilerMaxSamplesDelegate>(this));
-}
-
-std::string Profiler::nextProfileId() {
-  s_lastProfileId++;
-  return "cobalt::profiler[" + std::to_string(s_lastProfileId) + "]";
-}
-
-void Profiler::PerformStop(
-    script::EnvironmentSettings* environment_settings,
-    std::unique_ptr<script::ValuePromiseWrappable::Reference> promise_reference,
-    base::TimeTicks time_origin, std::string profiler_id) {
-  auto isolate = web::get_isolate(environment_settings);
-  auto profile =
-      cpu_profiler_->StopProfiling(toV8String(isolate, profiler_id_));
-  auto trace = ProfilerTraceBuilder::FromProfile(profile, time_origin_);
-  scoped_refptr<ProfilerTraceWrapper> result(new ProfilerTraceWrapper(trace));
-  cpu_profiler_->Dispose();
-  cpu_profiler_ = nullptr;
-  promise_reference->value().Resolve(result);
+void Profiler::DispatchSampleBufferFullEvent() {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  for (auto it = listeners_.begin(); it != listeners_.end(); ++it) {
+    (*it)->value().Run();
+  }
+  listeners_.clear();
 }
 
 Profiler::ProfilerTracePromise Profiler::Stop(
     script::EnvironmentSettings* environment_settings) {
+  SB_LOG(INFO) << "[PROFILER] STOPPING " + profiler_id_;
   script::HandlePromiseWrappable promise =
       web::get_script_value_factory(environment_settings)
           ->CreateInterfacePromise<scoped_refptr<ProfilerTraceWrapper>>();
@@ -145,7 +110,7 @@ Profiler::ProfilerTracePromise Profiler::Stop(
     context->message_loop()->task_runner()->PostTask(
         FROM_HERE,
         base::BindOnce(&Profiler::PerformStop, base::Unretained(this),
-                       environment_settings, std::move(promise_reference),
+                       profiler_group_, std::move(promise_reference),
                        std::move(time_origin_), std::move(profiler_id_)));
   } else {
     promise->Reject(new web::DOMException(web::DOMException::kInvalidStateErr,
@@ -154,5 +119,23 @@ Profiler::ProfilerTracePromise Profiler::Stop(
   return promise;
 }
 
+void Profiler::PerformStop(
+    ProfilerGroup* profiler_group,
+    std::unique_ptr<script::ValuePromiseWrappable::Reference> promise_reference,
+    base::TimeTicks time_origin, std::string profiler_id) {
+  SB_LOG(INFO) << "[PROFILER] STOPPED " + profiler_id_;
+  auto trace = profiler_group->ProfilerStop(this);
+  scoped_refptr<ProfilerTraceWrapper> result(new ProfilerTraceWrapper(trace));
+  promise_reference->value().Resolve(result);
+}
+
+void Profiler::Cancel() {
+  if (!stopped_) {
+    stopped_ = true;
+    profiler_group_->ProfilerStop(this);
+  }
+  profiler_group_ = nullptr;
+}
+
 }  // namespace js_profiler
 }  // namespace cobalt
diff --git a/cobalt/js_profiler/profiler.h b/cobalt/js_profiler/profiler.h
@@ -17,67 +17,78 @@
 
 #include <memory>
 #include <string>
+#include <vector>
 
+#include "base/threading/thread_checker.h"
 #include "cobalt/dom/performance_high_resolution_time.h"
+#include "cobalt/js_profiler/profiler_group.h"
 #include "cobalt/js_profiler/profiler_init_options.h"
 #include "cobalt/js_profiler/profiler_trace.h"
+#include "cobalt/script/callback_function.h"
 #include "cobalt/script/promise.h"
+#include "cobalt/script/script_value.h"
 #include "cobalt/script/value_handle.h"
 #include "cobalt/script/wrappable.h"
 #include "cobalt/web/event_target.h"
-#include "third_party/v8/include/cppgc/member.h"
 #include "third_party/v8/include/v8-profiler.h"
 
 namespace cobalt {
 namespace js_profiler {
 
-class Profiler : public cobalt::web::EventTarget {
+// Forward declaration of ProfilerGroup
+class ProfilerGroup;
+
+// TODO(b/326337485): Profiler should be a subclass of EventTarget.
+class Profiler : public script::Wrappable {
  public:
-  using ProfilerTracePromise = script::HandlePromiseWrappable;
+  static const int kBaseSampleIntervalMs = 10;
+  typedef script::HandlePromiseWrappable ProfilerTracePromise;
+  typedef script::CallbackFunction<void()> SampleBufferFullCallback;
+  typedef script::ScriptValue<SampleBufferFullCallback>
+      SampleBufferFullCallbackHolder;
+  typedef SampleBufferFullCallbackHolder::Reference
+      SampleBufferFullCallbackReference;
 
   Profiler(script::EnvironmentSettings* settings, ProfilerInitOptions options,
            script::ExceptionState* exception_state);
-  ~Profiler();
+  ~Profiler() override = default;
+
+  void AddEventListener(script::EnvironmentSettings* environment_settings,
+                        const std::string& name,
+                        const SampleBufferFullCallbackHolder& listener);
+
+  void DispatchSampleBufferFullEvent();
 
   ProfilerTracePromise Stop(script::EnvironmentSettings* environment_settings);
+  void Cancel();
 
   bool stopped() const { return stopped_; }
 
   dom::DOMHighResTimeStamp sample_interval() const { return sample_interval_; }
+  std::string ProfilerId() const { return profiler_id_; }
+  base::TimeTicks time_origin() const { return time_origin_; }
 
   DEFINE_WRAPPABLE_TYPE(Profiler);
 
-  virtual v8::CpuProfilingStatus ImplProfilingStart(
-      std::string profiler_id, v8::CpuProfilingOptions options,
-      script::EnvironmentSettings* settings);
-
  private:
-  void PerformStop(script::EnvironmentSettings* environment_settings,
+  void PerformStop(ProfilerGroup* profiler_group,
                    std::unique_ptr<script::ValuePromiseWrappable::Reference>
                        promise_reference,
                    base::TimeTicks time_origin, std::string profiler_id);
 
-  std::string nextProfileId();
-
   bool stopped_;
   dom::DOMHighResTimeStamp sample_interval_;
-  v8::CpuProfiler* cpu_profiler_ = nullptr;
   base::TimeTicks time_origin_;
   std::string profiler_id_;
-};
+  ProfilerGroup* profiler_group_;
+  // All samplebufferfull listeners. Prevents GC on callbacks owned by this
+  // object, by binding to global-wrappable.
+  std::vector<std::unique_ptr<SampleBufferFullCallbackReference>> listeners_;
 
-class ProfilerMaxSamplesDelegate : public v8::DiscardedSamplesDelegate {
- public:
-  explicit ProfilerMaxSamplesDelegate(Profiler* profiler)
-      : profiler_(profiler) {}
-  void Notify() override {
-    if (profiler_.Get()) {
-      profiler_->DispatchEvent(new web::Event("samplebufferfull"));
-    }
-  }
+  // Thread checker for the thread that creates this instance.
+  THREAD_CHECKER(thread_checker_);
 
- private:
-  cppgc::WeakMember<Profiler> profiler_;
+  DISALLOW_COPY_AND_ASSIGN(Profiler);
 };
 
 }  // namespace js_profiler

diff --git a/cobalt/js_profiler/profiler.idl b/cobalt/js_profiler/profiler.idl
@@ -20,9 +20,16 @@
   ConstructorCallWith=EnvironmentSettings,
   RaisesException = Constructor,
 ]
-interface Profiler : EventTarget {
+interface Profiler {
   readonly attribute DOMHighResTimeStamp sampleInterval;
   readonly attribute boolean stopped;
 
+  // TODO(b/326337485): This function mocks but does not fully emulate the EventTarget interface. It can
+  // take and call many callbacks as listeners. However, note that this class does not remove listeners
+  // or dispatch events. Use with caution.
+  [CallWith=EnvironmentSettings] void addEventListener(DOMString token, SampleBufferFullCallback listener);
+
   [CallWith=EnvironmentSettings] Promise<ProfilerTraceWrapper> stop();
 };
+
+callback SampleBufferFullCallback = void();