memory-infra: don't pass mojo callback to MDM from client process

Passing these callbacks from the client process code means that MDM begins
owning the callback. However, these callbacks *must* be run before
destruction. While this is not a problem with MDM per-se as MDM is a
leaky singleton, MDM ocassionally passes ownership of this callback to
a thread loop.

On destruction of the thread loop with the callback posted, the callback
could be destroyed without being run which can cause problems.

To solve this, instead of passing the callback into a situation where
it could be destroyed, we instead store the callback inside the client
process itself. As the client is also a leaky singleton, the destructor
of these callbacks will no longer be run so we do not have the above problem.

Bug: 788658
Change-Id: I9dd65fe7748e0afaedd1bf5a3d25ac9cadfe9853
Reviewed-on: https://chromium-review.googlesource.com/790591Reviewed-by: Primiano Tucci <primiano@chromium.org>
Commit-Queue: Primiano Tucci <primiano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519517}

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

@@ -75,18 +75,26 @@ void ClientProcessImpl::RequestChromeMemoryDump(
     const base::trace_event::MemoryDumpRequestArgs& args,
     const RequestChromeMemoryDumpCallback& callback) {
   DCHECK(!callback.is_null());
+  auto it_and_inserted =
+      pending_chrome_callbacks_.emplace(args.dump_guid, std::move(callback));
+  DCHECK(it_and_inserted.second) << "Duplicated request id " << args.dump_guid;
   base::trace_event::MemoryDumpManager::GetInstance()->CreateProcessDump(
       args, base::Bind(&ClientProcessImpl::OnChromeMemoryDumpDone,
-                       base::Unretained(this), callback, args));
+                       base::Unretained(this)));
 }
 
 void ClientProcessImpl::OnChromeMemoryDumpDone(
-    const RequestChromeMemoryDumpCallback& callback,
-    const base::trace_event::MemoryDumpRequestArgs& req_args,
     bool success,
     uint64_t dump_guid,
     std::unique_ptr<base::trace_event::ProcessMemoryDump> process_memory_dump) {
   DCHECK(success || !process_memory_dump);
+
+  auto callback_it = pending_chrome_callbacks_.find(dump_guid);
+  DCHECK(callback_it != pending_chrome_callbacks_.end());
+
+  auto callback = std::move(callback_it->second);
+  pending_chrome_callbacks_.erase(callback_it);
+
   if (!process_memory_dump) {
     callback.Run(false, dump_guid, nullptr);
     return;

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

@@ -72,8 +72,6 @@ class SERVICES_RESOURCE_COORDINATOR_PUBLIC_CPP_EXPORT ClientProcessImpl
 
   // Callback passed to base::MemoryDumpManager::CreateProcessDump().
   void OnChromeMemoryDumpDone(
-      const RequestChromeMemoryDumpCallback&,
-      const base::trace_event::MemoryDumpRequestArgs& req_args,
       bool success,
       uint64_t dump_guid,
       std::unique_ptr<base::trace_event::ProcessMemoryDump>);
@@ -89,6 +87,9 @@ class SERVICES_RESOURCE_COORDINATOR_PUBLIC_CPP_EXPORT ClientProcessImpl
   const mojom::ProcessType process_type_;
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
 
+  // Map containing pending chrome memory callbacks indexed by dump guid.
+  std::map<uint64_t, RequestChromeMemoryDumpCallback> pending_chrome_callbacks_;
+
   // TODO(ssid): This should be moved to coordinator instead of clients once we
   // have the whole chrome dumps sent via mojo, crbug.com/728199.
   std::unique_ptr<TracingObserver> tracing_observer_;

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

@@ -55,7 +55,6 @@ const char* kBackgroundButNotSummaryWhitelistedMDPName =
     "BackgroundButNotSummaryWhitelistedTestDumpProvider";
 const char* const kTestMDPWhitelist[] = {
     kWhitelistedMDPName, kBackgroundButNotSummaryWhitelistedMDPName, nullptr};
-const uint64_t kTestGuid = 0;
 
 // GTest matchers for MemoryDumpRequestArgs arguments.
 MATCHER(IsDetailedDump, "") {
@@ -166,20 +165,21 @@ class MemoryTracingIntegrationTest : public testing::Test {
       std::unique_ptr<base::trace_event::ProcessMemoryDump>* result = nullptr) {
     base::RunLoop run_loop;
     bool success = false;
-    MemoryDumpRequestArgs request_args{kTestGuid, dump_type, level_of_detail};
+    uint64_t req_guid = ++guid_counter_;
+    MemoryDumpRequestArgs request_args{req_guid, dump_type, level_of_detail};
     ClientProcessImpl::RequestChromeMemoryDumpCallback callback = base::Bind(
         [](bool* curried_success, base::Closure curried_quit_closure,
            std::unique_ptr<base::trace_event::ProcessMemoryDump>*
                curried_result,
-           bool success, uint64_t dump_guid,
+           uint64_t curried_expected_guid, bool success, uint64_t dump_guid,
            std::unique_ptr<base::trace_event::ProcessMemoryDump> result) {
-          EXPECT_EQ(kTestGuid, dump_guid);
+          EXPECT_EQ(curried_expected_guid, dump_guid);
           *curried_success = success;
           if (curried_result)
             *curried_result = std::move(result);
           curried_quit_closure.Run();
         },
-        &success, run_loop.QuitClosure(), result);
+        &success, run_loop.QuitClosure(), result, req_guid);
     client_process_->RequestChromeMemoryDump(request_args, callback);
     run_loop.Run();
     return success;
@@ -187,7 +187,8 @@ class MemoryTracingIntegrationTest : public testing::Test {
 
   void RequestChromeDump(MemoryDumpType dump_type,
                          MemoryDumpLevelOfDetail level_of_detail) {
-    MemoryDumpRequestArgs request_args{kTestGuid, dump_type, level_of_detail};
+    uint64_t req_guid = ++guid_counter_;
+    MemoryDumpRequestArgs request_args{req_guid, dump_type, level_of_detail};
     ClientProcessImpl::RequestChromeMemoryDumpCallback callback = base::Bind(
         [](bool success, uint64_t dump_guid,
            std::unique_ptr<base::trace_event::ProcessMemoryDump> result) {});
@@ -235,6 +236,7 @@ class MemoryTracingIntegrationTest : public testing::Test {
   std::unique_ptr<base::MessageLoop> message_loop_;
   std::unique_ptr<MockCoordinator> coordinator_;
   std::unique_ptr<ClientProcessImpl> client_process_;
+  uint64_t guid_counter_ = 0;
 };
 
 void MockCoordinator::RequestGlobalMemoryDump(