[tracing] Non-functional refactor of MemoryDumpManager

This CL is a refactor of MemoryDumpManager in light of the upcoming
changes to support asynchronous invocations of MemoryDumpProvider(s)
on client-specified threads.
This CL does't introduce any new function. The only minimal change
is turning the list of registered MDP(s) into a hash_set (which
contributes to simplify the design of the MDM).
The MDM never guaranteed to invoke MDP in order and it now makes even
less sense, in light of upcoming thread hops.

BUG=478008

Review URL: https://codereview.chromium.org/1087403004

Cr-Commit-Position: refs/heads/master@{#325653}

4 files changed, 102 insertions, 67 deletions

diff --git a/base/trace_event/memory_dump_manager.cc b/base/trace_event/memory_dump_manager.cc
index 2c4b18e..f59b3ec 100644
--- a/base/trace_event/memory_dump_manager.cc
+++ b/base/trace_event/memory_dump_manager.cc
@@ -17,6 +17,10 @@ namespace trace_event {
 
 namespace {
 
+// TODO(primiano): this should be smarter and should do something similar to
+// trace event synthetic delays.
+const char kTraceCategory[] = TRACE_DISABLED_BY_DEFAULT("memory-dumps");
+
 MemoryDumpManager* g_instance_for_testing = nullptr;
 const int kTraceEventNumArgs = 1;
 const char* kTraceEventArgNames[] = {"dumps"};
@@ -38,12 +42,45 @@ const char* MemoryDumpTypeToString(const MemoryDumpType& dump_type) {
   return "UNKNOWN";
 }
 
+// Internal class used to hold details about ProcessMemoryDump requests for the
+// current process.
+// TODO(primiano): In the upcoming CLs, ProcessMemoryDump will become async.
+// and this class will be used to convey more details across PostTask()s.
+class ProcessMemoryDumpHolder
+    : public RefCountedThreadSafe<ProcessMemoryDumpHolder> {
+ public:
+  ProcessMemoryDumpHolder(MemoryDumpRequestArgs req_args)
+      : req_args(req_args) {}
+
+  ProcessMemoryDump process_memory_dump;
+  const MemoryDumpRequestArgs req_args;
+
+ private:
+  friend class RefCountedThreadSafe<ProcessMemoryDumpHolder>;
+  virtual ~ProcessMemoryDumpHolder() {}
+  DISALLOW_COPY_AND_ASSIGN(ProcessMemoryDumpHolder);
+};
+
+void FinalizeDumpAndAddToTrace(
+    const scoped_refptr<ProcessMemoryDumpHolder>& pmd_holder) {
+  scoped_refptr<ConvertableToTraceFormat> event_value(new TracedValue());
+  pmd_holder->process_memory_dump.AsValueInto(
+      static_cast<TracedValue*>(event_value.get()));
+  const char* const event_name =
+      MemoryDumpTypeToString(pmd_holder->req_args.dump_type);
+
+  TRACE_EVENT_API_ADD_TRACE_EVENT(
+      TRACE_EVENT_PHASE_MEMORY_DUMP,
+      TraceLog::GetCategoryGroupEnabled(kTraceCategory), event_name,
+      pmd_holder->req_args.dump_guid, kTraceEventNumArgs, kTraceEventArgNames,
+      kTraceEventArgTypes, nullptr /* arg_values */, &event_value,
+      TRACE_EVENT_FLAG_HAS_ID);
+}
+
 }  // namespace
 
-// TODO(primiano): this should be smarter and should do something similar to
-// trace event synthetic delays.
-const char MemoryDumpManager::kTraceCategory[] =
-    TRACE_DISABLED_BY_DEFAULT("memory-dumps");
+// static
+const char* const MemoryDumpManager::kTraceCategoryForTesting = kTraceCategory;
 
 // static
 MemoryDumpManager* MemoryDumpManager::GetInstance() {
@@ -83,29 +120,15 @@ void MemoryDumpManager::SetDelegate(MemoryDumpManagerDelegate* delegate) {
 
 void MemoryDumpManager::RegisterDumpProvider(MemoryDumpProvider* mdp) {
   AutoLock lock(lock_);
-  if (std::find(dump_providers_registered_.begin(),
-                dump_providers_registered_.end(),
-                mdp) != dump_providers_registered_.end()) {
-    return;
-  }
-  dump_providers_registered_.push_back(mdp);
+  dump_providers_registered_.insert(mdp);
 }
 
 void MemoryDumpManager::UnregisterDumpProvider(MemoryDumpProvider* mdp) {
   AutoLock lock(lock_);
-
-  // Remove from the registered providers list.
-  auto it = std::find(dump_providers_registered_.begin(),
-                      dump_providers_registered_.end(), mdp);
-  if (it != dump_providers_registered_.end())
-    dump_providers_registered_.erase(it);
-
   // Remove from the enabled providers list. This is to deal with the case that
   // UnregisterDumpProvider is called while the trace is enabled.
-  it = std::find(dump_providers_enabled_.begin(), dump_providers_enabled_.end(),
-                 mdp);
-  if (it != dump_providers_enabled_.end())
-    dump_providers_enabled_.erase(it);
+  dump_providers_enabled_.erase(mdp);
+  dump_providers_registered_.erase(mdp);
 }
 
 void MemoryDumpManager::RequestGlobalDump(
@@ -141,47 +164,48 @@ void MemoryDumpManager::RequestGlobalDump(MemoryDumpType dump_type) {
 }
 
 // Creates a memory dump for the current process and appends it to the trace.
-void MemoryDumpManager::CreateProcessDump(
-    const MemoryDumpRequestArgs& args) {
+void MemoryDumpManager::CreateProcessDump(const MemoryDumpRequestArgs& args) {
+  scoped_refptr<ProcessMemoryDumpHolder> pmd_holder(
+      new ProcessMemoryDumpHolder(args));
+  ProcessMemoryDump* pmd = &pmd_holder->process_memory_dump;
   bool did_any_provider_dump = false;
-  scoped_ptr<ProcessMemoryDump> pmd(new ProcessMemoryDump());
 
   // Serialize dump generation so that memory dump providers don't have to deal
   // with thread safety.
   {
     AutoLock lock(lock_);
-    for (auto it = dump_providers_enabled_.begin();
-         it != dump_providers_enabled_.end();) {
-      dump_provider_currently_active_ = *it;
-      if (dump_provider_currently_active_->DumpInto(pmd.get())) {
-        did_any_provider_dump = true;
-        ++it;
-      } else {
-        LOG(ERROR) << "The memory dumper "
-                   << dump_provider_currently_active_->GetFriendlyName()
-                   << " failed, possibly due to sandboxing (crbug.com/461788), "
-                      "disabling it for current process. Try restarting chrome "
-                      "with the --no-sandbox switch.";
-        it = dump_providers_enabled_.erase(it);
-      }
-      dump_provider_currently_active_ = nullptr;
+    for (auto dump_provider_iter = dump_providers_enabled_.begin();
+         dump_provider_iter != dump_providers_enabled_.end();) {
+      // InvokeDumpProviderLocked will remove the MDP from the set if it fails.
+      MemoryDumpProvider* mdp = *dump_provider_iter;
+      ++dump_provider_iter;
+      // Invoke the dump provider synchronously.
+      did_any_provider_dump |= InvokeDumpProviderLocked(mdp, pmd);
     }
-  }
-
-  // Don't create a memory dump if all the dumpers failed.
-  if (!did_any_provider_dump)
-    return;
+  }  // AutoLock
 
-  scoped_refptr<ConvertableToTraceFormat> event_value(new TracedValue());
-  pmd->AsValueInto(static_cast<TracedValue*>(event_value.get()));
-  const char* const event_name = MemoryDumpTypeToString(args.dump_type);
+  // If at least one synchronous provider did dump, add the dump to the trace.
+  if (did_any_provider_dump)
+    FinalizeDumpAndAddToTrace(pmd_holder);
+}
 
-  TRACE_EVENT_API_ADD_TRACE_EVENT(
-      TRACE_EVENT_PHASE_MEMORY_DUMP,
-      TraceLog::GetCategoryGroupEnabled(kTraceCategory), event_name,
-      args.dump_guid, kTraceEventNumArgs, kTraceEventArgNames,
-      kTraceEventArgTypes, nullptr /* arg_values */, &event_value,
-      TRACE_EVENT_FLAG_HAS_ID);
+// Invokes the MemoryDumpProvider.DumpInto(), taking care of the failsafe logic
+// which disables the dumper when failing (crbug.com/461788). The caller must
+// hold the |lock_| when invoking this.
+bool MemoryDumpManager::InvokeDumpProviderLocked(MemoryDumpProvider* mdp,
+                                                 ProcessMemoryDump* pmd) {
+  lock_.AssertAcquired();
+  dump_provider_currently_active_ = mdp;
+  bool dump_successful = mdp->DumpInto(pmd);
+  dump_provider_currently_active_ = nullptr;
+  if (!dump_successful) {
+    LOG(ERROR) << "The memory dumper " << mdp->GetFriendlyName()
+               << " failed, possibly due to sandboxing (crbug.com/461788), "
+                  "disabling it for current process. Try restarting chrome "
+                  "with the --no-sandbox switch.";
+    dump_providers_enabled_.erase(mdp);
+  }
+  return dump_successful;
 }
 
 void MemoryDumpManager::OnTraceLogEnabled() {
@@ -193,8 +217,7 @@ void MemoryDumpManager::OnTraceLogEnabled() {
 
   AutoLock lock(lock_);
   if (enabled) {
-    dump_providers_enabled_.assign(dump_providers_registered_.begin(),
-                                   dump_providers_registered_.end());
+    dump_providers_enabled_ = dump_providers_registered_;
   } else {
     dump_providers_enabled_.clear();
   }
diff --git a/base/trace_event/memory_dump_manager.h b/base/trace_event/memory_dump_manager.h
index 6415cef..9490169 100644
--- a/base/trace_event/memory_dump_manager.h
+++ b/base/trace_event/memory_dump_manager.h
@@ -8,6 +8,8 @@
 #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/trace_event/memory_dump_request_args.h"
@@ -18,12 +20,15 @@ namespace trace_event {
 
 class MemoryDumpManagerDelegate;
 class MemoryDumpProvider;
+class ProcessMemoryDump;
 
 // This is the interface exposed to the rest of the codebase to deal with
 // memory tracing. The main entry point for clients is represented by
 // RequestDumpPoint(). The extension by Un(RegisterDumpProvider).
 class BASE_EXPORT MemoryDumpManager : public TraceLog::EnabledStateObserver {
  public:
+  static const char* const kTraceCategoryForTesting;
+
   static MemoryDumpManager* GetInstance();
 
   // Invoked once per process to register the TraceLog observer.
@@ -34,7 +39,7 @@ class BASE_EXPORT MemoryDumpManager : public TraceLog::EnabledStateObserver {
   void SetDelegate(MemoryDumpManagerDelegate* delegate);
 
   // MemoryDumpManager does NOT take memory ownership of |mdp|, which is
-  // expected to be a singleton.
+  // expected to either be a singleton or unregister itself.
   void RegisterDumpProvider(MemoryDumpProvider* mdp);
   void UnregisterDumpProvider(MemoryDumpProvider* mdp);
 
@@ -50,9 +55,6 @@ class BASE_EXPORT MemoryDumpManager : public TraceLog::EnabledStateObserver {
   // Same as above, but without callback.
   void RequestGlobalDump(MemoryDumpType dump_type);
 
-  // Creates a memory dump for the current process and appends it to the trace.
-  void CreateProcessDump(const MemoryDumpRequestArgs& args);
-
   // TraceLog::EnabledStateObserver implementation.
   void OnTraceLogEnabled() override;
   void OnTraceLogDisabled() override;
@@ -66,17 +68,23 @@ class BASE_EXPORT MemoryDumpManager : public TraceLog::EnabledStateObserver {
  private:
   friend struct DefaultDeleter<MemoryDumpManager>;  // For the testing instance.
   friend struct DefaultSingletonTraits<MemoryDumpManager>;
+  friend class MemoryDumpManagerDelegate;
   friend class MemoryDumpManagerTest;
 
-  static const char kTraceCategory[];
-
   static void SetInstanceForTesting(MemoryDumpManager* instance);
 
   MemoryDumpManager();
   virtual ~MemoryDumpManager();
 
-  std::vector<MemoryDumpProvider*> dump_providers_registered_;  // Not owned.
-  std::vector<MemoryDumpProvider*> dump_providers_enabled_;     // Not owned.
+  // Internal, used only by MemoryDumpManagerDelegate.
+  // Creates a memory dump for the current process and appends it to the trace.
+  void CreateProcessDump(const MemoryDumpRequestArgs& args);
+
+  bool InvokeDumpProviderLocked(MemoryDumpProvider* mdp,
+                                ProcessMemoryDump* pmd);
+
+  hash_set<MemoryDumpProvider*> dump_providers_registered_;  // Not owned.
+  hash_set<MemoryDumpProvider*> dump_providers_enabled_;     // Not owned.
 
   // TODO(primiano): this is required only until crbug.com/466121 gets fixed.
   MemoryDumpProvider* dump_provider_currently_active_;  // Not owned.
@@ -105,6 +113,10 @@ class BASE_EXPORT MemoryDumpManagerDelegate {
   MemoryDumpManagerDelegate() {}
   virtual ~MemoryDumpManagerDelegate() {}
 
+  void CreateProcessDump(const MemoryDumpRequestArgs& args) {
+    MemoryDumpManager::GetInstance()->CreateProcessDump(args);
+  }
+
  private:
   DISALLOW_COPY_AND_ASSIGN(MemoryDumpManagerDelegate);
 };
diff --git a/base/trace_event/memory_dump_manager_unittest.cc b/base/trace_event/memory_dump_manager_unittest.cc
index 0ae9568..f7ad195 100644
--- a/base/trace_event/memory_dump_manager_unittest.cc
+++ b/base/trace_event/memory_dump_manager_unittest.cc
@@ -23,7 +23,7 @@ class MemoryDumpManagerDelegateForTesting : public MemoryDumpManagerDelegate {
   void RequestGlobalMemoryDump(
       const base::trace_event::MemoryDumpRequestArgs& args,
       const MemoryDumpCallback& callback) override {
-    MemoryDumpManager::GetInstance()->CreateProcessDump(args);
+    CreateProcessDump(args);
   }
 };
 
@@ -44,7 +44,7 @@ class MemoryDumpManagerTest : public testing::Test {
   }
 
  protected:
-  const char* const kTraceCategory = MemoryDumpManager::kTraceCategory;
+  const char* kTraceCategory = MemoryDumpManager::kTraceCategoryForTesting;
 
   void EnableTracing(const char* category) {
     TraceLog::GetInstance()->SetEnabled(
@@ -66,7 +66,7 @@ class MockDumpProvider : public MemoryDumpProvider {
  public:
   MOCK_METHOD1(DumpInto, bool(ProcessMemoryDump* pmd));
 
-  // DumpInto() override for the ActiveDumpProviderConsistency test,
+  // DumpInto() override for the ActiveDumpProviderConsistency test.
   bool DumpIntoAndCheckDumpProviderCurrentlyActive(ProcessMemoryDump* pmd) {
     EXPECT_EQ(
         this,
diff --git a/base/trace_event/memory_dump_request_args.h b/base/trace_event/memory_dump_request_args.h
index fe7b0b9..4fb0335 100644
--- a/base/trace_event/memory_dump_request_args.h
+++ b/base/trace_event/memory_dump_request_args.h
@@ -24,7 +24,7 @@ enum class MemoryDumpType {
   LAST = EXPLICITLY_TRIGGERED // For IPC macros.
 };
 
-using MemoryDumpCallback = Callback<void(uint64 dump_guid, bool status)>;
+using MemoryDumpCallback = Callback<void(uint64 dump_guid, bool success)>;
 
 struct BASE_EXPORT MemoryDumpRequestArgs {
   // Globally unique identifier. In multi-process dumps, all processes issue a