[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..92eda1cf073b3b8435b11b819446a1fd9dd7bf01 100644
--- a/base/trace_event/memory_dump_manager.h
+++ b/base/trace_event/memory_dump_manager.h
@@ -5,6 +5,7 @@
 #ifndef BASE_TRACE_EVENT_MEMORY_DUMP_MANAGER_H_
 #define BASE_TRACE_EVENT_MEMORY_DUMP_MANAGER_H_
 
+#include <deque>
 #include <map>
 #include <memory>
 #include <set>
@@ -130,42 +131,64 @@ 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.
+  //
+  // As a consequence, the following rules apply for a safe use of MDPInfo:
+  // - It is safe to acquire and release MDPInfo scoped_refptrs only under the
+  //   |lock_|. This is to avoid racing with changes to |dump_providers_|.
+  // - It is safe to read the const fields without holind the |lock_|. Changes

[ssid, 2015/12/17 22:09:36]

s/holind/holding/

[ssid, 2015/12/17 22:09:36]

1. It is not safe to access the mdp pointer. Even if the mdp pointer is const
member, the object could have been destroyed while the pointer is just kept
around. The comment is misleading when it says const members can be accessed.

2. Currently the code assumes: Only thread-associated dump providers can be
unregistered. Only that thread can dump or unregister the dump provider should
be able to access or change the mdp_info.
So, there is no case where the dump happens when mdp_info is changed.
Lock would not be needed to change the members of mdp_info, when only one thread
can actually change it.

The only reason I see for the lock is the disabled check that happens in
previous thread and thread hop is avoided. It is easier to remove lock and do
thread hop before this check. It will be rare case where this thread hop goes
waste since the dump provider got un-registered after dump started. If such a
thread is registering and unregistering providers then a thread hop to that
thread would anyway happen for next provider.
The exception is when the PostTask fails, then it is safe to set it disabled
with holding the lock. This will be only case lock will be needed and used
rarely.

This would reduce locking and unlocking while dumping.

[Primiano Tucci (use gerrit), 2015/12/18 12:00:31]

Done.

[Primiano Tucci (use gerrit), 2015/12/18 12:00:31]

Absolutely correct. Added another point.
But this is the case today, isn't it? At most one ContinueAsyncProcessDump can
be executed (on random threads) per time, so that it intrinsically safe (and has
lock regions before and after).
You have to use the lock if you write fields in other parts of MDM.
Ok I see your point. Let me try to rewrite that part. I think you are right and
locking can be further avoided.
Yeah agree this is rare. Thanks for the feedback.

Let me give a try

+  //   to the MDPInfo must be locked, as the (Un)RegisterDumpprovider()
+  //   methods and the ContinueAsyncProcessDump can happen concurrently.
+  struct MemoryDumpProviderInfo : public RefCounted<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;
     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;
+
+   private:
+    friend class base::RefCounted<MemoryDumpProviderInfo>;
+    ~MemoryDumpProviderInfo();
 
-  using MemoryDumpProviderInfoSet = std::set<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 +196,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 +214,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::deque<scoped_refptr<MemoryDumpProviderInfo>> pending_dump_providers;
 
     // The trace-global session state.
     scoped_refptr<MemoryDumpSessionState> session_state;
@@ -226,9 +250,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 +261,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_;