Clean up components/heap_profiling.

This CL has no intended behavior change.

This CL cleans up test structure and updates some comments.

Change-Id: I3b5ed07813ec416dd67098d9f93b702a115cee09
Bug: 827545
Reviewed-on: https://chromium-review.googlesource.com/1014371
Commit-Queue: Erik Chen <erikchen@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551418}

chrome/browser/profiling_host/profiling_process_host.cc

@@ -136,7 +136,7 @@ void ProfilingProcessHost::SaveTraceWithHeapDumpToFile(
       },
       std::move(dest), std::move(done));
   Supervisor::GetInstance()->RequestTraceWithHeapDump(
-      std::move(finish_trace_callback), false /* anonymize */);
+      std::move(finish_trace_callback), /*anonymize=*/false);
 }
 
 void ProfilingProcessHost::RequestProcessReport(std::string trigger_name) {

chrome/browser/ui/webui/memory_internals_ui.cc

@@ -251,6 +251,7 @@ void MemoryInternalsDOMHandler::HandleReportProcess(
 
 void MemoryInternalsDOMHandler::HandleStartProfiling(
     const base::ListValue* args) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   if (!args->is_list() || args->GetList().size() != 1)
     return;
 
@@ -294,6 +295,8 @@ void MemoryInternalsDOMHandler::GetProfiledPids(
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   heap_profiling::Supervisor* supervisor =
       heap_profiling::Supervisor::GetInstance();
+
+  // The supervisor hasn't started, so return an empty list.
   if (!supervisor->HasStarted()) {
     content::BrowserThread::PostTask(
         content::BrowserThread::UI, FROM_HERE,
@@ -302,6 +305,7 @@ void MemoryInternalsDOMHandler::GetProfiledPids(
                        std::vector<base::ProcessId>()));
     return;
   }
+
   supervisor->GetProfiledPids(
       base::BindOnce(&MemoryInternalsDOMHandler::ReturnProcessListOnUIThread,
                      weak_factory_.GetWeakPtr(), std::move(children)));

components/heap_profiling/test_driver.cc

@@ -560,7 +560,7 @@ bool TestDriver::RunTest(const Options& options) {
   }
 
   if (running_on_ui_thread_) {
-    if (!CheckOrStartProfiling())
+    if (!CheckOrStartProfilingOnUIThreadWithNestedRunLoops())
       return false;
     Supervisor::GetInstance()->SetKeepSmallAllocations(true);
     if (ShouldProfileRenderer())
@@ -629,7 +629,8 @@ void TestDriver::GetHasStartedOnUIThread() {
 
 void TestDriver::CheckOrStartProfilingOnUIThreadAndSignal() {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
-  initialization_success_ = CheckOrStartProfiling();
+  initialization_success_ =
+      CheckOrStartProfilingOnUIThreadWithAsyncSignalling();
 
   // If the flag is true, then the WaitableEvent will be signaled after
   // profiling has started.
@@ -643,25 +644,19 @@ void TestDriver::SetKeepSmallAllocationsOnUIThreadAndSignal() {
   wait_for_ui_thread_.Signal();
 }
 
-bool TestDriver::CheckOrStartProfiling() {
+bool TestDriver::CheckOrStartProfilingOnUIThreadWithAsyncSignalling() {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
 
   if (options_.profiling_already_started) {
-    if (Supervisor::GetInstance()->HasStarted()) {
+    if (!Supervisor::GetInstance()->HasStarted()) {
+      LOG(ERROR) << "Profiling should have been started, but wasn't";
+      return false;
+    }
+
     // Even if profiling has started, it's possible that the allocator shim
     // has not yet been initialized. Wait for it.
     if (ShouldProfileBrowser()) {
-        bool already_initialized = false;
-        std::unique_ptr<base::RunLoop> run_loop;
-        if (running_on_ui_thread_) {
-          run_loop.reset(new base::RunLoop);
-          already_initialized = SetOnInitAllocatorShimCallbackForTesting(
-              run_loop->QuitClosure(), base::ThreadTaskRunnerHandle::Get());
-
-          if (!already_initialized)
-            run_loop->Run();
-        } else {
-          already_initialized = SetOnInitAllocatorShimCallbackForTesting(
+      bool already_initialized = SetOnInitAllocatorShimCallbackForTesting(
           base::Bind(&base::WaitableEvent::Signal,
                      base::Unretained(&wait_for_ui_thread_)),
           base::ThreadTaskRunnerHandle::Get());
@@ -669,13 +664,63 @@ bool TestDriver::CheckOrStartProfiling() {
         wait_for_profiling_to_start_ = true;
       }
     }
-      }
     return true;
   }
+
+  content::ServiceManagerConnection* connection =
+      content::ServiceManagerConnection::GetForProcess();
+  if (!connection) {
+    LOG(ERROR) << "A ServiceManagerConnection was not available for the "
+                  "current process.";
+    return false;
+  }
+
+  wait_for_profiling_to_start_ = true;
+  base::OnceClosure start_callback;
+
+  // If we're going to profile the browser, then wait for the allocator shim to
+  // start. Otherwise, wait for the Supervisor to start.
+  if (ShouldProfileBrowser()) {
+    SetOnInitAllocatorShimCallbackForTesting(
+        base::Bind(&base::WaitableEvent::Signal,
+                   base::Unretained(&wait_for_ui_thread_)),
+        base::ThreadTaskRunnerHandle::Get());
+  } else {
+    start_callback = base::BindOnce(&base::WaitableEvent::Signal,
+                                    base::Unretained(&wait_for_ui_thread_));
+  }
+
+  uint32_t sampling_rate = options_.should_sample
+                               ? (options_.sample_everything ? 2 : kSampleRate)
+                               : 1;
+  Supervisor::GetInstance()->Start(connection, options_.mode,
+                                   options_.stack_mode, sampling_rate,
+                                   std::move(start_callback));
+
+  return true;
+}
+
+bool TestDriver::CheckOrStartProfilingOnUIThreadWithNestedRunLoops() {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  if (options_.profiling_already_started) {
+    if (!Supervisor::GetInstance()->HasStarted()) {
       LOG(ERROR) << "Profiling should have been started, but wasn't";
       return false;
     }
 
+    // Even if profiling has started, it's possible that the allocator shim
+    // has not yet been initialized. Wait for it.
+    if (ShouldProfileBrowser()) {
+      std::unique_ptr<base::RunLoop> run_loop(new base::RunLoop);
+      bool already_initialized = SetOnInitAllocatorShimCallbackForTesting(
+          run_loop->QuitClosure(), base::ThreadTaskRunnerHandle::Get());
+      if (!already_initialized)
+        run_loop->Run();
+    }
+    return true;
+  }
+
   content::ServiceManagerConnection* connection =
       content::ServiceManagerConnection::GetForProcess();
   if (!connection) {
@@ -686,32 +731,16 @@ bool TestDriver::CheckOrStartProfiling() {
 
   // When this is not-null, initialization should wait for the QuitClosure to be
   // called.
-  std::unique_ptr<base::RunLoop> run_loop;
+  std::unique_ptr<base::RunLoop> run_loop(new base::RunLoop);
   base::OnceClosure start_callback;
 
   // If we're going to profile the browser, then wait for the allocator shim to
   // start. Otherwise, wait for the Supervisor to start.
   if (ShouldProfileBrowser()) {
-    if (running_on_ui_thread_) {
-      run_loop.reset(new base::RunLoop);
     SetOnInitAllocatorShimCallbackForTesting(
         run_loop->QuitClosure(), base::ThreadTaskRunnerHandle::Get());
   } else {
-      wait_for_profiling_to_start_ = true;
-      SetOnInitAllocatorShimCallbackForTesting(
-          base::Bind(&base::WaitableEvent::Signal,
-                     base::Unretained(&wait_for_ui_thread_)),
-          base::ThreadTaskRunnerHandle::Get());
-    }
-  } else {
-    if (running_on_ui_thread_) {
-      run_loop.reset(new base::RunLoop);
     start_callback = run_loop->QuitClosure();
-    } else {
-      wait_for_profiling_to_start_ = true;
-      start_callback = base::BindOnce(&base::WaitableEvent::Signal,
-                                      base::Unretained(&wait_for_ui_thread_));
-    }
   }
 
   uint32_t sampling_rate = options_.should_sample
@@ -721,7 +750,6 @@ bool TestDriver::CheckOrStartProfiling() {
                                    options_.stack_mode, sampling_rate,
                                    std::move(start_callback));
 
-  if (run_loop)
   run_loop->Run();
 
   return true;

components/heap_profiling/test_driver.h

@@ -89,7 +89,15 @@ class TestDriver {
 
   // If profiling is expected to already be started, confirm it.
   // Otherwise, start profiling with the given mode.
-  bool CheckOrStartProfiling();
+  // This method must only be called on platforms that supported nested run
+  // loops on the UI thread.
+  bool CheckOrStartProfilingOnUIThreadWithNestedRunLoops();
+
+  // If profiling is expected to already be started, confirm it.
+  // Otherwise, start profiling with the given mode.
+  // This method must only be called on platforms that are running the
+  // TestDriver from a non-UI thread, which allows for async signalling.
+  bool CheckOrStartProfilingOnUIThreadWithAsyncSignalling();
 
   // Performs allocations. These are expected to be profiled.
   void MakeTestAllocations();