Fix race condition when dump providers invoked within destroyed state

base/trace_event/memory_dump_manager.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/trace_event/memory_dump_manager.h"
 
 #include <algorithm>
 #include <utility>
 
 #include "base/atomic_sequence_num.h"
@@ skipping 394 matching lines @@
   // Initalizes the ThreadLocalEventBuffer to guarantee that the TRACE_EVENTs
   // in the PostTask below don't end up registering their own dump providers
   // (for discounting trace memory overhead) while holding the |lock_|.
   TraceLog::GetInstance()->InitializeThreadLocalEventBufferIfSupported();
 
   // If this was the last hop, create a trace event, add it to the trace and
   // finalize process dump (invoke callback).
   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 &&
       task_runner->RunsTasksOnCurrentThread()) {
     // If |dumps_on_single_thread_task_runner| is true then no PostTask is
     // required if we are on the right thread.
     return InvokeOnMemoryDump(pmd_async_state.release());
   }
 
   bool did_post_task = task_runner->PostTask(
@@ skipping 236 matching lines @@
 
 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);
   std::unique_ptr<Thread> dump_thread;
   {
     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 {
   return delegate_->GetTracingProcessId();
 }
 
 MemoryDumpManager::MemoryDumpProviderInfo::MemoryDumpProviderInfo(
     MemoryDumpProvider* dump_provider,
     const char* name,
     scoped_refptr<SequencedTaskRunner> task_runner,
@@ skipping 44 matching lines @@
   if (iter == process_dumps.end()) {
     std::unique_ptr<ProcessMemoryDump> new_pmd(
         new ProcessMemoryDump(session_state));
     iter = process_dumps.insert(std::make_pair(pid, std::move(new_pmd))).first;
   }
   return iter->second.get();
 }
 
 }  // namespace trace_event
 }  // namespace base