Allow unregistering MemoryDumpProviders during dump

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 5c75e4e82e882b3e0f49b63f72ffc094496106e8..349f7dbe21fabb3d2193e5133d17eaedd113fc66 100644
--- a/base/trace_event/memory_dump_manager.h
+++ b/base/trace_event/memory_dump_manager.h
@@ -103,6 +103,8 @@ class BASE_EXPORT MemoryDumpManager : public TraceLog::EnabledStateObserver {
   friend class MemoryDumpManagerDelegate;
   friend class MemoryDumpManagerTest;
   FRIEND_TEST_ALL_PREFIXES(MemoryDumpManagerTest, DisableFailingDumpers);
+  FRIEND_TEST_ALL_PREFIXES(MemoryDumpManagerTest,
+                           UnregisterDumperFromThreadWhileDumping);
 
   // 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.
@@ -115,6 +117,7 @@ class BASE_EXPORT MemoryDumpManager : public TraceLog::EnabledStateObserver {
     // 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;
+    bool operator==(const MemoryDumpProviderInfo& other) const;
 
     MemoryDumpProvider* const dump_provider;
     scoped_refptr<SingleThreadTaskRunner> task_runner;  // Optional.
@@ -123,6 +126,11 @@ class BASE_EXPORT MemoryDumpManager : public TraceLog::EnabledStateObserver {
     // as can be safely changed without impacting the order within the set.
     mutable int consecutive_failures;
     mutable bool disabled;
+
+    // When unregistering, we do not remove the provider immediately because a
+    // dump may be happening which depends on the set order. Instead, we clean
+    // up the set after the dump.
+    mutable bool unregistered;

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

I think you don't need this to be mutable.

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

I does need to be mutable, for the same reason that `disabled` and
`consecutive_failures` are; you cannot modify the entry through an iterator, the
iterator is const because changing an element might affect the ordering (in this
case it does not). The normal approach is to erase the entry, modify it and then
insert it again, but the point of the variable is to avoid modifying the set.

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

Ahh right.

   };
 
   using MemoryDumpProviderInfoSet = std::set<MemoryDumpProviderInfo>;
@@ -189,10 +197,6 @@ class BASE_EXPORT MemoryDumpManager : public TraceLog::EnabledStateObserver {
   // affinity (MDPs belonging to the same thread are adjacent).
   MemoryDumpProviderInfoSet dump_providers_;
 
-  // Flag used to signal that some provider was removed from |dump_providers_|
-  // and therefore the current memory dump (if any) should be aborted.
-  bool did_unregister_dump_provider_;
-
   // Shared among all the PMDs to keep state scoped to the tracing session.
   scoped_refptr<MemoryDumpSessionState> session_state_;