memory-infra: Never kill memory-infra background thread

base/trace_event/memory_dump_manager.cc

Index: base/trace_event/memory_dump_manager.cc
diff --git a/base/trace_event/memory_dump_manager.cc b/base/trace_event/memory_dump_manager.cc
index 10e5d1b13f6c6e22756b73e5e4da0c73dd1533a8..d449d78cd5c8c132666c82b4f02eb5a7b12a6228 100644
--- a/base/trace_event/memory_dump_manager.cc
+++ b/base/trace_event/memory_dump_manager.cc
@@ -489,6 +489,24 @@ bool MemoryDumpManager::IsDumpProviderRegisteredForTesting(
   return false;
 }
 
+scoped_refptr<base::SingleThreadTaskRunner> MemoryDumpManager::GetTaskRunner() {
+  lock_.AssertAcquired();
+
+  if (dump_thread_)
+    return dump_thread_->task_runner();
+
+  // Spin-up the thread used to invoke unbound dump providers.
+  std::unique_ptr<Thread> dump_thread(new Thread("MemoryInfra"));
+  if (!dump_thread->Start()) {

[Primiano Tucci (use gerrit), 2017/04/24 15:30:58]

I'd just violently CHECK(started) here.
Trying to handle the lack of a thread is going to be too hard and IMHO not
something we should try to do, unless crash/ proves us wrong :)
So I'd just do here:
dump_thread_ = MakeUnique<Thread>("MemoryInfra");
bool started = dump_thread_->Start();
CHECK(started);

[hjd, 2017/04/24 16:27:59]

Done.

+    LOG(ERROR) << "Failed to start the memory-infra thread for tracing";
+    NOTREACHED();
+  }
+
+  dump_thread_ = std::move(dump_thread);
+
+  return dump_thread_->task_runner();
+}
+
 void MemoryDumpManager::CreateProcessDump(
     const MemoryDumpRequestArgs& args,
     const ProcessMemoryDumpCallback& callback) {
@@ -512,12 +530,8 @@ void MemoryDumpManager::CreateProcessDump(
   {
     AutoLock lock(lock_);
 
-    // |dump_thread_| can be nullptr is tracing was disabled before reaching
-    // here. SetupNextMemoryDump() is robust enough to tolerate it and will
-    // NACK the dump.
     pmd_async_state.reset(new ProcessMemoryDumpAsyncState(
-        args, dump_providers_, session_state_, callback,
-        dump_thread_ ? dump_thread_->task_runner() : nullptr));
+        args, dump_providers_, session_state_, callback, GetTaskRunner()));
 
     // Safety check to prevent reaching here without calling RequestGlobalDump,
     // with disallowed modes. If |session_state_| is null then tracing is
@@ -551,22 +565,22 @@ void MemoryDumpManager::SetupNextMemoryDump(
   // (for discounting trace memory overhead) while holding the |lock_|.
   TraceLog::GetInstance()->InitializeThreadLocalEventBufferIfSupported();
 
-  // |dump_thread_| might be destroyed before getting this point.
-  // It means that tracing was disabled right before starting this dump.
-  // Anyway either tracing is stopped or this was the last hop, create a trace
+  // MDM might have been disabled before getting to this point.
+  // Anyway either MDM is disabled or this was the last hop, create a trace
   // event, add it to the trace and finalize process dump invoking the callback.
-  if (!pmd_async_state->dump_thread_task_runner.get()) {
+  if (!subtle::NoBarrier_Load(&memory_tracing_enabled_)) {

[Primiano Tucci (use gerrit), 2017/04/24 15:30:58]

I think this extra check is superfluous. The callback will fail once we get to
FinalizeDump() because we will find there that tracing was not enabled.
Unless I am missing some other case, I'd just rip lines 568-582 (this entire
block).
ssid wdyt?

[hjd, 2017/04/24 16:27:59]

Removing this causes a test
(MemoryDumpManagerTest.DisableTracingRightBeforeStartOfDump) to fail. Maybe we
should just delete it now though?

[ssid, 2017/04/25 03:21:47]

I agree with primiano. We could get to a state where we can create dumps without
enabling. Let's start with removing such conditions.
Also why is this still called "memory_tracing_enabled"? Can you make this
is_enabled_ if it's still needed?
I don't see any need for this being AtomicWord anymore, we can just make it bool
accessed in lock.

I'd prefer to remove this entirely. Only issue is with the thread check
blacklist. I will remove that in next CL. Please try to get rid of this variable
as much as possible?

     if (pmd_async_state->pending_dump_providers.empty()) {
-      VLOG(1) << kLogPrefix << " failed because dump thread was destroyed"
+      VLOG(1) << kLogPrefix << " failed because MemoryDumpManager was disabled"
               << " before finalizing the dump";
     } else {
-      VLOG(1) << kLogPrefix << " failed because dump thread was destroyed"
+      VLOG(1) << kLogPrefix << " failed because MemoryDumpManager was disabled"
               << " before dumping "
               << pmd_async_state->pending_dump_providers.back().get()->name;
     }
     pmd_async_state->dump_successful = false;
     pmd_async_state->pending_dump_providers.clear();
   }
+
   if (pmd_async_state->pending_dump_providers.empty())
     return FinalizeDumpAndAddToTrace(std::move(pmd_async_state));
 
@@ -585,7 +599,7 @@ void MemoryDumpManager::SetupNextMemoryDump(
   }
 
   // If the dump provider did not specify a task runner affinity, dump on
-  // |dump_thread_| which is already checked above for presence.
+  // |dump_thread_|.
   SequencedTaskRunner* task_runner = mdpinfo->task_runner.get();
   if (!task_runner) {
     DCHECK(mdpinfo->options.dumps_on_single_thread_task_runner);
@@ -786,12 +800,6 @@ void MemoryDumpManager::FinalizeDumpAndAddToTrace(
 
 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()) {
-    LOG(ERROR) << "Failed to start the memory-infra thread for tracing";
-    return;
-  }
 
   scoped_refptr<MemoryDumpSessionState> session_state =
       new MemoryDumpSessionState;
@@ -826,9 +834,6 @@ void MemoryDumpManager::Enable(
   DCHECK(delegate_);  // At this point we must have a delegate.
   session_state_ = session_state;
 
-  DCHECK(!dump_thread_);
-  dump_thread_ = std::move(dump_thread);
-
   subtle::NoBarrier_Store(&memory_tracing_enabled_, 1);
 
   MemoryDumpScheduler::Config periodic_config;
@@ -851,7 +856,7 @@ void MemoryDumpManager::Enable(
       MemoryPeakDetector::GetInstance()->Setup(
           BindRepeating(&MemoryDumpManager::GetDumpProvidersForPolling,
                         Unretained(this)),
-          dump_thread_->task_runner(),
+          GetTaskRunner(),
           BindRepeating(&OnPeakDetected, trigger.level_of_detail));
 
       MemoryPeakDetector::Config peak_config;
@@ -864,7 +869,7 @@ void MemoryDumpManager::Enable(
       // When peak detection is enabled, trigger a dump straight away as it
       // gives a good reference point for analyzing the trace.
       if (delegate_->IsCoordinator()) {
-        dump_thread_->task_runner()->PostTask(
+        GetTaskRunner()->PostTask(
             FROM_HERE, BindRepeating(&OnPeakDetected, trigger.level_of_detail));
       }
     }
@@ -872,8 +877,7 @@ void MemoryDumpManager::Enable(
 
   // Only coordinator process triggers periodic global memory dumps.
   if (delegate_->IsCoordinator() && !periodic_config.triggers.empty()) {
-    MemoryDumpScheduler::GetInstance()->Start(periodic_config,
-                                              dump_thread_->task_runner());
+    MemoryDumpScheduler::GetInstance()->Start(periodic_config, GetTaskRunner());
   }
 }
 
@@ -884,19 +888,12 @@ void MemoryDumpManager::Disable() {
   if (!subtle::NoBarrier_Load(&memory_tracing_enabled_))
     return;
   subtle::NoBarrier_Store(&memory_tracing_enabled_, 0);
-  std::unique_ptr<Thread> dump_thread;
   {
     AutoLock lock(lock_);
     MemoryDumpScheduler::GetInstance()->Stop();
     MemoryPeakDetector::GetInstance()->TearDown();
-    dump_thread = std::move(dump_thread_);
     session_state_ = nullptr;
   }
-
-  // Thread stops are blocking and must be performed outside of the |lock_|
-  // or will deadlock (e.g., if SetupNextMemoryDump() tries to acquire it).
-  if (dump_thread)
-    dump_thread->Stop();

[Primiano Tucci (use gerrit), 2017/04/24 15:30:58]

\o/ This, as a side effect, should fix crbug.com/702289 . Can you add it to the
BUG= in the cl description?

[hjd, 2017/04/24 16:27:59]

Done.

[ssid, 2017/04/25 03:21:47]

Yay

 }
 
 bool MemoryDumpManager::IsDumpModeAllowed(MemoryDumpLevelOfDetail dump_mode) {