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/24 09:31:19]

Please add a comment here:
// This is to deal with the corner case of a dump provider unregistering and
re-registering immediately after.

[Ruud van Asseldonk, 2015/08/24 10:08:37]

Done.

+  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;
+    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;
@@ skipping 18 matching lines @@
   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;
+    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();
+
+    if (mdp_info->unregistered) {

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

tip: you can omit the braces for single line ifs if you want.

[Ruud van Asseldonk, 2015/08/24 10:08:37]

Done.

+      dump_providers_.erase(mdp_info);
+    }
   }
 
   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;
 }
 
 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