tracing: fix edge case when dumping while trace being disabled

base/trace_event/memory_dump_manager.cc

Index: base/trace_event/memory_dump_manager.cc
diff --git a/base/trace_event/memory_dump_manager.cc b/base/trace_event/memory_dump_manager.cc
index eb82ac227919946154dc650a6faeef087a773718..8af0a8b16412fcb12462324d1391c6f14df2fff8 100644
--- a/base/trace_event/memory_dump_manager.cc
+++ b/base/trace_event/memory_dump_manager.cc
@@ -308,9 +308,12 @@ void MemoryDumpManager::CreateProcessDump(const MemoryDumpRequestArgs& args,
   scoped_ptr<ProcessMemoryDumpAsyncState> pmd_async_state;
   {
     AutoLock lock(lock_);
-    pmd_async_state.reset(
-        new ProcessMemoryDumpAsyncState(args, dump_providers_, session_state_,
-                                        callback, dump_thread_->task_runner()));
+    // |dump_thread_| can be nullptr is tracing was disabled before reaching
+    // here. ContinueAsyncProcessDump is robust enough to tolerate it and will
+    // NACK the dump.
+    pmd_async_state.reset(new ProcessMemoryDumpAsyncState(
+        args, dump_providers_, session_state_, callback,
+        dump_thread_ ? dump_thread_->task_runner() : nullptr));
   }
 
   TRACE_EVENT_WITH_FLOW0(kTraceCategory, "MemoryDumpManager::CreateProcessDump",
@@ -373,7 +376,12 @@ void MemoryDumpManager::ContinueAsyncProcessDump(
     task_runner = pmd_async_state->dump_thread_task_runner.get();
 
   bool post_task_failed = false;
-  if (!task_runner->BelongsToCurrentThread()) {
+  if (!task_runner) {
+    // If tracing was disabled before reaching CreateProcessDump() |task_runner|
+    // will be null, as the dump_thread_ would have been already torn down.
+    post_task_failed = true;
+    pmd_async_state->dump_successful = false;
+  } else if (!task_runner->BelongsToCurrentThread()) {
     // It's time to hop onto another thread.
     post_task_failed = !task_runner->PostTask(
         FROM_HERE, Bind(&MemoryDumpManager::ContinueAsyncProcessDump,
@@ -410,7 +418,7 @@ void MemoryDumpManager::ContinueAsyncProcessDump(
       }
     }
     should_dump = !mdpinfo->disabled && !post_task_failed;
-  }
+  }  // AutoLock lock(lock_);
 
   if (disabled_reason) {
     LOG(ERROR) << "Disabling MemoryDumpProvider \"" << mdpinfo->name << "\". "
@@ -480,7 +488,7 @@ void MemoryDumpManager::FinalizeDumpAndAddToTrace(
   }
 
   if (!pmd_async_state->callback.is_null()) {
-    pmd_async_state->callback.Run(dump_guid, true /* success */);

[ssid, 2016/01/22 15:31:14]

This used to always return true and currently the only case where dump can fail
is when tracing is disabled.
Tracing could have been disabled just before calling or in between
FinalizeDumpAndAddToTrace and the trace events might not even get added in
trace. I think the logic here should be:
success = memory_tracing_enabled_ && pmd_async_state->dump_successful

I am fine with code as is.

[Primiano Tucci (use gerrit), 2016/01/25 16:29:39]

You are right. Done.

+    pmd_async_state->callback.Run(dump_guid, pmd_async_state->dump_successful);
     pmd_async_state->callback.Reset();
   }
 
@@ -574,6 +582,9 @@ void MemoryDumpManager::OnTraceLogEnabled() {
 }
 
 void MemoryDumpManager::OnTraceLogDisabled() {
+  // There might be a memory dump in progress while this happens. Therefore,
+  // ensure that the MDM state which depends on the tracing enabled / disabled
+  // state is always accessed by the dumping methods holding the |lock_|.
   subtle::NoBarrier_Store(&memory_tracing_enabled_, 0);
   scoped_ptr<Thread> dump_thread;
   {
@@ -628,6 +639,7 @@ MemoryDumpManager::ProcessMemoryDumpAsyncState::ProcessMemoryDumpAsyncState(
     : req_args(req_args),
       session_state(session_state),
       callback(callback),
+      dump_successful(true),
       callback_task_runner(MessageLoop::current()->task_runner()),
       dump_thread_task_runner(dump_thread_task_runner) {
   pending_dump_providers.reserve(dump_providers.size());