[tracing] Allow asynchronous unregistration of unbound dump providers

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 67 matching lines @@
   TRACE_EVENT_NESTABLE_ASYNC_END1(
       MemoryDumpManager::kTraceCategory, "GlobalMemoryDump",
       TRACE_ID_MANGLE(dump_guid), "success", success);
 
   if (!wrapped_callback.is_null()) {
     wrapped_callback.Run(dump_guid, success);
     wrapped_callback.Reset();
   }
 }
 
+// Empty callback to PostTask the deletion of an owned MemoryDumpProvider.
+void DeleteMemoryDumpProvider(scoped_ptr<MemoryDumpProvider> mdp) {
+  // We just need an empty method that takes ownership of the scoped_ptr
+  // and de-scopes it, causing its deletion.
+}
+
 }  // namespace
 
 // static
 const char* const MemoryDumpManager::kTraceCategory =
     TRACE_DISABLED_BY_DEFAULT("memory-infra");
 
 // static
 const int MemoryDumpManager::kMaxConsecutiveFailuresCount = 3;
 
 // static
@@ skipping 113 matching lines @@
 }
 
 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) {
+  UnregisterDumpProviderInternal(mdp, false /* delete_async */);
+}
+
+void MemoryDumpManager::UnregisterAndDeleteDumpProviderAsync(
+    scoped_ptr<MemoryDumpProvider> mdp) {
+  UnregisterDumpProviderInternal(mdp.release(), true /* delete_async */);
+}
+
+void MemoryDumpManager::UnregisterDumpProviderInternal(MemoryDumpProvider* mdp,
+                                                       bool delete_async) {
   AutoLock lock(lock_);
+  scoped_ptr<MemoryDumpProvider> owned_mdp;
+  if (delete_async)
+    owned_mdp.reset(mdp);  // Auto delete in case of early outs.
 
   auto mdp_iter = dump_providers_.begin();
   for (; mdp_iter != dump_providers_.end(); ++mdp_iter) {
     if (mdp_iter->dump_provider == mdp)
       break;
   }
 
   if (mdp_iter == dump_providers_.end())
-    return;
+    return;  // Already unregistered.

[Ruud van Asseldonk, 2015/12/14 16:25:48]

So the async deletion might actually be synchronous?

 
   // 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 "
-      << "unregister itself in a racy way. Please file a crbug.";
+  if (!delete_async && subtle::NoBarrier_Load(&memory_tracing_enabled_)) {
+    DCHECK((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;
+
+  // Delete the descriptor immediately if there are no no dumps in progress.
+  // Otherwise, the prologue of ContinueAsyncProcessDump will take care.
+  if (outstanding_dumps_.empty()) {
+    dump_providers_.erase(mdp_iter);
+    // If |delete_async| == true, the |mdp| will be destroyed synchronously here
+    // as there is no reason to defer the deletion (and no thread to defer to).
+  } else if (delete_async) {
+    outstanding_dumps_.front()->dump_thread_task_runner->PostTask(
+        FROM_HERE, Bind(&DeleteMemoryDumpProvider, Passed(&owned_mdp)));

[Ruud van Asseldonk, 2015/12/14 16:25:48]

Having an in-flight message own the dump provider feels wrong. Perhaps it would
be better to have a container of owned dump providers in the async state, so
they will go out of scope together with the async state? Then you won’t need
|DeleteMemoryDumpProvider| which is a bit awkward. The dump provider does not
care on which thread it is deleted anyway.

+  }
 }
 
 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 31 matching lines @@
     MemoryDumpType dump_type,
     MemoryDumpLevelOfDetail level_of_detail) {
   RequestGlobalDump(dump_type, level_of_detail, MemoryDumpCallback());
 }
 
 void MemoryDumpManager::CreateProcessDump(const MemoryDumpRequestArgs& args,
                                           const MemoryDumpCallback& callback) {
   TRACE_EVENT_NESTABLE_ASYNC_BEGIN0(kTraceCategory, "ProcessMemoryDump",
                                     TRACE_ID_MANGLE(args.dump_guid));
 
+  bool no_dump_providers_registered;
   scoped_ptr<ProcessMemoryDumpAsyncState> pmd_async_state;
   {
     AutoLock lock(lock_);
+    no_dump_providers_registered = dump_providers_.empty();
     pmd_async_state.reset(new ProcessMemoryDumpAsyncState(
         args, dump_providers_.begin(), session_state_, callback,
         dump_thread_->task_runner()));
+    outstanding_dumps_.push_back(pmd_async_state.get());

[Ruud van Asseldonk, 2015/12/14 16:25:48]

By doing this you lose all of the advantages of a |scoped_ptr|, because now you
need to ensure that the pointer in |outstanding_dumps_| does not become stale.
In all places where you would have to call delete if this were a naked pointer,
you now have to remove the pointer from this vector. The STL version is called
|unique_ptr| for a reason.

   }
 
   TRACE_EVENT_WITH_FLOW0(kTraceCategory, "MemoryDumpManager::CreateProcessDump",
                          TRACE_ID_MANGLE(args.dump_guid),
                          TRACE_EVENT_FLAG_FLOW_OUT);
 
+  if (no_dump_providers_registered)
+    return FinalizeDumpAndAddToTrace(pmd_async_state.Pass());
+
   // 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.
 //  - 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();
 
+  // 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 ownsership of the |pmd_async_state| when a

[Ruud van Asseldonk, 2015/12/14 16:25:48]

/s/ownsership/ownership/

+  // thread goes away and consequently PostTask() fails. PostTask(), in fact,
+  // destroyes the scoped_ptr arguments upon failure. This would prevent us to
+  // to just skip the hop and move on. Hence the naked -> scoped ptr conversion.
+  auto pmd_async_state = make_scoped_ptr(owned_pmd_async_state);

[Ruud van Asseldonk, 2015/12/14 16:25:48]

I wonder if it would be simpler to just make |pmd_async_state| refcounted.

+  owned_pmd_async_state = nullptr;
+
   const uint64_t dump_guid = pmd_async_state->req_args.dump_guid;
   const char* dump_provider_name = 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;
 
   // 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;
+    DCHECK(mdp_info != dump_providers_.end());
     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) {
       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

[Ruud van Asseldonk, 2015/12/14 16:25:48]

/s/unlikley/unlikely/

       // 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;

[Ruud van Asseldonk, 2015/12/14 16:25:48]

This variable is now unused. (Didn't Clang warn about that?)

       scoped_refptr<SingleThreadTaskRunner> callback_task_runner =

[Ruud van Asseldonk, 2015/12/14 16:25:48]

Same here.

           pmd_async_state->callback_task_runner;
 
       const bool did_post_task = task_runner->PostTask(

[Ruud van Asseldonk, 2015/12/14 16:25:48]

From the |PostTask| documentation, this is no guarantee that you don't leak
|pmd_async_state|.

           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().
+        (void)pmd_async_state.release();

[Ruud van Asseldonk, 2015/12/14 16:25:48]

You can do |pmd_async_state.reset()| instead.

         return;
+      }
 
-      // The thread is gone. At this point the best thing we can do is to
-      // disable the dump provider and abort this dump.
+      // The thread is gone. Disable the dump provider, skip its dump and move
+      // on with the other dumper providers.
       mdp_info->disabled = true;

[Ruud van Asseldonk, 2015/12/14 16:25:48]

Shouldn't all dump providers that share that thread be disabled?

-      return AbortDumpLocked(callback, callback_task_runner, dump_guid);
+      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_EVENT_FLAG_FLOW_IN | TRACE_EVENT_FLAG_FLOW_OUT,
                            "dump_provider.name", dump_provider_name);
     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);
   }
 
   {
     AutoLock lock(lock_);
     auto mdp_info = pmd_async_state->next_dump_provider;
+    DCHECK(mdp_info != dump_providers_.end());
     if (dump_successful) {
       mdp_info->consecutive_failures = 0;
     } else if (!skip_dump) {
       ++mdp_info->consecutive_failures;
       if (mdp_info->consecutive_failures >= kMaxConsecutiveFailuresCount) {
         mdp_info->disabled = true;
       }
     }
     ++pmd_async_state->next_dump_provider;
-    finalize = pmd_async_state->next_dump_provider == dump_providers_.end();
-
+    if (pmd_async_state->next_dump_provider == dump_providers_.end()) {
+      finalize = true;
+      std::remove(outstanding_dumps_.begin(), outstanding_dumps_.end(),
+                  pmd_async_state.get());
+    }
     if (mdp_info->unregistered)
       dump_providers_.erase(mdp_info);
-  }
+  }  // AutoLock(lock_)
 
   if (!skip_dump && !dump_successful) {
     LOG(ERROR) << "MemoryDumpProvider \"" << dump_provider_name << "\" failed, "
                << "possibly due to sandboxing (crbug.com/461788)."
                << "Disabling dumper for current process. Try --no-sandbox.";
   }
 
   if (finalize)
     return FinalizeDumpAndAddToTrace(std::move(pmd_async_state));
 
-  ContinueAsyncProcessDump(std::move(pmd_async_state));
+  ContinueAsyncProcessDump(std::move(pmd_async_state.release()));

[Ruud van Asseldonk, 2015/12/14 16:25:48]

You don't need the move here.

 }
 
 // static
 void MemoryDumpManager::FinalizeDumpAndAddToTrace(
     scoped_ptr<ProcessMemoryDumpAsyncState> pmd_async_state) {
   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(
@@ skipping 28 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 141 matching lines @@
   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