Allow unregistering MemoryDumpProviders during dump

base/trace_event/memory_dump_manager.h

 // 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.
 
 #ifndef BASE_TRACE_EVENT_MEMORY_DUMP_MANAGER_H_
 #define BASE_TRACE_EVENT_MEMORY_DUMP_MANAGER_H_
 
 #include <set>
 
 #include "base/atomicops.h"
@@ skipping 85 matching lines @@
   const char* system_allocator_pool_name() const {
     return system_allocator_pool_name_;
   };
 
  private:
   friend struct DefaultDeleter<MemoryDumpManager>;  // For the testing instance.
   friend struct DefaultSingletonTraits<MemoryDumpManager>;
   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.
   struct MemoryDumpProviderInfo {
     MemoryDumpProviderInfo(
         MemoryDumpProvider* dump_provider,
         const scoped_refptr<SingleThreadTaskRunner>& task_runner);
     ~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;
+    bool operator==(const MemoryDumpProviderInfo& other) const;
 
     MemoryDumpProvider* const dump_provider;
     scoped_refptr<SingleThreadTaskRunner> task_runner;  // Optional.
 
     // 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;
+
+    // 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>;
 
   // Holds the state of a process memory dump that needs to be carried over
   // across threads in order to fulfil an asynchronous CreateProcessDump()
   // request. At any time exactly one thread owns a ProcessMemoryDumpAsyncState.
   struct ProcessMemoryDumpAsyncState {
     ProcessMemoryDumpAsyncState(
         MemoryDumpRequestArgs req_args,
@@ skipping 46 matching lines @@
 
   // 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);
 
   // An ordererd set of registered MemoryDumpProviderInfo(s), sorted by thread
   // 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_;
 
   MemoryDumpManagerDelegate* delegate_;  // Not owned.
 
   // Protects from concurrent accesses to the |dump_providers_*| and |delegate_|
   // to guard against disabling logging while dumping on another thread.
   Lock lock_;
 
   // Optimization to avoid attempting any memory dump (i.e. to not walk an empty
@@ skipping 41 matching lines @@
   }
 
  private:
   DISALLOW_COPY_AND_ASSIGN(MemoryDumpManagerDelegate);
 };
 
 }  // namespace trace_event
 }  // namespace base
 
 #endif  // BASE_TRACE_EVENT_MEMORY_DUMP_MANAGER_H_