[Reland #1] Clean up of memory instrumentation interface.

The original CL caused two race conditions to trigger more frequently. These
have been fixed at
https://chromium-review.googlesource.com/c/chromium/src/+/917243.

> This CL cleans up both the public interface and private implementation of memory
> instrumentation. It is in preparation for allowing memory instrumentation to
> asynchronously add heap dumps to traces.
>
> memory_instrumentation supports two interfaces: HeapProfilerHelper and
> Coordinator [specifically RequestGlobalMemoryDump]. They are used by different
> clients, with different privileges, for different purposes. The internal
> implementation used a single code path to support both, which further results in
> leakage of implementation details into the mojom interface, in the form of
> LevelOfDetail::VM_REGIONS_ONLY_FOR_HEAP_PROFILER.
>
> The previous implementation only allowed 1 global dump to be issued at a time.
> This causes problems for my upcoming refactor, which makes
> memory_instrumentation, during a global dump, request a heap dump from the
> profiling service. The profiling service in turn requests heaps from
> memory_instrumentation, resulting in a deadlock.
>
> This CL updates the implementation of CoordinatorImpl to use a separate queue of
> |in_progress_vm_region_requests_| to track heap dump requests. These are
> executed as soon as they are received. This CL also removes
> LevelOfDetail::VM_REGIONS_ONLY_FOR_HEAP_PROFILER, and updates
> GetVmRegionsForHeapProfiler to explicitly specify PIDs to dump.
>
> Bug: 758739
> Change-Id: I4add68c2e3acc8eeb66d99731fce59063ac5e23f
> Reviewed-on: https://chromium-review.googlesource.com/900222
> Commit-Queue: Erik Chen <erikchen@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Reviewed-by: Ilya Sherman <isherman@chromium.org>
> Reviewed-by: Primiano Tucci <primiano@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#536263}

Change-Id: I4add68c2e3acc8eeb66d99731fce59063ac5e23f
Bug: 758739
TBR: isherman@chromium.org, dcheng@chromium.org, primiano@chromium.org
Reviewed-on: https://chromium-review.googlesource.com/916501
Commit-Queue: Erik Chen <erikchen@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#536878}

chrome/browser/metrics/process_memory_metrics_emitter_unittest.cc

@@ -90,27 +90,14 @@ OSMemDumpPtr GetFakeOSMemDump(uint32_t resident_set_kb,
                               ) {
   using memory_instrumentation::mojom::VmRegion;
 
-  std::vector<memory_instrumentation::mojom::VmRegionPtr> vm_regions;
-  vm_regions.emplace_back(
-      VmRegion::New(0xdeadbeef,                      // start address
-                    0x4000,                          // size_in_bytes
-                    0x1234,                          // module_timestamp
-                    VmRegion::kProtectionFlagsRead,  // protection_flags
-                    "dummy_file",                    // mapped_file
-                    100,    // byte_stats_private_dirty_resident
-                    200,    // byte_stats_private_clean_resident
-                    300,    // byte_stats_shared_dirty_resident
-                    400,    // byte_stats_shared_clean_resident
-                    500,    // byte_stats_swapped,
-                    200));  // byte_stats_proportional_resident
+#if defined(OS_LINUX) || defined(OS_ANDROID)
   return memory_instrumentation::mojom::OSMemDump::New(
       resident_set_kb, private_footprint_kb, shared_footprint_kb,
-#if defined(OS_LINUX) || defined(OS_ANDROID)
-      std::move(vm_regions), private_swap_footprint_kb
+      private_swap_footprint_kb);
 #else
-      std::move(vm_regions)
+  return memory_instrumentation::mojom::OSMemDump::New(
+      resident_set_kb, private_footprint_kb, shared_footprint_kb);
 #endif
-      );
 }
 
 void PopulateBrowserMetrics(GlobalMemoryDumpPtr& global_dump,

chrome/browser/profiling_host/background_profiling_triggers_unittest.cc

@@ -37,14 +37,11 @@ constexpr uint32_t kProcessMallocTriggerKb = 2 * 1024 * 1024;  // 2 Gig
 OSMemDumpPtr GetFakeOSMemDump(uint32_t resident_set_kb,
                               uint32_t private_footprint_kb,
                               uint32_t shared_footprint_kb) {
-  using memory_instrumentation::mojom::VmRegion;
-  std::vector<memory_instrumentation::mojom::VmRegionPtr> vm_regions;
   return memory_instrumentation::mojom::OSMemDump::New(
-      resident_set_kb, private_footprint_kb, shared_footprint_kb,
+      resident_set_kb, private_footprint_kb, shared_footprint_kb
 #if defined(OS_LINUX) || defined(OS_ANDROID)
-      std::move(vm_regions), 0
-#else
-      std::move(vm_regions)
+      ,
+      0
 #endif
       );
 }

chrome/browser/profiling_host/profiling_test_driver.cc

@@ -114,7 +114,9 @@ int NumProcessesWithName(base::Value* dump_json, std::string name, int* pid) {
   return num_processes;
 }
 
-base::Value* FindHeapsV2(base::ProcessId pid, base::Value* dump_json) {
+base::Value* FindArgDump(base::ProcessId pid,
+                         base::Value* dump_json,
+                         const char* arg) {
   base::Value* events = dump_json->FindKey("traceEvents");
   base::Value* dumps = nullptr;
   base::Value* heaps_v2 = nullptr;
@@ -132,7 +134,7 @@ base::Value* FindHeapsV2(base::ProcessId pid, base::Value* dump_json) {
     if (static_cast<base::ProcessId>(found_pid->GetInt()) != pid)
       continue;
     dumps = &event;
-    heaps_v2 = dumps->FindPath({"args", "dumps", "heaps_v2"});
+    heaps_v2 = dumps->FindPath({"args", "dumps", arg});
     if (heaps_v2)
       return heaps_v2;
   }
@@ -499,7 +501,24 @@ bool ValidateSamplingAllocations(base::Value* heaps_v2,
                << approximate_count;
     return false;
   }
+  return true;
+}
 
+bool ValidateProcessMmaps(base::Value* process_mmaps,
+                          bool should_have_contents) {
+  base::Value* vm_regions = process_mmaps->FindKey("vm_regions");
+  size_t count = vm_regions->GetList().size();
+  if (should_have_contents) {
+    if (count == 0) {
+      LOG(ERROR) << "vm_regions should have contents, but doesn't";
+      return false;
+    }
+  } else {
+    if (count != 0) {
+      LOG(ERROR) << "vm_regions should be empty, but has contents";
+      return false;
+    }
+  }
   return true;
 }
 
@@ -741,7 +760,7 @@ void ProfilingTestDriver::TraceFinished(base::Closure closure,
 
 bool ProfilingTestDriver::ValidateBrowserAllocations(base::Value* dump_json) {
   base::Value* heaps_v2 =
-      FindHeapsV2(base::Process::Current().Pid(), dump_json);
+      FindArgDump(base::Process::Current().Pid(), dump_json, "heaps_v2");
 
   if (options_.mode != ProfilingProcessHost::Mode::kAll &&
       options_.mode != ProfilingProcessHost::Mode::kBrowser &&
@@ -823,6 +842,13 @@ bool ProfilingTestDriver::ValidateBrowserAllocations(base::Value* dump_json) {
     return false;
   }
 
+  base::Value* process_mmaps =
+      FindArgDump(base::Process::Current().Pid(), dump_json, "process_mmaps");
+  if (!ValidateProcessMmaps(process_mmaps, !HasPseudoFrames())) {
+    LOG(ERROR) << "Failed to validate browser process mmaps.";
+    return false;
+  }
+
   return true;
 }
 
@@ -835,13 +861,20 @@ bool ProfilingTestDriver::ValidateRendererAllocations(base::Value* dump_json) {
   }
 
   base::ProcessId renderer_pid = static_cast<base::ProcessId>(pid);
-  base::Value* heaps_v2 = FindHeapsV2(renderer_pid, dump_json);
+  base::Value* heaps_v2 = FindArgDump(renderer_pid, dump_json, "heaps_v2");
   if (ShouldProfileRenderer()) {
     if (!heaps_v2) {
       LOG(ERROR) << "Failed to find heaps v2 for renderer";
       return false;
     }
 
+    base::Value* process_mmaps =
+        FindArgDump(renderer_pid, dump_json, "process_mmaps");
+    if (!ValidateProcessMmaps(process_mmaps, !HasPseudoFrames())) {
+      LOG(ERROR) << "Failed to validate renderer process mmaps.";
+      return false;
+    }
+
     // ValidateDump doesn't always succeed for the renderer, since we don't do
     // anything to flush allocations, there are very few allocations recorded
     // by the heap profiler. When we do a heap dump, we prune small

chrome/profiling/memlog_connection_manager.cc

@@ -56,7 +56,7 @@ struct MemlogConnectionManager::DumpProcessesForTracingTracking
   mojom::ProfilingService::DumpProcessesForTracingCallback callback;
 
   // Info about the request.
-  memory_instrumentation::mojom::GlobalMemoryDumpPtr dump;
+  VmRegions vm_regions;
 
   // Collects the results.
   std::vector<profiling::mojom::SharedBufferWithSizePtr> results;
@@ -73,12 +73,14 @@ struct MemlogConnectionManager::Connection {
              mojom::ProfilingClientPtr client,
              scoped_refptr<MemlogReceiverPipe> p,
              mojom::ProcessType process_type,
-             uint32_t sampling_rate)
+             uint32_t sampling_rate,
+             mojom::StackMode stack_mode)
       : thread(base::StringPrintf("Sender %lld thread",
                                   static_cast<long long>(pid))),
         client(std::move(client)),
         pipe(p),
         process_type(process_type),
+        stack_mode(stack_mode),
         tracker(std::move(complete_cb), backtrace_storage),
         sampling_rate(sampling_rate) {}
 
@@ -88,12 +90,18 @@ struct MemlogConnectionManager::Connection {
     parser->DisconnectReceivers();
   }
 
+  bool HeapDumpNeedsVmRegions() {
+    return stack_mode == mojom::StackMode::NATIVE_WITHOUT_THREAD_NAMES ||
+           stack_mode == mojom::StackMode::NATIVE_WITH_THREAD_NAMES;
+  }
+
   base::Thread thread;
 
   mojom::ProfilingClientPtr client;
   scoped_refptr<MemlogReceiverPipe> pipe;
   scoped_refptr<MemlogStreamParser> parser;
   mojom::ProcessType process_type;
+  mojom::StackMode stack_mode;
 
   // Danger: This lives on the |thread| member above. The connection manager
   // lives on the I/O thread, so accesses to the variable must be synchronized.
@@ -155,7 +163,7 @@ void MemlogConnectionManager::OnNewConnection(
 
   auto connection = base::MakeUnique<Connection>(
       std::move(complete_cb), &backtrace_storage_, pid, std::move(client),
-      new_pipe, process_type, params->sampling_rate);
+      new_pipe, process_type, params->sampling_rate, params->stack_mode);
 
   base::Thread::Options options;
   options.message_loop_type = base::MessageLoop::TYPE_IO;
@@ -184,6 +192,18 @@ std::vector<base::ProcessId> MemlogConnectionManager::GetConnectionPids() {
   return results;
 }
 
+std::vector<base::ProcessId>
+MemlogConnectionManager::GetConnectionPidsThatNeedVmRegions() {
+  base::AutoLock lock(connections_lock_);
+  std::vector<base::ProcessId> results;
+  results.reserve(connections_.size());
+  for (const auto& pair : connections_) {
+    if (pair.second->HeapDumpNeedsVmRegions())
+      results.push_back(pair.first);
+  }
+  return results;
+}
+
 void MemlogConnectionManager::OnConnectionComplete(base::ProcessId pid) {
   base::AutoLock lock(connections_lock_);
   auto found = connections_.find(pid);
@@ -215,7 +235,7 @@ void MemlogConnectionManager::DumpProcessesForTracing(
     bool keep_small_allocations,
     bool strip_path_from_mapped_files,
     mojom::ProfilingService::DumpProcessesForTracingCallback callback,
-    memory_instrumentation::mojom::GlobalMemoryDumpPtr dump) {
+    VmRegions vm_regions) {
   base::AutoLock lock(connections_lock_);
 
   // Early out if there are no connections.
@@ -230,7 +250,7 @@ void MemlogConnectionManager::DumpProcessesForTracing(
       BacktraceStorage::Lock(&backtrace_storage_);
   tracking->waiting_responses = connections_.size();
   tracking->callback = std::move(callback);
-  tracking->dump = std::move(dump);
+  tracking->vm_regions = std::move(vm_regions);
   tracking->results.reserve(connections_.size());
 
   scoped_refptr<base::SingleThreadTaskRunner> task_runner =
@@ -277,25 +297,15 @@ void MemlogConnectionManager::DoDumpOneProcessForTracing(
     return;
   }
 
-  // Find the memory maps list for the given process.
-  memory_instrumentation::mojom::ProcessMemoryDump* process_dump = nullptr;
-  for (const auto& proc : tracking->dump->process_dumps) {
-    if (proc->pid == pid) {
-      process_dump = &*proc;
-      break;
-    }
-  }
-  if (!process_dump) {
-    DLOG(ERROR) << "Don't have a memory dump for PID " << pid;
-    if (tracking->waiting_responses == 0)
-      std::move(tracking->callback).Run(std::move(tracking->results));
-    return;
-  }
-
   CHECK(tracking->backtrace_storage_lock.IsLocked());
   ExportParams params;
   params.allocs = std::move(counts);
-  params.maps = std::move(process_dump->os_dump->memory_maps_for_heap_profiler);
+
+  auto it = tracking->vm_regions.find(pid);
+  if (it != tracking->vm_regions.end()) {
+    params.maps = std::move(it->second);
+  }
+
   params.context_map = std::move(context);
   params.mapped_strings = std::move(mapped_strings);
   params.process_type = process_type;

chrome/profiling/memlog_connection_manager.h

@@ -32,6 +32,10 @@ class SequencedTaskRunner;
 
 namespace profiling {
 
+using VmRegions =
+    std::unordered_map<base::ProcessId,
+                       std::vector<memory_instrumentation::mojom::VmRegionPtr>>;
+
 // Manages all connections and logging for each process. Pipes are supplied by
 // the pipe server and this class will connect them to a parser and logger.
 //
@@ -68,7 +72,7 @@ class MemlogConnectionManager {
       bool keep_small_allocations,
       bool strip_path_from_mapped_files,
       mojom::ProfilingService::DumpProcessesForTracingCallback callback,
-      memory_instrumentation::mojom::GlobalMemoryDumpPtr dump);
+      VmRegions vm_regions);
 
   void OnNewConnection(base::ProcessId pid,
                        mojom::ProfilingClientPtr client,
@@ -77,6 +81,7 @@ class MemlogConnectionManager {
                        mojom::ProfilingParamsPtr params);
 
   std::vector<base::ProcessId> GetConnectionPids();
+  std::vector<base::ProcessId> GetConnectionPidsThatNeedVmRegions();
 
  private:
   struct Connection;

chrome/profiling/profiling_service.cc

@@ -61,12 +61,22 @@ void ProfilingService::DumpProcessesForTracing(
         resource_coordinator::mojom::kServiceName, &helper_);
   }
 
-  // Need a memory map to make sense of the dump. The dump will be triggered
-  // in the memory map global dump callback.
-  helper_->GetVmRegionsForHeapProfiler(base::Bind(
-      &ProfilingService::OnGetVmRegionsCompleteForDumpProcessesForTracing,
-      weak_factory_.GetWeakPtr(), keep_small_allocations,
-      strip_path_from_mapped_files, base::Passed(&callback)));
+  std::vector<base::ProcessId> pids =
+      connection_manager_.GetConnectionPidsThatNeedVmRegions();
+  if (pids.empty()) {
+    connection_manager_.DumpProcessesForTracing(
+        keep_small_allocations, strip_path_from_mapped_files,
+        std::move(callback), VmRegions());
+  } else {
+    // Need a memory map to make sense of the dump. The dump will be triggered
+    // in the memory map global dump callback.
+    helper_->GetVmRegionsForHeapProfiler(
+        pids,
+        base::Bind(
+            &ProfilingService::OnGetVmRegionsCompleteForDumpProcessesForTracing,
+            weak_factory_.GetWeakPtr(), keep_small_allocations,
+            strip_path_from_mapped_files, base::Passed(&callback)));
+  }
 }
 
 void ProfilingService::GetProfiledPids(GetProfiledPidsCallback callback) {
@@ -77,19 +87,10 @@ void ProfilingService::OnGetVmRegionsCompleteForDumpProcessesForTracing(
     bool keep_small_allocations,
     bool strip_path_from_mapped_files,
     mojom::ProfilingService::DumpProcessesForTracingCallback callback,
-    bool success,
-    memory_instrumentation::mojom::GlobalMemoryDumpPtr dump) {
-  if (!success) {
-    DLOG(ERROR) << "GetVMRegions failed";
-    std::move(callback).Run(
-        std::vector<profiling::mojom::SharedBufferWithSizePtr>());
-    return;
-  }
-  // TODO(bug 752621) we should be asking and getting the memory map of only
-  // the process we want rather than querying all processes and filtering.
+    VmRegions vm_regions) {
   connection_manager_.DumpProcessesForTracing(
       keep_small_allocations, strip_path_from_mapped_files, std::move(callback),
-      std::move(dump));
+      std::move(vm_regions));
 }
 
 }  // namespace profiling

chrome/profiling/profiling_service.h

@@ -59,8 +59,7 @@ class ProfilingService : public service_manager::Service,
       bool keep_small_allocations,
       bool strip_path_from_mapped_files,
       mojom::ProfilingService::DumpProcessesForTracingCallback callback,
-      bool success,
-      memory_instrumentation::mojom::GlobalMemoryDumpPtr dump);
+      VmRegions vm_regions);
 
   service_manager::BinderRegistry registry_;
   mojo::Binding<mojom::ProfilingService> binding_;

services/resource_coordinator/memory_instrumentation/coordinator_impl.cc

@@ -11,6 +11,7 @@
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
+#include "base/callback_helpers.h"
 #include "base/command_line.h"
 #include "base/location.h"
 #include "base/logging.h"
@@ -97,17 +98,6 @@ void CoordinatorImpl::RequestGlobalMemoryDump(
     MemoryDumpLevelOfDetail level_of_detail,
     const std::vector<std::string>& allocator_dump_names,
     const RequestGlobalMemoryDumpCallback& callback) {
-  // Don't allow arbitary processes to obtain VM regions. Only the heap profiler
-  // is allowed to obtain them using the special method on the different
-  // interface.
-  if (level_of_detail ==
-      MemoryDumpLevelOfDetail::VM_REGIONS_ONLY_FOR_HEAP_PROFILER) {
-    bindings_.ReportBadMessage(
-        "Requested global memory dump using level of detail reserved for the "
-        "heap profiler.");
-    return;
-  }
-
   // This merely strips out the |dump_guid| argument.
   auto adapter = [](const RequestGlobalMemoryDumpCallback& callback,
                     bool success, uint64_t,
@@ -149,17 +139,6 @@ void CoordinatorImpl::RequestGlobalMemoryDumpAndAppendToTrace(
     MemoryDumpType dump_type,
     MemoryDumpLevelOfDetail level_of_detail,
     const RequestGlobalMemoryDumpAndAppendToTraceCallback& callback) {
-  // Don't allow arbitary processes to obtain VM regions. Only the heap profiler
-  // is allowed to obtain them using the special method on its own dedicated
-  // interface (HeapProfilingHelper).
-  if (level_of_detail ==
-      MemoryDumpLevelOfDetail::VM_REGIONS_ONLY_FOR_HEAP_PROFILER) {
-    bindings_.ReportBadMessage(
-        "Requested global memory dump using level of detail reserved for the "
-        "heap profiler.");
-    return;
-  }
-
   // This merely strips out the |dump_ptr| argument.
   auto adapter =
       [](const RequestGlobalMemoryDumpAndAppendToTraceCallback& callback,
@@ -172,19 +151,28 @@ void CoordinatorImpl::RequestGlobalMemoryDumpAndAppendToTrace(
 }
 
 void CoordinatorImpl::GetVmRegionsForHeapProfiler(
+    const std::vector<base::ProcessId>& pids,
     const GetVmRegionsForHeapProfilerCallback& callback) {
-  // This merely strips out the |dump_guid| argument.
-  auto adapter = [](const RequestGlobalMemoryDumpCallback& callback,
-                    bool success, uint64_t dump_guid,
-                    mojom::GlobalMemoryDumpPtr global_memory_dump) {
-    callback.Run(success, std::move(global_memory_dump));
-  };
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  uint64_t dump_guid = ++next_dump_id_;
+  std::unique_ptr<QueuedVmRegionRequest> request =
+      std::make_unique<QueuedVmRegionRequest>(dump_guid, callback);
+  in_progress_vm_region_requests_[dump_guid] = std::move(request);
 
-  QueuedRequest::Args args(
-      MemoryDumpType::EXPLICITLY_TRIGGERED,
-      MemoryDumpLevelOfDetail::VM_REGIONS_ONLY_FOR_HEAP_PROFILER, {},
-      false /* add_to_trace */, base::kNullProcessId);
-  RequestGlobalMemoryDumpInternal(args, base::BindRepeating(adapter, callback));
+  std::vector<QueuedRequestDispatcher::ClientInfo> clients;
+  for (const auto& kv : clients_) {
+    auto client_identity = kv.second->identity;
+    const base::ProcessId pid = GetProcessIdForClientIdentity(client_identity);
+    clients.emplace_back(kv.second->client.get(), pid, kv.second->process_type);
+  }
+
+  QueuedVmRegionRequest* request_ptr =
+      in_progress_vm_region_requests_[dump_guid].get();
+  auto os_callback = base::Bind(&CoordinatorImpl::OnOSMemoryDumpForVMRegions,
+                                base::Unretained(this), dump_guid);
+  QueuedRequestDispatcher::SetUpAndDispatchVmRegionRequest(request_ptr, clients,
+                                                           pids, os_callback);
+  FinalizeVmRegionDumpIfAllManagersReplied(dump_guid);
 }
 
 void CoordinatorImpl::RegisterClientProcess(
@@ -222,6 +210,29 @@ void CoordinatorImpl::UnregisterClientProcess(
     }
     FinalizeGlobalMemoryDumpIfAllManagersReplied();
   }
+
+  for (auto& pair : in_progress_vm_region_requests_) {
+    QueuedVmRegionRequest* request = pair.second.get();
+    auto it = request->pending_responses.begin();
+    while (it != request->pending_responses.end()) {
+      auto current = it++;
+      if (*current == client_process) {
+        request->pending_responses.erase(current);
+      }
+    }
+  }
+
+  // Try to finalize all outstanding vm region requests.
+  for (auto& pair : in_progress_vm_region_requests_) {
+    // PostTask to avoid re-entrancy or modification of data-structure during
+    // iteration.
+    base::SequencedTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::BindOnce(
+            &CoordinatorImpl::FinalizeVmRegionDumpIfAllManagersReplied,
+            base::Unretained(this), pair.second->dump_guid));
+  }
+
   size_t num_deleted = clients_.erase(client_process);
   DCHECK(num_deleted == 1);
 }
@@ -384,6 +395,38 @@ void CoordinatorImpl::OnOSMemoryDumpResponse(uint64_t dump_guid,
   FinalizeGlobalMemoryDumpIfAllManagersReplied();
 }
 
+void CoordinatorImpl::OnOSMemoryDumpForVMRegions(uint64_t dump_guid,
+                                                 mojom::ClientProcess* client,
+                                                 bool success,
+                                                 OSMemDumpMap os_dumps) {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  auto request_it = in_progress_vm_region_requests_.find(dump_guid);
+  DCHECK(request_it != in_progress_vm_region_requests_.end());
+
+  QueuedVmRegionRequest* request = request_it->second.get();
+  auto it = request->pending_responses.find(client);
+  DCHECK(it != request->pending_responses.end());
+  request->pending_responses.erase(it);
+  request->responses[client].os_dumps = std::move(os_dumps);
+
+  FinalizeVmRegionDumpIfAllManagersReplied(request->dump_guid);
+}
+
+void CoordinatorImpl::FinalizeVmRegionDumpIfAllManagersReplied(
+    uint64_t dump_guid) {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+
+  auto it = in_progress_vm_region_requests_.find(dump_guid);
+  DCHECK(it != in_progress_vm_region_requests_.end());
+  if (!it->second->pending_responses.empty())
+    return;
+
+  QueuedRequestDispatcher::VmRegions results =
+      QueuedRequestDispatcher::FinalizeVmRegionRequest(it->second.get());
+  it->second->callback.Run(std::move(results));
+  in_progress_vm_region_requests_.erase(it);
+}
+
 void CoordinatorImpl::RemovePendingResponse(
     mojom::ClientProcess* client,
     QueuedRequest::PendingResponse::Type type) {

services/resource_coordinator/memory_instrumentation/coordinator_impl.h

@@ -73,6 +73,7 @@ class CoordinatorImpl : public Coordinator,
 
   // mojom::HeapProfilerHelper implementation.
   void GetVmRegionsForHeapProfiler(
+      const std::vector<base::ProcessId>& pids,
       const GetVmRegionsForHeapProfilerCallback&) override;
 
  protected:
@@ -120,6 +121,14 @@ class CoordinatorImpl : public Coordinator,
                               bool success,
                               OSMemDumpMap);
 
+  // Callback of RequestOSMemoryDumpForVmRegions.
+  void OnOSMemoryDumpForVMRegions(uint64_t dump_guid,
+                                  mojom::ClientProcess* client,
+                                  bool success,
+                                  OSMemDumpMap);
+
+  void FinalizeVmRegionDumpIfAllManagersReplied(uint64_t dump_guid);
+
   void RemovePendingResponse(mojom::ClientProcess*,
                              QueuedRequest::PendingResponse::Type);
 
@@ -139,6 +148,26 @@ class CoordinatorImpl : public Coordinator,
   // Outstanding dump requests, enqueued via RequestGlobalMemoryDump().
   std::list<QueuedRequest> queued_memory_dump_requests_;
 
+  // Outstanding vm region requests, enqueued via GetVmRegionsForHeapProfiler().
+  // This is kept in a separate list from |queued_memory_dump_requests_| for two
+  // reasons:
+  //   1) The profiling service is queried during a memory dump request, but the
+  //   profiling service in turn needs to query for vm regions. Keeping this in
+  //   the same list as |queued_memory_dump_requests_| would require this class
+  //   to support concurrent requests.
+  //
+  //   2) Vm region requests are only ever requested by the profiling service,
+  //   which uses the HeapProfilerHelper interface. Keeping the requests
+  //   separate means we can avoid littering the RequestGlobalMemoryDump
+  //   interface with flags intended for HeapProfilerHelper. This was already
+  //   technically possible before, but required some additional plumbing. Now
+  //   the separation is much cleaner.
+  //
+  // Unlike queued_memory_dump_requests_, all requests are executed in parallel.
+  // The key is a |dump_guid|.
+  std::map<uint64_t, std::unique_ptr<QueuedVmRegionRequest>>
+      in_progress_vm_region_requests_;
+
   // There may be extant callbacks in |queued_memory_dump_requests_|. The
   // bindings_ must be closed before destroying the un-run callbacks.
   mojo::BindingSet<mojom::Coordinator, service_manager::Identity> bindings_;
@@ -150,6 +179,8 @@ class CoordinatorImpl : public Coordinator,
 
   // Maintains a map of service_manager::Identity -> pid for registered clients.
   std::unique_ptr<ProcessMap> process_map_;
+
+  // Dump IDs are unique across both heap dump and memory dump requests.
   uint64_t next_dump_id_;
   std::unique_ptr<TracingObserver> tracing_observer_;
 

services/resource_coordinator/memory_instrumentation/coordinator_impl_unittest.cc

@@ -120,8 +120,9 @@ class CoordinatorImplTest : public testing::Test {
   }
 
   void GetVmRegionsForHeapProfiler(
+      const std::vector<base::ProcessId>& pids,
       GetVmRegionsForHeapProfilerCallback callback) {
-    coordinator_->GetVmRegionsForHeapProfiler(callback);
+    coordinator_->GetVmRegionsForHeapProfiler(pids, callback);
   }
 
   void ReduceCoordinatorClientProcessTimeout() {
@@ -226,15 +227,19 @@ class MockGlobalMemoryDumpAndAppendToTraceCallback {
 class MockGetVmRegionsForHeapProfilerCallback {
  public:
   MockGetVmRegionsForHeapProfilerCallback() = default;
-  MOCK_METHOD2(OnCall, void(bool, GlobalMemoryDump*));
-
-  void Run(bool success, GlobalMemoryDumpPtr ptr) {
-    OnCall(success, ptr.get());
+  MOCK_METHOD1(
+      OnCall,
+      void(const std::unordered_map<base::ProcessId,
+                                    std::vector<mojom::VmRegionPtr>>&));
+
+  void Run(std::unordered_map<base::ProcessId, std::vector<mojom::VmRegionPtr>>
+               results) {
+    OnCall(results);
   }
 
   GetVmRegionsForHeapProfilerCallback Get() {
-    return base::Bind(&MockGetVmRegionsForHeapProfilerCallback::Run,
-                      base::Unretained(this));
+    return base::BindRepeating(&MockGetVmRegionsForHeapProfilerCallback::Run,
+                               base::Unretained(this));
   }
 };
 
@@ -695,40 +700,40 @@ TEST_F(CoordinatorImplTest, VmRegionsForHeapProfiler) {
 #endif  // defined(OS_LINUX)
 
   MockGetVmRegionsForHeapProfilerCallback callback;
-  EXPECT_CALL(callback, OnCall(true, NotNull()))
-      .WillOnce(Invoke([&run_loop](bool success,
-                                   GlobalMemoryDump* global_dump) {
-        ASSERT_EQ(2U, global_dump->process_dumps.size());
-        mojom::ProcessMemoryDumpPtr browser_dump = nullptr;
-        mojom::ProcessMemoryDumpPtr renderer_dump = nullptr;
-        for (mojom::ProcessMemoryDumpPtr& dump : global_dump->process_dumps) {
-          if (dump->process_type == mojom::ProcessType::BROWSER) {
-            browser_dump = std::move(dump);
-            ASSERT_EQ(kBrowserPid, browser_dump->pid);
-          } else if (dump->process_type == mojom::ProcessType::RENDERER) {
-            renderer_dump = std::move(dump);
-            ASSERT_EQ(kRendererPid, renderer_dump->pid);
-          }
-        }
-        const std::vector<mojom::VmRegionPtr>& browser_mmaps =
-            browser_dump->os_dump->memory_maps_for_heap_profiler;
-        ASSERT_EQ(3u, browser_mmaps.size());
-        for (int i = 0; i < 3; i++) {
-          EXPECT_EQ(GetFakeAddrForVmRegion(browser_dump->pid, i),
-                    browser_mmaps[i]->start_address);
-        }
-
-        const std::vector<mojom::VmRegionPtr>& renderer_mmaps =
-            renderer_dump->os_dump->memory_maps_for_heap_profiler;
-        ASSERT_EQ(3u, renderer_mmaps.size());
-        for (int i = 0; i < 3; i++) {
-          EXPECT_EQ(GetFakeAddrForVmRegion(renderer_dump->pid, i),
-                    renderer_mmaps[i]->start_address);
-        }
-        run_loop.Quit();
-      }));
+  EXPECT_CALL(callback, OnCall(_))
+      .WillOnce(Invoke(
+          [&run_loop](
+              const std::unordered_map<
+                  base::ProcessId, std::vector<mojom::VmRegionPtr>>& results) {
+            ASSERT_EQ(2U, results.size());
+
+            auto browser_it = results.find(kBrowserPid);
+            ASSERT_TRUE(browser_it != results.end());
+            auto renderer_it = results.find(kRendererPid);
+            ASSERT_TRUE(renderer_it != results.end());
+
+            const std::vector<mojom::VmRegionPtr>& browser_mmaps =
+                browser_it->second;
+            ASSERT_EQ(3u, browser_mmaps.size());
+            for (int i = 0; i < 3; i++) {
+              EXPECT_EQ(GetFakeAddrForVmRegion(kBrowserPid, i),
+                        browser_mmaps[i]->start_address);
+            }
+
+            const std::vector<mojom::VmRegionPtr>& renderer_mmaps =
+                renderer_it->second;
+            ASSERT_EQ(3u, renderer_mmaps.size());
+            for (int i = 0; i < 3; i++) {
+              EXPECT_EQ(GetFakeAddrForVmRegion(kRendererPid, i),
+                        renderer_mmaps[i]->start_address);
+            }
+            run_loop.Quit();
+          }));
 
-  GetVmRegionsForHeapProfiler(callback.Get());
+  std::vector<base::ProcessId> pids;
+  pids.push_back(kBrowserPid);
+  pids.push_back(kRendererPid);
+  GetVmRegionsForHeapProfiler(pids, callback.Get());
   run_loop.Run();
 }
 

services/resource_coordinator/memory_instrumentation/queued_request.cc

@@ -19,6 +19,16 @@ QueuedRequest::Args::Args(MemoryDumpType dump_type,
 QueuedRequest::Args::Args(const Args& args) = default;
 QueuedRequest::Args::~Args() = default;
 
+QueuedRequest::PendingResponse::PendingResponse(
+    const mojom::ClientProcess* client,
+    Type type)
+    : client(client), type(type) {}
+
+bool QueuedRequest::PendingResponse::operator<(
+    const PendingResponse& other) const {
+  return std::tie(client, type) < std::tie(other.client, other.type);
+}
+
 QueuedRequest::Response::Response() {}
 QueuedRequest::Response::~Response() = default;
 
@@ -37,14 +47,14 @@ base::trace_event::MemoryDumpRequestArgs QueuedRequest::GetRequestArgs() {
   return request_args;
 }
 
-QueuedRequest::PendingResponse::PendingResponse(
-    const mojom::ClientProcess* client,
-    Type type)
-    : client(client), type(type) {}
+QueuedVmRegionRequest::Response::Response() = default;
+QueuedVmRegionRequest::Response::~Response() = default;
 
-bool QueuedRequest::PendingResponse::operator<(
-    const PendingResponse& other) const {
-  return std::tie(client, type) < std::tie(other.client, other.type);
-}
+QueuedVmRegionRequest::QueuedVmRegionRequest(
+    uint64_t dump_guid,
+    const mojom::HeapProfilerHelper::GetVmRegionsForHeapProfilerCallback&
+        callback)
+    : dump_guid(dump_guid), callback(callback) {}
+QueuedVmRegionRequest::~QueuedVmRegionRequest() = default;
 
-}  // namespace memory_instrumentation
\ No newline at end of file
+}  // namespace memory_instrumentation

services/resource_coordinator/memory_instrumentation/queued_request.h

@@ -19,11 +19,12 @@ using base::trace_event::MemoryDumpType;
 
 namespace memory_instrumentation {
 
+using OSMemDumpMap =
+    std::unordered_map<base::ProcessId,
+                       memory_instrumentation::mojom::RawOSMemDumpPtr>;
+
 // Holds data for pending requests enqueued via RequestGlobalMemoryDump().
 struct QueuedRequest {
-  using OSMemDumpMap =
-      std::unordered_map<base::ProcessId,
-                         memory_instrumentation::mojom::RawOSMemDumpPtr>;
   using RequestGlobalMemoryDumpInternalCallback = base::Callback<
       void(bool, uint64_t, memory_instrumentation::mojom::GlobalMemoryDumpPtr)>;
 
@@ -99,7 +100,7 @@ struct QueuedRequest {
   // set contains a |PendingResponse| for each |RequestChromeMemoryDump| and
   // |RequestOSMemoryDump| call that has not yet replied or been canceled (due
   // to the client disconnecting).
-  std::set<QueuedRequest::PendingResponse> pending_responses;
+  std::set<PendingResponse> pending_responses;
   std::map<mojom::ClientProcess*, Response> responses;
   int failed_memory_dump_count = 0;
   bool dump_in_progress = false;
@@ -109,6 +110,28 @@ struct QueuedRequest {
   base::Time start_time;
 };
 
+// Holds data for pending requests enqueued via GetVmRegionsForHeapProfiler().
+struct QueuedVmRegionRequest {
+  QueuedVmRegionRequest(
+      uint64_t dump_guid,
+      const mojom::HeapProfilerHelper::GetVmRegionsForHeapProfilerCallback&
+          callback);
+  ~QueuedVmRegionRequest();
+  const uint64_t dump_guid;
+  const mojom::HeapProfilerHelper::GetVmRegionsForHeapProfilerCallback callback;
+
+  struct Response {
+    Response();
+    ~Response();
+
+    base::ProcessId process_id;
+    OSMemDumpMap os_dumps;
+  };
+
+  std::set<mojom::ClientProcess*> pending_responses;
+  std::map<mojom::ClientProcess*, Response> responses;
+};
+
 }  // namespace memory_instrumentation
 
-#endif  // SERVICES_RESOURCE_COORDINATOR_MEMORY_INSTRUMENTATION_QUEUED_REQUEST_H_
\ No newline at end of file
+#endif  // SERVICES_RESOURCE_COORDINATOR_MEMORY_INSTRUMENTATION_QUEUED_REQUEST_H_

services/resource_coordinator/memory_instrumentation/queued_request_dispatcher.cc

@@ -203,6 +203,7 @@ std::unique_ptr<TracedValue> GetChromeDumpAndGlobalAndEdgesTracedValue(
 
 }  // namespace
 
+// static
 void QueuedRequestDispatcher::SetUpAndDispatch(
     QueuedRequest* request,
     const std::vector<ClientInfo>& clients,
@@ -301,6 +302,89 @@ void QueuedRequestDispatcher::SetUpAndDispatch(
   }
 }
 
+// static
+void QueuedRequestDispatcher::SetUpAndDispatchVmRegionRequest(
+    QueuedVmRegionRequest* request,
+    const std::vector<ClientInfo>& clients,
+    const std::vector<base::ProcessId>& desired_pids,
+    const OsCallback& os_callback) {
+// On Linux, OS stats can only be dumped from a privileged process to
+// get around to sandboxing/selinux restrictions (see crbug.com/461788).
+#if defined(OS_LINUX)
+  mojom::ClientProcess* browser_client = nullptr;
+  base::ProcessId browser_client_pid = 0;
+  for (const auto& client_info : clients) {
+    if (client_info.process_type == mojom::ProcessType::BROWSER) {
+      browser_client = client_info.client;
+      browser_client_pid = client_info.pid;
+      break;
+    }
+  }
+
+  if (!browser_client) {
+    DLOG(ERROR) << "Missing browser client.";
+    return;
+  }
+
+  request->pending_responses.insert(browser_client);
+  request->responses[browser_client].process_id = browser_client_pid;
+  const auto callback = base::Bind(os_callback, browser_client);
+  browser_client->RequestOSMemoryDump(true /* wants_mmaps */, desired_pids,
+                                      callback);
+#else
+  for (const auto& client_info : clients) {
+    if (std::find(desired_pids.begin(), desired_pids.end(), client_info.pid) !=
+        desired_pids.end()) {
+      mojom::ClientProcess* client = client_info.client;
+      request->pending_responses.insert(client);
+      request->responses[client].process_id = client_info.pid;
+      client->RequestOSMemoryDump(true /* wants_mmaps */,
+                                  {base::kNullProcessId},
+                                  base::Bind(os_callback, client));
+    }
+  }
+#endif  // defined(OS_LINUX)
+}
+
+// static
+QueuedRequestDispatcher::VmRegions
+QueuedRequestDispatcher::FinalizeVmRegionRequest(
+    QueuedVmRegionRequest* request) {
+  VmRegions results;
+  for (auto& response : request->responses) {
+    const base::ProcessId& original_pid = response.second.process_id;
+
+    // |response| accumulates the replies received by each client process.
+    // On Linux, the browser process will provide all OS dumps. On non-Linux,
+    // each client process provides 1 OS dump, % the case where the client is
+    // disconnected mid dump.
+    OSMemDumpMap& extra_os_dumps = response.second.os_dumps;
+#if defined(OS_LINUX)
+    for (auto& kv : extra_os_dumps) {
+      auto pid = kv.first == base::kNullProcessId ? original_pid : kv.first;
+      DCHECK(results.find(pid) == results.end());
+      results.emplace(pid, std::move(kv.second->memory_maps));
+    }
+#else
+    // This can be empty if the client disconnects before providing both
+    // dumps. See UnregisterClientProcess().
+    DCHECK_LE(extra_os_dumps.size(), 1u);
+    for (auto& kv : extra_os_dumps) {
+      // When the OS dump comes from child processes, the pid is supposed to be
+      // not used. We know the child process pid at the time of the request and
+      // also wouldn't trust pids coming from child processes.
+      DCHECK_EQ(base::kNullProcessId, kv.first);
+
+      // Check we don't receive duplicate OS dumps for the same process.
+      DCHECK(results.find(original_pid) == results.end());
+
+      results.emplace(original_pid, std::move(kv.second->memory_maps));
+    }
+#endif
+  }
+  return results;
+}
+
 void QueuedRequestDispatcher::Finalize(QueuedRequest* request,
                                        TracingObserver* tracing_observer) {
   DCHECK(request->dump_in_progress);
@@ -427,13 +511,6 @@ void QueuedRequestDispatcher::Finalize(QueuedRequest* request,
     if (!valid)
       continue;
 
-    if (request->args.level_of_detail ==
-        MemoryDumpLevelOfDetail::VM_REGIONS_ONLY_FOR_HEAP_PROFILER) {
-      DCHECK(request->wants_mmaps());
-      os_dump->memory_maps_for_heap_profiler =
-          std::move(raw_os_dump->memory_maps);
-    }
-
     // TODO(hjd): not sure we need an empty instance for the !SUMMARY_ONLY
     // requests. Check and make the else branch a nullptr otherwise.
     mojom::ChromeMemDumpPtr chrome_dump =

services/resource_coordinator/memory_instrumentation/queued_request_dispatcher.h

@@ -30,6 +30,9 @@ class QueuedRequestDispatcher {
       std::unique_ptr<base::trace_event::ProcessMemoryDump>)>;
   using OsCallback =
       base::Callback<void(mojom::ClientProcess*, bool, OSMemDumpMap)>;
+  using VmRegions = std::unordered_map<
+      base::ProcessId,
+      std::vector<memory_instrumentation::mojom::VmRegionPtr>>;
 
   struct ClientInfo {
     ClientInfo(mojom::ClientProcess* client,
@@ -55,6 +58,13 @@ class QueuedRequestDispatcher {
   static void Finalize(QueuedRequest* request,
                        TracingObserver* tracing_observer);
 
+  static void SetUpAndDispatchVmRegionRequest(
+      QueuedVmRegionRequest* request,
+      const std::vector<ClientInfo>& clients,
+      const std::vector<base::ProcessId>& desired_pids,
+      const OsCallback& os_callback);
+  static VmRegions FinalizeVmRegionRequest(QueuedVmRegionRequest* request);
+
  private:
   static bool AddChromeMemoryDumpToTrace(
       const base::trace_event::MemoryDumpRequestArgs& args,
@@ -67,4 +77,4 @@ class QueuedRequestDispatcher {
 
 }  // namespace memory_instrumentation
 
-#endif  // SERVICES_RESOURCE_COORDINATOR_MEMORY_INSTRUMENTATION_QUEUED_REQUEST_DISPATCHER_H_
\ No newline at end of file
+#endif  // SERVICES_RESOURCE_COORDINATOR_MEMORY_INSTRUMENTATION_QUEUED_REQUEST_DISPATCHER_H_

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

@@ -62,10 +62,6 @@ EnumTraits<memory_instrumentation::mojom::LevelOfDetail,
       return memory_instrumentation::mojom::LevelOfDetail::BACKGROUND;
     case base::trace_event::MemoryDumpLevelOfDetail::LIGHT:
       return memory_instrumentation::mojom::LevelOfDetail::LIGHT;
-    case base::trace_event::MemoryDumpLevelOfDetail::
-        VM_REGIONS_ONLY_FOR_HEAP_PROFILER:
-      return memory_instrumentation::mojom::LevelOfDetail::
-          VM_REGIONS_ONLY_FOR_HEAP_PROFILER;
     case base::trace_event::MemoryDumpLevelOfDetail::DETAILED:
       return memory_instrumentation::mojom::LevelOfDetail::DETAILED;
     default:
@@ -87,11 +83,6 @@ bool EnumTraits<memory_instrumentation::mojom::LevelOfDetail,
     case memory_instrumentation::mojom::LevelOfDetail::LIGHT:
       *out = base::trace_event::MemoryDumpLevelOfDetail::LIGHT;
       break;
-    case memory_instrumentation::mojom::LevelOfDetail::
-        VM_REGIONS_ONLY_FOR_HEAP_PROFILER:
-      *out = base::trace_event::MemoryDumpLevelOfDetail::
-          VM_REGIONS_ONLY_FOR_HEAP_PROFILER;
-      break;
     case memory_instrumentation::mojom::LevelOfDetail::DETAILED:
       *out = base::trace_event::MemoryDumpLevelOfDetail::DETAILED;
       break;

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

@@ -18,7 +18,6 @@ enum DumpType {
 enum LevelOfDetail {
   BACKGROUND,
   LIGHT,
-  VM_REGIONS_ONLY_FOR_HEAP_PROFILER,
   DETAILED
 };
 
@@ -147,11 +146,6 @@ struct OSMemDump {
   // in kilobytes. For more details, see https://goo.gl/3kPb9S.
   uint32 shared_footprint_kb = 0;
 
-  // This field is present only when the dump is obtained through
-  // Coordinator.GetVmRegionsForHeapProfiler(), empty in the generic case of
-  // RequestGlobalMemoryDump().
-  array<VmRegion> memory_maps_for_heap_profiler;
-
   // This is private swapped memory in kilobytes reported on Linux and Android
   // only.
   [EnableIf=OS_LINUX_OR_OS_ANDROID]
@@ -263,8 +257,7 @@ interface Coordinator {
 // Used by the Chrome heap profiler
 interface HeapProfilerHelper {
   // Broadcasts a RequestOSMemoryDump-only request for all registered client
-  // processes, retrieves only their memory maps and returns them in the field
-  // |global_memory_dump.process_dumps.os_dump.memory_maps_for_heap_profiler|.
-  GetVmRegionsForHeapProfiler() =>
-      (bool success, GlobalMemoryDump? global_memory_dump);
+  // processes and retrieves only their memory maps.
+  GetVmRegionsForHeapProfiler(array<mojo.common.mojom.ProcessId> pids) =>
+      (map<mojo.common.mojom.ProcessId, array<VmRegion>> vm_regions);
 };