[tracing] Allow asynchronous unregistration of unbound dump providers

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 181 matching lines @@
     const MemoryDumpProvider::Options& options) {
   if (dumper_registrations_ignored_for_testing_)
     return;
 
   scoped_refptr<MemoryDumpProviderInfo> mdpinfo =
       new MemoryDumpProviderInfo(mdp, name, task_runner, options);
 
   {
     AutoLock lock(lock_);
     bool already_registered = !dump_providers_.insert(mdpinfo).second;
-    // This actually happen in some tests which don't have a clean tear-down
+    // This actually happens in some tests which don't have a clean tear-down
     // path for RenderThreadImpl::Init().
     if (already_registered)
       return;
   }
 
   if (heap_profiling_enabled_)
     mdp->OnHeapProfilingEnabled(true);
 }
 
 void MemoryDumpManager::RegisterDumpProvider(
     MemoryDumpProvider* mdp,
     const char* name,
     const scoped_refptr<SingleThreadTaskRunner>& task_runner) {
   RegisterDumpProvider(mdp, name, task_runner, MemoryDumpProvider::Options());
 }
 
 void MemoryDumpManager::UnregisterDumpProvider(MemoryDumpProvider* mdp) {
+  UnregisterDumpProviderInternal(mdp, false /* delete_async */);
+}
+
+void MemoryDumpManager::UnregisterAndDeleteSoonDumpProvider(
+    scoped_ptr<MemoryDumpProvider> mdp) {
+  UnregisterDumpProviderInternal(mdp.release(), true /* delete_async */);
+}
+
+void MemoryDumpManager::UnregisterDumpProviderInternal(
+    MemoryDumpProvider* mdp,
+    bool take_mdp_ownership_and_delete_async) {
+  scoped_ptr<MemoryDumpProvider> owned_mdp;
+  if (take_mdp_ownership_and_delete_async)
+    owned_mdp.reset(mdp);
+
   AutoLock lock(lock_);
 
   auto mdp_iter = dump_providers_.begin();
   for (; mdp_iter != dump_providers_.end(); ++mdp_iter) {
     if ((*mdp_iter)->dump_provider == mdp)
       break;
   }
 
   if (mdp_iter == dump_providers_.end())
-    return;
+    return;  // Not registered / already unregistered.
 
-  // Unregistration of a MemoryDumpProvider while tracing is ongoing is safe
-  // only if the MDP has specified a thread affinity (via task_runner()) AND
-  // the unregistration happens on the same thread (so the MDP cannot unregister
-  // and OnMemoryDump() at the same time).
-  // Otherwise, it is not possible to guarantee that its unregistration is
-  // race-free. If you hit this DCHECK, your MDP has a bug.
-  DCHECK(!subtle::NoBarrier_Load(&memory_tracing_enabled_) ||
-         ((*mdp_iter)->task_runner &&
-          (*mdp_iter)->task_runner->BelongsToCurrentThread()))
-      << "MemoryDumpProvider \"" << (*mdp_iter)->name << "\" attempted to "
-      << "unregister itself in a racy way. Please file a crbug.";
+  if (take_mdp_ownership_and_delete_async) {
+    // The MDP will be deleted whenever the MDPInfo struct will, that is either:
+    // - At the end of this function, if no dump is in progress.
+    // - In the prologue of the ContinueAsyncProcessDump().
+    DCHECK(!(*mdp_iter)->owned_dump_provider);
+    (*mdp_iter)->owned_dump_provider = std::move(owned_mdp);
+  } else if (subtle::NoBarrier_Load(&memory_tracing_enabled_)) {

[Ruud van Asseldonk, 2016/01/04 15:43:01]

Can we just get rid of the if here and unconditionally have the DCHECKs below if
we don’t take ownership? The check for |memory_tracing_enabled| is nobarrier, so
its value should be taken as a hint at best.

[Primiano Tucci (use gerrit), 2016/01/04 16:17:05]

Ok, as discussed offline this makes sense.
I'm just doing this in a separate CL  (crrev.com/1559023002) as there is a
chance something might be relying on that and the CL could be reverted. I don't
want to lose this CL in that case.

+    // If you hit this DCHECK, your dump provider has a bug.
+    // Unregistration of a MemoryDumpProvider is safe only if:
+    // - The MDP has specified a thread affinity (via task_runner()) AND
+    //   the unregistration happens on the same thread (so the MDP cannot
+    //   unregister and be in the middle of a OnMemoryDump() at the same time.
+    // - The MDP has NOT specified a thread affinity and its ownership is
+    //   transferred via UnregisterAndDeleteSoonDumpProvider().
+    // In all the other cases, it is not possible to guarantee that the
+    // unregistration will not race with OnMemoryDump() calls.
+    DCHECK((*mdp_iter)->task_runner &&
+           (*mdp_iter)->task_runner->BelongsToCurrentThread())
+        << "MemoryDumpProvider \"" << (*mdp_iter)->name << "\" attempted to "
+        << "unregister itself in a racy way. Please file a crbug.";
+  }
 
   // The MDPInfo instance can still be referenced by the
   // |ProcessMemoryDumpAsyncState.pending_dump_providers|. For this reason
   // the MDPInfo is flagged as disabled. It will cause ContinueAsyncProcessDump
   // to just skip it, without actually invoking the |mdp|, which might be
   // destroyed by the caller soon after this method returns.
   (*mdp_iter)->disabled = true;
   dump_providers_.erase(mdp_iter);
 }
 
@@ skipping 106 matching lines @@
       pmd_async_state->pending_dump_providers.back().get();
 
   // 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()) {
     // It's time to hop onto another thread.
-    const bool did_post_task = task_runner->PostTask(
+    post_task_failed = !task_runner->PostTask(
         FROM_HERE, Bind(&MemoryDumpManager::ContinueAsyncProcessDump,
                         Unretained(this), Unretained(pmd_async_state.get())));
-    if (did_post_task) {
+    if (!post_task_failed) {
       // Ownership is tranferred to the next ContinueAsyncProcessDump().
       ignore_result(pmd_async_state.release());
       return;
     }
-    // The thread is gone. Skip the dump provider and keep going.
-    mdpinfo->disabled = true;
   }
 
-  // At this point wither we are on the right thread (|mdpinfo.task_runner|)
-  // to access mdp fields, or the right thread is gone (and |disabled| == true).
+  // At this point either:
+  // - The MDP has a task runner affinity and we are on the right thread.
+  // - The MDP has a task runner affinity but the underlying thread is gone,
+  //   hence the above |post_task_failed| == true.
+  // - The MDP does NOT have a task runner affinity. A locked access is required
+  //   to R/W |disabled| (for the UnregisterAndDeleteSoonDumpProvider() case).
+  bool should_dump;
+  const char* disabled_reason = nullptr;
+  {
+    AutoLock lock(lock_);
+    if (mdpinfo->consecutive_failures >= kMaxConsecutiveFailuresCount) {
+      mdpinfo->disabled = true;
+      disabled_reason =
+          "Dump failure, possibly related with sandboxing (crbug.com/461788). "
+          "Try --no-sandbox.";
+    } else if (post_task_failed) {
+      disabled_reason = "The thread it was meant to dump onto is gone.";
+      mdpinfo->disabled = true;
+    }
+    should_dump = !mdpinfo->disabled;
+  }
 
-  if (!mdpinfo->disabled) {
+  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",
                            TRACE_ID_MANGLE(pmd_async_state->req_args.dump_guid),
                            TRACE_EVENT_FLAG_FLOW_IN | TRACE_EVENT_FLAG_FLOW_OUT,
                            "dump_provider.name", mdpinfo->name);
 
     // Pid of the target process being dumped. Often kNullProcessId (= current
     // process), non-zero when the coordinator process creates dumps on behalf
     // of child processes (see crbug.com/461788).
     ProcessId target_pid = mdpinfo->options.target_pid;
     ProcessMemoryDump* pmd =
         pmd_async_state->GetOrCreateMemoryDumpContainerForProcess(target_pid);
     MemoryDumpArgs args = {pmd_async_state->req_args.level_of_detail};
     bool dump_successful = mdpinfo->dump_provider->OnMemoryDump(args, pmd);
-
-    if (dump_successful) {
-      mdpinfo->consecutive_failures = 0;
-    } else {
-      ++mdpinfo->consecutive_failures;
-      if (mdpinfo->consecutive_failures >= kMaxConsecutiveFailuresCount) {
-        mdpinfo->disabled = true;
-        LOG(ERROR) << "MemoryDumpProvider \"" << mdpinfo->name << "\" failed, "
-                   << "possibly due to sandboxing (crbug.com/461788)."
-                   << "Disabling dumper for current process. Try --no-sandbox.";
-      }
-    }
+    mdpinfo->consecutive_failures =
+        dump_successful ? 0 : mdpinfo->consecutive_failures + 1;
   }  // if (!mdpinfo->disabled)
 
   pmd_async_state->pending_dump_providers.pop_back();
   ContinueAsyncProcessDump(pmd_async_state.release());
 }
 
 // static
 void MemoryDumpManager::FinalizeDumpAndAddToTrace(
     scoped_ptr<ProcessMemoryDumpAsyncState> pmd_async_state) {
   DCHECK(pmd_async_state->pending_dump_providers.empty());
@@ skipping 194 matching lines @@
   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