Revert "Add RequestPrivateMemoryFootprint method to MemoryInstrumentation."

This reverts commit 43e035ee.

Reason for revert: browser_test failures on
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Win7%20Tests%20%28dbg%29%281%29 starting around 69747

Small subset of tests failing:
PrerenderBrowserTest.PrerenderPageNewTab
DefaultIsolation/TaskManagerOOPIFBrowserTest.OrderingOfDependentRows/0
PrerenderBrowserTest.PrerenderDownloadClientRedirect
PrintPreviewBrowserTest.TaskManagerNewPrintPreview
TabManagerTest.OomPressureListener

Stack:
Received fatal exception EXCEPTION_BREAKPOINT
Backtrace:
base::win::RegisterInvalidParamHandler [0x738B4167+71]
invalid_parameter [0x6FDAECD1+161]
std::_Tree_const_iterator<std::_Tree_val<std::_Tree_simple_types<std::pair<unsigned long const ,std::unique_ptr<memory_instrumentation::GlobalDumpGraph::Process,std::default_delete<memory_instrumentation::GlobalDumpGraph::Process> > > > > >::operator* [0x6296CB0C+156]
std::_Tree_const_iterator<std::_Tree_val<std::_Tree_simple_types<std::pair<unsigned long const ,std::unique_ptr<memory_instrumentation::GlobalDumpGraph::Process,std::default_delete<memory_instrumentation::GlobalDumpGraph::Process> > > > > >::operator-> [0x6299A3C1+17]
memory_instrumentation::QueuedRequestDispatcher::Finalize [0x62998EF4+3364]
memory_instrumentation::CoordinatorImpl::FinalizeGlobalMemoryDumpIfAllManagersReplied [0x629303EB+587]
memory_instrumentation::CoordinatorImpl::OnOSMemoryDumpResponse [0x629324F3+819]

Original change's description:
> Add RequestPrivateMemoryFootprint method to MemoryInstrumentation.
> 
> Most clients only need private memory footprint, but the existing methods would
> still query all the MemoryDumpProviders and then filter the results. This CL
> adds a new method that avoids querying MemoryDumpProviders.
> 
> Bug: 851712
> Change-Id: I269be2b5a74c32205ba6f4087a9bb8affb9cffb6
> Reviewed-on: https://chromium-review.googlesource.com/1098623
> Reviewed-by: Primiano Tucci <primiano@chromium.org>
> Reviewed-by: Nico Weber <thakis@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Commit-Queue: Erik Chen <erikchen@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#567417}

TBR=dcheng@chromium.org,thakis@chromium.org,primiano@chromium.org,erikchen@chromium.org,lalitm@chromium.org

Change-Id: I5dd27418aad49f2fcfbcb10c5510f11b91f4d875
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 851712
Reviewed-on: https://chromium-review.googlesource.com/1102518Reviewed-by: Robert Liao <robliao@chromium.org>
Commit-Queue: Robert Liao <robliao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567641}

chrome/browser/memory_details.cc

@@ -360,10 +360,9 @@ void MemoryDetails::CollectChildInfoOnUIThread() {
   // Using AdaptCallbackForRepeating allows for an easier transition to
   // OnceCallbacks for https://crbug.com/714018.
   memory_instrumentation::MemoryInstrumentation::GetInstance()
-      ->RequestPrivateMemoryFootprint(
-          base::kNullProcessId,
-          base::AdaptCallbackForRepeating(
-              base::BindOnce(&MemoryDetails::DidReceiveMemoryDump, this)));
+      ->RequestGlobalDump(std::vector<std::string>(),
+                          base::AdaptCallbackForRepeating(base::BindOnce(
+                              &MemoryDetails::DidReceiveMemoryDump, this)));
 }
 
 void MemoryDetails::DidReceiveMemoryDump(

chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc

@@ -586,7 +586,7 @@ void DataReductionProxyMetricsObserver::RequestProcessDump(
     memory_instrumentation::MemoryInstrumentation::RequestGlobalDumpCallback
         callback) {
   memory_instrumentation::MemoryInstrumentation::GetInstance()
-      ->RequestPrivateMemoryFootprint(pid, callback);
+      ->RequestGlobalDumpForPid(pid, callback);
 }
 
 }  // namespace data_reduction_proxy

chrome/browser/prerender/prerender_contents.cc

@@ -655,10 +655,10 @@ void PrerenderContents::DestroyWhenUsingTooManyResources() {
   // Using AdaptCallbackForRepeating allows for an easier transition to
   // OnceCallbacks for https://crbug.com/714018.
   memory_instrumentation::MemoryInstrumentation::GetInstance()
-      ->RequestPrivateMemoryFootprint(
-          process_pid_, base::AdaptCallbackForRepeating(base::BindOnce(
-                            &PrerenderContents::DidGetMemoryUsage,
-                            weak_factory_.GetWeakPtr())));
+      ->RequestGlobalDumpForPid(process_pid_,
+                                base::AdaptCallbackForRepeating(base::BindOnce(
+                                    &PrerenderContents::DidGetMemoryUsage,
+                                    weak_factory_.GetWeakPtr())));
 }
 
 void PrerenderContents::DidGetMemoryUsage(

chrome/browser/profiling_host/background_profiling_triggers.cc

@@ -122,8 +122,7 @@ void BackgroundProfilingTriggers::PerformMemoryUsageChecks() {
       [](base::WeakPtr<BackgroundProfilingTriggers> weak_ptr,
          std::vector<base::ProcessId> result) {
         memory_instrumentation::MemoryInstrumentation::GetInstance()
-            ->RequestPrivateMemoryFootprint(
-                base::kNullProcessId,
+            ->RequestGlobalDump(
                 base::Bind(&BackgroundProfilingTriggers::OnReceivedMemoryDump,
                            std::move(weak_ptr), std::move(result)));
       },

chrome/browser/resource_coordinator/render_process_probe.cc

@@ -140,11 +140,9 @@ void RenderProcessProbeImpl::
 
   // Dispatch the memory collection request.
   memory_instrumentation::MemoryInstrumentation::GetInstance()
-      ->RequestPrivateMemoryFootprint(
-          base::kNullProcessId,
-          base::BindRepeating(&RenderProcessProbeImpl::
-                                  ProcessGlobalMemoryDumpAndDispatchOnIOThread,
-                              base::Unretained(this), collection_start_time));
+      ->RequestGlobalDump(base::BindRepeating(
+          &RenderProcessProbeImpl::ProcessGlobalMemoryDumpAndDispatchOnIOThread,
+          base::Unretained(this), collection_start_time));
 
   RenderProcessInfoMap::iterator iter = render_process_info_map_.begin();
   while (iter != render_process_info_map_.end()) {

chrome/browser/task_manager/sampling/task_manager_impl.cc

@@ -586,8 +586,7 @@ void TaskManagerImpl::Refresh() {
     auto callback = base::Bind(&TaskManagerImpl::OnReceivedMemoryDump,
                                weak_ptr_factory_.GetWeakPtr());
     memory_instrumentation::MemoryInstrumentation::GetInstance()
-        ->RequestPrivateMemoryFootprint(base::kNullProcessId,
-                                        std::move(callback));
+        ->RequestGlobalDump(std::move(callback));
   }
 
   if (base::FeatureList::IsEnabled(network::features::kNetworkService) &&

content/browser/tracing/memory_instrumentation_browsertest.cc

@@ -53,16 +53,15 @@ std::unique_ptr<GlobalMemoryDump> DoGlobalDump() {
   std::unique_ptr<GlobalMemoryDump> result = nullptr;
   base::RunLoop run_loop;
   memory_instrumentation::MemoryInstrumentation::GetInstance()
-      ->RequestGlobalDump(
-          {}, base::Bind(
-                  [](base::Closure quit_closure,
-                     std::unique_ptr<GlobalMemoryDump>* out_result,
-                     bool success, std::unique_ptr<GlobalMemoryDump> result) {
-                    EXPECT_TRUE(success);
-                    *out_result = std::move(result);
-                    std::move(quit_closure).Run();
-                  },
-                  run_loop.QuitClosure(), &result));
+      ->RequestGlobalDump(base::Bind(
+          [](base::Closure quit_closure,
+             std::unique_ptr<GlobalMemoryDump>* out_result, bool success,
+             std::unique_ptr<GlobalMemoryDump> result) {
+            EXPECT_TRUE(success);
+            *out_result = std::move(result);
+            std::move(quit_closure).Run();
+          },
+          run_loop.QuitClosure(), &result));
   run_loop.Run();
   return result;
 }

services/resource_coordinator/memory_instrumentation/coordinator_impl.cc

@@ -120,8 +120,7 @@ void CoordinatorImpl::RequestGlobalMemoryDump(
   };
 
   QueuedRequest::Args args(dump_type, level_of_detail, allocator_dump_names,
-                           false /* add_to_trace */, base::kNullProcessId,
-                           /*memory_footprint_only=*/false);
+                           false /* add_to_trace */, base::kNullProcessId);
   RequestGlobalMemoryDumpInternal(args,
                                   base::BindOnce(adapter, std::move(callback)));
 }
@@ -148,27 +147,7 @@ void CoordinatorImpl::RequestGlobalMemoryDumpForPid(
   QueuedRequest::Args args(
       base::trace_event::MemoryDumpType::SUMMARY_ONLY,
       base::trace_event::MemoryDumpLevelOfDetail::BACKGROUND,
-      allocator_dump_names, false /* add_to_trace */, pid,
-      /*memory_footprint_only=*/false);
-  RequestGlobalMemoryDumpInternal(args,
-                                  base::BindOnce(adapter, std::move(callback)));
-}
-
-void CoordinatorImpl::RequestPrivateMemoryFootprint(
-    base::ProcessId pid,
-    RequestPrivateMemoryFootprintCallback callback) {
-  // This merely strips out the |dump_guid| argument; this is not relevant
-  // as we are not adding to trace.
-  auto adapter = [](RequestPrivateMemoryFootprintCallback callback,
-                    bool success, uint64_t,
-                    mojom::GlobalMemoryDumpPtr global_memory_dump) {
-    std::move(callback).Run(success, std::move(global_memory_dump));
-  };
-
-  QueuedRequest::Args args(
-      base::trace_event::MemoryDumpType::SUMMARY_ONLY,
-      base::trace_event::MemoryDumpLevelOfDetail::BACKGROUND, {},
-      false /* add_to_trace */, pid, /*memory_footprint_only=*/true);
+      allocator_dump_names, false /* add_to_trace */, pid);
   RequestGlobalMemoryDumpInternal(args,
                                   base::BindOnce(adapter, std::move(callback)));
 }
@@ -185,8 +164,7 @@ void CoordinatorImpl::RequestGlobalMemoryDumpAndAppendToTrace(
   };
 
   QueuedRequest::Args args(dump_type, level_of_detail, {},
-                           true /* add_to_trace */, base::kNullProcessId,
-                           /*memory_footprint_only=*/false);
+                           true /* add_to_trace */, base::kNullProcessId);
   RequestGlobalMemoryDumpInternal(args,
                                   base::BindOnce(adapter, std::move(callback)));
 }

services/resource_coordinator/memory_instrumentation/coordinator_impl.h

@@ -67,9 +67,6 @@ class CoordinatorImpl : public Coordinator,
       base::ProcessId,
       const std::vector<std::string>& allocator_dump_names,
       RequestGlobalMemoryDumpForPidCallback) override;
-  void RequestPrivateMemoryFootprint(
-      base::ProcessId,
-      RequestPrivateMemoryFootprintCallback) override;
   void RequestGlobalMemoryDumpAndAppendToTrace(
       base::trace_event::MemoryDumpType,
       base::trace_event::MemoryDumpLevelOfDetail,

services/resource_coordinator/memory_instrumentation/queued_request.cc

@@ -10,14 +10,12 @@ QueuedRequest::Args::Args(MemoryDumpType dump_type,
                           MemoryDumpLevelOfDetail level_of_detail,
                           const std::vector<std::string>& allocator_dump_names,
                           bool add_to_trace,
-                          base::ProcessId pid,
-                          bool memory_footprint_only)
+                          base::ProcessId pid)
     : dump_type(dump_type),
       level_of_detail(level_of_detail),
       allocator_dump_names(allocator_dump_names),
       add_to_trace(add_to_trace),
-      pid(pid),
-      memory_footprint_only(memory_footprint_only) {}
+      pid(pid) {}
 QueuedRequest::Args::Args(const Args& args) = default;
 QueuedRequest::Args::~Args() = default;
 

services/resource_coordinator/memory_instrumentation/queued_request.h

@@ -33,8 +33,7 @@ struct QueuedRequest {
          MemoryDumpLevelOfDetail level_of_detail,
          const std::vector<std::string>& allocator_dump_names,
          bool add_to_trace,
-         base::ProcessId pid,
-         bool memory_footprint_only);
+         base::ProcessId pid);
     Args(const Args&);
     ~Args();
 
@@ -43,10 +42,6 @@ struct QueuedRequest {
     const std::vector<std::string> allocator_dump_names;
     const bool add_to_trace;
     const base::ProcessId pid;
-
-    // If this member is |true|, then no MemoryDumpProviders are queried. The
-    // only other relevant member is |pid|.
-    const bool memory_footprint_only;
   };
 
   struct PendingResponse {

services/resource_coordinator/memory_instrumentation/queued_request_dispatcher.cc

@@ -216,20 +216,18 @@ void QueuedRequestDispatcher::SetUpAndDispatch(
     request->responses[client].process_id = client_info.pid;
     request->responses[client].process_type = client_info.process_type;
 
-    // Don't request a chrome memory dump at all if the client only wants the
-    // a memory footprint.
+    // Don't request a chrome memory dump at all if the client wants only the
+    // processes' vm regions, which are retrieved via RequestOSMemoryDump().
     //
     // This must occur before the call to RequestOSMemoryDump, as
     // ClientProcessImpl will [for macOS], delay the calculations for the
     // OSMemoryDump until the Chrome memory dump is finished. See
     // https://bugs.chromium.org/p/chromium/issues/detail?id=812346#c16 for more
     // details.
-    if (!request->args.memory_footprint_only) {
-      request->pending_responses.insert({client, ResponseType::kChromeDump});
-      client->RequestChromeMemoryDump(
-          request->GetRequestArgs(),
-          base::BindOnce(std::move(chrome_callback), client));
-    }
+    request->pending_responses.insert({client, ResponseType::kChromeDump});
+    client->RequestChromeMemoryDump(
+        request->GetRequestArgs(),
+        base::BindOnce(std::move(chrome_callback), client));
 
 // On most platforms each process can dump data about their own process
 // so ask each process to do so Linux is special see below.
@@ -498,16 +496,11 @@ void QueuedRequestDispatcher::Finalize(QueuedRequest* request,
       }
     }
 
-    bool valid = false;
-    if (request->args.memory_footprint_only) {
-      valid = raw_os_dump;
-    } else {
-      // Ignore incomplete results (can happen if the client
-      // crashes/disconnects).
-      valid = raw_os_dump && raw_chrome_dump &&
-              (request->memory_map_option() == mojom::MemoryMapOption::NONE ||
-               (raw_os_dump && !raw_os_dump->memory_maps.empty()));
-    }
+    // Ignore incomplete results (can happen if the client crashes/disconnects).
+    const bool valid =
+        raw_os_dump && raw_chrome_dump &&
+        (request->memory_map_option() == mojom::MemoryMapOption::NONE ||
+         (raw_os_dump && !raw_os_dump->memory_maps.empty()));
 
     if (!valid)
       continue;

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

@@ -51,6 +51,11 @@ MemoryInstrumentation::~MemoryInstrumentation() {
   g_instance = nullptr;
 }
 
+void MemoryInstrumentation::RequestGlobalDump(
+    RequestGlobalDumpCallback callback) {
+  RequestGlobalDump({}, callback);
+}
+
 void MemoryInstrumentation::RequestGlobalDump(
     const std::vector<std::string>& allocator_dump_names,
     RequestGlobalDumpCallback callback) {
@@ -61,12 +66,10 @@ void MemoryInstrumentation::RequestGlobalDump(
       base::BindRepeating(&WrapGlobalMemoryDump, callback));
 }
 
-void MemoryInstrumentation::RequestPrivateMemoryFootprint(
+void MemoryInstrumentation::RequestGlobalDumpForPid(
     base::ProcessId pid,
     RequestGlobalDumpCallback callback) {
-  const auto& coordinator = GetCoordinatorBindingForCurrentThread();
-  coordinator->RequestPrivateMemoryFootprint(
-      pid, base::BindRepeating(&WrapGlobalMemoryDump, callback));
+  RequestGlobalDumpForPid(pid, {}, callback);
 }
 
 void MemoryInstrumentation::RequestGlobalDumpForPid(

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

@@ -40,6 +40,16 @@ class SERVICES_RESOURCE_COORDINATOR_PUBLIC_CPP_EXPORT MemoryInstrumentation {
                              const std::string& service_name);
   static MemoryInstrumentation* GetInstance();
 
+  // Requests a global memory dump.
+  // Returns asynchronously, via the callback argument:
+  //  (true, global_dump) if succeeded;
+  //  (false, nullptr) if failed.
+  // The callback (if not null), will be posted on the same thread of the
+  // RequestGlobalDump() call.
+  // TODO(lalitm): this is temporary until we migrate task manager, metrics
+  // etc to use the new method.
+  void RequestGlobalDump(RequestGlobalDumpCallback);
+
   // Requests a global memory dump with |allocator_dump_names| indicating
   // the name of allocator dumps in which the consumer is interested. If
   // |allocator_dump_names| is empty, no dumps will be returned.
@@ -49,21 +59,16 @@ class SERVICES_RESOURCE_COORDINATOR_PUBLIC_CPP_EXPORT MemoryInstrumentation {
   //  but missing data.
   // The callback (if not null), will be posted on the same thread of the
   // RequestGlobalDump() call.
-  // Note: Even if |allocator_dump_names| is empty, all MemoryDumpProviders will
-  // still be queried.
   void RequestGlobalDump(const std::vector<std::string>& allocator_dump_names,
                          RequestGlobalDumpCallback);
 
-  // Returns asynchronously, via the callback argument:
-  //  (true, global_dump) if succeeded;
-  //  (false, global_dump) if failed, with global_dump being non-null
-  //  but missing data.
+  // Requests a global memory dump.
+  // Returns asynchronously, via the callback argument, the global memory
+  // dump with the process memory dump for the given pid or null if the dump
+  // failed.
   // The callback (if not null), will be posted on the same thread of the
-  // RequestPrivateMemoryFootprint() call.
-  // Passing a null |pid| is the same as requesting the PrivateMemoryFootprint
-  // for all processes.
-  void RequestPrivateMemoryFootprint(base::ProcessId pid,
-                                     RequestGlobalDumpCallback);
+  // RequestGlobalDumpForPid() call.
+  void RequestGlobalDumpForPid(base::ProcessId pid, RequestGlobalDumpCallback);
 
   // Requests a global memory dump with |allocator_dump_names| indicating
   // the name of allocator dumps in which the consumer is interested. If

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

@@ -123,10 +123,6 @@ class MockCoordinator : public Coordinator, public mojom::Coordinator {
       const std::vector<std::string>& allocator_dump_names,
       RequestGlobalMemoryDumpForPidCallback) override {}
 
-  void RequestPrivateMemoryFootprint(
-      base::ProcessId pid,
-      RequestPrivateMemoryFootprintCallback) override {}
-
   void RequestGlobalMemoryDumpAndAppendToTrace(
       MemoryDumpType dump_type,
       MemoryDumpLevelOfDetail level_of_detail,

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

@@ -279,12 +279,6 @@ interface Coordinator {
                                 array<string> allocator_dump_names) =>
       (bool success, GlobalMemoryDump? global_memory_dump);
 
-  // Requesting a null pid is the same as requesting all pids.
-  // The returned dump only contains OS metrics. |chrome_allocator_dumps| will
-  // be empty.
-  RequestPrivateMemoryFootprint(mojo_base.mojom.ProcessId pid) =>
-      (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(DumpType dump_type,