[tracing] Allow asynchronous unregistration of unbound dump providers

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..97427212a9c1cb11db349ed55af630e95d409175 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"
@@ -80,6 +81,15 @@ class BASE_EXPORT MemoryDumpManager : public TraceLog::EnabledStateObserver {
       const MemoryDumpProvider::Options& options);
   void UnregisterDumpProvider(MemoryDumpProvider* mdp);
 
+  // Unregisters an unbound (no task_runner affinity) dump provider and takes
+  // care about its deletion asynchronously.
+  // Can be used only for for dump providers with no task-runner affinity.
+  // This method takes ownership of the dump provider and guarantees that:
+  //  - The |mdp| will be deleted at some point in the near future.
+  //  - Its deletion will not happen concurrently with the OnMemoryDump() call.

[Ruud van Asseldonk, 2015/12/14 16:25:49]

/s/concurrently/in parallel/?

+  // Note that OnMemoryDump() calls can still happen after this method returns.
+  void UnregisterAndDeleteDumpProviderAsync(scoped_ptr<MemoryDumpProvider> mdp);

[Ruud van Asseldonk, 2015/12/14 16:25:48]

This feels like an awkward API. It looks like what you really want is shared
ownership; the memory dump manager must be able to keep the dump provider alive.
Would it be possible to make |MemoryDumpProvider| atomically refcounted instead?

+
   // Requests a memory dump. The dump might happen or not depending on the
   // filters and categories specified when enabling tracing.
   // The optional |callback| is executed asynchronously, on an arbitrary thread,
@@ -226,9 +236,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,7 +247,10 @@ 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);
+
+  void UnregisterDumpProviderInternal(MemoryDumpProvider* mdp,
+                                      bool delete_async);

[Ruud van Asseldonk, 2015/12/14 16:25:48]

This is ambiguous; does |delete_async = false| mean "delete synchronously" or
does it mean "do not delete at all"?

Instead of taking a boolean argument, consider having an enum

    enum class DeletionMode {
      kDoNotDelete,
      kDeleteAsync
    };

It is more readable at the call site too.

 
   // An ordererd set of registered MemoryDumpProviderInfo(s), sorted by thread
   // affinity (MDPs belonging to the same thread are adjacent).
@@ -254,6 +264,12 @@ class BASE_EXPORT MemoryDumpManager : public TraceLog::EnabledStateObserver {
   // When true, this instance is in charge of coordinating periodic dumps.
   bool is_coordinator_;
 
+  // Contains one entry per each dump that has been started (via

[Ruud van Asseldonk, 2015/12/14 16:25:49]

Nit: "per" or "for each", not "per each".

+  // CreateProcessDump()) and is not completed yet.
+  // Only the const members of the stores ProcessMemoryDumpAsyncState are safe

[Ruud van Asseldonk, 2015/12/14 16:25:49]

/s/stores/stored/?

+  // to read and only while holding the |lock_|.

[Ruud van Asseldonk, 2015/12/14 16:25:49]

That is a violation of the general |const| contract. Since C++11 |const| means
thread safe (bitwise const or internally synchronised).

This talk by Herb Sutter explains it in detail:

https://channel9.msdn.com/posts/C-and-Beyond-2012-Herb-Sutter-You-dont-know-b...

Also, does this mean that the non-const members are _never_ safe to read?

+  std::vector<const ProcessMemoryDumpAsyncState*> outstanding_dumps_;
+
   // Protects from concurrent accesses to the |dump_providers_*| and |delegate_|
   // to guard against disabling logging while dumping on another thread.
   Lock lock_;