[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 199 matching lines @@
 }
 
 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::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();
   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 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
   // 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 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",
                            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