memory-infra: plumb whether to append to trace or not into CoordinatorImpl

Currently the RequestGlobalDump has an ambiguous dual semantic: return results
or add to the trace. For historical reasons both semantics have been collapsed 
in the same function, but this is now getting too hairy, making the codebase hard
to reason about, and caused some bugs.

Splitting the functions names as they clearly achieve two different functions
with different outcomes, specifically:
When adding the dump to the trace, clients don't care about the returned
object (which also happens to be empty). Likewise when returning the dump
(e.g. for SUMMARY_ONLY) the clients don't care about the dump_guid.

Bug: 775039
Change-Id: Ia0861bcd20775802813a35f5a138fa022521f416
Reviewed-on: https://chromium-review.googlesource.com/721540
Commit-Queue: Lalit Maganti <lalitm@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Reviewed-by: Primiano Tucci <primiano@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#510055}

chrome/browser/metrics/process_memory_metrics_emitter.cc

@@ -191,7 +191,6 @@ ProcessMemoryMetricsEmitter::~ProcessMemoryMetricsEmitter() {}
 
 void ProcessMemoryMetricsEmitter::ReceivedMemoryDump(
     bool success,
-    uint64_t dump_guid,
     memory_instrumentation::mojom::GlobalMemoryDumpPtr ptr) {
   memory_dump_in_progress_ = false;
   if (!success)

chrome/browser/metrics/process_memory_metrics_emitter.h

@@ -44,7 +44,6 @@ class ProcessMemoryMetricsEmitter
   // is finished taking a memory dump.
   virtual void ReceivedMemoryDump(
       bool success,
-      uint64_t dump_guid,
       memory_instrumentation::mojom::GlobalMemoryDumpPtr ptr);
 
   // Virtual for testing. Callback invoked when resource_coordinator service

chrome/browser/metrics/process_memory_metrics_emitter_browsertest.cc

@@ -77,11 +77,9 @@ class ProcessMemoryMetricsEmitterFake : public ProcessMemoryMetricsEmitter {
 
   void ReceivedMemoryDump(
       bool success,
-      uint64_t dump_guid,
       memory_instrumentation::mojom::GlobalMemoryDumpPtr ptr) override {
     EXPECT_TRUE(success);
-    ProcessMemoryMetricsEmitter::ReceivedMemoryDump(success, dump_guid,
-                                                    std::move(ptr));
+    ProcessMemoryMetricsEmitter::ReceivedMemoryDump(success, std::move(ptr));
     finished_memory_dump_ = true;
     QuitIfFinished();
   }

chrome/browser/metrics/process_memory_metrics_emitter_unittest.cc

@@ -35,10 +35,8 @@ class ProcessMemoryMetricsEmitterFake : public ProcessMemoryMetricsEmitter {
 
   void ReceivedMemoryDump(
       bool success,
-      uint64_t dump_guid,
       memory_instrumentation::mojom::GlobalMemoryDumpPtr ptr) override {
-    ProcessMemoryMetricsEmitter::ReceivedMemoryDump(success, dump_guid,
-                                                    std::move(ptr));
+    ProcessMemoryMetricsEmitter::ReceivedMemoryDump(success, std::move(ptr));
   }
 
   void ReceivedProcessInfos(ProcessInfoVector process_infos) override {
@@ -326,7 +324,6 @@ class ProcessMemoryMetricsEmitterTest
 TEST_P(ProcessMemoryMetricsEmitterTest, CollectsSingleProcessUKMs) {
   base::flat_map<const char*, int64_t> expected_metrics =
       GetExpectedProcessMetrics(GetParam());
-  uint64_t dump_guid = 333;
 
   GlobalMemoryDumpPtr global_dump(
       memory_instrumentation::mojom::GlobalMemoryDump::New());
@@ -335,7 +332,7 @@ TEST_P(ProcessMemoryMetricsEmitterTest, CollectsSingleProcessUKMs) {
   scoped_refptr<ProcessMemoryMetricsEmitterFake> emitter(
       new ProcessMemoryMetricsEmitterFake(test_ukm_recorder_));
   emitter->ReceivedProcessInfos(ProcessInfoVector());
-  emitter->ReceivedMemoryDump(true, dump_guid, std::move(global_dump));
+  emitter->ReceivedMemoryDump(true, std::move(global_dump));
 
   EXPECT_EQ(2u, test_ukm_recorder_.entries_count());
   CheckMemoryUkmEntryMetrics(0, expected_metrics);
@@ -352,7 +349,6 @@ TEST_F(ProcessMemoryMetricsEmitterTest, CollectsExtensionProcessUKMs) {
       GetExpectedRendererMetrics();
   expected_metrics["NumberOfExtensions"] = 1;
   expected_metrics["Uptime"] = 21;
-  uint64_t dump_guid = 333;
 
   GlobalMemoryDumpPtr global_dump(
       memory_instrumentation::mojom::GlobalMemoryDump::New());
@@ -361,7 +357,7 @@ TEST_F(ProcessMemoryMetricsEmitterTest, CollectsExtensionProcessUKMs) {
   scoped_refptr<ProcessMemoryMetricsEmitterFake> emitter(
       new ProcessMemoryMetricsEmitterFake(test_ukm_recorder_));
   emitter->ReceivedProcessInfos(ProcessInfoVector());
-  emitter->ReceivedMemoryDump(true, dump_guid, std::move(global_dump));
+  emitter->ReceivedMemoryDump(true, std::move(global_dump));
 
   EXPECT_EQ(2u, test_ukm_recorder_.entries_count());
   CheckMemoryUkmEntryMetrics(0, expected_metrics);
@@ -372,7 +368,6 @@ TEST_F(ProcessMemoryMetricsEmitterTest, CollectsManyProcessUKMsSingleDump) {
       ProcessType::BROWSER, ProcessType::RENDERER, ProcessType::GPU,
       ProcessType::GPU,     ProcessType::RENDERER, ProcessType::BROWSER,
   };
-  uint64_t dump_guid = 333;
 
   GlobalMemoryDumpPtr global_dump(
       memory_instrumentation::mojom::GlobalMemoryDump::New());
@@ -386,7 +381,7 @@ TEST_F(ProcessMemoryMetricsEmitterTest, CollectsManyProcessUKMsSingleDump) {
   scoped_refptr<ProcessMemoryMetricsEmitterFake> emitter(
       new ProcessMemoryMetricsEmitterFake(test_ukm_recorder_));
   emitter->ReceivedProcessInfos(ProcessInfoVector());
-  emitter->ReceivedMemoryDump(true, dump_guid, std::move(global_dump));
+  emitter->ReceivedMemoryDump(true, std::move(global_dump));
 
   EXPECT_EQ(7u, test_ukm_recorder_.entries_count());
   for (size_t i = 0; i < entries_ptypes.size(); ++i) {
@@ -413,7 +408,7 @@ TEST_F(ProcessMemoryMetricsEmitterTest, CollectsManyProcessUKMsManyDumps) {
       entries_metrics.push_back(expected_metrics);
     }
     emitter->ReceivedProcessInfos(ProcessInfoVector());
-    emitter->ReceivedMemoryDump(true, i, std::move(global_dump));
+    emitter->ReceivedMemoryDump(true, std::move(global_dump));
   }
 
   EXPECT_EQ(8u, test_ukm_recorder_.entries_count());
@@ -433,7 +428,7 @@ TEST_F(ProcessMemoryMetricsEmitterTest, ReceiveProcessInfoFirst) {
   scoped_refptr<ProcessMemoryMetricsEmitterFake> emitter(
       new ProcessMemoryMetricsEmitterFake(test_ukm_recorder_));
   emitter->ReceivedProcessInfos(GetProcessInfo(test_ukm_recorder_));
-  emitter->ReceivedMemoryDump(true, 0xBEEF, std::move(global_dump));
+  emitter->ReceivedMemoryDump(true, std::move(global_dump));
 
   EXPECT_EQ(1,
             test_ukm_recorder_.CountEntries(
@@ -466,7 +461,7 @@ TEST_F(ProcessMemoryMetricsEmitterTest, ReceiveProcessInfoSecond) {
 
   scoped_refptr<ProcessMemoryMetricsEmitterFake> emitter(
       new ProcessMemoryMetricsEmitterFake(test_ukm_recorder_));
-  emitter->ReceivedMemoryDump(true, 0xBEEF, std::move(global_dump));
+  emitter->ReceivedMemoryDump(true, std::move(global_dump));
   emitter->ReceivedProcessInfos(GetProcessInfo(test_ukm_recorder_));
 
   EXPECT_EQ(1,
@@ -501,7 +496,7 @@ TEST_F(ProcessMemoryMetricsEmitterTest, ProcessInfoHasTwoURLs) {
 
   scoped_refptr<ProcessMemoryMetricsEmitterFake> emitter(
       new ProcessMemoryMetricsEmitterFake(test_ukm_recorder_));
-  emitter->ReceivedMemoryDump(true, 0xBEEF, std::move(global_dump));
+  emitter->ReceivedMemoryDump(true, std::move(global_dump));
   emitter->ReceivedProcessInfos(GetProcessInfo(test_ukm_recorder_));
 
   // Check that if there are two URLs, neither is emitted.

content/browser/tracing/background_memory_tracing_observer.cc

@@ -104,7 +104,7 @@ void BackgroundMemoryTracingObserver::OnTracingEnabled(
           base::trace_event::MemoryDumpType::EXPLICITLY_TRIGGERED,
           base::trace_event::MemoryDumpLevelOfDetail::BACKGROUND,
           memory_instrumentation::MemoryInstrumentation::
-              RequestGlobalDumpAndAppendToTraceCallback());
+              RequestGlobalMemoryDumpAndAppendToTraceCallback());
 }
 
 }  // namespace content

content/browser/tracing/memory_tracing_browsertest.cc

@@ -76,10 +76,9 @@ class MemoryTracingTest : public ContentBrowserTest {
       const MemoryDumpLevelOfDetail& level_of_detail,
       const base::Closure& closure) {
     uint32_t request_index = next_request_index_++;
-    memory_instrumentation::MemoryInstrumentation::
-        RequestGlobalDumpAndAppendToTraceCallback callback = base::Bind(
-            &MemoryTracingTest::OnGlobalMemoryDumpDone, base::Unretained(this),
-            base::ThreadTaskRunnerHandle::Get(), closure, request_index);
+    auto callback = base::Bind(
+        &MemoryTracingTest::OnGlobalMemoryDumpDone, base::Unretained(this),
+        base::ThreadTaskRunnerHandle::Get(), closure, request_index);
     if (from_renderer_thread) {
       PostTaskToInProcessRendererAndWait(base::Bind(
           &memory_instrumentation::MemoryInstrumentation::

services/resource_coordinator/memory_instrumentation/coordinator_impl.cc

@@ -158,51 +158,26 @@ void CoordinatorImpl::BindCoordinatorRequest(
 void CoordinatorImpl::RequestGlobalMemoryDump(
     const base::trace_event::MemoryDumpRequestArgs& args_in,
     const RequestGlobalMemoryDumpCallback& callback) {
-  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
-
-  UMA_HISTOGRAM_COUNTS_1000("Memory.Experimental.Debug.GlobalDumpQueueLength",
-                            queued_memory_dump_requests_.size());
-
-  bool another_dump_is_queued = !queued_memory_dump_requests_.empty();
-
-  // TODO(primiano): remove dump_guid from the request. For the moment callers
-  // should just pass a zero |dump_guid| in input. It should be an out-only arg.
-  DCHECK_EQ(0u, args_in.dump_guid);
-  base::trace_event::MemoryDumpRequestArgs args = args_in;
-  args.dump_guid = ++next_dump_id_;
-
-  // If this is a periodic or peak memory dump request and there already is
-  // another request in the queue with the same level of detail, there's no
-  // point in enqueuing this request.
-  if (another_dump_is_queued &&
-      args.dump_type !=
-          base::trace_event::MemoryDumpType::EXPLICITLY_TRIGGERED &&
-      args.dump_type != base::trace_event::MemoryDumpType::SUMMARY_ONLY &&
-      args.dump_type != base::trace_event::MemoryDumpType::VM_REGIONS_ONLY) {
-    for (const auto& request : queued_memory_dump_requests_) {
-      if (request.args.level_of_detail == args.level_of_detail) {
-        VLOG(1) << "RequestGlobalMemoryDump("
-                << base::trace_event::MemoryDumpTypeToString(args.dump_type)
-                << ") skipped because another dump request with the same "
-                   "level of detail ("
-                << base::trace_event::MemoryDumpLevelOfDetailToString(
-                       args.level_of_detail)
-                << ") is already in the queue";
-        callback.Run(false /* success */, args.dump_guid,
-                     nullptr /* global_memory_dump */);
-        return;
-      }
-    }
-  }
-
-  queued_memory_dump_requests_.emplace_back(args, callback);
-
-  // If another dump is already in queued, this dump will automatically be
-  // scheduled when the other dump finishes.
-  if (another_dump_is_queued)
-    return;
+  // This merely strips out the |dump_guid| argument.
+  auto callback_adapter = [](const RequestGlobalMemoryDumpCallback& callback,
+                             bool success, uint64_t,
+                             mojom::GlobalMemoryDumpPtr global_memory_dump) {
+    callback.Run(success, std::move(global_memory_dump));
+  };
+  RequestGlobalMemoryDumpInternal(args_in, false,
+                                  base::Bind(callback_adapter, callback));
+}
 
-  PerformNextQueuedGlobalMemoryDump();
+void CoordinatorImpl::RequestGlobalMemoryDumpAndAppendToTrace(
+    const base::trace_event::MemoryDumpRequestArgs& args_in,
+    const RequestGlobalMemoryDumpAndAppendToTraceCallback& callback) {
+  // This merely strips out the |dump_ptr| argument.
+  auto callback_adapter =
+      [](const RequestGlobalMemoryDumpAndAppendToTraceCallback& callback,
+         bool success, uint64_t dump_guid,
+         mojom::GlobalMemoryDumpPtr) { callback.Run(success, dump_guid); };
+  RequestGlobalMemoryDumpInternal(args_in, true,
+                                  base::Bind(callback_adapter, callback));
 }
 
 void CoordinatorImpl::GetVmRegionsForHeapProfiler(
@@ -210,16 +185,7 @@ void CoordinatorImpl::GetVmRegionsForHeapProfiler(
   base::trace_event::MemoryDumpRequestArgs args{
       0 /* dump_guid */, base::trace_event::MemoryDumpType::VM_REGIONS_ONLY,
       base::trace_event::MemoryDumpLevelOfDetail::DETAILED};
-
-  // This merely strips out the |dump_guid| argument, which is not used by
-  // the GetVmRegionsForHeapProfiler() callback.
-  auto callback_adapter =
-      [](const GetVmRegionsForHeapProfilerCallback& vm_regions_callback,
-         bool success, uint64_t,
-         mojom::GlobalMemoryDumpPtr global_memory_dump) {
-        vm_regions_callback.Run(success, std::move(global_memory_dump));
-      };
-  RequestGlobalMemoryDump(args, base::Bind(callback_adapter, callback));
+  RequestGlobalMemoryDump(args, callback);
 }
 
 void CoordinatorImpl::RegisterClientProcess(
@@ -262,6 +228,57 @@ void CoordinatorImpl::UnregisterClientProcess(
   DCHECK(num_deleted == 1);
 }
 
+void CoordinatorImpl::RequestGlobalMemoryDumpInternal(
+    const base::trace_event::MemoryDumpRequestArgs& args_in,
+    bool add_to_trace,
+    const RequestGlobalMemoryDumpInternalCallback& callback) {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+
+  UMA_HISTOGRAM_COUNTS_1000("Memory.Experimental.Debug.GlobalDumpQueueLength",
+                            queued_memory_dump_requests_.size());
+
+  bool another_dump_is_queued = !queued_memory_dump_requests_.empty();
+
+  // TODO(primiano): remove dump_guid from the request. For the moment callers
+  // should just pass a zero |dump_guid| in input. It should be an out-only arg.
+  DCHECK_EQ(0u, args_in.dump_guid);
+  base::trace_event::MemoryDumpRequestArgs args = args_in;
+  args.dump_guid = ++next_dump_id_;
+
+  // If this is a periodic or peak memory dump request and there already is
+  // another request in the queue with the same level of detail, there's no
+  // point in enqueuing this request.
+  if (another_dump_is_queued &&
+      args.dump_type !=
+          base::trace_event::MemoryDumpType::EXPLICITLY_TRIGGERED &&
+      args.dump_type != base::trace_event::MemoryDumpType::SUMMARY_ONLY &&
+      args.dump_type != base::trace_event::MemoryDumpType::VM_REGIONS_ONLY) {
+    for (const auto& request : queued_memory_dump_requests_) {
+      if (request.args.level_of_detail == args.level_of_detail) {
+        VLOG(1) << "RequestGlobalMemoryDump("
+                << base::trace_event::MemoryDumpTypeToString(args.dump_type)
+                << ") skipped because another dump request with the same "
+                   "level of detail ("
+                << base::trace_event::MemoryDumpLevelOfDetailToString(
+                       args.level_of_detail)
+                << ") is already in the queue";
+        callback.Run(false /* success */, 0 /* dump_guid */,
+                     nullptr /* global_memory_dump */);
+        return;
+      }
+    }
+  }
+
+  queued_memory_dump_requests_.emplace_back(args, callback);
+
+  // If another dump is already in queued, this dump will automatically be
+  // scheduled when the other dump finishes.
+  if (another_dump_is_queued)
+    return;
+
+  PerformNextQueuedGlobalMemoryDump();
+}
+
 void CoordinatorImpl::PerformNextQueuedGlobalMemoryDump() {
   using ResponseType = QueuedMemoryDumpRequest::PendingResponse::Type;
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
@@ -581,7 +598,7 @@ CoordinatorImpl::QueuedMemoryDumpRequest::Response::~Response() {}
 
 CoordinatorImpl::QueuedMemoryDumpRequest::QueuedMemoryDumpRequest(
     const base::trace_event::MemoryDumpRequestArgs& args,
-    const RequestGlobalMemoryDumpCallback callback)
+    const RequestGlobalMemoryDumpInternalCallback& callback)
     : args(args), callback(callback) {}
 
 CoordinatorImpl::QueuedMemoryDumpRequest::~QueuedMemoryDumpRequest() {}

services/resource_coordinator/memory_instrumentation/coordinator_impl.h

@@ -53,6 +53,9 @@ class CoordinatorImpl : public Coordinator, public mojom::Coordinator {
   void UnregisterClientProcess(mojom::ClientProcess*);
   void RequestGlobalMemoryDump(const base::trace_event::MemoryDumpRequestArgs&,
                                const RequestGlobalMemoryDumpCallback&) override;
+  void RequestGlobalMemoryDumpAndAppendToTrace(
+      const base::trace_event::MemoryDumpRequestArgs&,
+      const RequestGlobalMemoryDumpAndAppendToTraceCallback&) override;
   void GetVmRegionsForHeapProfiler(
       const GetVmRegionsForHeapProfilerCallback&) override;
 
@@ -67,6 +70,8 @@ class CoordinatorImpl : public Coordinator, public mojom::Coordinator {
  private:
   using OSMemDumpMap =
       std::unordered_map<base::ProcessId, mojom::RawOSMemDumpPtr>;
+  using RequestGlobalMemoryDumpInternalCallback =
+      base::Callback<void(bool, uint64_t, mojom::GlobalMemoryDumpPtr)>;
   friend std::default_delete<CoordinatorImpl>;  // For testing
   friend class CoordinatorImplTest;             // For testing
 
@@ -74,10 +79,10 @@ class CoordinatorImpl : public Coordinator, public mojom::Coordinator {
   struct QueuedMemoryDumpRequest {
     QueuedMemoryDumpRequest(
         const base::trace_event::MemoryDumpRequestArgs& args,
-        const RequestGlobalMemoryDumpCallback callback);
+        const RequestGlobalMemoryDumpInternalCallback& callback);
     ~QueuedMemoryDumpRequest();
     const base::trace_event::MemoryDumpRequestArgs args;
-    const RequestGlobalMemoryDumpCallback callback;
+    const RequestGlobalMemoryDumpInternalCallback callback;
 
     bool wants_mmaps() const {
       return args.level_of_detail ==
@@ -143,6 +148,11 @@ class CoordinatorImpl : public Coordinator, public mojom::Coordinator {
     const mojom::ProcessType process_type;
   };
 
+  void RequestGlobalMemoryDumpInternal(
+      const base::trace_event::MemoryDumpRequestArgs& args,
+      bool add_to_trace,
+      const RequestGlobalMemoryDumpInternalCallback& callback);
+
   // Callback of RequestChromeMemoryDump.
   void OnChromeMemoryDumpResponse(
       mojom::ClientProcess*,

services/resource_coordinator/memory_instrumentation/coordinator_impl_unittest.cc

@@ -161,10 +161,10 @@ class MockClientProcess : public mojom::ClientProcess {
 class MockGlobalMemoryDumpCallback {
  public:
   MockGlobalMemoryDumpCallback() = default;
-  MOCK_METHOD3(OnCall, void(bool, uint64_t, GlobalMemoryDump*));
+  MOCK_METHOD2(OnCall, void(bool, GlobalMemoryDump*));
 
-  void Run(bool success, uint64_t dump_guid, GlobalMemoryDumpPtr ptr) {
-    OnCall(success, dump_guid, ptr.get());
+  void Run(bool success, GlobalMemoryDumpPtr ptr) {
+    OnCall(success, ptr.get());
   }
 
   RequestGlobalMemoryDumpCallback Get() {
@@ -217,7 +217,7 @@ TEST_F(CoordinatorImplTest, NoClients) {
       MemoryDumpLevelOfDetail::DETAILED};
 
   MockGlobalMemoryDumpCallback callback;
-  EXPECT_CALL(callback, OnCall(true, Ne(0u), NotNull()));
+  EXPECT_CALL(callback, OnCall(true, NotNull()));
   RequestGlobalMemoryDump(args, callback.Get());
 }
 
@@ -236,7 +236,7 @@ TEST_F(CoordinatorImplTest, SeveralClients) {
       MemoryDumpLevelOfDetail::DETAILED};
 
   MockGlobalMemoryDumpCallback callback;
-  EXPECT_CALL(callback, OnCall(true, Ne(0u), NotNull()))
+  EXPECT_CALL(callback, OnCall(true, NotNull()))
       .WillOnce(RunClosure(run_loop.QuitClosure()));
   RequestGlobalMemoryDump(args, callback.Get());
   run_loop.Run();
@@ -263,10 +263,10 @@ TEST_F(CoordinatorImplTest, MissingChromeDump) {
           }));
 
   MockGlobalMemoryDumpCallback callback;
-  EXPECT_CALL(callback,
-              OnCall(true, Ne(0u),
-                     Pointee(Field(&mojom::GlobalMemoryDump::process_dumps,
-                                   IsEmpty()))))
+  EXPECT_CALL(
+      callback,
+      OnCall(true, Pointee(Field(&mojom::GlobalMemoryDump::process_dumps,
+                                 IsEmpty()))))
       .WillOnce(RunClosure(run_loop.QuitClosure()));
   RequestGlobalMemoryDump(args, callback.Get());
   run_loop.Run();
@@ -291,10 +291,10 @@ TEST_F(CoordinatorImplTest, MissingOsDump) {
           }));
 
   MockGlobalMemoryDumpCallback callback;
-  EXPECT_CALL(callback,
-              OnCall(true, Ne(0u),
-                     Pointee(Field(&mojom::GlobalMemoryDump::process_dumps,
-                                   IsEmpty()))))
+  EXPECT_CALL(
+      callback,
+      OnCall(true, Pointee(Field(&mojom::GlobalMemoryDump::process_dumps,
+                                 IsEmpty()))))
       .WillOnce(RunClosure(run_loop.QuitClosure()));
   RequestGlobalMemoryDump(args, callback.Get());
   run_loop.Run();
@@ -339,7 +339,7 @@ TEST_F(CoordinatorImplTest, ClientCrashDuringGlobalDump) {
           }));
 
   MockGlobalMemoryDumpCallback callback;
-  EXPECT_CALL(callback, OnCall(false, Ne(0u), NotNull()))
+  EXPECT_CALL(callback, OnCall(false, NotNull()))
       .WillOnce(RunClosure(run_loop.QuitClosure()));
   RequestGlobalMemoryDump(args, callback.Get());
   run_loop.Run();
@@ -370,7 +370,7 @@ TEST_F(CoordinatorImplTest, SingleClientCrashDuringGlobalDump) {
           }));
 
   MockGlobalMemoryDumpCallback callback;
-  EXPECT_CALL(callback, OnCall(false, Ne(0u), NotNull()))
+  EXPECT_CALL(callback, OnCall(false, NotNull()))
       .WillOnce(RunClosure(run_loop.QuitClosure()));
   RequestGlobalMemoryDump(args, callback.Get());
   run_loop.Run();
@@ -476,8 +476,8 @@ TEST_F(CoordinatorImplTest, GlobalMemoryDumpStruct) {
 #endif  // defined(OS_LINUX)
 
   MockGlobalMemoryDumpCallback callback;
-  EXPECT_CALL(callback, OnCall(true, Ne(0u), NotNull()))
-      .WillOnce(Invoke([&run_loop](bool success, uint64_t dump_guid,
+  EXPECT_CALL(callback, OnCall(true, NotNull()))
+      .WillOnce(Invoke([&run_loop](bool success,
                                    GlobalMemoryDump* global_dump) {
         EXPECT_TRUE(success);
         EXPECT_EQ(2U, global_dump->process_dumps.size());

services/resource_coordinator/public/cpp/memory_instrumentation/client_process_impl.cc

@@ -113,8 +113,9 @@ void ClientProcessImpl::RequestGlobalMemoryDump_NoCallback(
     return;
   }
 
-  coordinator_->RequestGlobalMemoryDump(
-      args, mojom::Coordinator::RequestGlobalMemoryDumpCallback());
+  coordinator_->RequestGlobalMemoryDumpAndAppendToTrace(
+      args,
+      mojom::Coordinator::RequestGlobalMemoryDumpAndAppendToTraceCallback());
 }
 
 void ClientProcessImpl::EnableHeapProfiling(

services/resource_coordinator/public/cpp/memory_instrumentation/memory_instrumentation.cc

@@ -47,32 +47,19 @@ MemoryInstrumentation::~MemoryInstrumentation() {
 void MemoryInstrumentation::RequestGlobalDump(
     RequestGlobalDumpCallback callback) {
   const auto& coordinator = GetCoordinatorBindingForCurrentThread();
-  auto callback_adapter = [](RequestGlobalDumpCallback callback, bool success,
-                             uint64_t dump_id, mojom::GlobalMemoryDumpPtr ptr) {
-    if (callback)
-      callback.Run(success, std::move(ptr));
-  };
   base::trace_event::MemoryDumpRequestArgs args = {
       0, MemoryDumpType::SUMMARY_ONLY, MemoryDumpLevelOfDetail::BACKGROUND};
-  coordinator->RequestGlobalMemoryDump(args,
-                                       base::Bind(callback_adapter, callback));
+  coordinator->RequestGlobalMemoryDump(args, callback);
 }
 
 void MemoryInstrumentation::RequestGlobalDumpAndAppendToTrace(
     MemoryDumpType dump_type,
     MemoryDumpLevelOfDetail level_of_detail,
-    RequestGlobalDumpAndAppendToTraceCallback callback) {
+    RequestGlobalMemoryDumpAndAppendToTraceCallback callback) {
   const auto& coordinator = GetCoordinatorBindingForCurrentThread();
-  auto callback_adapter = [](RequestGlobalDumpAndAppendToTraceCallback callback,
-                             bool success, uint64_t dump_id,
-                             mojom::GlobalMemoryDumpPtr) {
-    if (callback)
-      callback.Run(success, dump_id);
-  };
   base::trace_event::MemoryDumpRequestArgs args = {0, dump_type,
                                                    level_of_detail};
-  coordinator->RequestGlobalMemoryDump(args,
-                                       base::Bind(callback_adapter, callback));
+  coordinator->RequestGlobalMemoryDumpAndAppendToTrace(args, callback);
 }
 
 void MemoryInstrumentation::GetVmRegionsForHeapProfiler(

services/resource_coordinator/public/cpp/memory_instrumentation/memory_instrumentation.h

@@ -31,7 +31,7 @@ class SERVICES_RESOURCE_COORDINATOR_PUBLIC_CPP_EXPORT MemoryInstrumentation {
   using MemoryDumpLevelOfDetail = base::trace_event::MemoryDumpLevelOfDetail;
   using RequestGlobalDumpCallback =
       base::Callback<void(bool success, mojom::GlobalMemoryDumpPtr)>;
-  using RequestGlobalDumpAndAppendToTraceCallback =
+  using RequestGlobalMemoryDumpAndAppendToTraceCallback =
       base::Callback<void(bool success, uint64_t dump_id)>;
 
   static void CreateInstance(service_manager::Connector*,
@@ -57,7 +57,7 @@ class SERVICES_RESOURCE_COORDINATOR_PUBLIC_CPP_EXPORT MemoryInstrumentation {
   void RequestGlobalDumpAndAppendToTrace(
       MemoryDumpType,
       MemoryDumpLevelOfDetail,
-      RequestGlobalDumpAndAppendToTraceCallback);
+      RequestGlobalMemoryDumpAndAppendToTraceCallback);
 
   // Requests a global dump retrieving only the memory maps for all the client
   // processes registered. Does not add anything to the trace. The returned

services/resource_coordinator/public/cpp/memory_instrumentation/tracing_integration_unittest.cc

@@ -148,6 +148,10 @@ class MockCoordinator : public Coordinator, public mojom::Coordinator {
   void RequestGlobalMemoryDump(const MemoryDumpRequestArgs& args,
                                const RequestGlobalMemoryDumpCallback&) override;
 
+  void RequestGlobalMemoryDumpAndAppendToTrace(
+      const MemoryDumpRequestArgs& args,
+      const RequestGlobalMemoryDumpAndAppendToTraceCallback&) override;
+
   void GetVmRegionsForHeapProfiler(
       const GetVmRegionsForHeapProfilerCallback&) override {}
 
@@ -265,7 +269,14 @@ void MockCoordinator::RequestGlobalMemoryDump(
     const MemoryDumpRequestArgs& args,
     const RequestGlobalMemoryDumpCallback& callback) {
   client_->RequestChromeDump(args.dump_type, args.level_of_detail);
-  callback.Run(args.dump_guid, true, mojom::GlobalMemoryDumpPtr());
+  callback.Run(true, mojom::GlobalMemoryDumpPtr());
+}
+
+void MockCoordinator::RequestGlobalMemoryDumpAndAppendToTrace(
+    const MemoryDumpRequestArgs& args,
+    const RequestGlobalMemoryDumpAndAppendToTraceCallback& callback) {
+  client_->RequestChromeDump(args.dump_type, args.level_of_detail);
+  callback.Run(args.dump_guid, true);
 }
 
 // Tests that the MemoryDumpProvider(s) are invoked even if tracing has not

services/resource_coordinator/public/interfaces/memory_instrumentation/memory_instrumentation.mojom

@@ -217,11 +217,15 @@ interface Coordinator {
   // Registers a client process.
   RegisterClientProcess(ClientProcess client_process, ProcessType process_type);
 
-  // Broadcasts a dump request to all registered client processes, injects the
-  // dump in the trace buffer (if tracing is enabled) and returns a summarized
-  // dump back (if args.dump_type == SUMMARY_ONLY).
+  // Broadcasts a dump request to all registered client processes and returns a
+  // summarized dump back (if args.dump_type == SUMMARY_ONLY).
   RequestGlobalMemoryDump(RequestArgs args) =>
-      (bool success, uint64 dump_id, GlobalMemoryDump? global_memory_dump);
+      (bool success, GlobalMemoryDump? global_memory_dump);
+
+  // Broadcasts a dump request to all registered client processes and injects the
+  // dump in the trace buffer (if tracing is enabled).
+  RequestGlobalMemoryDumpAndAppendToTrace(RequestArgs args) =>
+      (bool success, uint64 dump_id);
 
   // Broadcasts a RequestOSMemoryDump-only request for all registered client
   // processes, retrieves only their memory maps and returns them in the field