[tracing] Refactor MemoryDumpManager for upcoming multiprocess dumps

This CL introduces minimal code change with no functional behavioral
changes to the MemoryDumpManager, in particular:
 - Move the definition of structs and types used to request memory
   dumps to a separate file (memory_dump_request_args.h). Rationale:
   these structs will be used in upcoming CLs by IPCs and this header
   avoids to include the full MDM header and breaks include loops.
 - Rename the RequestDumpPoint to RequestGlobalDumpPoint, to make it
   explicit that the dump will be global.
 - Turn the internal dump request parameters into a struct
   (MemoryDumpRequestArgs). This will be serialized over IPC and serves
   as an extensions point for the future (add extra dumpe parameters
   without having to rewrite all the IPC / message filters call sites)

More context and design doc are available in the attached BUG.

BUG=462930

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

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

8 files changed, 99 insertions, 54 deletions

diff --git a/base/trace_event/BUILD.gn b/base/trace_event/BUILD.gn
index 479a918..c0c6a05 100644
--- a/base/trace_event/BUILD.gn
+++ b/base/trace_event/BUILD.gn
@@ -11,6 +11,7 @@ source_set("trace_event") {
     "memory_dump_manager.h",
     "memory_dump_provider.cc",
     "memory_dump_provider.h",
+    "memory_dump_request_args.h",
     "process_memory_dump.cc",
     "process_memory_dump.h",
     "process_memory_maps.cc",
diff --git a/base/trace_event/memory_dump_manager.cc b/base/trace_event/memory_dump_manager.cc
index 0ec5d19..ff53b71 100644
--- a/base/trace_event/memory_dump_manager.cc
+++ b/base/trace_event/memory_dump_manager.cc
@@ -23,15 +23,15 @@ const char* kTraceEventArgNames[] = {"dumps"};
 const unsigned char kTraceEventArgTypes[] = {TRACE_VALUE_TYPE_CONVERTABLE};
 StaticAtomicSequenceNumber g_next_guid;
 
-const char* DumpPointTypeToString(const DumpPointType& dump_point_type) {
-  switch (dump_point_type) {
-    case DumpPointType::TASK_BEGIN:
+const char* MemoryDumpTypeToString(const MemoryDumpType& dump_type) {
+  switch (dump_type) {
+    case MemoryDumpType::TASK_BEGIN:
       return "TASK_BEGIN";
-    case DumpPointType::TASK_END:
+    case MemoryDumpType::TASK_END:
       return "TASK_END";
-    case DumpPointType::PERIODIC_INTERVAL:
+    case MemoryDumpType::PERIODIC_INTERVAL:
       return "PERIODIC_INTERVAL";
-    case DumpPointType::EXPLICITLY_TRIGGERED:
+    case MemoryDumpType::EXPLICITLY_TRIGGERED:
       return "EXPLICITLY_TRIGGERED";
   }
   NOTREACHED();
@@ -61,6 +61,7 @@ void MemoryDumpManager::SetInstanceForTesting(MemoryDumpManager* instance) {
 
 MemoryDumpManager::MemoryDumpManager()
     : dump_provider_currently_active_(nullptr), memory_tracing_enabled_(0) {
+  g_next_guid.GetNext();  // Make sure that first guid is not zero.
 }
 
 MemoryDumpManager::~MemoryDumpManager() {
@@ -99,8 +100,10 @@ void MemoryDumpManager::UnregisterDumpProvider(MemoryDumpProvider* mdp) {
     dump_providers_enabled_.erase(it);
 }
 
-void MemoryDumpManager::RequestDumpPoint(DumpPointType dump_point_type) {
-  // TODO(primiano): this will have more logic to coordinate dump points across
+void MemoryDumpManager::RequestGlobalDump(
+    MemoryDumpType dump_type,
+    const MemoryDumpCallback& callback) {
+  // TODO(primiano): this will have more logic to coordinate memory dumps across
   // multiple processes via IPC. See crbug.com/462930.
 
   // Bail out immediately if tracing is not enabled at all.
@@ -110,21 +113,22 @@ void MemoryDumpManager::RequestDumpPoint(DumpPointType dump_point_type) {
   // TODO(primiano): Make guid actually unique (cross-process) by hashing it
   // with the PID. See crbug.com/462931 for details.
   const uint64 guid = g_next_guid.GetNext();
-  CreateLocalDumpPoint(dump_point_type, guid);
+  MemoryDumpRequestArgs args = {guid, dump_type};
+  CreateProcessDump(args);
 }
 
-void MemoryDumpManager::BroadcastDumpRequest() {
-  NOTREACHED();  // TODO(primiano): implement IPC synchronization.
+void MemoryDumpManager::RequestGlobalDump(MemoryDumpType dump_type) {
+  RequestGlobalDump(dump_type, MemoryDumpCallback());
 }
 
-// Creates a dump point for the current process and appends it to the trace.
-void MemoryDumpManager::CreateLocalDumpPoint(DumpPointType dump_point_type,
-                                             uint64 guid) {
+// Creates a memory dump for the current process and appends it to the trace.
+void MemoryDumpManager::CreateProcessDump(
+    const MemoryDumpRequestArgs& args) {
   bool did_any_provider_dump = false;
   scoped_ptr<ProcessMemoryDump> pmd(new ProcessMemoryDump());
 
-  // Serialize dump point generation so that memory dump providers don't have to
-  // deal with thread safety.
+  // 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();
@@ -145,19 +149,20 @@ void MemoryDumpManager::CreateLocalDumpPoint(DumpPointType dump_point_type,
     }
   }
 
-  // Don't create a dump point if all the dumpers failed.
+  // Don't create a memory dump if all the dumpers failed.
   if (!did_any_provider_dump)
     return;
 
   scoped_refptr<ConvertableToTraceFormat> event_value(new TracedValue());
   pmd->AsValueInto(static_cast<TracedValue*>(event_value.get()));
-  const char* const event_name = DumpPointTypeToString(dump_point_type);
+  const char* const event_name = MemoryDumpTypeToString(args.dump_type);
 
   TRACE_EVENT_API_ADD_TRACE_EVENT(
       TRACE_EVENT_PHASE_MEMORY_DUMP,
-      TraceLog::GetCategoryGroupEnabled(kTraceCategory), event_name, guid,
-      kTraceEventNumArgs, kTraceEventArgNames, kTraceEventArgTypes,
-      NULL /* arg_values */, &event_value, TRACE_EVENT_FLAG_HAS_ID);
+      TraceLog::GetCategoryGroupEnabled(kTraceCategory), event_name,
+      args.dump_guid, kTraceEventNumArgs, kTraceEventArgNames,
+      kTraceEventArgTypes, nullptr /* arg_values */, &event_value,
+      TRACE_EVENT_FLAG_HAS_ID);
 }
 
 void MemoryDumpManager::OnTraceLogEnabled() {
diff --git a/base/trace_event/memory_dump_manager.h b/base/trace_event/memory_dump_manager.h
index 04d0135..1699be5 100644
--- a/base/trace_event/memory_dump_manager.h
+++ b/base/trace_event/memory_dump_manager.h
@@ -10,6 +10,7 @@
 #include "base/atomicops.h"
 #include "base/memory/singleton.h"
 #include "base/synchronization/lock.h"
+#include "base/trace_event/memory_dump_request_args.h"
 #include "base/trace_event/trace_event.h"
 
 namespace base {
@@ -17,15 +18,6 @@ namespace trace_event {
 
 class MemoryDumpProvider;
 
-// Captures the reason why a dump point is being requested. This is to allow
-// selective enabling of dump points, filtering and post-processing.
-enum class DumpPointType {
-  TASK_BEGIN,         // Dumping memory at the beginning of a message-loop task.
-  TASK_END,           // Dumping memory at the ending of a message-loop task.
-  PERIODIC_INTERVAL,  // Dumping memory at periodic intervals.
-  EXPLICITLY_TRIGGERED,  // Non maskable dump request.
-};
-
 // 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).
@@ -43,7 +35,18 @@ class BASE_EXPORT MemoryDumpManager : public TraceLog::EnabledStateObserver {
 
   // Requests a memory dump. The dump might happen or not depending on the
   // filters and categories specified when enabling tracing.
-  void RequestDumpPoint(DumpPointType dump_point_type);
+  // 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,
+                         const MemoryDumpCallback& callback);
+
+  // 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;
@@ -67,23 +70,17 @@ class BASE_EXPORT MemoryDumpManager : public TraceLog::EnabledStateObserver {
   MemoryDumpManager();
   virtual ~MemoryDumpManager();
 
-  // Broadcasts the dump requests to the other processes.
-  void BroadcastDumpRequest();
-
-  // Creates a dump point for the current process and appends it to the trace.
-  void CreateLocalDumpPoint(DumpPointType dump_point_type, uint64 guid);
-
   std::vector<MemoryDumpProvider*> dump_providers_registered_;  // Not owned.
   std::vector<MemoryDumpProvider*> dump_providers_enabled_;     // Not owned.
 
   // TODO(primiano): this is required only until crbug.com/466121 gets fixed.
-  MemoryDumpProvider* dump_provider_currently_active_;  // Now owned.
+  MemoryDumpProvider* dump_provider_currently_active_;  // Not owned.
 
   // Protects from concurrent accesses to the |dump_providers_*|, e.g., tearing
-  // down logging while creating a dump point on another thread.
+  // down logging while creating a memory dump on another thread.
   Lock lock_;
 
-  // Optimization to avoid attempting any dump point (i.e. to not walk an empty
+  // 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_;
 
diff --git a/base/trace_event/memory_dump_manager_unittest.cc b/base/trace_event/memory_dump_manager_unittest.cc
index 65e719f..f63e487 100644
--- a/base/trace_event/memory_dump_manager_unittest.cc
+++ b/base/trace_event/memory_dump_manager_unittest.cc
@@ -70,7 +70,7 @@ TEST_F(MemoryDumpManagerTest, SingleDumper) {
   // Check that the dumper is not called if the memory category is not enabled.
   EnableTracing("foo-and-bar-but-not-memory");
   EXPECT_CALL(mdp, DumpInto(_)).Times(0);
-  mdm_->RequestDumpPoint(DumpPointType::EXPLICITLY_TRIGGERED);
+  mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED);
   DisableTracing();
 
   // Now repeat enabling the memory category and check that the dumper is
@@ -78,7 +78,7 @@ TEST_F(MemoryDumpManagerTest, SingleDumper) {
   EnableTracing(kTraceCategory);
   EXPECT_CALL(mdp, DumpInto(_)).Times(3).WillRepeatedly(Return(true));
   for (int i = 0; i < 3; ++i)
-    mdm_->RequestDumpPoint(DumpPointType::EXPLICITLY_TRIGGERED);
+    mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED);
   DisableTracing();
 
   mdm_->UnregisterDumpProvider(&mdp);
@@ -86,7 +86,7 @@ TEST_F(MemoryDumpManagerTest, SingleDumper) {
   // Finally check the unregister logic (no calls to the mdp after unregister).
   EnableTracing(kTraceCategory);
   EXPECT_CALL(mdp, DumpInto(_)).Times(0);
-  mdm_->RequestDumpPoint(DumpPointType::EXPLICITLY_TRIGGERED);
+  mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED);
   TraceLog::GetInstance()->SetDisabled();
 }
 
@@ -96,11 +96,11 @@ TEST_F(MemoryDumpManagerTest, UnregisterDumperWhileTracing) {
 
   EnableTracing(kTraceCategory);
   EXPECT_CALL(mdp, DumpInto(_)).Times(1).WillRepeatedly(Return(true));
-  mdm_->RequestDumpPoint(DumpPointType::EXPLICITLY_TRIGGERED);
+  mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED);
 
   mdm_->UnregisterDumpProvider(&mdp);
   EXPECT_CALL(mdp, DumpInto(_)).Times(0);
-  mdm_->RequestDumpPoint(DumpPointType::EXPLICITLY_TRIGGERED);
+  mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED);
 
   DisableTracing();
 }
@@ -114,7 +114,7 @@ TEST_F(MemoryDumpManagerTest, MultipleDumpers) {
   EnableTracing(kTraceCategory);
   EXPECT_CALL(mdp1, DumpInto(_)).Times(1).WillRepeatedly(Return(true));
   EXPECT_CALL(mdp2, DumpInto(_)).Times(0);
-  mdm_->RequestDumpPoint(DumpPointType::EXPLICITLY_TRIGGERED);
+  mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED);
   DisableTracing();
 
   // Invert: enable mdp1 and disable mdp2.
@@ -123,7 +123,7 @@ TEST_F(MemoryDumpManagerTest, MultipleDumpers) {
   EnableTracing(kTraceCategory);
   EXPECT_CALL(mdp1, DumpInto(_)).Times(0);
   EXPECT_CALL(mdp2, DumpInto(_)).Times(1).WillRepeatedly(Return(true));
-  mdm_->RequestDumpPoint(DumpPointType::EXPLICITLY_TRIGGERED);
+  mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED);
   DisableTracing();
 
   // Enable both mdp1 and mdp2.
@@ -131,7 +131,7 @@ TEST_F(MemoryDumpManagerTest, MultipleDumpers) {
   EnableTracing(kTraceCategory);
   EXPECT_CALL(mdp1, DumpInto(_)).Times(1).WillRepeatedly(Return(true));
   EXPECT_CALL(mdp2, DumpInto(_)).Times(1).WillRepeatedly(Return(true));
-  mdm_->RequestDumpPoint(DumpPointType::EXPLICITLY_TRIGGERED);
+  mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED);
   DisableTracing();
 }
 
@@ -148,11 +148,11 @@ TEST_F(MemoryDumpManagerTest, DisableFailingDumpers) {
 
   EXPECT_CALL(mdp1, DumpInto(_)).Times(1).WillRepeatedly(Return(false));
   EXPECT_CALL(mdp2, DumpInto(_)).Times(1).WillRepeatedly(Return(true));
-  mdm_->RequestDumpPoint(DumpPointType::EXPLICITLY_TRIGGERED);
+  mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED);
 
   EXPECT_CALL(mdp1, DumpInto(_)).Times(0);
   EXPECT_CALL(mdp2, DumpInto(_)).Times(1).WillRepeatedly(Return(false));
-  mdm_->RequestDumpPoint(DumpPointType::EXPLICITLY_TRIGGERED);
+  mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED);
 
   DisableTracing();
 }
@@ -177,8 +177,8 @@ TEST_F(MemoryDumpManagerTest, ActiveDumpProviderConsistency) {
       .WillRepeatedly(Invoke(
           &mdp2,
           &MockDumpProvider::DumpIntoAndCheckDumpProviderCurrentlyActive));
-  mdm_->RequestDumpPoint(DumpPointType::EXPLICITLY_TRIGGERED);
-  mdm_->RequestDumpPoint(DumpPointType::EXPLICITLY_TRIGGERED);
+  mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED);
+  mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED);
   DisableTracing();
 }
 
diff --git a/base/trace_event/memory_dump_provider.h b/base/trace_event/memory_dump_provider.h
index 5cc3a6e..62d562c 100644
--- a/base/trace_event/memory_dump_provider.h
+++ b/base/trace_event/memory_dump_provider.h
@@ -16,7 +16,7 @@ class ProcessMemoryDump;
 // The contract interface that memory dump providers must implement.
 class BASE_EXPORT MemoryDumpProvider {
  public:
-  // Called by the MemoryDumpManager when generating dump points.
+  // Called by the MemoryDumpManager when generating memory dumps.
   // Returns: true if the |pmd| was successfully populated, false otherwise.
   virtual bool DumpInto(ProcessMemoryDump* pmd) = 0;
 
diff --git a/base/trace_event/memory_dump_request_args.h b/base/trace_event/memory_dump_request_args.h
new file mode 100644
index 0000000..fe7b0b9
--- /dev/null
+++ b/base/trace_event/memory_dump_request_args.h
@@ -0,0 +1,41 @@
+// 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_REQUEST_H_
+#define BASE_TRACE_EVENT_MEMORY_DUMP_REQUEST_H_
+
+// This file defines the types and structs used to issue memory dump requests.
+// These are also used in the IPCs for coordinating inter-process memory dumps.
+
+#include "base/base_export.h"
+#include "base/callback.h"
+
+namespace base {
+namespace trace_event {
+
+// Captures the reason why a memory dump is being requested. This is to allow
+// selective enabling of dumps, filtering and post-processing.
+enum class MemoryDumpType {
+  TASK_BEGIN,         // Dumping memory at the beginning of a message-loop task.
+  TASK_END,           // Dumping memory at the ending of a message-loop task.
+  PERIODIC_INTERVAL,  // Dumping memory at periodic intervals.
+  EXPLICITLY_TRIGGERED,  // Non maskable dump request.
+  LAST = EXPLICITLY_TRIGGERED // For IPC macros.
+};
+
+using MemoryDumpCallback = Callback<void(uint64 dump_guid, bool status)>;
+
+struct BASE_EXPORT MemoryDumpRequestArgs {
+  // Globally unique identifier. In multi-process dumps, all processes issue a
+  // local dump with the same guid. This allows the trace importers to
+  // reconstruct the global dump.
+  uint64 dump_guid;
+
+  MemoryDumpType dump_type;
+};
+
+}  // namespace trace_event
+}  // namespace base
+
+#endif  // BASE_TRACE_EVENT_MEMORY_DUMP_REQUEST_H_
diff --git a/base/trace_event/process_memory_dump.h b/base/trace_event/process_memory_dump.h
index df6cc82..b59ae67 100644
--- a/base/trace_event/process_memory_dump.h
+++ b/base/trace_event/process_memory_dump.h
@@ -20,7 +20,7 @@ class ConvertableToTraceFormat;
 class MemoryDumpManager;
 
 // ProcessMemoryDump is as a strongly typed container which enforces the data
-// model for each memory dump point and holds the dumps produced by the
+// model for each memory dump and holds the dumps produced by the
 // MemoryDumpProvider(s) for a specific process.
 // At trace generation time (i.e. when AsValue() is called), ProcessMemoryDump
 // will compose a key-value dictionary of the various dumps obtained at trace
diff --git a/base/trace_event/trace_event.gypi b/base/trace_event/trace_event.gypi
index ca6c076..36e3ab3 100644
--- a/base/trace_event/trace_event.gypi
+++ b/base/trace_event/trace_event.gypi
@@ -11,6 +11,7 @@
       'trace_event/memory_dump_manager.h',
       'trace_event/memory_dump_provider.cc',
       'trace_event/memory_dump_provider.h',
+      'trace_event/memory_dump_request_args.h',
       'trace_event/process_memory_dump.cc',
       'trace_event/process_memory_dump.h',
       'trace_event/process_memory_maps.cc',