OOP HP: Unify code for testing and heap dump upload.

Previously, there was two sets of mostly identical logic to start a trace,
ensure that heap dumps are collected, and then report the results.

Bug: 753218
Change-Id: I8dba3c94256a8da9e9364b49bd86275db14a9df3
Reviewed-on: https://chromium-review.googlesource.com/777554
Commit-Queue: Erik Chen <erikchen@chromium.org>
Reviewed-by: Brett Wilson <brettw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519927}

chrome/browser/profiling_host/profiling_process_host.cc

@@ -319,6 +319,14 @@ void ProfilingProcessHost::OnDumpProcessesForTracingCallback(
         kTraceEventArgTypes, nullptr /* arg_values */, &wrapper,
         TRACE_EVENT_FLAG_HAS_ID);
   }
+
+  content::BrowserThread::GetTaskRunnerForThread(content::BrowserThread::UI)
+      ->PostTask(FROM_HERE,
+                 base::Bind(&ProfilingProcessHost::DumpProcessFinishedUIThread,
+                            base::Unretained(this)));
+}
+
+void ProfilingProcessHost::DumpProcessFinishedUIThread() {
   if (dump_process_for_tracing_callback_) {
     std::move(dump_process_for_tracing_callback_).Run();
     dump_process_for_tracing_callback_.Reset();
@@ -457,25 +465,52 @@ void ProfilingProcessHost::RequestProcessReport(std::string trigger_name) {
     return;
   }
 
+  auto finish_report_callback = base::BindOnce(
+      [](std::string trigger_name, bool success, std::string trace) {
+        if (success) {
+          UploadTraceToCrashServer(std::move(trace), std::move(trigger_name));
+        }
+      },
+      std::move(trigger_name));
+  RequestTraceWithHeapDump(std::move(finish_report_callback), false);
+}
+
+void ProfilingProcessHost::RequestTraceWithHeapDump(
+    TraceFinishedCallback callback,
+    bool stop_immediately_after_heap_dump_for_tests) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  if (!connector_) {
+    DLOG(ERROR)
+        << "Requesting heap dump when profiling process hasn't started.";
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::BindOnce(std::move(callback), false, std::string()));
+    return;
+  }
+
   bool result = content::TracingController::GetInstance()->StartTracing(
       GetBackgroundTracingConfig(), base::Closure());
-  if (!result)
+  if (!result) {
+    DLOG(ERROR) << "Requesting heap dump when tracing has already started.";
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::BindOnce(std::move(callback), false, std::string()));
     return;
+  }
 
-  // Once the trace has stopped, upload the log to the crash server.
+  // Once the trace has stopped, run |callback| on the UI thread. At this point,
+  // ownership of |callback| has been transfered to |finish_sink_callback|.
   auto finish_sink_callback = base::Bind(
-      [](std::string trigger_name,
+      [](TraceFinishedCallback callback,
          std::unique_ptr<const base::DictionaryValue> metadata,
          base::RefCountedString* in) {
         std::string result;
         result.swap(in->data());
         content::BrowserThread::GetTaskRunnerForThread(
             content::BrowserThread::UI)
-            ->PostTask(FROM_HERE, base::BindOnce(&UploadTraceToCrashServer,
-                                                 std::move(result),
-                                                 std::move(trigger_name)));
+            ->PostTask(FROM_HERE, base::BindOnce(std::move(callback), true,
+                                                 std::move(result)));
       },
-      std::move(trigger_name));
+      base::Passed(std::move(callback)));
 
   scoped_refptr<content::TracingController::TraceDataEndpoint> sink =
       content::TracingController::CreateStringEndpoint(
@@ -486,16 +521,18 @@ void ProfilingProcessHost::RequestProcessReport(std::string trigger_name) {
           &content::TracingController::StopTracing),
       base::Unretained(content::TracingController::GetInstance()), sink);
 
-  // Wait 10 seconds, then end the trace.
-  base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-      FROM_HERE, std::move(stop_tracing_closure),
-      base::TimeDelta::FromSeconds(10));
-}
-
-void ProfilingProcessHost::SetDumpProcessForTracingCallback(
-    base::OnceClosure callback) {
-  DCHECK(!dump_process_for_tracing_callback_);
-  dump_process_for_tracing_callback_ = std::move(callback);
+  if (stop_immediately_after_heap_dump_for_tests) {
+    // There is no race condition between setting
+    // |dump_process_for_tracing_callback_| and starting tracing, since running
+    // the callback is an asynchronous task executed on the UI thread.
+    DCHECK(!dump_process_for_tracing_callback_);
+    dump_process_for_tracing_callback_ = std::move(stop_tracing_closure);
+  } else {
+    // Wait 10 seconds, then end the trace.
+    base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+        FROM_HERE, std::move(stop_tracing_closure),
+        base::TimeDelta::FromSeconds(10));
+  }
 }
 
 void ProfilingProcessHost::MakeConnector(

chrome/browser/profiling_host/profiling_process_host.h

@@ -115,10 +115,22 @@ class ProfilingProcessHost : public content::BrowserChildProcessObserver,
   // memory data to the crash server (slow-report).
   void RequestProcessReport(std::string trigger_name);
 
-  // For testing. Only one can be set at a time. Will be called after the
-  // profiling process dumps heaps into the trace log. No guarantees are made
-  // about the task queue on which the callback will be Run.
-  void SetDumpProcessForTracingCallback(base::OnceClosure callback);
+  using TraceFinishedCallback =
+      base::OnceCallback<void(bool success, std::string trace_json)>;
+
+  // This method must be called from the UI thread. |callback| will be called
+  // asynchronously on the UI thread. If
+  // |stop_immediately_after_heap_dump_for_tests| is true, then |callback| will
+  // be called as soon as the heap dump is added to the trace. Otherwise,
+  // |callback| will be called after 10s. This gives time for the
+  // MemoryDumpProviders to dump to the trace, which is asynchronous and has no
+  // finish notification. This intentionally avoids waiting for the heap-dump
+  // finished signal, in case there's a problem with the profiling process and
+  // the heap-dump is never added to the trace.
+  // Public for testing.
+  void RequestTraceWithHeapDump(
+      TraceFinishedCallback callback,
+      bool stop_immediately_after_heap_dump_for_tests);
 
  private:
   friend struct base::DefaultSingletonTraits<ProfilingProcessHost>;
@@ -161,6 +173,9 @@ class ProfilingProcessHost : public content::BrowserChildProcessObserver,
   // Starts the profiling process.
   void LaunchAsService();
 
+  // Called on the UI thread after the heap dump has been added to the trace.
+  void DumpProcessFinishedUIThread();
+
   // Sends the end of the data pipe to the profiling service.
   void AddClientToProfilingService(profiling::mojom::ProfilingClientPtr client,
                                    base::ProcessId pid,
@@ -235,7 +250,8 @@ class ProfilingProcessHost : public content::BrowserChildProcessObserver,
   // a renderer process if one is already not going.
   bool always_sample_for_tests_;
 
-  // For tests.
+  // Only used for testing. Must only ever be used from the UI thread. Will be
+  // called after the profiling process dumps heaps into the trace log.
   base::OnceClosure dump_process_for_tracing_callback_;
 
   DISALLOW_COPY_AND_ASSIGN(ProfilingProcessHost);

chrome/browser/profiling_host/profiling_test_driver.cc

@@ -253,7 +253,7 @@ bool ProfilingTestDriver::RunTest(const Options& options) {
   }
 
   std::unique_ptr<base::Value> dump_json =
-      base::JSONReader::Read(serialized_trace_->data());
+      base::JSONReader::Read(serialized_trace_);
   if (!dump_json) {
     LOG(ERROR) << "Failed to deserialize trace.";
     return false;
@@ -352,44 +352,22 @@ void ProfilingTestDriver::CollectResults(bool synchronous) {
                                         base::Unretained(&wait_for_ui_thread_));
   }
 
-  // Once the ProfilingProcessHost has dumped to the trace, stop the trace and
-  // collate the results into |result|, then quit the nested run loop.
-  auto finish_sink_callback = base::Bind(
-      [](scoped_refptr<base::RefCountedString>* result, base::Closure finished,
-         std::unique_ptr<const base::DictionaryValue> metadata,
-         base::RefCountedString* in) {
-        *result = in;
-        std::move(finished).Run();
-      },
-      &serialized_trace_, std::move(finish_tracing_closure));
-
-  scoped_refptr<content::TracingController::TraceDataEndpoint> sink =
-      content::TracingController::CreateStringEndpoint(
-          std::move(finish_sink_callback));
-  base::OnceClosure stop_tracing_closure = base::BindOnce(
-      base::IgnoreResult<bool (content::TracingController::*)(  // NOLINT
-          const scoped_refptr<content::TracingController::TraceDataEndpoint>&)>(
-          &content::TracingController::StopTracing),
-      base::Unretained(content::TracingController::GetInstance()), sink);
-  base::OnceClosure stop_tracing_ui_thread_closure =
-      base::BindOnce(base::IgnoreResult(&base::TaskRunner::PostTask),
-                     base::ThreadTaskRunnerHandle::Get(), FROM_HERE,
-                     std::move(stop_tracing_closure));
-  profiling::ProfilingProcessHost::GetInstance()
-      ->SetDumpProcessForTracingCallback(
-          std::move(stop_tracing_ui_thread_closure));
-
-  // Spin a nested RunLoop until the heap dump has been added to the trace.
-  content::TracingController::GetInstance()->StartTracing(
-      base::trace_event::TraceConfig(
-          base::trace_event::TraceConfigMemoryTestUtil::
-              GetTraceConfig_PeriodicTriggers(100000, 100000)),
-      base::Closure());
+  profiling::ProfilingProcessHost::GetInstance()->RequestTraceWithHeapDump(
+      base::Bind(&ProfilingTestDriver::TraceFinished, base::Unretained(this),
+                 std::move(finish_tracing_closure)),
+      true);
 
   if (synchronous)
     run_loop->Run();
 }
 
+void ProfilingTestDriver::TraceFinished(base::Closure closure,
+                                        bool success,
+                                        std::string trace_json) {
+  serialized_trace_.swap(trace_json);
+  std::move(closure).Run();
+}
+
 bool ProfilingTestDriver::ValidateBrowserAllocations(base::Value* dump_json) {
   base::Value* heaps_v2 =
       FindHeapsV2(base::Process::Current().Pid(), dump_json);

chrome/browser/profiling_host/profiling_test_driver.h

@@ -84,6 +84,10 @@ class ProfilingTestDriver {
   // signal |wait_for_ui_thread_|.
   void CollectResults(bool synchronous);
 
+  void TraceFinished(base::Closure closure,
+                     bool success,
+                     std::string trace_json);
+
   bool ValidateBrowserAllocations(base::Value* dump_json);
   bool ValidateRendererAllocations(base::Value* dump_json);
 
@@ -100,7 +104,7 @@ class ProfilingTestDriver {
   base::PartitionAllocatorGeneric partition_allocator_;
 
   // Contains nothing until |CollectResults| has been called.
-  scoped_refptr<base::RefCountedString> serialized_trace_;
+  std::string serialized_trace_;
 
   // Whether the test was invoked on the ui thread.
   bool running_on_ui_thread_ = true;