Change TracingSamplerProfiler observer from async to sync

This CL is changing the kind of observer used by the sampling profiler
wrapper class. The proposed implementation is to use the sync version
which is fixing early startup tracing/sampling.

The previous code was using a Async observer to keep the class
thread-safe. Unfortunately, the sampling profiler is not able to start
early enough on startup. The hooks are at the right place.

The reason why it's not working on startup is because the tasks queue is
already loaded of startup tasks to be executed on the main thread.
Thus, after activating tracing, there is a long gab between the point
where the tigger occurs and when the sampling profiler got created and
start to collect samples (e.g. OnTraceLogEnabled).

To avoid this delay, we prefer using a Sync observer. That forces the class
to be thread-safe.

R=oysteine@chromium.org
CC=​ssid@chromium.org, wittman@chromium.org

Change-Id: I02a1deb2f8aa705ccbf5fb6e7026f4588ca1b319
Reviewed-on: https://chromium-review.googlesource.com/c/1359293Reviewed-by: Kenneth Russell <kbr@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: oysteine <oysteine@chromium.org>
Reviewed-by: Ken Rockot <rockot@google.com>
Commit-Queue: Etienne Bergeron <etienneb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614784}

chrome/app/chrome_main_delegate.cc

@@ -518,12 +518,10 @@ void ChromeMainDelegate::PostEarlyInitialization(bool is_running_tests) {
   chrome_feature_list_creator_->CreateFeatureList();
   PostFieldTrialInitialization();
 
-  // Initializes the resouce bundle and determines the locale.
+  // Initializes the resource bundle and determines the locale.
   std::string actual_locale =
       LoadLocalState(chrome_feature_list_creator_.get(), is_running_tests);
   chrome_feature_list_creator_->SetApplicationLocale(actual_locale);
-
-  tracing_sampler_profiler_->OnMessageLoopStarted();
 }
 
 bool ChromeMainDelegate::ShouldCreateFeatureList() {

components/tracing/common/tracing_sampler_profiler.cc

@@ -142,35 +142,32 @@ void TracingSamplerProfiler::CreateOnChildThread() {
     slot.get()->Set(base::WrapUnique(
         new TracingSamplerProfiler(base::PlatformThread::CurrentId())));
   }
-  TracingSamplerProfiler* profiler = slot.get()->Get().get();
-  profiler->OnMessageLoopStarted();
 }
 
 TracingSamplerProfiler::TracingSamplerProfiler(
     base::PlatformThreadId sampled_thread_id)
-    : sampled_thread_id_(sampled_thread_id), weak_ptr_factory_(this) {
+    : sampled_thread_id_(sampled_thread_id) {
+  DCHECK_NE(sampled_thread_id_, base::kInvalidThreadId);
+
   // Make sure tracing system notices profiler category.
   TRACE_EVENT_WARMUP_CATEGORY(TRACE_DISABLED_BY_DEFAULT("cpu_profiler"));
 
-  DCHECK_NE(sampled_thread_id_, base::kInvalidThreadId);
-
-  // In case tracing is currently running, start the sample profiler. The trace
-  // category can be enabled only if tracing is enabled.
+  // If tracing was enabled before initializing this class, we missed the
+  // OnTraceLogEnabled() event. Synthesize it so we can late-join the party.
+  // If the observer is added after the calling |OnTraceLogEnabled|, there is
+  // a race condition where tracing can be turned on between. By using this
+  // ordering, the |OnTraceLogEnabled| will be called twice if tracing is turned
+  // on between.
+  base::trace_event::TraceLog::GetInstance()->AddEnabledStateObserver(this);
   OnTraceLogEnabled();
 }
 
 TracingSamplerProfiler::~TracingSamplerProfiler() {
-  base::trace_event::TraceLog::GetInstance()->RemoveAsyncEnabledStateObserver(
-      this);
-}
-
-void TracingSamplerProfiler::OnMessageLoopStarted() {
-  base::trace_event::TraceLog::GetInstance()->AddAsyncEnabledStateObserver(
-      weak_ptr_factory_.GetWeakPtr());
-  OnTraceLogEnabled();
+  base::trace_event::TraceLog::GetInstance()->RemoveEnabledStateObserver(this);
 }
 
 void TracingSamplerProfiler::OnTraceLogEnabled() {
+  base::AutoLock lock(lock_);
   // Ensure there was not an instance of the profiler already running.
   if (profiler_.get())
     return;
@@ -205,6 +202,7 @@ void TracingSamplerProfiler::OnTraceLogEnabled() {
 }
 
 void TracingSamplerProfiler::OnTraceLogDisabled() {
+  base::AutoLock lock(lock_);
   if (!profiler_.get())
     return;
   // Stop and release the stack sampling profiler.

components/tracing/common/tracing_sampler_profiler.h

@@ -20,9 +20,15 @@ namespace tracing {
 
 // This class is a bridge between the base stack sampling profiler and chrome
 // tracing. It's listening to TraceLog enabled/disabled events and it's starting
-// a stack profiler on the current thread if needed.
+// a stack profiler on the current thread if needed. The sampling profiler is
+// lazily instantiated when tracing is activated and released when tracing is
+// disabled.
+//
+// The TracingSamplerProfiler must be created and destroyed on the sampled
+// thread. The tracelog observers can be called on any thread which force the
+// field |profiler_| to be thread-safe.
 class TRACING_EXPORT TracingSamplerProfiler
-    : public base::trace_event::TraceLog::AsyncEnabledStateObserver {
+    : public base::trace_event::TraceLog::EnabledStateObserver {
  public:
   // Creates sampling profiler on main thread. Since the message loop might not
   // be setup when creating this profiler, the client must call
@@ -36,11 +42,8 @@ class TRACING_EXPORT TracingSamplerProfiler
 
   ~TracingSamplerProfiler() override;
 
-  // Notify the profiler that the message loop for current thread is started.
-  // Only required for main thread.
-  void OnMessageLoopStarted();
-
   // trace_event::TraceLog::EnabledStateObserver implementation:
+  // These methods are thread-safe.
   void OnTraceLogEnabled() override;
   void OnTraceLogDisabled() override;
 
@@ -48,9 +51,9 @@ class TRACING_EXPORT TracingSamplerProfiler
   explicit TracingSamplerProfiler(base::PlatformThreadId sampled_thread_id);
 
   const base::PlatformThreadId sampled_thread_id_;
-  std::unique_ptr<base::StackSamplingProfiler> profiler_;
 
-  base::WeakPtrFactory<TracingSamplerProfiler> weak_ptr_factory_;
+  base::Lock lock_;
+  std::unique_ptr<base::StackSamplingProfiler> profiler_;  // under |lock_|
 
   DISALLOW_COPY_AND_ASSIGN(TracingSamplerProfiler);
 };

components/tracing/common/tracing_sampler_profiler_unittest.cc

@@ -144,7 +144,6 @@ class TracingSampleProfilerTest : public testing::Test {
 
 TEST_F(TracingSampleProfilerTest, OnSampleCompleted) {
   auto profiler = TracingSamplerProfiler::CreateOnMainThread();
-  profiler->OnMessageLoopStarted();
   BeginTrace();
   base::RunLoop().RunUntilIdle();
   WaitForEvents();
@@ -156,7 +155,6 @@ TEST_F(TracingSampleProfilerTest, OnSampleCompleted) {
 TEST_F(TracingSampleProfilerTest, JoinRunningTracing) {
   BeginTrace();
   auto profiler = TracingSamplerProfiler::CreateOnMainThread();
-  profiler->OnMessageLoopStarted();
   base::RunLoop().RunUntilIdle();
   WaitForEvents();
   EndTracing();

content/gpu/gpu_main.cc

@@ -340,7 +340,6 @@ int GpuMain(const MainFunctionParams& parameters) {
   // Setup tracing sampler profiler as early as possible.
   std::unique_ptr<tracing::TracingSamplerProfiler> tracing_sampler_profiler =
       tracing::TracingSamplerProfiler::CreateOnMainThread();
-  tracing_sampler_profiler->OnMessageLoopStarted();
 
 #if defined(OS_ANDROID)
   base::trace_event::MemoryDumpManager::GetInstance()->RegisterDumpProvider(

content/renderer/renderer_main.cc

@@ -214,7 +214,6 @@ int RendererMain(const MainFunctionParams& parameters) {
     // Setup tracing sampler profiler as early as possible.
     auto tracing_sampler_profiler =
         tracing::TracingSamplerProfiler::CreateOnMainThread();
-    tracing_sampler_profiler->OnMessageLoopStarted();
 
     if (need_sandbox)
       should_run_loop = platform.EnableSandbox();