memory-infra: Never kill memory-infra background thread

base/trace_event/memory_dump_manager.cc

 // Copyright 2015 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_dump_manager.h"
 
 #include <inttypes.h>
 #include <stdio.h>
 
 #include <algorithm>
@@ skipping 471 matching lines @@
     MemoryDumpProvider* provider) {
   AutoLock lock(lock_);
 
   for (const auto& info : dump_providers_) {
     if (info->dump_provider == provider)
       return true;
   }
   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) {
   char guid_str[20];
   sprintf(guid_str, "0x%" PRIx64, args.dump_guid);
   TRACE_EVENT_NESTABLE_ASYNC_BEGIN1(kTraceCategory, "ProcessMemoryDump",
                                     TRACE_ID_LOCAL(args.dump_guid), "dump_guid",
                                     TRACE_STR_COPY(guid_str));
 
   // If argument filter is enabled then only background mode dumps should be
   // allowed. In case the trace config passed for background tracing session
   // missed the allowed modes argument, it crashes here instead of creating
   // unexpected dumps.
   if (TraceLog::GetInstance()
           ->GetCurrentTraceConfig()
           .IsArgumentFilterEnabled()) {
     CHECK_EQ(MemoryDumpLevelOfDetail::BACKGROUND, args.level_of_detail);
   }
 
   std::unique_ptr<ProcessMemoryDumpAsyncState> pmd_async_state;
   {
     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
     // disabled.
     CHECK(!session_state_ ||
           session_state_->IsDumpModeAllowed(args.level_of_detail));
 
     // If enabled, holds back the peak detector resetting its estimation window.
     MemoryPeakDetector::GetInstance()->Throttle();
   }
@@ skipping 13 matching lines @@
 // |lock_| is used in these functions purely to ensure consistency w.r.t.
 // (un)registrations of |dump_providers_|.
 void MemoryDumpManager::SetupNextMemoryDump(
     std::unique_ptr<ProcessMemoryDumpAsyncState> pmd_async_state) {
   HEAP_PROFILER_SCOPED_IGNORE;
   // Initalizes the ThreadLocalEventBuffer to guarantee that the TRACE_EVENTs
   // in the PostTask below don't end up registering their own dump providers
   // (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));
 
   // Read MemoryDumpProviderInfo thread safety considerations in
   // memory_dump_manager.h when accessing |mdpinfo| fields.
   MemoryDumpProviderInfo* mdpinfo =
       pmd_async_state->pending_dump_providers.back().get();
 
   // If we are in background tracing, we should invoke only the whitelisted
   // providers. Ignore other providers and continue.
   if (pmd_async_state->req_args.level_of_detail ==
           MemoryDumpLevelOfDetail::BACKGROUND &&
       !mdpinfo->whitelisted_for_background_mode) {
     pmd_async_state->pending_dump_providers.pop_back();
     return SetupNextMemoryDump(std::move(pmd_async_state));
   }
 
   // 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);
     task_runner = pmd_async_state->dump_thread_task_runner.get();
     DCHECK(task_runner);
   }
 
   if (mdpinfo->options.dumps_on_single_thread_task_runner &&
       task_runner->RunsTasksOnCurrentThread()) {
     // If |dumps_on_single_thread_task_runner| is true then no PostTask is
@@ skipping 180 matching lines @@
     pmd_async_state->callback.Run(dump_guid, dump_successful, result);
     pmd_async_state->callback.Reset();
   }
 
   TRACE_EVENT_NESTABLE_ASYNC_END0(kTraceCategory, "ProcessMemoryDump",
                                   TRACE_ID_LOCAL(dump_guid));
 }
 
 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;
   session_state->SetAllowedDumpModes(memory_dump_config.allowed_dump_modes);
   session_state->set_heap_profiler_breakdown_threshold_bytes(
       memory_dump_config.heap_profiler_options.breakdown_threshold_bytes);
   if (heap_profiling_enabled_) {
     // If heap profiling is enabled, the stack frame deduplicator and type name
     // deduplicator will be in use. Add a metadata events to write the frames
     // and type IDs.
@@ skipping 14 matching lines @@
         "typeNames",
         MakeUnique<SessionStateConvertableProxy<TypeNameDeduplicator>>(
             session_state, &MemoryDumpSessionState::type_name_deduplicator));
   }
 
   AutoLock lock(lock_);
 
   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;
   bool peak_detector_configured = false;
   for (const auto& trigger : memory_dump_config.triggers) {
     if (!session_state_->IsDumpModeAllowed(trigger.level_of_detail)) {
       NOTREACHED();
       continue;
     }
     if (trigger.trigger_type == MemoryDumpType::PERIODIC_INTERVAL) {
       if (periodic_config.triggers.empty()) {
         periodic_config.callback = BindRepeating(&OnPeriodicSchedulerTick);
       }
       periodic_config.triggers.push_back(
           {trigger.level_of_detail, trigger.min_time_between_dumps_ms});
     } else if (trigger.trigger_type == MemoryDumpType::PEAK_MEMORY_USAGE) {
       // At most one peak trigger is allowed.
       CHECK(!peak_detector_configured);
       peak_detector_configured = true;
       MemoryPeakDetector::GetInstance()->Setup(
           BindRepeating(&MemoryDumpManager::GetDumpProvidersForPolling,
                         Unretained(this)),
-          dump_thread_->task_runner(),
+          GetTaskRunner(),
           BindRepeating(&OnPeakDetected, trigger.level_of_detail));
 
       MemoryPeakDetector::Config peak_config;
       peak_config.polling_interval_ms = 10;
       peak_config.min_time_between_peaks_ms = trigger.min_time_between_dumps_ms;
       peak_config.enable_verbose_poll_tracing =
           trigger.level_of_detail == MemoryDumpLevelOfDetail::DETAILED;
       MemoryPeakDetector::GetInstance()->Start(peak_config);
 
       // 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));
       }
     }
   }
 
   // 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());
   }
 }
 
 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_|.
   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) {
   AutoLock lock(lock_);
   if (!session_state_)
     return false;
   return session_state_->IsDumpModeAllowed(dump_mode);
 }
 
 MemoryDumpManager::ProcessMemoryDumpAsyncState::ProcessMemoryDumpAsyncState(
@@ skipping 22 matching lines @@
   if (iter == process_dumps.end()) {
     std::unique_ptr<ProcessMemoryDump> new_pmd(
         new ProcessMemoryDump(session_state, dump_args));
     iter = process_dumps.insert(std::make_pair(pid, std::move(new_pmd))).first;
   }
   return iter->second.get();
 }
 
 }  // namespace trace_event
 }  // namespace base