memory-infra: Stop using RequestGlobalDump in MemoryDumpManagerTest

This CL changes the memory-infra unittests to not depend on
the RequestGlobalDump() API. From a logical viewpoint,
the RequestGlobalDump() is not needed for an in-process-only
test like MemoryDumpManagerTest.
From a more practical viewpoint, this refactoring is required
in preparation of the next CLs that will remove
MemoryDumpManager::RequestGlobalDump() method and move all the
clients to directly use the service.

This CL doesn't cause any behavior change in memory-infra.

BUG=720352
TBR=thakis

Change-Id: I82ee1bf94dab55798be727f0ae70a1fede6634ca
Reviewed-on: https://chromium-review.googlesource.com/525534Reviewed-by: Primiano Tucci <primiano@chromium.org>
Reviewed-by: siddhartha sivakumar <ssid@chromium.org>
Reviewed-by: Helen Li <xunjieli@chromium.org>
Reviewed-by: Hector Dearman <hjd@chromium.org>
Commit-Queue: siddhartha sivakumar <ssid@chromium.org>
Cr-Commit-Position: refs/heads/master@{#477851}

base/BUILD.gn

@@ -966,6 +966,7 @@ component("base") {
     "trace_event/memory_allocator_dump_guid.h",
     "trace_event/memory_dump_manager.cc",
     "trace_event/memory_dump_manager.h",
+    "trace_event/memory_dump_manager_test_utils.h",
     "trace_event/memory_dump_provider.h",
     "trace_event/memory_dump_provider_info.cc",
     "trace_event/memory_dump_provider_info.h",

base/trace_event/memory_dump_manager.cc

@@ -130,9 +130,6 @@ void OnPeriodicSchedulerTick(MemoryDumpLevelOfDetail level_of_detail) {
 const char* const MemoryDumpManager::kTraceCategory =
     TRACE_DISABLED_BY_DEFAULT("memory-infra");
 
-// static
-const char* const MemoryDumpManager::kLogPrefix = "Memory-infra dump";
-
 // static
 const int MemoryDumpManager::kMaxConsecutiveFailuresCount = 3;
 
@@ -159,6 +156,7 @@ MemoryDumpManager* MemoryDumpManager::GetInstance() {
 // static
 std::unique_ptr<MemoryDumpManager>
 MemoryDumpManager::CreateInstanceForTesting() {
+  DCHECK(!g_instance_for_testing);
   std::unique_ptr<MemoryDumpManager> instance(new MemoryDumpManager());
   g_instance_for_testing = instance.get();
   return instance;
@@ -428,10 +426,8 @@ void MemoryDumpManager::RequestGlobalDump(
     MemoryDumpType dump_type,
     MemoryDumpLevelOfDetail level_of_detail,
     const GlobalMemoryDumpCallback& callback) {
-  // If |request_dump_function_| is null MDM hasn't been initialized yet.
-  if (request_dump_function_.is_null()) {
-    VLOG(1) << kLogPrefix << " failed because"
-            << " memory dump manager is not enabled.";
+  if (!is_initialized()) {
+    VLOG(1) << "RequestGlobalDump() FAIL: MemoryDumpManager is not initialized";
     if (!callback.is_null())
       callback.Run(0u /* guid */, false /* success */);
     return;
@@ -468,7 +464,8 @@ void MemoryDumpManager::GetDumpProvidersForPolling(
 void MemoryDumpManager::RequestGlobalDump(
     MemoryDumpType dump_type,
     MemoryDumpLevelOfDetail level_of_detail) {
-  RequestGlobalDump(dump_type, level_of_detail, GlobalMemoryDumpCallback());
+  auto noop_callback = [](uint64_t dump_guid, bool success) {};
+  RequestGlobalDump(dump_type, level_of_detail, Bind(noop_callback));
 }
 
 bool MemoryDumpManager::IsDumpProviderRegisteredForTesting(
@@ -499,6 +496,15 @@ MemoryDumpManager::GetOrCreateBgTaskRunnerLocked() {
 void MemoryDumpManager::CreateProcessDump(
     const MemoryDumpRequestArgs& args,
     const ProcessMemoryDumpCallback& callback) {
+  if (!is_initialized()) {
+    VLOG(1) << "CreateProcessDump() FAIL: MemoryDumpManager is not initialized";
+    if (!callback.is_null()) {
+      callback.Run(args.dump_guid, false /* success */,
+                   Optional<MemoryDumpCallbackResult>());
+    }
+    return;
+  }
+
   char guid_str[20];
   sprintf(guid_str, "0x%" PRIx64, args.dump_guid);
   TRACE_EVENT_NESTABLE_ASYNC_BEGIN1(kTraceCategory, "ProcessMemoryDump",
@@ -732,7 +738,7 @@ void MemoryDumpManager::FinalizeDumpAndAddToTrace(
 
   // The results struct to fill.
   // TODO(hjd): Transitional until we send the full PMD. See crbug.com/704203
-  base::Optional<MemoryDumpCallbackResult> result;
+  Optional<MemoryDumpCallbackResult> result;
 
   bool dump_successful = pmd_async_state->dump_successful;
 
@@ -862,7 +868,7 @@ void MemoryDumpManager::SetupForTracing(
     }
   }
 
-  // Only coordinator process triggers periodic global memory dumps.
+  // Only coordinator process triggers periodic memory dumps.
   if (is_coordinator_ && !periodic_config.triggers.empty()) {
     MemoryDumpScheduler::GetInstance()->Start(periodic_config,
                                               GetOrCreateBgTaskRunnerLocked());

base/trace_event/memory_dump_manager.h

@@ -46,7 +46,6 @@ class BASE_EXPORT MemoryDumpManager {
                              const GlobalMemoryDumpCallback& callback)>;
 
   static const char* const kTraceCategory;
-  static const char* const kLogPrefix;
 
   // This value is returned as the tracing id of the child processes by
   // GetTracingProcessId() when tracing is not enabled.
@@ -113,13 +112,12 @@ class BASE_EXPORT MemoryDumpManager {
   // to notify about the completion of the global dump (i.e. after all the
   // processes have dumped) and its success (true iff all the dumps were
   // successful).
-  void RequestGlobalDump(MemoryDumpType dump_type,
-                         MemoryDumpLevelOfDetail level_of_detail,
-                         const GlobalMemoryDumpCallback& callback);
+  void RequestGlobalDump(MemoryDumpType,
+                         MemoryDumpLevelOfDetail,
+                         const GlobalMemoryDumpCallback&);
 
   // Same as above (still asynchronous), but without callback.
-  void RequestGlobalDump(MemoryDumpType dump_type,
-                         MemoryDumpLevelOfDetail level_of_detail);
+  void RequestGlobalDump(MemoryDumpType, MemoryDumpLevelOfDetail);
 
   // Prepare MemoryDumpManager for RequestGlobalMemoryDump calls for tracing
   // related modes (non-SUMMARY_ONLY).
@@ -280,6 +278,9 @@ class BASE_EXPORT MemoryDumpManager {
   void GetDumpProvidersForPolling(
       std::vector<scoped_refptr<MemoryDumpProviderInfo>>*);
 
+  // Returns true if Initialize() has been called, false otherwise.
+  bool is_initialized() const { return !request_dump_function_.is_null(); }
+
   // An ordererd set of registered MemoryDumpProviderInfo(s), sorted by task
   // runner affinity (MDPs belonging to the same task runners are adjacent).
   MemoryDumpProviderInfo::OrderedSet dump_providers_;

base/trace_event/memory_dump_manager_test_utils.h

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef BASE_TRACE_EVENT_MEMORY_DUMP_MANAGER_TEST_UTILS_H_
+#define BASE_TRACE_EVENT_MEMORY_DUMP_MANAGER_TEST_UTILS_H_
+
+#include "base/bind.h"
+#include "base/trace_event/memory_dump_manager.h"
+#include "base/trace_event/memory_dump_request_args.h"
+
+namespace base {
+namespace trace_event {
+
+// Short-circuit the RequestGlobalDump() calls to CreateProcessDump().
+// Rationale: only the in-process logic is required for unittests.
+void RequestGlobalDumpForInProcessTesting(
+    const MemoryDumpRequestArgs& args,
+    const GlobalMemoryDumpCallback& global_callback) {
+  // Turns a ProcessMemoryDumpCallback into a GlobalMemoryDumpCallback.
+  auto callback_adapter = [](const GlobalMemoryDumpCallback& global_callback,
+                             uint64_t dump_guid, bool success,
+                             const Optional<MemoryDumpCallbackResult>& result) {
+    if (!global_callback.is_null())
+      global_callback.Run(dump_guid, success);
+  };
+  MemoryDumpManager::GetInstance()->CreateProcessDump(
+      args, Bind(callback_adapter, global_callback));
+};
+
+// Short circuits the RequestGlobalDumpFunction() to CreateProcessDump(),
+// effectively allowing to use both in unittests with the same behavior.
+// Unittests are in-process only and don't require all the multi-process
+// dump handshaking (which would require bits outside of base).
+void InitializeMemoryDumpManagerForInProcessTesting(bool is_coordinator) {
+  MemoryDumpManager* instance = MemoryDumpManager::GetInstance();
+  instance->set_dumper_registrations_ignored_for_testing(true);
+  instance->Initialize(BindRepeating(&RequestGlobalDumpForInProcessTesting),
+                       is_coordinator);
+  instance->set_dumper_registrations_ignored_for_testing(false);
+}
+
+}  // namespace trace_event
+}  // namespace base
+
+#endif  // BASE_TRACE_EVENT_MEMORY_DUMP_MANAGER_TEST_UTILS_H_

net/url_request/url_request_quic_perftest.cc

@@ -19,6 +19,7 @@
 #include "base/test/trace_event_analyzer.h"
 #include "base/time/time.h"
 #include "base/trace_event/memory_dump_manager.h"
+#include "base/trace_event/memory_dump_manager_test_utils.h"
 #include "base/trace_event/memory_dump_request_args.h"
 #include "base/trace_event/process_memory_dump.h"
 #include "base/trace_event/trace_buffer.h"
@@ -87,37 +88,12 @@ void PrintPerfTest(const std::string& name,
                          static_cast<double>(value), unit, true);
 }
 
-void RequestGlobalDumpCallback(base::Closure quit_closure,
-                               uint64_t,
-                               bool success) {
-  base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, quit_closure);
-  ASSERT_TRUE(success);
-}
-
-void ProcessDumpCallbackAdapter(
-    base::trace_event::GlobalMemoryDumpCallback callback,
-    uint64_t dump_guid,
-    bool success,
-    const base::Optional<base::trace_event::MemoryDumpCallbackResult>&) {
-  callback.Run(dump_guid, success);
-}
-
-void RequestGlobalMemoryDumpCallback(
-    const base::trace_event::MemoryDumpRequestArgs& args,
-    const base::trace_event::GlobalMemoryDumpCallback& callback) {
-  base::trace_event::ProcessMemoryDumpCallback process_callback =
-      base::Bind(&ProcessDumpCallbackAdapter, callback);
-  base::trace_event::MemoryDumpManager::GetInstance()->CreateProcessDump(
-      args, process_callback);
-}
-
 class URLRequestQuicPerfTest : public ::testing::Test {
  protected:
   URLRequestQuicPerfTest() : message_loop_(new base::MessageLoopForIO()) {
     memory_dump_manager_ =
         base::trace_event::MemoryDumpManager::CreateInstanceForTesting();
-    memory_dump_manager_->Initialize(
-        base::BindRepeating(&RequestGlobalMemoryDumpCallback),
+    base::trace_event::InitializeMemoryDumpManagerForInProcessTesting(
         /*is_coordinator_process=*/false);
     memory_dump_manager_->set_dumper_registrations_ignored_for_testing(false);
     context_ = base::MakeUnique<TestURLRequestContext>(true);
@@ -269,11 +245,17 @@ TEST_F(URLRequestQuicPerfTest, TestGetRequest) {
       base::trace_event::TraceLog::RECORDING_MODE);
 
   base::RunLoop run_loop;
-  base::trace_event::MemoryDumpManager::GetInstance()->RequestGlobalDump(
-      base::trace_event::MemoryDumpType::EXPLICITLY_TRIGGERED,
-      base::trace_event::MemoryDumpLevelOfDetail::LIGHT,
-      base::Bind(&RequestGlobalDumpCallback, run_loop.QuitClosure()));
-
+  base::trace_event::MemoryDumpRequestArgs args{
+      1 /* dump_guid*/, base::trace_event::MemoryDumpType::EXPLICITLY_TRIGGERED,
+      base::trace_event::MemoryDumpLevelOfDetail::LIGHT};
+  auto on_memory_dump_done =
+      [](base::Closure quit_closure, uint64_t dump_guid, bool success,
+         const base::Optional<base::trace_event::MemoryDumpCallbackResult>&) {
+        base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, quit_closure);
+        ASSERT_TRUE(success);
+      };
+  base::trace_event::MemoryDumpManager::GetInstance()->CreateProcessDump(
+      args, base::Bind(on_memory_dump_done, run_loop.QuitClosure()));
   run_loop.Run();
   base::trace_event::TraceLog::GetInstance()->SetDisabled();
   std::unique_ptr<trace_analyzer::TraceAnalyzer> analyzer =

services/resource_coordinator/memory_instrumentation/coordinator_impl.cc

@@ -117,7 +117,7 @@ void CoordinatorImpl::RequestGlobalMemoryDump(
       args.dump_type != base::trace_event::MemoryDumpType::SUMMARY_ONLY) {
     for (const auto& request : queued_memory_dump_requests_) {
       if (request.args.level_of_detail == args.level_of_detail) {
-        VLOG(1) << base::trace_event::MemoryDumpManager::kLogPrefix << " ("
+        VLOG(1) << "RequestGlobalMemoryDump("
                 << base::trace_event::MemoryDumpTypeToString(args.dump_type)
                 << ") skipped because another dump request with the same "
                    "level of detail ("
@@ -219,8 +219,7 @@ void CoordinatorImpl::OnProcessMemoryDumpResponse(
 
   if (!success) {
     ++failed_memory_dump_count_;
-    VLOG(1) << base::trace_event::MemoryDumpManager::kLogPrefix
-            << " failed because of NACK from client";
+    VLOG(1) << "RequestGlobalMemoryDump() FAIL: NACK from client process";
   }
 
   FinalizeGlobalMemoryDumpIfAllManagersReplied();