Reland of [tracing] Simplify logic of MemoryDumpManager

Reason for revert:
Original CL: crrev.com/1536533004
Revert: crrev.com/1540283003
Reason for reland:
  The CL was reverted because caused a LayoutTest failure
  (pending-version-change-stuck-works-with-terminate.html).
  The failure was due to a race condition in an unrelated part of the
  codebase (see crbug.com/571432). The original CL managed to just
  unveil reliably the race. The race condition has been fixed in
  crrev.com/1544663002. I verified that the LayoutTest doesn't crash
  locally anymore.

Original issue's description:
> Revert of [tracing] Simplify logic of MemoryDumpManager (patchset #9 id:160001 of https://codereview.chromium.org/1536533004/ )
>
> Reason for revert:
> Caused crash in pending-version-change-stuck-works-with-terminate.html layout test.
>
> See:
> https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.10/builds/9506/steps/webkit_tests/logs/stdio
>
> Original issue's description:
> > [tracing] Simplify logic of MemoryDumpManager
> >
> > This CL simplifies the unregistration logic of MemoryDumpManager.
> > This is to make the overall code more readable and simplify upcoming
> > changes.
> > Prior to this CL, the MemoryDumpManager was keeping only one list to
> > keep track of the registered dump providers. This caused the
> > unregistration logic to be tricky because:
> >  - We couldn't remove the MDPInfo straight away, as that might
> >    cause invalidation of the |next_dump_provider| iterator.
> >  - We had to flag the MDPInfo as unregistered and postpone the deletion
> >    on the next dump.
> > This has a major drawback: if we keep registering and unregistering
> > dump providers when no tracing is happening at all, the dump_providers_
> > list grows without bounds. This is bad.
> >
> > This CL changes the logic as follows:
> >  - MDPInfo becomes refcounted. At any time it can be referenced by:
> >    - The MDM's |dump_providers_| list.
> >    - The |ProcessMemoryDumpAsyncState.pending_dump_providers| list.
> >  - Upon each dump, the dump provider list is snapshotted in the
> >    |ProcessMemoryDumpAsyncState.pending_dump_providers|
> >  - Upon unregistration of a dump provider we just remove the MDPInfo
> >    from the MDM's |dump_providers_|. If a dump is ongoing, the
> >    ProcessMemoryDumpAsyncState will keep it refcounted and delete it
> >    during the dump.
> >
> > This CL does not add or change any behavior of the MemoryDumpManager,
> > with the exception of:
> >  - Fixing a corner case when dumping with no dump providers registered
> >    (See changes in the unittest).
> >  - Making the fail-safe logic more strict: if a dumper fails once, it
> >    will stay disabled forever.
> >
> > BUG=461788
> >
> > Committed: https://crrev.com/9734733909e7cb41ef5c153f3c2d0927e8209133
> > Cr-Commit-Position: refs/heads/master@{#366374}
>
> TBR=ruuda@google.com,ssid@chromium.org
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG=461788
>
> Committed: https://crrev.com/6a7365c4c5ad3e3083ce3d9a269c3e1d8fdb35bc
> Cr-Commit-Position: refs/heads/master@{#366386}

TBR=ruuda@google.com,ssid@chromium.org
BUG=461788

Review URL: https://codereview.chromium.org/1546583002

Cr-Commit-Position: refs/heads/master@{#366588}

3 files changed, 226 insertions, 188 deletions

diff --git a/base/trace_event/memory_dump_manager.cc b/base/trace_event/memory_dump_manager.cc
index cbce210..f5c3c4f 100644
--- a/base/trace_event/memory_dump_manager.cc
+++ b/base/trace_event/memory_dump_manager.cc
@@ -193,17 +193,16 @@ void MemoryDumpManager::RegisterDumpProvider(
   if (dumper_registrations_ignored_for_testing_)
     return;
 
-  MemoryDumpProviderInfo mdp_info(mdp, name, task_runner, options);
-  AutoLock lock(lock_);
-  auto iter_new = dump_providers_.insert(mdp_info);
-
-  // If there was a previous entry, replace it with the new one. This is to deal
-  // with the case where a dump provider unregisters itself and then re-
-  // registers before a memory dump happens, so its entry was still in the
-  // collection but flagged |unregistered|.
-  if (!iter_new.second) {
-    dump_providers_.erase(iter_new.first);
-    dump_providers_.insert(mdp_info);
+  scoped_refptr<MemoryDumpProviderInfo> mdpinfo =
+      new MemoryDumpProviderInfo(mdp, name, task_runner, options);
+
+  {
+    AutoLock lock(lock_);
+    bool already_registered = !dump_providers_.insert(mdpinfo).second;
+    // This actually happen in some tests which don't have a clean tear-down
+    // path for RenderThreadImpl::Init().
+    if (already_registered)
+      return;
   }
 
   if (heap_profiling_enabled_)
@@ -222,7 +221,7 @@ void MemoryDumpManager::UnregisterDumpProvider(MemoryDumpProvider* mdp) {
 
   auto mdp_iter = dump_providers_.begin();
   for (; mdp_iter != dump_providers_.end(); ++mdp_iter) {
-    if (mdp_iter->dump_provider == mdp)
+    if ((*mdp_iter)->dump_provider == mdp)
       break;
   }
 
@@ -236,12 +235,18 @@ void MemoryDumpManager::UnregisterDumpProvider(MemoryDumpProvider* mdp) {
   // 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 "
+         ((*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.";
 
-  mdp_iter->unregistered = true;
+  // 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);
 }
 
 void MemoryDumpManager::RequestGlobalDump(
@@ -296,9 +301,9 @@ void MemoryDumpManager::CreateProcessDump(const MemoryDumpRequestArgs& args,
   scoped_ptr<ProcessMemoryDumpAsyncState> pmd_async_state;
   {
     AutoLock lock(lock_);
-    pmd_async_state.reset(new ProcessMemoryDumpAsyncState(
-        args, dump_providers_.begin(), session_state_, callback,
-        dump_thread_->task_runner()));
+    pmd_async_state.reset(
+        new ProcessMemoryDumpAsyncState(args, dump_providers_, session_state_,
+                                        callback, dump_thread_->task_runner()));
   }
 
   TRACE_EVENT_WITH_FLOW0(kTraceCategory, "MemoryDumpManager::CreateProcessDump",
@@ -308,7 +313,7 @@ void MemoryDumpManager::CreateProcessDump(const MemoryDumpRequestArgs& args,
   // 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(std::move(pmd_async_state));
+  ContinueAsyncProcessDump(pmd_async_state.release());
 }
 
 // At most one ContinueAsyncProcessDump() can be active at any time for a given
@@ -318,128 +323,103 @@ void MemoryDumpManager::CreateProcessDump(const MemoryDumpRequestArgs& args,
 // means of subsequent PostTask(s).
 //
 // 1) Prologue:
-//   - Check if the dump provider is disabled, if so skip the dump.
+//   - If this was the last hop, create a trace event, add it to the trace
+//     and finalize (invoke callback).
 //   - Check if we are on the right thread. If not hop and continue there.
+//   - Check if the dump provider is disabled, if so skip the dump.
 // 2) Invoke the dump provider's OnMemoryDump() (unless skipped).
 // 3) Epilogue:
-//  - Unregister the dump provider if it failed too many times consecutively.
-//  - Advance the |next_dump_provider| iterator to the next dump provider.
-//  - If this was the last hop, create a trace event, add it to the trace
-//    and finalize (invoke callback).
-
+//   - Unregister the dump provider if it failed too many times consecutively.
+//   - Pop() the MDP from the |pending_dump_providers| list, eventually
+//     destroying the MDPInfo if that was unregistered in the meantime.
 void MemoryDumpManager::ContinueAsyncProcessDump(
-    scoped_ptr<ProcessMemoryDumpAsyncState> pmd_async_state) {
+    ProcessMemoryDumpAsyncState* owned_pmd_async_state) {
   // Initalizes the ThreadLocalEventBuffer to guarantee that the TRACE_EVENTs
   // 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();
 
-  const uint64_t dump_guid = pmd_async_state->req_args.dump_guid;
-  const char* dump_provider_name = nullptr;
-
-  // 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 pid;
-
-  // 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 theory |owned_pmd_async_state| should be a scoped_ptr. The only reason
+  // why it isn't is because of the corner case logic of |did_post_task| below,
+  // which needs to take back the ownership of the |pmd_async_state| when a
+  // thread goes away and consequently the PostTask() fails.
+  // Unfortunately, PostTask() destroys the scoped_ptr arguments upon failure
+  // to prevent accidental leaks. Using a scoped_ptr would prevent us to to
+  // skip the hop and move on. Hence the manual naked -> scoped ptr juggling.
+  auto pmd_async_state = make_scoped_ptr(owned_pmd_async_state);
+  owned_pmd_async_state = nullptr;
+
+  if (pmd_async_state->pending_dump_providers.empty())
+    return FinalizeDumpAndAddToTrace(std::move(pmd_async_state));
 
-    auto mdp_info = pmd_async_state->next_dump_provider;
-    mdp = mdp_info->dump_provider;
-    dump_provider_name = mdp_info->name;
-    pid = mdp_info->options.target_pid;
-
-    // If the dump provider did not specify a thread affinity, dump on
-    // |dump_thread_|.
-    SingleThreadTaskRunner* task_runner = mdp_info->task_runner.get();
-    if (!task_runner)
-      task_runner = pmd_async_state->dump_thread_task_runner.get();
-
-    // |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.
-    // |task_runner|, however, should always be non-null.
-    DCHECK(task_runner);
-
-    if (mdp_info->disabled || mdp_info->unregistered) {
-      skip_dump = true;
-    } else if (!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->callback_task_runner;
-
-      const bool did_post_task = task_runner->PostTask(
-          FROM_HERE, Bind(&MemoryDumpManager::ContinueAsyncProcessDump,
-                          Unretained(this), Passed(&pmd_async_state)));
-      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;
-      return AbortDumpLocked(callback, callback_task_runner, dump_guid);
+  // Read MemoryDumpProviderInfo thread safety considerations in
+  // memory_dump_manager.h when accessing |mdpinfo| fields.
+  MemoryDumpProviderInfo* mdpinfo =
+      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();
+
+  if (!task_runner->BelongsToCurrentThread()) {
+    // It's time to hop onto another thread.
+    const bool did_post_task = task_runner->PostTask(
+        FROM_HERE, Bind(&MemoryDumpManager::ContinueAsyncProcessDump,
+                        Unretained(this), Unretained(pmd_async_state.get())));
+    if (did_post_task) {
+      // Ownership is tranferred to the next ContinueAsyncProcessDump().
+      ignore_result(pmd_async_state.release());
+      return;
     }
-  }  // AutoLock(lock_)
+    // The thread is gone. Skip the dump provider and keep going.
+    mdpinfo->disabled = true;
+  }
 
-  // Invoke the dump provider without holding the |lock_|.
-  bool finalize = false;
-  bool dump_successful = false;
+  // 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).
 
-  if (!skip_dump) {
+  if (!mdpinfo->disabled) {
+    // Invoke the dump provider.
     TRACE_EVENT_WITH_FLOW1(kTraceCategory,
                            "MemoryDumpManager::ContinueAsyncProcessDump",
-                           TRACE_ID_MANGLE(dump_guid),
+                           TRACE_ID_MANGLE(pmd_async_state->req_args.dump_guid),
                            TRACE_EVENT_FLAG_FLOW_IN | TRACE_EVENT_FLAG_FLOW_OUT,
-                           "dump_provider.name", dump_provider_name);
+                           "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};
-    ProcessMemoryDump* process_memory_dump =
-        pmd_async_state->GetOrCreateMemoryDumpContainerForProcess(pid);
-    dump_successful = mdp->OnMemoryDump(args, process_memory_dump);
-  }
+    bool dump_successful = mdpinfo->dump_provider->OnMemoryDump(args, pmd);
 
-  {
-    AutoLock lock(lock_);
-    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;
+      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.";
       }
     }
-    ++pmd_async_state->next_dump_provider;
-    finalize = pmd_async_state->next_dump_provider == dump_providers_.end();
-
-    if (mdp_info->unregistered)
-      dump_providers_.erase(mdp_info);
-  }
-
-  if (!skip_dump && !dump_successful) {
-    LOG(ERROR) << "MemoryDumpProvider \"" << dump_provider_name << "\" failed, "
-               << "possibly due to sandboxing (crbug.com/461788)."
-               << "Disabling dumper for current process. Try --no-sandbox.";
-  }
-
-  if (finalize)
-    return FinalizeDumpAndAddToTrace(std::move(pmd_async_state));
+  }  // if (!mdpinfo->disabled)
 
-  ContinueAsyncProcessDump(std::move(pmd_async_state));
+  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());
   const uint64_t dump_guid = pmd_async_state->req_args.dump_guid;
   if (!pmd_async_state->callback_task_runner->BelongsToCurrentThread()) {
     scoped_refptr<SingleThreadTaskRunner> callback_task_runner =
@@ -483,20 +463,6 @@ void MemoryDumpManager::FinalizeDumpAndAddToTrace(
                                   TRACE_ID_MANGLE(dump_guid));
 }
 
-// static
-void MemoryDumpManager::AbortDumpLocked(
-    MemoryDumpCallback callback,
-    scoped_refptr<SingleThreadTaskRunner> task_runner,
-    uint64_t dump_guid) {
-  if (callback.is_null())
-    return;  // There is nothing to NACK.
-
-  // Post the callback even if we are already on the right thread to avoid
-  // invoking the callback while holding the lock_.
-  task_runner->PostTask(FROM_HERE,
-                        Bind(callback, dump_guid, false /* success */));
-}
-
 void MemoryDumpManager::OnTraceLogEnabled() {
   bool enabled;
   TRACE_EVENT_CATEGORY_GROUP_ENABLED(kTraceCategory, &enabled);
@@ -541,11 +507,6 @@ void MemoryDumpManager::OnTraceLogEnabled() {
   session_state_ = new MemoryDumpSessionState(stack_frame_deduplicator,
                                               type_name_deduplicator);
 
-  for (auto it = dump_providers_.begin(); it != dump_providers_.end(); ++it) {
-    it->disabled = false;
-    it->consecutive_failures = 0;
-  }
-
   subtle::NoBarrier_Store(&memory_tracing_enabled_, 1);
 
   // TODO(primiano): This is a temporary hack to disable periodic memory dumps
@@ -617,31 +578,36 @@ MemoryDumpManager::MemoryDumpProviderInfo::MemoryDumpProviderInfo(
       task_runner(task_runner),
       options(options),
       consecutive_failures(0),
-      disabled(false),
-      unregistered(false) {}
+      disabled(false) {}
 
 MemoryDumpManager::MemoryDumpProviderInfo::~MemoryDumpProviderInfo() {}
 
-bool MemoryDumpManager::MemoryDumpProviderInfo::operator<(
-    const MemoryDumpProviderInfo& other) const {
-  if (task_runner == other.task_runner)
-    return dump_provider < other.dump_provider;
+bool MemoryDumpManager::MemoryDumpProviderInfo::Comparator::operator()(
+    const scoped_refptr<MemoryDumpManager::MemoryDumpProviderInfo>& a,
+    const scoped_refptr<MemoryDumpManager::MemoryDumpProviderInfo>& b) const {
+  if (!a || !b)
+    return a.get() < b.get();
   // Ensure that unbound providers (task_runner == nullptr) always run last.
-  return !(task_runner < other.task_runner);
+  // Rationale: some unbound dump providers are known to be slow, keep them last
+  // to avoid skewing timings of the other dump providers.
+  return std::tie(a->task_runner, a->dump_provider) >
+         std::tie(b->task_runner, b->dump_provider);
 }
 
 MemoryDumpManager::ProcessMemoryDumpAsyncState::ProcessMemoryDumpAsyncState(
     MemoryDumpRequestArgs req_args,
-    MemoryDumpProviderInfoSet::iterator next_dump_provider,
+    const MemoryDumpProviderInfo::OrderedSet& dump_providers,
     const scoped_refptr<MemoryDumpSessionState>& session_state,
     MemoryDumpCallback callback,
     const scoped_refptr<SingleThreadTaskRunner>& dump_thread_task_runner)
     : req_args(req_args),
-      next_dump_provider(next_dump_provider),
       session_state(session_state),
       callback(callback),
       callback_task_runner(MessageLoop::current()->task_runner()),
-      dump_thread_task_runner(dump_thread_task_runner) {}
+      dump_thread_task_runner(dump_thread_task_runner) {
+  pending_dump_providers.reserve(dump_providers.size());
+  pending_dump_providers.assign(dump_providers.rbegin(), dump_providers.rend());
+}
 
 MemoryDumpManager::ProcessMemoryDumpAsyncState::~ProcessMemoryDumpAsyncState() {
 }
diff --git a/base/trace_event/memory_dump_manager.h b/base/trace_event/memory_dump_manager.h
index a5037cf..5391411 100644
--- a/base/trace_event/memory_dump_manager.h
+++ b/base/trace_event/memory_dump_manager.h
@@ -8,6 +8,7 @@
 #include <map>
 #include <memory>
 #include <set>
+#include <vector>
 
 #include "base/atomicops.h"
 #include "base/containers/hash_tables.h"
@@ -130,42 +131,62 @@ class BASE_EXPORT MemoryDumpManager : public TraceLog::EnabledStateObserver {
   friend class MemoryDumpManagerDelegate;
   friend class MemoryDumpManagerTest;
 
-  // Descriptor struct used to hold information about registered MDPs. It is
-  // deliberately copyable, in order to allow it to be used as std::set value.
-  struct MemoryDumpProviderInfo {
+  // Descriptor used to hold information about registered MDPs.
+  // Some important considerations about lifetime of this object:
+  // - In nominal conditions, all the MemoryDumpProviderInfo instances live in
+  //   the |dump_providers_| collection (% unregistration while dumping).
+  // - Upon each dump they (actually their scoped_refptr-s) are copied into
+  //   the ProcessMemoryDumpAsyncState. This is to allow removal (see below).
+  // - When the MDP.OnMemoryDump() is invoked, the corresponding MDPInfo copy
+  //   inside ProcessMemoryDumpAsyncState is removed.
+  // - In nominal conditions, the MDPInfo is destroyed in the
+  //   UnregisterDumpProvider() call.
+  // - If UnregisterDumpProvider() is called while a dump is in progress, the
+  //   MDPInfo is destroyed in the epilogue of ContinueAsyncProcessDump(), when
+  //   the copy inside ProcessMemoryDumpAsyncState is erase()-d.
+  // - The non-const fields of MemoryDumpProviderInfo are safe to access only
+  //   in the |task_runner| thread, unless the thread has been destroyed.
+  struct MemoryDumpProviderInfo
+      : public RefCountedThreadSafe<MemoryDumpProviderInfo> {
+    // Define a total order based on the thread (i.e. |task_runner|) affinity,
+    // so that all MDP belonging to the same thread are adjacent in the set.
+    struct Comparator {
+      bool operator()(const scoped_refptr<MemoryDumpProviderInfo>& a,
+                      const scoped_refptr<MemoryDumpProviderInfo>& b) const;
+    };
+    using OrderedSet =
+        std::set<scoped_refptr<MemoryDumpProviderInfo>, Comparator>;
+
     MemoryDumpProviderInfo(
         MemoryDumpProvider* dump_provider,
         const char* name,
         const scoped_refptr<SingleThreadTaskRunner>& task_runner,
         const MemoryDumpProvider::Options& options);
-    ~MemoryDumpProviderInfo();
-
-    // Define a total order based on the thread (i.e. |task_runner|) affinity,
-    // so that all MDP belonging to the same thread are adjacent in the set.
-    bool operator<(const MemoryDumpProviderInfo& other) const;
 
     MemoryDumpProvider* const dump_provider;
+
+    // Human readable name, for debugging and testing. Not necessarily unique.
     const char* const name;
 
     // The task_runner affinity. Can be nullptr, in which case the dump provider
     // will be invoked on |dump_thread_|.
-    scoped_refptr<SingleThreadTaskRunner> task_runner;
+    const scoped_refptr<SingleThreadTaskRunner> task_runner;
 
     // The |options| arg passed to RegisterDumpProvider().
     const MemoryDumpProvider::Options options;
 
-    // For fail-safe logic (auto-disable failing MDPs). These fields are mutable
-    // as can be safely changed without impacting the order within the set.
-    mutable int consecutive_failures;
-    mutable bool disabled;
+    // For fail-safe logic (auto-disable failing MDPs).
+    int consecutive_failures;
 
-    // When a dump provider unregisters, it is flagged as |unregistered| and it
-    // is removed only upon the next memory dump. This is to avoid altering the
-    // |dump_providers_| collection while a dump is in progress.
-    mutable bool unregistered;
-  };
+    // Flagged either by the auto-disable logic or during unregistration.
+    bool disabled;
 
-  using MemoryDumpProviderInfoSet = std::set<MemoryDumpProviderInfo>;
+   private:
+    friend class base::RefCountedThreadSafe<MemoryDumpProviderInfo>;
+    ~MemoryDumpProviderInfo();
+
+    DISALLOW_COPY_AND_ASSIGN(MemoryDumpProviderInfo);
+  };
 
   // Holds the state of a process memory dump that needs to be carried over
   // across threads in order to fulfil an asynchronous CreateProcessDump()
@@ -173,7 +194,7 @@ class BASE_EXPORT MemoryDumpManager : public TraceLog::EnabledStateObserver {
   struct ProcessMemoryDumpAsyncState {
     ProcessMemoryDumpAsyncState(
         MemoryDumpRequestArgs req_args,
-        MemoryDumpProviderInfoSet::iterator next_dump_provider,
+        const MemoryDumpProviderInfo::OrderedSet& dump_providers,
         const scoped_refptr<MemoryDumpSessionState>& session_state,
         MemoryDumpCallback callback,
         const scoped_refptr<SingleThreadTaskRunner>& dump_thread_task_runner);
@@ -191,9 +212,10 @@ class BASE_EXPORT MemoryDumpManager : public TraceLog::EnabledStateObserver {
     // The arguments passed to the initial CreateProcessDump() request.
     const MemoryDumpRequestArgs req_args;
 
-    // The |dump_providers_| iterator to the next dump provider that should be
-    // invoked (or dump_providers_.end() if at the end of the sequence).
-    MemoryDumpProviderInfoSet::iterator next_dump_provider;
+    // An ordered sequence of dump providers that have to be invoked to complete
+    // the dump. This is a copy of |dump_providers_| at the beginning of a dump
+    // and becomes empty at the end, when all dump providers have been invoked.
+    std::vector<scoped_refptr<MemoryDumpProviderInfo>> pending_dump_providers;
 
     // The trace-global session state.
     scoped_refptr<MemoryDumpSessionState> session_state;
@@ -226,9 +248,6 @@ class BASE_EXPORT MemoryDumpManager : public TraceLog::EnabledStateObserver {
   static void SetInstanceForTesting(MemoryDumpManager* instance);
   static void FinalizeDumpAndAddToTrace(
       scoped_ptr<ProcessMemoryDumpAsyncState> pmd_async_state);
-  static void AbortDumpLocked(MemoryDumpCallback callback,
-                              scoped_refptr<SingleThreadTaskRunner> task_runner,
-                              uint64_t dump_guid);
 
   // Internal, used only by MemoryDumpManagerDelegate.
   // Creates a memory dump for the current process and appends it to the trace.
@@ -240,11 +259,11 @@ class BASE_EXPORT MemoryDumpManager : public TraceLog::EnabledStateObserver {
   // Continues the ProcessMemoryDump started by CreateProcessDump(), hopping
   // across threads as needed as specified by MDPs in RegisterDumpProvider().
   void ContinueAsyncProcessDump(
-      scoped_ptr<ProcessMemoryDumpAsyncState> pmd_async_state);
+      ProcessMemoryDumpAsyncState* owned_pmd_async_state);
 
   // An ordererd set of registered MemoryDumpProviderInfo(s), sorted by thread
   // affinity (MDPs belonging to the same thread are adjacent).
-  MemoryDumpProviderInfoSet dump_providers_;
+  MemoryDumpProviderInfo::OrderedSet dump_providers_;
 
   // Shared among all the PMDs to keep state scoped to the tracing session.
   scoped_refptr<MemoryDumpSessionState> session_state_;
diff --git a/base/trace_event/memory_dump_manager_unittest.cc b/base/trace_event/memory_dump_manager_unittest.cc
index 34c98b0..cf0f305 100644
--- a/base/trace_event/memory_dump_manager_unittest.cc
+++ b/base/trace_event/memory_dump_manager_unittest.cc
@@ -4,8 +4,10 @@
 
 #include "base/trace_event/memory_dump_manager.h"
 
+#include <vector>
+
 #include "base/bind_helpers.h"
-#include "base/memory/scoped_vector.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
 #include "base/strings/stringprintf.h"
@@ -202,10 +204,13 @@ TEST_F(MemoryDumpManagerTest, SingleDumper) {
   // Finally check the unregister logic: the delegate will be invoked but not
   // the dump provider, as it has been unregistered.
   EnableTracingWithLegacyCategories(MemoryDumpManager::kTraceCategory);
-  EXPECT_CALL(*delegate_, RequestGlobalMemoryDump(_, _)).Times(1);
+  EXPECT_CALL(*delegate_, RequestGlobalMemoryDump(_, _)).Times(3);
   EXPECT_CALL(mdp, OnMemoryDump(_, _)).Times(0);
-  RequestGlobalDumpAndWait(MemoryDumpType::EXPLICITLY_TRIGGERED,
-                           MemoryDumpLevelOfDetail::DETAILED);
+
+  for (int i = 0; i < 3; ++i) {
+    RequestGlobalDumpAndWait(MemoryDumpType::EXPLICITLY_TRIGGERED,
+                             MemoryDumpLevelOfDetail::DETAILED);
+  }
   DisableTracing();
 }
 
@@ -531,13 +536,14 @@ TEST_F(MemoryDumpManagerTest, UnregisterDumperWhileDumping) {
 // dumping from a different thread than the dumping thread.
 TEST_F(MemoryDumpManagerTest, UnregisterDumperFromThreadWhileDumping) {
   InitializeMemoryDumpManager(false /* is_coordinator */);
-  ScopedVector<TestIOThread> threads;
-  ScopedVector<MockMemoryDumpProvider> mdps;
+  std::vector<scoped_ptr<TestIOThread>> threads;
+  std::vector<scoped_ptr<MockMemoryDumpProvider>> mdps;
 
   for (int i = 0; i < 2; i++) {
-    threads.push_back(new TestIOThread(TestIOThread::kAutoStart));
-    mdps.push_back(new MockMemoryDumpProvider());
-    RegisterDumpProvider(mdps.back(), threads.back()->task_runner(),
+    threads.push_back(
+        make_scoped_ptr(new TestIOThread(TestIOThread::kAutoStart)));
+    mdps.push_back(make_scoped_ptr(new MockMemoryDumpProvider()));
+    RegisterDumpProvider(mdps.back().get(), threads.back()->task_runner(),
                          kDefaultOptions);
   }
 
@@ -545,10 +551,10 @@ TEST_F(MemoryDumpManagerTest, UnregisterDumperFromThreadWhileDumping) {
 
   // When OnMemoryDump is called on either of the dump providers, it will
   // unregister the other one.
-  for (MockMemoryDumpProvider* mdp : mdps) {
+  for (const scoped_ptr<MockMemoryDumpProvider>& mdp : mdps) {
     int other_idx = (mdps.front() == mdp);
-    TestIOThread* other_thread = threads[other_idx];
-    MockMemoryDumpProvider* other_mdp = mdps[other_idx];
+    TestIOThread* other_thread = threads[other_idx].get();
+    MockMemoryDumpProvider* other_mdp = mdps[other_idx].get();
     auto on_dump = [this, other_thread, other_mdp, &on_memory_dump_call_count](
         const MemoryDumpArgs& args, ProcessMemoryDump* pmd) {
       other_thread->PostTaskAndWait(
@@ -571,7 +577,54 @@ TEST_F(MemoryDumpManagerTest, UnregisterDumperFromThreadWhileDumping) {
   RequestGlobalDumpAndWait(MemoryDumpType::EXPLICITLY_TRIGGERED,
                            MemoryDumpLevelOfDetail::DETAILED);
   ASSERT_EQ(1, on_memory_dump_call_count);
-  ASSERT_EQ(true, last_callback_success_);
+  ASSERT_TRUE(last_callback_success_);
+
+  DisableTracing();
+}
+
+// If a thread (with a dump provider living on it) is torn down during a dump
+// its dump provider should be skipped but the dump itself should succeed.
+TEST_F(MemoryDumpManagerTest, TearDownThreadWhileDumping) {
+  InitializeMemoryDumpManager(false /* is_coordinator */);
+  std::vector<scoped_ptr<TestIOThread>> threads;
+  std::vector<scoped_ptr<MockMemoryDumpProvider>> mdps;
+
+  for (int i = 0; i < 2; i++) {
+    threads.push_back(
+        make_scoped_ptr(new TestIOThread(TestIOThread::kAutoStart)));
+    mdps.push_back(make_scoped_ptr(new MockMemoryDumpProvider()));
+    RegisterDumpProvider(mdps.back().get(), threads.back()->task_runner(),
+                         kDefaultOptions);
+  }
+
+  int on_memory_dump_call_count = 0;
+
+  // When OnMemoryDump is called on either of the dump providers, it will
+  // tear down the thread of the other one.
+  for (const scoped_ptr<MockMemoryDumpProvider>& mdp : mdps) {
+    int other_idx = (mdps.front() == mdp);
+    TestIOThread* other_thread = threads[other_idx].get();
+    auto on_dump = [other_thread, &on_memory_dump_call_count](
+        const MemoryDumpArgs& args, ProcessMemoryDump* pmd) {
+      other_thread->Stop();
+      on_memory_dump_call_count++;
+      return true;
+    };
+
+    // OnMemoryDump is called once for the provider that dumps first, and zero
+    // times for the other provider.
+    EXPECT_CALL(*mdp, OnMemoryDump(_, _))
+        .Times(AtMost(1))
+        .WillOnce(Invoke(on_dump));
+  }
+
+  last_callback_success_ = false;
+  EnableTracingWithLegacyCategories(MemoryDumpManager::kTraceCategory);
+  EXPECT_CALL(*delegate_, RequestGlobalMemoryDump(_, _)).Times(1);
+  RequestGlobalDumpAndWait(MemoryDumpType::EXPLICITLY_TRIGGERED,
+                           MemoryDumpLevelOfDetail::DETAILED);
+  ASSERT_EQ(1, on_memory_dump_call_count);
+  ASSERT_TRUE(last_callback_success_);
 
   DisableTracing();
 }
@@ -761,9 +814,9 @@ TEST_F(MemoryDumpManagerTest, DisableTracingWhileDumping) {
   tracing_disabled_event.Signal();
   run_loop.Run();
 
-  // RequestGlobalMemoryDump() should be NACK-ed because one of the threads
-  // threads died before we had a chance to PostTask onto them.
-  EXPECT_FALSE(last_callback_success_);
+  // RequestGlobalMemoryDump() should still suceed even if some threads were
+  // torn down during the dump.
+  EXPECT_TRUE(last_callback_success_);
 }
 
 TEST_F(MemoryDumpManagerTest, DumpOnBehalfOfOtherProcess) {