[tracing] Allow asynchronous unregistration of unbound dump providers

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 <map>
 #include <memory>
 #include <set>
+#include <vector>
 
 #include "base/atomicops.h"
 #include "base/containers/hash_tables.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/singleton.h"
 #include "base/synchronization/lock.h"
 #include "base/timer/timer.h"
 #include "base/trace_event/memory_dump_request_args.h"
 #include "base/trace_event/process_memory_dump.h"
 #include "base/trace_event/trace_event.h"
@@ skipping 52 matching lines @@
       MemoryDumpProvider* mdp,
       const char* name,
       const scoped_refptr<SingleThreadTaskRunner>& task_runner);
   void RegisterDumpProvider(
       MemoryDumpProvider* mdp,
       const char* name,
       const scoped_refptr<SingleThreadTaskRunner>& task_runner,
       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,
   // to notify about the completion of the global dump (i.e. after all the
   // processes have dumped) and its success (true iff all the dumps were
   // successful).
   void RequestGlobalDump(MemoryDumpType dump_type,
                          MemoryDumpLevelOfDetail level_of_detail,
                          const MemoryDumpCallback& callback);
 
@@ skipping 126 matching lines @@
 
   static const int kMaxConsecutiveFailuresCount;
   static const char* const kSystemAllocatorPoolName;
 
   MemoryDumpManager();
   ~MemoryDumpManager() override;
 
   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.
   // |callback| will be invoked asynchronously upon completion on the same
   // thread on which CreateProcessDump() was called.
   void CreateProcessDump(const MemoryDumpRequestArgs& args,
                          const MemoryDumpCallback& callback);
 
   // 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).
   MemoryDumpProviderInfoSet dump_providers_;
 
   // Shared among all the PMDs to keep state scoped to the tracing session.
   scoped_refptr<MemoryDumpSessionState> session_state_;
 
   MemoryDumpManagerDelegate* delegate_;  // Not owned.
 
   // 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_;
 
   // Optimization to avoid attempting any memory dump (i.e. to not walk an empty
   // dump_providers_enabled_ list) when tracing is not enabled.
   subtle::AtomicWord memory_tracing_enabled_;
 
   // For time-triggered periodic dumps.
   RepeatingTimer periodic_dump_timer_;
@@ skipping 35 matching lines @@
   }
 
  private:
   DISALLOW_COPY_AND_ASSIGN(MemoryDumpManagerDelegate);
 };
 
 }  // namespace trace_event
 }  // namespace base
 
 #endif  // BASE_TRACE_EVENT_MEMORY_DUMP_MANAGER_H_