[tracing] Simplify logic of MemoryDumpManager

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 <algorithm>
 #include <utility>
 
 #include "base/atomic_sequence_num.h"
@@ skipping 175 matching lines @@
 }
 
 void MemoryDumpManager::RegisterDumpProvider(
     MemoryDumpProvider* mdp,
     const char* name,
     const scoped_refptr<SingleThreadTaskRunner>& task_runner,
     const MemoryDumpProvider::Options& options) {
   if (dumper_registrations_ignored_for_testing_)
     return;
 
-  MemoryDumpProviderInfo mdp_info(mdp, name, task_runner, options);
+  scoped_refptr<MemoryDumpProviderInfo> mdpinfo =
+      new MemoryDumpProviderInfo(mdp, name, task_runner, options);
   AutoLock lock(lock_);
-  auto iter_new = dump_providers_.insert(mdp_info);
-
-  // If there was a previous entry, replace it with the new one. This is to deal
-  // with the case where a dump provider unregisters itself and then re-
-  // registers before a memory dump happens, so its entry was still in the
-  // collection but flagged |unregistered|.
-  if (!iter_new.second) {
-    dump_providers_.erase(iter_new.first);
-    dump_providers_.insert(mdp_info);
-  }
+  dump_providers_.insert(mdpinfo);
 
   if (heap_profiling_enabled_)
     mdp->OnHeapProfilingEnabled(true);
 }
 
 void MemoryDumpManager::RegisterDumpProvider(
     MemoryDumpProvider* mdp,
     const char* name,
     const scoped_refptr<SingleThreadTaskRunner>& task_runner) {
   RegisterDumpProvider(mdp, name, task_runner, MemoryDumpProvider::Options());
 }
 
 void MemoryDumpManager::UnregisterDumpProvider(MemoryDumpProvider* mdp) {
   AutoLock lock(lock_);
 
   auto mdp_iter = dump_providers_.begin();
   for (; mdp_iter != dump_providers_.end(); ++mdp_iter) {
-    if (mdp_iter->dump_provider == mdp)
+    if ((*mdp_iter)->dump_provider == mdp)
       break;
   }
 
   if (mdp_iter == dump_providers_.end())
     return;
 
   // Unregistration of a MemoryDumpProvider while tracing is ongoing is safe
   // only if the MDP has specified a thread affinity (via task_runner()) AND
   // the unregistration happens on the same thread (so the MDP cannot unregister
   // and OnMemoryDump() at the same time).
   // Otherwise, it is not possible to guarantee that its unregistration is
   // race-free. If you hit this DCHECK, your MDP has a bug.
   DCHECK(!subtle::NoBarrier_Load(&memory_tracing_enabled_) ||
-         (mdp_iter->task_runner &&
-          mdp_iter->task_runner->BelongsToCurrentThread()))
-      << "MemoryDumpProvider \"" << mdp_iter->name << "\" attempted to "
+         ((*mdp_iter)->task_runner &&
+          (*mdp_iter)->task_runner->BelongsToCurrentThread()))
+      << "MemoryDumpProvider \"" << (*mdp_iter)->name << "\" attempted to "
       << "unregister itself in a racy way. Please file a crbug.";
 
-  mdp_iter->unregistered = true;
+  // The MDPInfo instance can still be refcounted by the
+  // |ProcessMemoryDumpAsyncState.pending_dump_providers|. For this reason
+  // the MDPInfo is flagged as disabled. It will cause ContinueAsyncProcessDump
+  // to just skip it, without actually invoking the |mdp|, which might be
+  // destroyed by the caller soon after this method returns.
+  (*mdp_iter)->disabled = true;
+  dump_providers_.erase(mdp_iter);
 }
 
 void MemoryDumpManager::RequestGlobalDump(
     MemoryDumpType dump_type,
     MemoryDumpLevelOfDetail level_of_detail,
     const MemoryDumpCallback& callback) {
   // Bail out immediately if tracing is not enabled at all.
   if (!UNLIKELY(subtle::NoBarrier_Load(&memory_tracing_enabled_))) {
     if (!callback.is_null())
       callback.Run(0u /* guid */, false /* success */);
@@ skipping 34 matching lines @@
 }
 
 void MemoryDumpManager::CreateProcessDump(const MemoryDumpRequestArgs& args,
                                           const MemoryDumpCallback& callback) {
   TRACE_EVENT_NESTABLE_ASYNC_BEGIN0(kTraceCategory, "ProcessMemoryDump",
                                     TRACE_ID_MANGLE(args.dump_guid));
 
   scoped_ptr<ProcessMemoryDumpAsyncState> pmd_async_state;
   {
     AutoLock lock(lock_);
-    pmd_async_state.reset(new ProcessMemoryDumpAsyncState(
-        args, dump_providers_.begin(), session_state_, callback,
-        dump_thread_->task_runner()));
+    pmd_async_state.reset(
+        new ProcessMemoryDumpAsyncState(args, dump_providers_, session_state_,
+                                        callback, dump_thread_->task_runner()));
   }
 
   TRACE_EVENT_WITH_FLOW0(kTraceCategory, "MemoryDumpManager::CreateProcessDump",
                          TRACE_ID_MANGLE(args.dump_guid),
                          TRACE_EVENT_FLAG_FLOW_OUT);
 
   // Start the thread hop. |dump_providers_| are kept sorted by thread, so
   // ContinueAsyncProcessDump will hop at most once per thread (w.r.t. thread
   // affinity specified by the MemoryDumpProvider(s) in RegisterDumpProvider()).
-  ContinueAsyncProcessDump(std::move(pmd_async_state));
+  ContinueAsyncProcessDump(pmd_async_state.release());
 }
 
 // At most one ContinueAsyncProcessDump() can be active at any time for a given
 // PMD, regardless of status of the |lock_|. |lock_| is used here purely to
 // ensure consistency w.r.t. (un)registrations of |dump_providers_|.
 // The linearization of dump providers' OnMemoryDump invocations is achieved by
 // means of subsequent PostTask(s).
 //
 // 1) Prologue:
 //   - Check if the dump provider is disabled, if so skip the dump.
 //   - Check if we are on the right thread. If not hop and continue there.
 // 2) Invoke the dump provider's OnMemoryDump() (unless skipped).
 // 3) Epilogue:
 //  - Unregister the dump provider if it failed too many times consecutively.
-//  - Advance the |next_dump_provider| iterator to the next dump provider.
+//  - Pop() the MDP from the |pending_dump_providers| list.
 //  - If this was the last hop, create a trace event, add it to the trace
 //    and finalize (invoke callback).
 
 void MemoryDumpManager::ContinueAsyncProcessDump(
-    scoped_ptr<ProcessMemoryDumpAsyncState> pmd_async_state) {
+    ProcessMemoryDumpAsyncState* owned_pmd_async_state) {
   // 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();
 
-  const uint64_t dump_guid = pmd_async_state->req_args.dump_guid;
-  const char* dump_provider_name = nullptr;
+  // In theory |owned_pmd_async_state| should be a scoped_ptr. The only reason
+  // why it isn't is because of the corner case logic of |did_post_task| below,
+  // which needs to take back the ownership of the |pmd_async_state| when a
+  // thread goes away and consequently the PostTask() fails.
+  // Unfortunatelly, in fact, PostTask() destroyes the scoped_ptr arguments upon

[ssid, 2015/12/17 22:09:36]

s/Unfortunatelly/Unfortunately
s/destroyes/destroys

[Primiano Tucci (use gerrit), 2015/12/18 12:00:31]

Done.

+  // failure (to prevent accidental leaks). This would prevent us to to just
+  // skip the hop and move on. Hence the manual naked -> scoped ptr juggling.
+  auto pmd_async_state = make_scoped_ptr(owned_pmd_async_state);
+  owned_pmd_async_state = nullptr;
 
-  // Pid of the target process being dumped. Often kNullProcessId (= current
-  // process), non-zero when the coordinator process creates dumps on behalf
-  // of child processes (see crbug.com/461788).
-  ProcessId pid;
+  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.front().get();
+
+  // If the dump provider did not specify a thread affinity, dump on
+  // |dump_thread_|.
+  SingleThreadTaskRunner* task_runner = mdpinfo->task_runner.get();
+  if (!task_runner)
+    task_runner = pmd_async_state->dump_thread_task_runner.get();
+
+  // |dump_thread_| might have been Stop()-ed at this point (if tracing was
+  // disabled in the meanwhile). In such case the PostTask() below will fail.
+  // |task_runner|, however, should always be non-null.
+  DCHECK(task_runner);
 
   // DO NOT put any LOG() statement in the locked sections, as in some contexts
   // (GPU process) LOG() ends up performing PostTask/IPCs.
-  MemoryDumpProvider* mdp;
   bool skip_dump = false;
   {
     AutoLock lock(lock_);
-
-    auto mdp_info = pmd_async_state->next_dump_provider;
-    mdp = mdp_info->dump_provider;
-    dump_provider_name = mdp_info->name;
-    pid = mdp_info->options.target_pid;
-
-    // If the dump provider did not specify a thread affinity, dump on
-    // |dump_thread_|.
-    SingleThreadTaskRunner* task_runner = mdp_info->task_runner.get();
-    if (!task_runner)
-      task_runner = pmd_async_state->dump_thread_task_runner.get();
-
-    // |dump_thread_| might have been Stop()-ed at this point (if tracing was
-    // disabled in the meanwhile). In such case the PostTask() below will fail.
-    // |task_runner|, however, should always be non-null.
-    DCHECK(task_runner);
-
-    if (mdp_info->disabled || mdp_info->unregistered) {
+    // The MDP can be disabled if:
+    // - it failed too many times (see failsafe logic below).
+    // - UnregisterDumpProvider() was called in the meanwhile.
+    if (mdpinfo->disabled) {
       skip_dump = true;
     } else if (!task_runner->BelongsToCurrentThread()) {
       // It's time to hop onto another thread.
-
-      // Copy the callback + arguments just for the unlikley case in which
-      // PostTask fails. In such case the Bind helper will destroy the
-      // pmd_async_state and we must keep a copy of the fields to notify the
-      // abort.
-      MemoryDumpCallback callback = pmd_async_state->callback;
-      scoped_refptr<SingleThreadTaskRunner> callback_task_runner =
-          pmd_async_state->callback_task_runner;
-
       const bool did_post_task = task_runner->PostTask(
           FROM_HERE, Bind(&MemoryDumpManager::ContinueAsyncProcessDump,
-                          Unretained(this), Passed(&pmd_async_state)));
-      if (did_post_task)
+                          Unretained(this), Unretained(pmd_async_state.get())));
+      if (did_post_task) {
+        // Ownership is tranferred to the next ContinueAsyncProcessDump().
+        ignore_result(pmd_async_state.release());
         return;
-
+      }
       // The thread is gone. At this point the best thing we can do is to
       // disable the dump provider and abort this dump.
-      mdp_info->disabled = true;
-      return AbortDumpLocked(callback, callback_task_runner, dump_guid);
+      mdpinfo->disabled = true;
+      skip_dump = true;
     }
   }  // AutoLock(lock_)
 
   // Invoke the dump provider without holding the |lock_|.
-  bool finalize = false;
   bool dump_successful = false;
-
   if (!skip_dump) {
     TRACE_EVENT_WITH_FLOW1(kTraceCategory,
                            "MemoryDumpManager::ContinueAsyncProcessDump",
-                           TRACE_ID_MANGLE(dump_guid),
+                           TRACE_ID_MANGLE(pmd_async_state->req_args.dump_guid),
                            TRACE_EVENT_FLAG_FLOW_IN | TRACE_EVENT_FLAG_FLOW_OUT,
-                           "dump_provider.name", dump_provider_name);
+                           "dump_provider.name", mdpinfo->name);
+
+    // Pid of the target process being dumped. Often kNullProcessId (= current
+    // process), non-zero when the coordinator process creates dumps on behalf
+    // of child processes (see crbug.com/461788).
+    ProcessId target_pid = mdpinfo->options.target_pid;
+    ProcessMemoryDump* pmd =
+        pmd_async_state->GetOrCreateMemoryDumpContainerForProcess(target_pid);
     MemoryDumpArgs args = {pmd_async_state->req_args.level_of_detail};
-    ProcessMemoryDump* process_memory_dump =
-        pmd_async_state->GetOrCreateMemoryDumpContainerForProcess(pid);
-    dump_successful = mdp->OnMemoryDump(args, process_memory_dump);
-  }
+    dump_successful = mdpinfo->dump_provider->OnMemoryDump(args, pmd);
+  }  // if (!skip_dump)
 
   {
     AutoLock lock(lock_);
-    auto mdp_info = pmd_async_state->next_dump_provider;
     if (dump_successful) {
-      mdp_info->consecutive_failures = 0;
+      mdpinfo->consecutive_failures = 0;
     } else if (!skip_dump) {
-      ++mdp_info->consecutive_failures;
-      if (mdp_info->consecutive_failures >= kMaxConsecutiveFailuresCount) {
-        mdp_info->disabled = true;
+      ++mdpinfo->consecutive_failures;
+      if (mdpinfo->consecutive_failures >= kMaxConsecutiveFailuresCount) {
+        mdpinfo->disabled = true;
       }
     }
-    ++pmd_async_state->next_dump_provider;
-    finalize = pmd_async_state->next_dump_provider == dump_providers_.end();
-
-    if (mdp_info->unregistered)
-      dump_providers_.erase(mdp_info);
-  }
+    pmd_async_state->pending_dump_providers.pop_front();
+  }  // AutoLock(lock_)
 
   if (!skip_dump && !dump_successful) {
-    LOG(ERROR) << "MemoryDumpProvider \"" << dump_provider_name << "\" failed, "
+    LOG(ERROR) << "MemoryDumpProvider \"" << mdpinfo->name << "\" failed, "
                << "possibly due to sandboxing (crbug.com/461788)."
                << "Disabling dumper for current process. Try --no-sandbox.";
   }
 
-  if (finalize)
+  if (pmd_async_state->pending_dump_providers.empty())
     return FinalizeDumpAndAddToTrace(std::move(pmd_async_state));
 
-  ContinueAsyncProcessDump(std::move(pmd_async_state));
+  ContinueAsyncProcessDump(pmd_async_state.release());
 }
 
 // static
 void MemoryDumpManager::FinalizeDumpAndAddToTrace(
     scoped_ptr<ProcessMemoryDumpAsyncState> pmd_async_state) {
+  DCHECK(pmd_async_state->pending_dump_providers.empty());
   const uint64_t dump_guid = pmd_async_state->req_args.dump_guid;
   if (!pmd_async_state->callback_task_runner->BelongsToCurrentThread()) {
     scoped_refptr<SingleThreadTaskRunner> callback_task_runner =
         pmd_async_state->callback_task_runner;
     callback_task_runner->PostTask(
         FROM_HERE, Bind(&MemoryDumpManager::FinalizeDumpAndAddToTrace,
                         Passed(&pmd_async_state)));
     return;
   }
 
@@ skipping 23 matching lines @@
 
   if (!pmd_async_state->callback.is_null()) {
     pmd_async_state->callback.Run(dump_guid, true /* success */);
     pmd_async_state->callback.Reset();
   }
 
   TRACE_EVENT_NESTABLE_ASYNC_END0(kTraceCategory, "ProcessMemoryDump",
                                   TRACE_ID_MANGLE(dump_guid));
 }
 
-// static
-void MemoryDumpManager::AbortDumpLocked(
-    MemoryDumpCallback callback,
-    scoped_refptr<SingleThreadTaskRunner> task_runner,
-    uint64_t dump_guid) {
-  if (callback.is_null())
-    return;  // There is nothing to NACK.
-
-  // Post the callback even if we are already on the right thread to avoid
-  // invoking the callback while holding the lock_.
-  task_runner->PostTask(FROM_HERE,
-                        Bind(callback, dump_guid, false /* success */));
-}
-
 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();
@@ skipping 24 matching lines @@
     TRACE_EVENT_API_ADD_METADATA_EVENT(
         "typeNames", "typeNames",
         scoped_refptr<ConvertableToTraceFormat>(type_name_deduplicator));
   }
 
   DCHECK(!dump_thread_);
   dump_thread_ = std::move(dump_thread);
   session_state_ = new MemoryDumpSessionState(stack_frame_deduplicator,
                                               type_name_deduplicator);
 
-  for (auto it = dump_providers_.begin(); it != dump_providers_.end(); ++it) {
-    it->disabled = false;
-    it->consecutive_failures = 0;
+  for (const scoped_refptr<MemoryDumpProviderInfo>& mdpinfo : dump_providers_) {
+    mdpinfo->disabled = false;
+    mdpinfo->consecutive_failures = 0;
   }
 
   subtle::NoBarrier_Store(&memory_tracing_enabled_, 1);
 
   // TODO(primiano): This is a temporary hack to disable periodic memory dumps
   // when running memory benchmarks until telemetry uses TraceConfig to
   // enable/disable periodic dumps. See crbug.com/529184 .
   if (!is_coordinator_ ||
       CommandLine::ForCurrentProcess()->HasSwitch(
           "enable-memory-benchmarking")) {
@@ skipping 53 matching lines @@
 MemoryDumpManager::MemoryDumpProviderInfo::MemoryDumpProviderInfo(
     MemoryDumpProvider* dump_provider,
     const char* name,
     const scoped_refptr<SingleThreadTaskRunner>& task_runner,
     const MemoryDumpProvider::Options& options)
     : dump_provider(dump_provider),
       name(name),
       task_runner(task_runner),
       options(options),
       consecutive_failures(0),
-      disabled(false),
-      unregistered(false) {}
+      disabled(false) {}
 
 MemoryDumpManager::MemoryDumpProviderInfo::~MemoryDumpProviderInfo() {}
 
-bool MemoryDumpManager::MemoryDumpProviderInfo::operator<(
-    const MemoryDumpProviderInfo& other) const {
-  if (task_runner == other.task_runner)
-    return dump_provider < other.dump_provider;
+bool MemoryDumpManager::MemoryDumpProviderInfo::Comparator::operator()(
+    const scoped_refptr<MemoryDumpManager::MemoryDumpProviderInfo>& a,
+    const scoped_refptr<MemoryDumpManager::MemoryDumpProviderInfo>& b) const {
+  if (!a || !b)
+    return a.get() < b.get();
+  if (a->task_runner == b->task_runner)
+    return a->dump_provider < b->dump_provider;
   // Ensure that unbound providers (task_runner == nullptr) always run last.
-  return !(task_runner < other.task_runner);
+  return !(a->task_runner < b->task_runner);
 }
 
 MemoryDumpManager::ProcessMemoryDumpAsyncState::ProcessMemoryDumpAsyncState(
     MemoryDumpRequestArgs req_args,
-    MemoryDumpProviderInfoSet::iterator next_dump_provider,
+    const MemoryDumpProviderInfo::OrderedSet& dump_providers,
     const scoped_refptr<MemoryDumpSessionState>& session_state,
     MemoryDumpCallback callback,
     const scoped_refptr<SingleThreadTaskRunner>& dump_thread_task_runner)
     : req_args(req_args),
-      next_dump_provider(next_dump_provider),
+      pending_dump_providers(dump_providers.begin(), dump_providers.end()),

[Ruud van Asseldonk, 2015/12/17 20:20:39]

If you make this a vector, this will cause insertion of the elements one by one
because set does not have a random access iterator. That will likely waste
memory too due to the growth factor. Instead you can call |reserve| and |assign|
then.

[Primiano Tucci (use gerrit), 2015/12/18 12:00:31]

ok switches to a vector (and using that as a stack).

       session_state(session_state),
       callback(callback),
       callback_task_runner(MessageLoop::current()->task_runner()),
       dump_thread_task_runner(dump_thread_task_runner) {}
 
 MemoryDumpManager::ProcessMemoryDumpAsyncState::~ProcessMemoryDumpAsyncState() {
 }
 
 ProcessMemoryDump* MemoryDumpManager::ProcessMemoryDumpAsyncState::
     GetOrCreateMemoryDumpContainerForProcess(ProcessId pid) {
   auto iter = process_dumps.find(pid);
   if (iter == process_dumps.end()) {
     scoped_ptr<ProcessMemoryDump> new_pmd(new ProcessMemoryDump(session_state));
     iter = process_dumps.insert(std::make_pair(pid, std::move(new_pmd))).first;
   }
   return iter->second.get();
 }
 
 }  // namespace trace_event
 }  // namespace base