Allow unregistering MemoryDumpProviders during dump

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 bafd121af1e461cc246c3664d6b269079c9b752d..b193e4944e1a4e41957422d474a5b83da4c61ce6 100644
--- a/base/trace_event/memory_dump_manager.cc
+++ b/base/trace_event/memory_dump_manager.cc
@@ -93,8 +93,7 @@ void MemoryDumpManager::SetInstanceForTesting(MemoryDumpManager* instance) {
 }
 
 MemoryDumpManager::MemoryDumpManager()
-    : did_unregister_dump_provider_(false),
-      delegate_(nullptr),
+    : delegate_(nullptr),
       memory_tracing_enabled_(0),
       tracing_process_id_(kInvalidTracingProcessId),
       system_allocator_pool_name_(nullptr),
@@ -152,7 +151,13 @@ void MemoryDumpManager::RegisterDumpProvider(
     const scoped_refptr<SingleThreadTaskRunner>& task_runner) {
   MemoryDumpProviderInfo mdp_info(mdp, task_runner);
   AutoLock lock(lock_);
-  dump_providers_.insert(mdp_info);
+  auto iter_new = dump_providers_.insert(mdp_info);
+
+  // If there was a previous entry, replace it with the new one.

[Primiano Tucci (use gerrit), 2015/08/21 09:00:31]

Is this really necessary? To me the fact that something can doubly-register is a
bug.
we should just hard fail in that case. What I'd do here is

auto iter_new = dump_providers_.insert(mdp_info);
const bool did_insert = iter_new.second;
DCHECK(did_insert);

And NOT handle gracefully the erase + reinsert below.

[Ruud van Asseldonk, 2015/08/21 10:51:40]

If the provider registers itself twice, that is a bug, but if a provider
unregisters and then registers again, the entry might still be there, with
`unregistered` set to true. If it is not expected for providers to ever register
again after unregistering then this is redundant, or we could add an assertion.

+  if (!iter_new.second) {
+    dump_providers_.erase(iter_new.first);
+    dump_providers_.insert(mdp_info);
+  }
 }
 
 void MemoryDumpManager::RegisterDumpProvider(MemoryDumpProvider* mdp) {
@@ -183,8 +188,7 @@ void MemoryDumpManager::UnregisterDumpProvider(MemoryDumpProvider* mdp) {
       << "The MemoryDumpProvider attempted to unregister itself in a racy way. "
       << "Please file a crbug.";
 
-  dump_providers_.erase(mdp_iter);
-  did_unregister_dump_provider_ = true;
+  mdp_iter->unregistered = true;
 }
 
 void MemoryDumpManager::RequestGlobalDump(MemoryDumpType dump_type,
@@ -228,7 +232,6 @@ void MemoryDumpManager::CreateProcessDump(const MemoryDumpRequestArgs& args,
   scoped_ptr<ProcessMemoryDumpAsyncState> pmd_async_state;
   {
     AutoLock lock(lock_);
-    did_unregister_dump_provider_ = false;
     pmd_async_state.reset(new ProcessMemoryDumpAsyncState(
         args, dump_providers_.begin(), session_state_, callback));
   }
@@ -268,18 +271,10 @@ void MemoryDumpManager::ContinueAsyncProcessDump(
   bool skip_dump = false;
   {
     AutoLock lock(lock_);
-    // In the unlikely event that a dump provider was unregistered while
-    // dumping, abort the dump, as that would make |next_dump_provider| invalid.
-    // Registration, on the other hand, is safe as per std::set<> contract.
-    if (did_unregister_dump_provider_) {
-      return AbortDumpLocked(pmd_async_state->callback,
-                             pmd_async_state->task_runner,
-                             pmd_async_state->req_args.dump_guid);
-    }
 
     auto* mdp_info = &*pmd_async_state->next_dump_provider;
     mdp = mdp_info->dump_provider;
-    if (mdp_info->disabled) {
+    if (mdp_info->disabled || mdp_info->unregistered) {
       skip_dump = true;
     } else if (mdp_info->task_runner &&
                !mdp_info->task_runner->BelongsToCurrentThread()) {
@@ -318,11 +313,6 @@ void MemoryDumpManager::ContinueAsyncProcessDump(
 
   {
     AutoLock lock(lock_);
-    if (did_unregister_dump_provider_) {
-      return AbortDumpLocked(pmd_async_state->callback,
-                             pmd_async_state->task_runner,
-                             pmd_async_state->req_args.dump_guid);
-    }
     auto* mdp_info = &*pmd_async_state->next_dump_provider;
     if (dump_successful) {
       mdp_info->consecutive_failures = 0;
@@ -334,6 +324,16 @@ void MemoryDumpManager::ContinueAsyncProcessDump(
     }
     ++pmd_async_state->next_dump_provider;
     finalize = pmd_async_state->next_dump_provider == dump_providers_.end();
+
+    // Remove providers that unregistered since the last dump.

[Primiano Tucci (use gerrit), 2015/08/21 09:00:30]

Hmm I don't like too much the idea of iterating over the full set of dump
providers on every dump. I think we can do something more efficient here.

My proposal:

1. Turn mdp_info into an iterator (remove &* on line 275), not a ptr (now it's
safe to do that as you are guaranteed you never remove anything outside of this
function).

2. On line  325 do:
++pmd_async_state->next_dump_provider;
if (mdp_info->unregistered)
  dump_providers_.erase(mdp_info)
// Nothing below this point should use mdp_info.

This should always be safe and from a performance viewpoint will be:
1) Just a if check for most cases (if you don't unregister anything)
2) A straight removal from the set, only if you need to actually unregister

[Ruud van Asseldonk, 2015/08/21 10:51:40]

Done.

+    if (finalize) {
+      auto i = dump_providers_.begin();
+      while (i != dump_providers_.end()) {
+        auto current = i++;
+        if (current->unregistered)
+          dump_providers_.erase(current);
+      }
+    }
   }
 
   if (!skip_dump && !dump_successful) {
@@ -454,8 +454,8 @@ MemoryDumpManager::MemoryDumpProviderInfo::MemoryDumpProviderInfo(
     : dump_provider(dump_provider),
       task_runner(task_runner),
       consecutive_failures(0),
-      disabled(false) {
-}
+      disabled(false),
+      unregistered(false) {}
 
 MemoryDumpManager::MemoryDumpProviderInfo::~MemoryDumpProviderInfo() {
 }
@@ -467,6 +467,12 @@ bool MemoryDumpManager::MemoryDumpProviderInfo::operator<(
   return task_runner < other.task_runner;
 }
 
+bool MemoryDumpManager::MemoryDumpProviderInfo::operator==(

[Primiano Tucci (use gerrit), 2015/08/21 09:00:30]

Why is this needed? AFAIK std::set relies only on the < operator.

[Ruud van Asseldonk, 2015/08/21 10:51:40]

It isn’t needed.

+    const MemoryDumpProviderInfo& other) const {
+  return dump_provider == other.dump_provider &&
+         task_runner == other.task_runner;
+}
+
 MemoryDumpManager::ProcessMemoryDumpAsyncState::ProcessMemoryDumpAsyncState(
     MemoryDumpRequestArgs req_args,
     MemoryDumpProviderInfoSet::iterator next_dump_provider,