tracing: fix edge case when dumping while trace being disabled

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 290 matching lines @@
 }
 
 void MemoryDumpManager::CreateProcessDump(const MemoryDumpRequestArgs& args,
                                           const MemoryDumpCallback& callback) {
   TRACE_EVENT_NESTABLE_ASYNC_BEGIN0(kTraceCategory, "ProcessMemoryDump",
                                     TRACE_ID_MANGLE(args.dump_guid));
 
   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",
                          TRACE_ID_MANGLE(args.dump_guid),
                          TRACE_EVENT_FLAG_FLOW_OUT);
 
   // Start the thread hop. |dump_providers_| are kept sorted by thread, so
   // ContinueAsyncProcessDump will hop at most once per thread (w.r.t. thread
   // affinity specified by the MemoryDumpProvider(s) in RegisterDumpProvider()).
   ContinueAsyncProcessDump(pmd_async_state.release());
@@ skipping 42 matching lines @@
 
   // If the dump provider did not specify a thread affinity, dump on
   // |dump_thread_|. Note that |dump_thread_| might have been Stop()-ed at this
   // point (if tracing was disabled in the meanwhile). In such case the
   // PostTask() below will fail, but |task_runner| should always be non-null.
   SingleThreadTaskRunner* task_runner = mdpinfo->task_runner.get();
   if (!task_runner)
     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,
                         Unretained(this), Unretained(pmd_async_state.get())));
     if (!post_task_failed) {
       // Ownership is tranferred to the next ContinueAsyncProcessDump().
       ignore_result(pmd_async_state.release());
       return;
     }
   }
@@ skipping 16 matching lines @@
             " Try --no-sandbox.";
       } else if (post_task_failed && mdpinfo->task_runner) {
         // Don't disable unbound dump providers. The utility thread is normally
         // shutdown when disabling the trace and getting here in this case is
         // expected.
         mdpinfo->disabled = true;
         disabled_reason = "The thread it was meant to dump onto is gone.";
       }
     }
     should_dump = !mdpinfo->disabled && !post_task_failed;
-  }
+  }  // AutoLock lock(lock_);
 
   if (disabled_reason) {
     LOG(ERROR) << "Disabling MemoryDumpProvider \"" << mdpinfo->name << "\". "
                << disabled_reason;
   }
 
   if (should_dump) {
     // Invoke the dump provider.
     TRACE_EVENT_WITH_FLOW1(kTraceCategory,
                            "MemoryDumpManager::ContinueAsyncProcessDump",
@@ skipping 49 matching lines @@
 
     TRACE_EVENT_API_ADD_TRACE_EVENT_WITH_PROCESS_ID(
         TRACE_EVENT_PHASE_MEMORY_DUMP,
         TraceLog::GetCategoryGroupEnabled(kTraceCategory), event_name,
         dump_guid, pid, kTraceEventNumArgs, kTraceEventArgNames,
         kTraceEventArgTypes, nullptr /* arg_values */, &event_value,
         TRACE_EVENT_FLAG_HAS_ID);
   }
 
   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();
   }
 
   TRACE_EVENT_NESTABLE_ASYNC_END0(kTraceCategory, "ProcessMemoryDump",
                                   TRACE_ID_MANGLE(dump_guid));
 }
 
 void MemoryDumpManager::OnTraceLogEnabled() {
   bool enabled;
   TRACE_EVENT_CATEGORY_GROUP_ENABLED(kTraceCategory, &enabled);
@@ skipping 73 matching lines @@
   }
   DCHECK_EQ(0u, heavy_dump_period_ms % min_timer_period_ms);
   g_heavy_dumps_rate = heavy_dump_period_ms / min_timer_period_ms;
 
   periodic_dump_timer_.Start(FROM_HERE,
                              TimeDelta::FromMilliseconds(min_timer_period_ms),
                              base::Bind(&RequestPeriodicGlobalDump));
 }
 
 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;
   {
     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 ContinueAsyncProcessDump() tries to acquire it).
@@ skipping 34 matching lines @@
 
 MemoryDumpManager::ProcessMemoryDumpAsyncState::ProcessMemoryDumpAsyncState(
     MemoryDumpRequestArgs req_args,
     const MemoryDumpProviderInfo::OrderedSet& dump_providers,
     const scoped_refptr<MemoryDumpSessionState>& session_state,
     MemoryDumpCallback callback,
     const scoped_refptr<SingleThreadTaskRunner>& dump_thread_task_runner)
     : 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());
   pending_dump_providers.assign(dump_providers.rbegin(), dump_providers.rend());
 }
 
 MemoryDumpManager::ProcessMemoryDumpAsyncState::~ProcessMemoryDumpAsyncState() {
 }
 
 ProcessMemoryDump* MemoryDumpManager::ProcessMemoryDumpAsyncState::
     GetOrCreateMemoryDumpContainerForProcess(ProcessId pid) {
   auto iter = process_dumps.find(pid);
   if (iter == process_dumps.end()) {
     scoped_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