Allow unregistering MemoryDumpProviders during dump

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 "base/atomic_sequence_num.h"
 #include "base/command_line.h"
@@ skipping 75 matching lines @@
 }
 
 // static
 void MemoryDumpManager::SetInstanceForTesting(MemoryDumpManager* instance) {
   if (instance)
     instance->skip_core_dumpers_auto_registration_for_testing_ = true;
   g_instance_for_testing = 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),
       skip_core_dumpers_auto_registration_for_testing_(false) {
   g_next_guid.GetNext();  // Make sure that first guid is not zero.
 }
 
 MemoryDumpManager::~MemoryDumpManager() {
   base::trace_event::TraceLog::GetInstance()->RemoveEnabledStateObserver(this);
 }
@@ skipping 37 matching lines @@
   AutoLock lock(lock_);
   DCHECK_EQ(static_cast<MemoryDumpManagerDelegate*>(nullptr), delegate_);
   delegate_ = delegate;
 }
 
 void MemoryDumpManager::RegisterDumpProvider(
     MemoryDumpProvider* mdp,
     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) {
   RegisterDumpProvider(mdp, nullptr);
 }
 
 void MemoryDumpManager::UnregisterDumpProvider(MemoryDumpProvider* mdp) {
   AutoLock lock(lock_);
 
   auto mdp_iter = dump_providers_.begin();
@@ skipping 10 matching lines @@
   // 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_IMPLIES(
       subtle::NoBarrier_Load(&memory_tracing_enabled_),
       mdp_iter->task_runner && mdp_iter->task_runner->BelongsToCurrentThread())
       << "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,
                                           const MemoryDumpArgs& dump_args,
                                           const MemoryDumpCallback& callback) {
   // Bail out immediately if tracing is not enabled at all.
   if (!UNLIKELY(subtle::NoBarrier_Load(&memory_tracing_enabled_))) {
     if (!callback.is_null())
       callback.Run(0u /* guid */, false /* success */);
     return;
@@ skipping 23 matching lines @@
 void MemoryDumpManager::RequestGlobalDump(MemoryDumpType dump_type,
                                           const MemoryDumpArgs& dump_args) {
   RequestGlobalDump(dump_type, dump_args, MemoryDumpCallback());
 }
 
 void MemoryDumpManager::CreateProcessDump(const MemoryDumpRequestArgs& args,
                                           const MemoryDumpCallback& callback) {
   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));
   }
 
   // Start the thread hop. |dump_providers_| are kept sorted by thread, so
   // ContinueAsyncProcessDump will hop at most once per thread (w.r.t. thread
   // affinity specified by the MemoryDumpProvider(s) in RegisterDumpProvider()).
   ContinueAsyncProcessDump(pmd_async_state.Pass());
 }
 
@@ skipping 19 matching lines @@
   // in the PostTask below don't end up registering their own dump providers
   // (for discounting trace memory overhead) while holding the |lock_|.
   TraceLog::GetInstance()->InitializeThreadLocalEventBufferIfSupported();
 
   // DO NOT put any LOG() statement in the locked sections, as in some contexts
   // (GPU process) LOG() ends up performing PostTask/IPCs.
   MemoryDumpProvider* mdp;
   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()) {
       // It's time to hop onto another thread.
 
       // Copy the callback + arguments just for the unlikley case in which
       // PostTask fails. In such case the Bind helper will destroy the
       // pmd_async_state and we must keep a copy of the fields to notify the
       // abort.
       MemoryDumpCallback callback = pmd_async_state->callback;
       scoped_refptr<SingleThreadTaskRunner> callback_task_runner =
           pmd_async_state->task_runner;
       const uint64 dump_guid = pmd_async_state->req_args.dump_guid;
 
       const bool did_post_task = mdp_info->task_runner->PostTask(
           FROM_HERE, Bind(&MemoryDumpManager::ContinueAsyncProcessDump,
                           Unretained(this), Passed(pmd_async_state.Pass())));
       if (did_post_task)
         return;
 
       // The thread is gone. At this point the best thing we can do is to
       // disable the dump provider and abort this dump.
       mdp_info->disabled = true;

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

I wonder at this point if we should also gracefully handle this other case (in
another CL)

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

Does it happen in practice?

[Primiano Tucci (use gerrit), 2015/08/24 09:31:19]

I don't know, that was I was wondering.
It's just that one speculation of mine "In the unlikely event" turned out to be
not so unlikely. So I wonder whether this is like that :)
Anyways, this doesn't belong to the current CL .

       return AbortDumpLocked(callback, callback_task_runner, dump_guid);
     }
   }  // AutoLock(lock_)
 
   // Invoke the dump provider without holding the |lock_|.
   bool finalize = false;
   bool dump_successful = false;
 
   if (!skip_dump) {
     dump_successful = mdp->OnMemoryDump(pmd_async_state->req_args.dump_args,
                                         &pmd_async_state->process_memory_dump);
   }
 
   {
     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;
     } else if (!skip_dump) {
       ++mdp_info->consecutive_failures;
       if (mdp_info->consecutive_failures >= kMaxConsecutiveFailuresCount) {
         mdp_info->disabled = true;
       }
     }
     ++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) {
     LOG(ERROR) << "A memory dumper failed, possibly due to sandboxing "
                   "(crbug.com/461788). Disabling dumper for current process. "
                   "Try restarting chrome with the --no-sandbox switch.";
   }
 
   if (finalize)
     return FinalizeDumpAndAddToTrace(pmd_async_state.Pass());
@@ skipping 100 matching lines @@
 uint64 MemoryDumpManager::GetTracingProcessId() const {
   return delegate_->GetTracingProcessId();
 }
 
 MemoryDumpManager::MemoryDumpProviderInfo::MemoryDumpProviderInfo(
     MemoryDumpProvider* dump_provider,
     const scoped_refptr<SingleThreadTaskRunner>& task_runner)
     : dump_provider(dump_provider),
       task_runner(task_runner),
       consecutive_failures(0),
-      disabled(false) {
-}
+      disabled(false),
+      unregistered(false) {}
 
 MemoryDumpManager::MemoryDumpProviderInfo::~MemoryDumpProviderInfo() {
 }
 
 bool MemoryDumpManager::MemoryDumpProviderInfo::operator<(
     const MemoryDumpProviderInfo& other) const {
   if (task_runner == other.task_runner)
     return dump_provider < other.dump_provider;
   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,
     const scoped_refptr<MemoryDumpSessionState>& session_state,
     MemoryDumpCallback callback)
     : process_memory_dump(session_state),
       req_args(req_args),
       next_dump_provider(next_dump_provider),
       callback(callback),
       task_runner(MessageLoop::current()->task_runner()) {
 }
 
 MemoryDumpManager::ProcessMemoryDumpAsyncState::~ProcessMemoryDumpAsyncState() {
 }
 
 }  // namespace trace_event
 }  // namespace base