[tracing] Simplify logic of MemoryDumpManager

base/trace_event/memory_dump_manager.h

Index: base/trace_event/memory_dump_manager.h
diff --git a/base/trace_event/memory_dump_manager.h b/base/trace_event/memory_dump_manager.h
index a5037cfcef21aa1afa42f3196ce550ddec6a9034..539141158c65706822bda710d570b374e889c5f6 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_;