Fix race condition when dump providers invoked within destroyed state

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 35b27d9a03a0a0212b0f807e83068881f3d4395b..9c6aad9f8b38be295dd7ac421732cbe4b7e4acef 100644
--- a/base/trace_event/memory_dump_manager.cc
+++ b/base/trace_event/memory_dump_manager.cc
@@ -412,26 +412,27 @@ void MemoryDumpManager::SetupNextMemoryDump(
   if (pmd_async_state->pending_dump_providers.empty())
     return FinalizeDumpAndAddToTrace(std::move(pmd_async_state));
 
+  // |dump_thread_| might be destroyed before getting this point.
+  // It means that tracing was disabled a moment ago.
+  if (!pmd_async_state->dump_thread_task_runner.get()) {

[Primiano Tucci (use gerrit), 2016/04/13 17:08:41]

Ok looking a bit more to this CL it seems that the cleanest way to fix all of
this is to do this check up in ::CreateProcess dump, and early out there.
SOrry for the back an forth, I know I suggested this, but by looking at the code
I don't see any reason why we should postpone this decision here.
Nothing should change between the dumps, as all the state we care about is
propagated by the AsyncState.

[kraynov, 2016/04/13 17:17:41]

Don't know the reason but it seems to be intentionally designed this way because
there is a test expecting that CreateProcessDump will do some work.

+    pmd_async_state->dump_successful = false;
+    while (!pmd_async_state->pending_dump_providers.empty()) {
+      pmd_async_state->pending_dump_providers.pop_back();
+    }
+    return FinalizeDumpAndAddToTrace(std::move(pmd_async_state));
+  }
+
   // Read MemoryDumpProviderInfo thread safety considerations in
   // memory_dump_manager.h when accessing |mdpinfo| fields.
   MemoryDumpProviderInfo* mdpinfo =
       pmd_async_state->pending_dump_providers.back().get();
 
   // If the dump provider did not specify a task runner affinity, dump on
-  // |dump_thread_|. Note that |dump_thread_| might have been destroyed
-  // meanwhile.
+  // |dump_thread_| which is already checked above for presence.
   SequencedTaskRunner* task_runner = mdpinfo->task_runner.get();
   if (!task_runner) {
     DCHECK(mdpinfo->options.dumps_on_single_thread_task_runner);
     task_runner = pmd_async_state->dump_thread_task_runner.get();
-    if (!task_runner) {
-      // If tracing was disabled before reaching CreateProcessDump() the
-      // dump_thread_ would have been already torn down. Nack current dump and
-      // continue.
-      pmd_async_state->dump_successful = false;
-      pmd_async_state->pending_dump_providers.pop_back();
-      return SetupNextMemoryDump(std::move(pmd_async_state));
-    }
   }
 
   if (mdpinfo->options.dumps_on_single_thread_task_runner &&
@@ -688,14 +689,15 @@ void MemoryDumpManager::OnTraceLogDisabled() {
   {
     AutoLock lock(lock_);
     dump_thread = std::move(dump_thread_);
-    session_state_ = nullptr;
   }
 
   // Thread stops are blocking and must be performed outside of the |lock_|
   // or will deadlock (e.g., if SetupNextMemoryDump() tries to acquire it).
   periodic_dump_timer_.Stop();
-  if (dump_thread)
+  if (dump_thread) {
     dump_thread->Stop();
+    session_state_ = nullptr;

[Primiano Tucci (use gerrit), 2016/04/13 17:08:41]

why are you moving this outside of the lock?
we should not rely on the precise point where session_state_ gets cleared. It
should always be accessed under the lock.

[kraynov, 2016/04/13 17:17:41]

Acknowledged.

+  }
 }
 
 uint64_t MemoryDumpManager::GetTracingProcessId() const {