memory-infra: Start disentangling tracing from memory-infra

Starts refactoring the tracing specific code from MemoryDumpManager
into a new class MemoryTracingObserver. This shouldn't change the
behaviour of the MemoryDumpManager.

BUG=703184

Review-Url: https://codereview.chromium.org/2820433005
Cr-Commit-Position: refs/heads/master@{#466936}

base/BUILD.gn

@@ -981,6 +981,8 @@ component("base") {
     "trace_event/memory_infra_background_whitelist.h",
     "trace_event/memory_peak_detector.cc",
     "trace_event/memory_peak_detector.h",
+    "trace_event/memory_tracing_observer.cc",
+    "trace_event/memory_tracing_observer.h",
     "trace_event/memory_usage_estimator.cc",
     "trace_event/memory_usage_estimator.h",
     "trace_event/process_memory_dump.cc",

base/trace_event/memory_dump_manager.cc

@@ -36,6 +36,7 @@
 #include "base/trace_event/memory_dump_session_state.h"
 #include "base/trace_event/memory_infra_background_whitelist.h"
 #include "base/trace_event/memory_peak_detector.h"
+#include "base/trace_event/memory_tracing_observer.h"
 #include "base/trace_event/process_memory_dump.h"
 #include "base/trace_event/trace_event.h"
 #include "base/trace_event/trace_event_argument.h"
@@ -50,10 +51,6 @@ namespace trace_event {
 
 namespace {
 
-const int kTraceEventNumArgs = 1;
-const char* kTraceEventArgNames[] = {"dumps"};
-const unsigned char kTraceEventArgTypes[] = {TRACE_VALUE_TYPE_CONVERTABLE};
-
 StaticAtomicSequenceNumber g_next_guid;
 MemoryDumpManager* g_instance_for_testing = nullptr;
 
@@ -199,7 +196,6 @@ MemoryDumpManager::MemoryDumpManager()
 }
 
 MemoryDumpManager::~MemoryDumpManager() {
-  TraceLog::GetInstance()->RemoveEnabledStateObserver(this);
 }
 
 void MemoryDumpManager::EnableHeapProfilingIfNeeded() {
@@ -290,14 +286,9 @@ void MemoryDumpManager::Initialize(
                                         TraceLog::FILTERING_MODE);
   }
 
-  // If tracing was enabled before initializing MemoryDumpManager, we missed the
-  // OnTraceLogEnabled() event. Synthetize it so we can late-join the party.
-  // IsEnabled is called before adding observer to avoid calling
-  // OnTraceLogEnabled twice.
-  bool is_tracing_already_enabled = TraceLog::GetInstance()->IsEnabled();
-  TraceLog::GetInstance()->AddEnabledStateObserver(this);
-  if (is_tracing_already_enabled)
-    OnTraceLogEnabled();
+  // TODO(hjd): Move out of MDM. See: crbug.com/703184
+  tracing_observer_ =
+      MakeUnique<MemoryTracingObserver>(TraceLog::GetInstance(), this);
 }
 
 void MemoryDumpManager::RegisterDumpProvider(
@@ -723,7 +714,6 @@ uint32_t MemoryDumpManager::GetDumpsSumKb(const std::string& pattern,
   return sum / 1024;
 }
 
-// static
 void MemoryDumpManager::FinalizeDumpAndAddToTrace(
     std::unique_ptr<ProcessMemoryDumpAsyncState> pmd_async_state) {
   HEAP_PROFILER_SCOPED_IGNORE;
@@ -734,7 +724,7 @@ void MemoryDumpManager::FinalizeDumpAndAddToTrace(
         pmd_async_state->callback_task_runner;
     callback_task_runner->PostTask(
         FROM_HERE, BindOnce(&MemoryDumpManager::FinalizeDumpAndAddToTrace,
-                            Passed(&pmd_async_state)));
+                            Unretained(this), Passed(&pmd_async_state)));
     return;
   }
 
@@ -743,26 +733,17 @@ 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;
+
+  bool dump_successful = pmd_async_state->dump_successful;
+
   for (const auto& kv : pmd_async_state->process_dumps) {
     ProcessId pid = kv.first;  // kNullProcessId for the current process.
     ProcessMemoryDump* process_memory_dump = kv.second.get();
-    std::unique_ptr<TracedValue> traced_value(new TracedValue);
-    process_memory_dump->AsValueInto(traced_value.get());
-    traced_value->SetString("level_of_detail",
-                            MemoryDumpLevelOfDetailToString(
-                                pmd_async_state->req_args.level_of_detail));
-    const char* const event_name =
-        MemoryDumpTypeToString(pmd_async_state->req_args.dump_type);
-
-    std::unique_ptr<ConvertableToTraceFormat> event_value(
-        std::move(traced_value));
-    TRACE_EVENT_API_ADD_TRACE_EVENT_WITH_PROCESS_ID(
-        TRACE_EVENT_PHASE_MEMORY_DUMP,
-        TraceLog::GetCategoryGroupEnabled(kTraceCategory), event_name,
-        trace_event_internal::kGlobalScope, dump_guid, pid,
-        kTraceEventNumArgs, kTraceEventArgNames,
-        kTraceEventArgTypes, nullptr /* arg_values */, &event_value,
-        TRACE_EVENT_FLAG_HAS_ID);
+
+    bool added_to_trace = tracing_observer_->AddDumpToTraceIfEnabled(
+        &pmd_async_state->req_args, pid, process_memory_dump);
+
+    dump_successful = dump_successful && added_to_trace;
 
     // TODO(hjd): Transitional until we send the full PMD. See crbug.com/704203
     // Don't try to fill the struct in detailed mode since it is hard to avoid
@@ -794,17 +775,8 @@ void MemoryDumpManager::FinalizeDumpAndAddToTrace(
     }
   }
 
-  bool tracing_still_enabled;
-  TRACE_EVENT_CATEGORY_GROUP_ENABLED(kTraceCategory, &tracing_still_enabled);
-  if (!tracing_still_enabled) {
-    pmd_async_state->dump_successful = false;
-    VLOG(1) << kLogPrefix << " failed because tracing was disabled before"
-            << " the dump was completed";
-  }
-
   if (!pmd_async_state->callback.is_null()) {
-    pmd_async_state->callback.Run(dump_guid, pmd_async_state->dump_successful,
-                                  result);
+    pmd_async_state->callback.Run(dump_guid, dump_successful, result);
     pmd_async_state->callback.Reset();
   }
 
@@ -812,17 +784,8 @@ void MemoryDumpManager::FinalizeDumpAndAddToTrace(
                                   TRACE_ID_LOCAL(dump_guid));
 }
 
-void MemoryDumpManager::OnTraceLogEnabled() {
-  bool enabled;
-  TRACE_EVENT_CATEGORY_GROUP_ENABLED(kTraceCategory, &enabled);
-  if (!enabled)
-    return;
-
-  // Initialize the TraceLog for the current thread. This is to avoid that the
-  // TraceLog memory dump provider is registered lazily in the PostTask() below
-  // while the |lock_| is taken;
-  TraceLog::GetInstance()->InitializeThreadLocalEventBufferIfSupported();
-
+void MemoryDumpManager::Enable(
+    const TraceConfig::MemoryDumpConfig& memory_dump_config) {
   // Spin-up the thread used to invoke unbound dump providers.
   std::unique_ptr<Thread> dump_thread(new Thread("MemoryInfra"));
   if (!dump_thread->Start()) {
@@ -830,10 +793,6 @@ void MemoryDumpManager::OnTraceLogEnabled() {
     return;
   }
 
-  const TraceConfig& trace_config =
-      TraceLog::GetInstance()->GetCurrentTraceConfig();
-  const TraceConfig::MemoryDumpConfig& memory_dump_config =
-      trace_config.memory_dump_config();
   scoped_refptr<MemoryDumpSessionState> session_state =
       new MemoryDumpSessionState;
   session_state->SetAllowedDumpModes(memory_dump_config.allowed_dump_modes);
@@ -918,7 +877,7 @@ void MemoryDumpManager::OnTraceLogEnabled() {
   }
 }
 
-void MemoryDumpManager::OnTraceLogDisabled() {
+void MemoryDumpManager::Disable() {
   // There might be a memory dump in progress while this happens. Therefore,
   // ensure that the MDM state which depends on the tracing enabled / disabled
   // state is always accessed by the dumping methods holding the |lock_|.

base/trace_event/memory_dump_manager.h

@@ -39,6 +39,7 @@ class Thread;
 
 namespace trace_event {
 
+class MemoryTracingObserver;
 class MemoryDumpManagerDelegate;
 class MemoryDumpProvider;
 class MemoryDumpSessionState;
@@ -46,7 +47,7 @@ class MemoryDumpSessionState;
 // This is the interface exposed to the rest of the codebase to deal with
 // memory tracing. The main entry point for clients is represented by
 // RequestDumpPoint(). The extension by Un(RegisterDumpProvider).
-class BASE_EXPORT MemoryDumpManager : public TraceLog::EnabledStateObserver {
+class BASE_EXPORT MemoryDumpManager {
  public:
   static const char* const kTraceCategory;
   static const char* const kLogPrefix;
@@ -119,9 +120,15 @@ class BASE_EXPORT MemoryDumpManager : public TraceLog::EnabledStateObserver {
   void RequestGlobalDump(MemoryDumpType dump_type,
                          MemoryDumpLevelOfDetail level_of_detail);
 
-  // TraceLog::EnabledStateObserver implementation.
-  void OnTraceLogEnabled() override;
-  void OnTraceLogDisabled() override;
+  // Prepare MemoryDumpManager for RequestGlobalMemoryDump calls.
+  // Starts the MemoryDumpManager thread.
+  // Also uses the given config to initialize the peak detector,
+  // scheduler and heap profiler.
+  void Enable(const TraceConfig::MemoryDumpConfig&);
+
+  // Tearsdown the MemoryDumpManager thread and various other state set up by
+  // Enable.
+  void Disable();
 
   // Enable heap profiling if kEnableHeapProfiling is specified.
   void EnableHeapProfilingIfNeeded();
@@ -230,11 +237,12 @@ class BASE_EXPORT MemoryDumpManager : public TraceLog::EnabledStateObserver {
   static const char* const kSystemAllocatorPoolName;
 
   MemoryDumpManager();
-  ~MemoryDumpManager() override;
+  virtual ~MemoryDumpManager();
 
   static void SetInstanceForTesting(MemoryDumpManager* instance);
   static uint32_t GetDumpsSumKb(const std::string&, const ProcessMemoryDump*);
-  static void FinalizeDumpAndAddToTrace(
+
+  void FinalizeDumpAndAddToTrace(
       std::unique_ptr<ProcessMemoryDumpAsyncState> pmd_async_state);
 
   // Internal, used only by MemoryDumpManagerDelegate.
@@ -284,6 +292,7 @@ class BASE_EXPORT MemoryDumpManager : public TraceLog::EnabledStateObserver {
       strict_thread_check_blacklist_;
 
   std::unique_ptr<MemoryDumpManagerDelegate> delegate_;
+  std::unique_ptr<MemoryTracingObserver> tracing_observer_;
 
   // Protects from concurrent accesses to the |dump_providers_*| and |delegate_|
   // to guard against disabling logging while dumping on another thread.

base/trace_event/memory_dump_manager_unittest.cc

@@ -1015,64 +1015,6 @@ TEST_F(MemoryDumpManagerTest, TraceConfigExpectationsWhenIsCoordinator) {
   DisableTracing();
 }
 
-// Tests against race conditions that might arise when disabling tracing in the
-// middle of a global memory dump.
-// Flaky on iOS, see crbug.com/706961
-#if defined(OS_IOS)
-#define MAYBE_DisableTracingWhileDumping DISABLED_DisableTracingWhileDumping
-#else
-#define MAYBE_DisableTracingWhileDumping DisableTracingWhileDumping
-#endif
-TEST_F(MemoryDumpManagerTest, MAYBE_DisableTracingWhileDumping) {
-  base::WaitableEvent tracing_disabled_event(
-      WaitableEvent::ResetPolicy::AUTOMATIC,
-      WaitableEvent::InitialState::NOT_SIGNALED);
-  InitializeMemoryDumpManager(false /* is_coordinator */);
-
-  // Register a bound dump provider.
-  std::unique_ptr<Thread> mdp_thread(new Thread("test thread"));
-  mdp_thread->Start();
-  MockMemoryDumpProvider mdp_with_affinity;
-  RegisterDumpProvider(&mdp_with_affinity, mdp_thread->task_runner(),
-                       kDefaultOptions);
-
-  // Register also an unbound dump provider. Unbound dump providers are always
-  // invoked after bound ones.
-  MockMemoryDumpProvider unbound_mdp;
-  RegisterDumpProvider(&unbound_mdp, nullptr, kDefaultOptions);
-
-  EnableTracingWithLegacyCategories(MemoryDumpManager::kTraceCategory);
-  EXPECT_CALL(*delegate_, RequestGlobalMemoryDump(_, _)).Times(1);
-  EXPECT_CALL(mdp_with_affinity, OnMemoryDump(_, _))
-      .Times(1)
-      .WillOnce(
-          Invoke([&tracing_disabled_event](const MemoryDumpArgs&,
-                                           ProcessMemoryDump* pmd) -> bool {
-            tracing_disabled_event.Wait();
-
-            // At this point tracing has been disabled and the
-            // MemoryDumpManager.dump_thread_ has been shut down.
-            return true;
-          }));
-
-  // |unbound_mdp| should never be invoked because the thread for unbound dump
-  // providers has been shutdown in the meanwhile.
-  EXPECT_CALL(unbound_mdp, OnMemoryDump(_, _)).Times(0);
-
-  last_callback_success_ = true;
-  RunLoop run_loop;
-  GlobalMemoryDumpCallback callback =
-      Bind(&MemoryDumpManagerTest::GlobalDumpCallbackAdapter, Unretained(this),
-           ThreadTaskRunnerHandle::Get(), run_loop.QuitClosure());
-  mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
-                          MemoryDumpLevelOfDetail::DETAILED, callback);
-  DisableTracing();
-  tracing_disabled_event.Signal();
-  run_loop.Run();
-
-  EXPECT_FALSE(last_callback_success_);
-}
-
 // Tests against race conditions that can happen if tracing is disabled before
 // the CreateProcessDump() call. Real-world regression: crbug.com/580295 .
 TEST_F(MemoryDumpManagerTest, DisableTracingRightBeforeStartOfDump) {
@@ -1308,5 +1250,36 @@ TEST_F(MemoryDumpManagerTest, TestBlacklistedUnsafeUnregistration) {
   thread.Stop();
 }
 
+// Tests that we can manually take a dump without enabling tracing.
+TEST_F(MemoryDumpManagerTest, DumpWithTracingDisabled) {
+  InitializeMemoryDumpManager(false /* is_coordinator */);
+  MockMemoryDumpProvider mdp;
+  RegisterDumpProvider(&mdp, ThreadTaskRunnerHandle::Get());
+
+  DisableTracing();
+
+  const TraceConfig& trace_config =
+      TraceConfig(TraceConfigMemoryTestUtil::GetTraceConfig_NoTriggers());
+  const TraceConfig::MemoryDumpConfig& memory_dump_config =
+      trace_config.memory_dump_config();
+
+  mdm_->Enable(memory_dump_config);
+
+  EXPECT_CALL(*delegate_, RequestGlobalMemoryDump(_, _)).Times(3);
+  EXPECT_CALL(mdp, OnMemoryDump(_, _)).Times(3).WillRepeatedly(Return(true));
+  last_callback_success_ = true;
+  for (int i = 0; i < 3; ++i)
+    RequestGlobalDumpAndWait(MemoryDumpType::EXPLICITLY_TRIGGERED,
+                             MemoryDumpLevelOfDetail::DETAILED);
+  // The callback result should actually be false since (for the moment at
+  // least) a true result means that as well as the dump generally being
+  // successful we also managed to add the dump to the trace.
+  EXPECT_FALSE(last_callback_success_);
+
+  mdm_->Disable();
+
+  mdm_->UnregisterDumpProvider(&mdp);
+}
+
 }  // namespace trace_event
 }  // namespace base

base/trace_event/memory_tracing_observer.cc

+// 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.
+
+#include "base/trace_event/memory_tracing_observer.h"
+
+#include "base/trace_event/memory_dump_manager.h"
+#include "base/trace_event/trace_event_argument.h"
+
+namespace base {
+namespace trace_event {
+
+namespace {
+
+const int kTraceEventNumArgs = 1;
+const char* kTraceEventArgNames[] = {"dumps"};
+const unsigned char kTraceEventArgTypes[] = {TRACE_VALUE_TYPE_CONVERTABLE};
+
+bool IsMemoryInfraTracingEnabled() {
+  bool enabled;
+  TRACE_EVENT_CATEGORY_GROUP_ENABLED(MemoryDumpManager::kTraceCategory,
+                                     &enabled);
+  return enabled;
+}
+
+};  // namespace
+
+MemoryTracingObserver::MemoryTracingObserver(
+    TraceLog* trace_log,
+    MemoryDumpManager* memory_dump_manager)
+    : memory_dump_manager_(memory_dump_manager), trace_log_(trace_log) {
+  // If tracing was enabled before initializing MemoryDumpManager, we missed the
+  // OnTraceLogEnabled() event. Synthetize it so we can late-join the party.
+  // IsEnabled is called before adding observer to avoid calling
+  // OnTraceLogEnabled twice.
+  bool is_tracing_already_enabled = trace_log_->IsEnabled();
+  trace_log_->AddEnabledStateObserver(this);
+  if (is_tracing_already_enabled)
+    OnTraceLogEnabled();
+}
+
+MemoryTracingObserver::~MemoryTracingObserver() {
+  trace_log_->RemoveEnabledStateObserver(this);
+}
+
+void MemoryTracingObserver::OnTraceLogEnabled() {
+  if (!IsMemoryInfraTracingEnabled())
+    return;
+
+  // Initialize the TraceLog for the current thread. This is to avoids that the
+  // TraceLog memory dump provider is registered lazily during the MDM Enable()
+  TraceLog::GetInstance()->InitializeThreadLocalEventBufferIfSupported();
+
+  const TraceConfig& trace_config =
+      TraceLog::GetInstance()->GetCurrentTraceConfig();
+  const TraceConfig::MemoryDumpConfig& memory_dump_config =
+      trace_config.memory_dump_config();
+
+  memory_dump_manager_->Enable(memory_dump_config);
+}
+
+void MemoryTracingObserver::OnTraceLogDisabled() {
+  memory_dump_manager_->Disable();
+}
+
+bool MemoryTracingObserver::AddDumpToTraceIfEnabled(
+    const MemoryDumpRequestArgs* req_args,
+    const ProcessId pid,
+    const ProcessMemoryDump* process_memory_dump) {
+  // If tracing has been disabled early out to avoid the cost of serializing the
+  // dump then ignoring the result.
+  if (!IsMemoryInfraTracingEnabled())
+    return false;
+
+  const uint64_t dump_guid = req_args->dump_guid;
+
+  std::unique_ptr<TracedValue> traced_value(new TracedValue);
+  process_memory_dump->AsValueInto(traced_value.get());
+  traced_value->SetString("level_of_detail", MemoryDumpLevelOfDetailToString(
+                                                 req_args->level_of_detail));
+  const char* const event_name = MemoryDumpTypeToString(req_args->dump_type);
+
+  std::unique_ptr<ConvertableToTraceFormat> event_value(
+      std::move(traced_value));
+  TRACE_EVENT_API_ADD_TRACE_EVENT_WITH_PROCESS_ID(
+      TRACE_EVENT_PHASE_MEMORY_DUMP,
+      TraceLog::GetCategoryGroupEnabled(MemoryDumpManager::kTraceCategory),
+      event_name, trace_event_internal::kGlobalScope, dump_guid, pid,
+      kTraceEventNumArgs, kTraceEventArgNames, kTraceEventArgTypes,
+      nullptr /* arg_values */, &event_value, TRACE_EVENT_FLAG_HAS_ID);
+
+  return true;
+}
+
+}  // namespace trace_event
+}  // namespace base

base/trace_event/memory_tracing_observer.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_TRACING_OBSERVER_H_
+#define BASE_TRACE_EVENT_MEMORY_TRACING_OBSERVER_H_
+
+#include "base/macros.h"
+#include "base/trace_event/memory_dump_manager.h"
+#include "base/trace_event/trace_event.h"
+
+namespace base {
+
+namespace trace_event {
+
+// Observes TraceLog for Enable/Disable events and when they occur Enables and
+// Disables the MemoryDumpManager with the correct state based on reading the
+// trace log. Also provides a method for adding a dump to the trace.
+class BASE_EXPORT MemoryTracingObserver
+    : public TraceLog::EnabledStateObserver {
+ public:
+  static const char* const kTraceCategory;
+
+  MemoryTracingObserver(TraceLog*, MemoryDumpManager*);
+  ~MemoryTracingObserver() override;
+
+  // TraceLog::EnabledStateObserver implementation.
+  void OnTraceLogEnabled() override;
+  void OnTraceLogDisabled() override;
+
+  bool AddDumpToTraceIfEnabled(const MemoryDumpRequestArgs*,
+                               const ProcessId,
+                               const ProcessMemoryDump*);
+
+ private:
+  MemoryDumpManager* const memory_dump_manager_;
+  TraceLog* const trace_log_;
+
+  DISALLOW_COPY_AND_ASSIGN(MemoryTracingObserver);
+};
+
+}  // namespace trace_event
+}  // namespace base
+
+#endif  // BASE_TRACE_EVENT_MEMORY_TRACING_OBSERVER_H_

tools/gn/bootstrap/bootstrap.py

@@ -524,6 +524,7 @@ def write_gn_ninja(path, root_gen_dir, options):
       'base/trace_event/memory_dump_session_state.cc',
       'base/trace_event/memory_infra_background_whitelist.cc',
       'base/trace_event/memory_peak_detector.cc',
+      'base/trace_event/memory_tracing_observer.cc',
       'base/trace_event/process_memory_dump.cc',
       'base/trace_event/process_memory_maps.cc',
       'base/trace_event/process_memory_totals.cc',