[tracing] Allow asynchronous unregistration of unbound dump providers

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 f5c3c4feb4b91c8e6835a154239cc55979e476ef..99a3c541f2f3401bfc828dd4f045083906ff5c3b 100644
--- a/base/trace_event/memory_dump_manager.cc
+++ b/base/trace_event/memory_dump_manager.cc
@@ -217,6 +217,21 @@ void MemoryDumpManager::RegisterDumpProvider(
 }
 
 void MemoryDumpManager::UnregisterDumpProvider(MemoryDumpProvider* mdp) {
+  UnregisterDumpProviderInternal(mdp, false /* delete_async */);
+}
+
+void MemoryDumpManager::UnregisterAndDeleteDumpProviderAsync(
+    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);  // Auto delete in corner early out cases.
+
   AutoLock lock(lock_);
 
   auto mdp_iter = dump_providers_.begin();
@@ -226,19 +241,29 @@ void MemoryDumpManager::UnregisterDumpProvider(MemoryDumpProvider* mdp) {
   }
 
   if (mdp_iter == dump_providers_.end())
-    return;
-
-  // 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.";
+    return;  // Not registered / already unregistered.
+
+  // 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 UnregisterAndDeleteDumpProviderAsync().
+  // In all the other cases, it is not possible to guarantee that the
+  // unregistration will not race with OnMemoryDump() calls.
+  if (!take_mdp_ownership_and_delete_async &&
+      subtle::NoBarrier_Load(&memory_tracing_enabled_)) {

[Primiano Tucci (use gerrit), 2015/12/22 15:04:46]

ah this logic is wrong.
The else below should be:
else if (take_ownership) ...

or something equivalent

[Ruud van Asseldonk, 2015/12/29 11:32:46]

Indeed, disabling tracing should not cause the dumpers to be deleted, should it?

+    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.";
+  } else {
+    // 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().
+    std::swap((*mdp_iter)->owned_dump_provider, owned_mdp);
+  }
 
   // The MDPInfo instance can still be referenced by the
   // |ProcessMemoryDumpAsyncState.pending_dump_providers|. For this reason
@@ -365,24 +390,45 @@ void MemoryDumpManager::ContinueAsyncProcessDump(
   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 UnregisterAndDeleteDumpProviderAsync() case).
+  bool skip_dump;
+  {
+    AutoLock lock(lock_);
+    if (mdpinfo->consecutive_failures >= kMaxConsecutiveFailuresCount) {
+      mdpinfo->disabled = true;
+      skip_dump = true;
+      LOG(ERROR) << "MemoryDumpProvider \"" << mdpinfo->name << "\" failed, "

[ssid, 2015/12/22 15:50:09]

Isn't logging inside lock slow?

[Primiano Tucci (use gerrit), 2016/01/04 14:44:38]

Done.

+                 << "possibly due to sandboxing (crbug.com/461788)."

[ssid, 2015/12/22 15:50:09]

Do you think I should remove this line in my CL, or at least change the log
statement. At that point this if conditions can be merged. and log can be made
common, something like:
"MemoryDumpProvider %s is disabled due to multiple failures or missing thread"

[Primiano Tucci (use gerrit), 2016/01/04 14:44:38]

Done here. and solved the lock issue

+                 << "Disabling dumper for current process. Try --no-sandbox.";
+    } else if (post_task_failed) {
+      // The thread is gone. Disable MDP forever.
+      mdpinfo->disabled = true;
+      skip_dump = true;
+    } else {
+      // The MDP might have been unregistered while we were hopping threads.

[ssid, 2015/12/22 15:50:09]

I think this comment is not needed. Moreover the mdp could have been
unregistered while dumping on other threads probably?

Also the else condition is not needed.
I think the code should be:

if (consecutive_failures >= kMaxConsecutiveFailuresCount or post_task_failed) {
  mdpinfo->disabled = true;
  LOG(ERROR) << "MemoryDumpProvider %s is disabled due to multiple failures or
missing thread";
}
skip_dump = mdpinfo->disabled;

But if you are keen on adding log only for that case, you could have 2
conditions. else is still not needed.

[Primiano Tucci (use gerrit), 2016/01/04 14:44:38]

Right. It's already described in the comment block above.
Switched to the 2 conditions. Done.

+      skip_dump = mdpinfo->disabled;
+    }
+  }
 
-  if (!mdpinfo->disabled) {
+  if (!skip_dump) {

[ssid, 2015/12/22 15:50:09]

I wonder why shouldn't this be should_dump or similarly positive. You are
checking for ! of negative here. I get confused every time i read this code.

[Primiano Tucci (use gerrit), 2016/01/04 14:44:38]

Right, with the current logic should_dump makes more sense.

     // Invoke the dump provider.
     TRACE_EVENT_WITH_FLOW1(kTraceCategory,
                            "MemoryDumpManager::ContinueAsyncProcessDump",
@@ -398,18 +444,8 @@ void MemoryDumpManager::ContinueAsyncProcessDump(
         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();