[tracing] Fix races in TraceLog's EnabledStateObserver

components/tracing/child_memory_dump_manager_delegate_impl.cc

Index: components/tracing/child_memory_dump_manager_delegate_impl.cc
diff --git a/components/tracing/child_memory_dump_manager_delegate_impl.cc b/components/tracing/child_memory_dump_manager_delegate_impl.cc
index 2b537c98bef4adf95bd51800b88e48235932eee9..fe5c952f6a9a91ecf83ce42e81e49b543eb42974 100644
--- a/components/tracing/child_memory_dump_manager_delegate_impl.cc
+++ b/components/tracing/child_memory_dump_manager_delegate_impl.cc
@@ -9,6 +9,14 @@
 
 namespace tracing {
 
+namespace {
+void AbortDumpRequest(const base::trace_event::MemoryDumpRequestArgs& args,
+                      const base::trace_event::MemoryDumpCallback& callback) {
+  if (!callback.is_null())
+    callback.Run(args.dump_guid, false /* success */);
+}
+}  // namespace
+
 // static
 ChildMemoryDumpManagerDelegateImpl*
 ChildMemoryDumpManagerDelegateImpl::GetInstance() {
@@ -21,50 +29,62 @@ ChildMemoryDumpManagerDelegateImpl::ChildMemoryDumpManagerDelegateImpl()
     : ctmf_(nullptr),
       tracing_process_id_(
           base::trace_event::MemoryDumpManager::kInvalidTracingProcessId) {
-  base::trace_event::MemoryDumpManager::GetInstance()->Initialize(
-      this /* delegate */, false /* is_coordinator */);
 }
 
 ChildMemoryDumpManagerDelegateImpl::~ChildMemoryDumpManagerDelegateImpl() {}
 
 void ChildMemoryDumpManagerDelegateImpl::SetChildTraceMessageFilter(
     ChildTraceMessageFilter* ctmf) {
+  const auto& task_runner = ctmf ? (ctmf->ipc_task_runner()) : nullptr;
   // Check that we are either registering the CTMF or tearing it down, but not
   // replacing a valid instance with another one (should never happen).
   DCHECK(ctmf_ == nullptr || (ctmf == nullptr && ctmf_task_runner_ != nullptr));
   ctmf_ = ctmf;
-  ctmf_task_runner_ = ctmf ? (ctmf->ipc_task_runner()) : nullptr;
+
+  {
+    base::AutoLock lock(lock_);
+    ctmf_task_runner_ = task_runner;
+  }
+
+  if (ctmf) {
+    base::trace_event::MemoryDumpManager::GetInstance()->Initialize(

[dsinclair, 2015/09/16 17:14:55]

Why does the initialize call move from the constructor to the destructor?

[Primiano Tucci (use gerrit), 2015/09/16 17:46:20]

Uh? this is not the dtor. This is the SetChildTraceMessageFilter.
Rationale is: there is no point initializing the manager before we have set the
filter. If we do that, we know upfront that we'll NACK requests until we get
filter set, so this is to avoid any further issues.
In other words: it should be safe leaving it where it is, but there is no real
point anticipating the initialization there.
This is the point where the MDM + CMDMDI become useful.

[dsinclair, 2015/09/16 17:53:03]

Doh, skipped line 36 when I scanned up and just saw 34. ECANTREAD.

+      this /* delegate */, false /* is_coordinator */);
+  }
 }
 
 // Invoked in child processes by the MemoryDumpManager.
 void ChildMemoryDumpManagerDelegateImpl::RequestGlobalMemoryDump(
     const base::trace_event::MemoryDumpRequestArgs& args,
     const base::trace_event::MemoryDumpCallback& callback) {
+  // RequestGlobalMemoryDump can be called on any thread, cannot access
+  // ctmf_task_runner_ as it could be racy.
+  scoped_refptr<base::SingleThreadTaskRunner> ctmf_task_runner;
+  {
+    base::AutoLock lock(lock_);
+    ctmf_task_runner = ctmf_task_runner_;
+  }
+
   // Bail out if we receive a dump request from the manager before the
   // ChildTraceMessageFilter has been initialized.
-  if (!ctmf_task_runner_) {
-    if (!callback.is_null())
-      callback.Run(args.dump_guid, false /* success */);
-    return;
-  }
+  if (!ctmf_task_runner)
+    return AbortDumpRequest(args, callback);
 
   // Make sure we access |ctmf_| only on the thread where it lives to avoid
   // races on shutdown.
-  if (!ctmf_task_runner_->BelongsToCurrentThread()) {
-    ctmf_task_runner_->PostTask(
+  if (!ctmf_task_runner->BelongsToCurrentThread()) {
+    const bool did_post_task = ctmf_task_runner->PostTask(
         FROM_HERE,
         base::Bind(&ChildMemoryDumpManagerDelegateImpl::RequestGlobalMemoryDump,
                    base::Unretained(this), args, callback));
+    if (!did_post_task)
+      return AbortDumpRequest(args, callback);
     return;
   }
 
   // The ChildTraceMessageFilter could have been destroyed while hopping on the
   // right thread. If this is the case, bail out.
-  if (!ctmf_) {
-    if (!callback.is_null())
-      callback.Run(args.dump_guid, false /* success */);
-    return;
-  }
+  if (!ctmf_)
+    return AbortDumpRequest(args, callback);
 
   // Send the request up to the browser process' MessageDumpmanager.
   ctmf_->SendGlobalMemoryDumpRequest(args, callback);