[tracing] Enable support for MemoryDumpProvider thread affinity

This is a follow up to crrev.com/1089493003, which introduced the
initial support to the asynchronous generation of memory dumps.
This CL introduces an optional TaskRunner argument to the the
constructor of MemoryDumpProvider. If specified, the MemoryDumpManager
will guarantee that all the calls to MemoryDumpProvider.DumpInto()
will be posted on the given thread. The DumpInto() calls are
still guaranteed to be serialized, therefore, the access to the pmd
argument of DumpInto is guaranteed to be thread-safe.
In other words: two MDP can DumpInto() from different threads, but
the MDM will ensure that it never happens at the same time.
The only contract that the MemoryDumpProvider has to respect is that,
if the MDP wants to unregister while tracing is active, the
MemoryDumpManager.UnregisterDumpProvider() call must happen on the
same thread of task_runner(). Any other alternative would be racy.

BUG=477558

Review URL: https://codereview.chromium.org/1078983004

Cr-Commit-Position: refs/heads/master@{#325801}

5 files changed, 209 insertions, 26 deletions

diff --git a/base/trace_event/memory_dump_manager.cc b/base/trace_event/memory_dump_manager.cc
index d6bfa6f..dd6164b 100644
--- a/base/trace_event/memory_dump_manager.cc
+++ b/base/trace_event/memory_dump_manager.cc
@@ -51,12 +51,24 @@ class ProcessMemoryDumpHolder
  public:
   ProcessMemoryDumpHolder(MemoryDumpRequestArgs req_args,
                           MemoryDumpCallback callback)
-      : req_args(req_args), callback(callback) {}
+      : req_args(req_args),
+        callback(callback),
+        task_runner(MessageLoop::current()->task_runner()),
+        num_pending_async_requests(0) {}
 
   ProcessMemoryDump process_memory_dump;
   const MemoryDumpRequestArgs req_args;
+
+  // Callback passed to the initial call to CreateProcessDump().
   MemoryDumpCallback callback;
 
+  // Thread on which FinalizeDumpAndAddToTrace() should be called, which is the
+  // same that invoked the initial CreateProcessDump().
+  const scoped_refptr<SingleThreadTaskRunner> task_runner;
+
+  // Number of pending ContinueAsyncProcessDump() calls.
+  int num_pending_async_requests;
+
  private:
   friend class RefCountedThreadSafe<ProcessMemoryDumpHolder>;
   virtual ~ProcessMemoryDumpHolder() {}
@@ -65,6 +77,14 @@ class ProcessMemoryDumpHolder
 
 void FinalizeDumpAndAddToTrace(
     const scoped_refptr<ProcessMemoryDumpHolder>& pmd_holder) {
+  DCHECK_EQ(0, pmd_holder->num_pending_async_requests);
+
+  if (!pmd_holder->task_runner->BelongsToCurrentThread()) {
+    pmd_holder->task_runner->PostTask(
+        FROM_HERE, Bind(&FinalizeDumpAndAddToTrace, pmd_holder));
+    return;
+  }
+
   scoped_refptr<ConvertableToTraceFormat> event_value(new TracedValue());
   pmd_holder->process_memory_dump.AsValueInto(
       static_cast<TracedValue*>(event_value.get()));
@@ -132,6 +152,19 @@ void MemoryDumpManager::RegisterDumpProvider(MemoryDumpProvider* mdp) {
 
 void MemoryDumpManager::UnregisterDumpProvider(MemoryDumpProvider* mdp) {
   AutoLock lock(lock_);
+
+  // 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 DumpInto() 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_IMPLIES(
+      subtle::NoBarrier_Load(&memory_tracing_enabled_),
+      mdp->task_runner() && mdp->task_runner()->BelongsToCurrentThread())
+      << "The MemoryDumpProvider " << mdp->GetFriendlyName() << " attempted to "
+      << "unregister itself in a racy way. Please file a crbug.";
+
   // Remove from the enabled providers list. This is to deal with the case that
   // UnregisterDumpProvider is called while the trace is enabled.
   dump_providers_enabled_.erase(mdp);
@@ -178,8 +211,15 @@ void MemoryDumpManager::CreateProcessDump(const MemoryDumpRequestArgs& args,
   ProcessMemoryDump* pmd = &pmd_holder->process_memory_dump;
   bool did_any_provider_dump = false;
 
-  // Serialize dump generation so that memory dump providers don't have to deal
-  // with thread safety.
+  // Iterate over the active dump providers and invoke DumpInto(pmd).
+  // The MDM guarantees linearity (at most one MDP is active within one
+  // process) and thread-safety (MDM enforces the right locking when entering /
+  // leaving the MDP.DumpInto() call). This is to simplify the clients' design
+  // and not let the MDPs worry about locking.
+  // As regards thread affinity, depending on the MDP configuration (see
+  // memory_dump_provider.h), the DumpInto() invocation can happen:
+  //  - Synchronousy on the MDM thread, when MDP.task_runner() is not set.
+  //  - Posted on MDP.task_runner(), when MDP.task_runner() is set.
   {
     AutoLock lock(lock_);
     for (auto dump_provider_iter = dump_providers_enabled_.begin();
@@ -187,19 +227,30 @@ void MemoryDumpManager::CreateProcessDump(const MemoryDumpRequestArgs& args,
       // InvokeDumpProviderLocked will remove the MDP from the set if it fails.
       MemoryDumpProvider* mdp = *dump_provider_iter;
       ++dump_provider_iter;
-      // Invoke the dump provider synchronously.
-      did_any_provider_dump |= InvokeDumpProviderLocked(mdp, pmd);
+      if (mdp->task_runner()) {
+        // The DumpInto() call must be posted.
+        bool did_post_async_task = mdp->task_runner()->PostTask(
+            FROM_HERE, Bind(&MemoryDumpManager::ContinueAsyncProcessDump,
+                            Unretained(this), Unretained(mdp), pmd_holder));
+        // The thread underlying the TaskRunner might have gone away.
+        if (did_post_async_task)
+          ++pmd_holder->num_pending_async_requests;
+      } else {
+        // Invoke the dump provider synchronously.
+        did_any_provider_dump |= InvokeDumpProviderLocked(mdp, pmd);
+      }
     }
   }  // AutoLock
 
-  // If at least one synchronous provider did dump, add the dump to the trace.
-  if (did_any_provider_dump)
+  // If at least one synchronous provider did dump and there are no pending
+  // asynchronous requests, add the dump to the trace and invoke the callback
+  // straight away (FinalizeDumpAndAddToTrace() takes care of the callback).
+  if (did_any_provider_dump && pmd_holder->num_pending_async_requests == 0)
     FinalizeDumpAndAddToTrace(pmd_holder);
 }
 
 // Invokes the MemoryDumpProvider.DumpInto(), taking care of the failsafe logic
-// which disables the dumper when failing (crbug.com/461788). The caller must
-// hold the |lock_| when invoking this.
+// which disables the dumper when failing (crbug.com/461788).
 bool MemoryDumpManager::InvokeDumpProviderLocked(MemoryDumpProvider* mdp,
                                                  ProcessMemoryDump* pmd) {
   lock_.AssertAcquired();
@@ -216,6 +267,36 @@ bool MemoryDumpManager::InvokeDumpProviderLocked(MemoryDumpProvider* mdp,
   return dump_successful;
 }
 
+// This is posted to arbitrary threads as a continuation of CreateProcessDump(),
+// when one or more MemoryDumpProvider(s) require the DumpInto() call to happen
+// on a different thread.
+void MemoryDumpManager::ContinueAsyncProcessDump(
+    MemoryDumpProvider* mdp,
+    scoped_refptr<ProcessMemoryDumpHolder> pmd_holder) {
+  bool should_finalize_dump = false;
+  {
+    // The lock here is to guarantee that different asynchronous dumps on
+    // different threads are still serialized, so that the MemoryDumpProvider
+    // has a consistent view of the |pmd| argument passed.
+    AutoLock lock(lock_);
+    ProcessMemoryDump* pmd = &pmd_holder->process_memory_dump;
+
+    // Check if the MemoryDumpProvider is still there. It might have been
+    // destroyed and unregistered while hopping threads.
+    if (dump_providers_enabled_.count(mdp))
+      InvokeDumpProviderLocked(mdp, pmd);
+
+    // Finalize the dump appending it to the trace if this was the last
+    // asynchronous request pending.
+    --pmd_holder->num_pending_async_requests;
+    if (pmd_holder->num_pending_async_requests == 0)
+      should_finalize_dump = true;
+  }  // AutoLock(lock_)
+
+  if (should_finalize_dump)
+    FinalizeDumpAndAddToTrace(pmd_holder);
+}
+
 void MemoryDumpManager::OnTraceLogEnabled() {
   // TODO(primiano): at this point we query  TraceLog::GetCurrentCategoryFilter
   // to figure out (and cache) which dumpers should be enabled or not.
diff --git a/base/trace_event/memory_dump_manager.h b/base/trace_event/memory_dump_manager.h
index 26cebc9..a311d7c 100644
--- a/base/trace_event/memory_dump_manager.h
+++ b/base/trace_event/memory_dump_manager.h
@@ -18,6 +18,10 @@
 namespace base {
 namespace trace_event {
 
+namespace {
+class ProcessMemoryDumpHolder;
+}
+
 class MemoryDumpManagerDelegate;
 class MemoryDumpProvider;
 class ProcessMemoryDump;
@@ -78,13 +82,16 @@ class BASE_EXPORT MemoryDumpManager : public TraceLog::EnabledStateObserver {
 
   // Internal, used only by MemoryDumpManagerDelegate.
   // Creates a memory dump for the current process and appends it to the trace.
-  // |callback| will be invoked upon completion on the same thread on which
-  // CreateProcessDump() was called.
+  // |callback| will be invoked asynchronously upon completion on the same
+  // thread on which CreateProcessDump() was called.
   void CreateProcessDump(const MemoryDumpRequestArgs& args,
                          const MemoryDumpCallback& callback);
 
   bool InvokeDumpProviderLocked(MemoryDumpProvider* mdp,
                                 ProcessMemoryDump* pmd);
+  void ContinueAsyncProcessDump(
+      MemoryDumpProvider* mdp,
+      scoped_refptr<ProcessMemoryDumpHolder> pmd_holder);
 
   hash_set<MemoryDumpProvider*> dump_providers_registered_;  // Not owned.
   hash_set<MemoryDumpProvider*> dump_providers_enabled_;     // Not owned.
diff --git a/base/trace_event/memory_dump_manager_unittest.cc b/base/trace_event/memory_dump_manager_unittest.cc
index 659a06c..638ae7d 100644
--- a/base/trace_event/memory_dump_manager_unittest.cc
+++ b/base/trace_event/memory_dump_manager_unittest.cc
@@ -4,6 +4,11 @@
 
 #include "base/trace_event/memory_dump_manager.h"
 
+#include "base/bind_helpers.h"
+#include "base/memory/scoped_vector.h"
+#include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
+#include "base/threading/thread.h"
 #include "base/trace_event/memory_dump_provider.h"
 #include "base/trace_event/process_memory_dump.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -30,6 +35,7 @@ class MemoryDumpManagerDelegateForTesting : public MemoryDumpManagerDelegate {
 class MemoryDumpManagerTest : public testing::Test {
  public:
   void SetUp() override {
+    message_loop_.reset(new MessageLoop());
     mdm_.reset(new MemoryDumpManager());
     MemoryDumpManager::SetInstanceForTesting(mdm_.get());
     ASSERT_EQ(mdm_, MemoryDumpManager::GetInstance());
@@ -40,9 +46,17 @@ class MemoryDumpManagerTest : public testing::Test {
   void TearDown() override {
     MemoryDumpManager::SetInstanceForTesting(nullptr);
     mdm_.reset();
+    message_loop_.reset();
     TraceLog::DeleteForTesting();
   }
 
+  void DumpCallbackAdapter(scoped_refptr<SingleThreadTaskRunner> task_runner,
+                           Closure closure,
+                           uint64 dump_guid,
+                           bool success) {
+    task_runner->PostTask(FROM_HERE, closure);
+  }
+
  protected:
   const char* kTraceCategory = MemoryDumpManager::kTraceCategoryForTesting;
 
@@ -56,6 +70,7 @@ class MemoryDumpManagerTest : public testing::Test {
   scoped_ptr<MemoryDumpManager> mdm_;
 
  private:
+  scoped_ptr<MessageLoop> message_loop_;
   MemoryDumpManagerDelegateForTesting delegate_;
 
   // We want our singleton torn down after each test.
@@ -64,6 +79,9 @@ class MemoryDumpManagerTest : public testing::Test {
 
 class MockDumpProvider : public MemoryDumpProvider {
  public:
+  MockDumpProvider() {}
+  MockDumpProvider(const scoped_refptr<SingleThreadTaskRunner>& task_runner)
+      : MemoryDumpProvider(task_runner) {}
   MOCK_METHOD1(DumpInto, bool(ProcessMemoryDump* pmd));
 
   // DumpInto() override for the ActiveDumpProviderConsistency test.
@@ -74,6 +92,12 @@ class MockDumpProvider : public MemoryDumpProvider {
     return true;
   }
 
+  // DumpInto() override for the RespectTaskRunnerAffinity test.
+  bool DumpIntoAndCheckTaskRunner(ProcessMemoryDump* pmd) {
+    EXPECT_TRUE(task_runner()->RunsTasksOnCurrentThread());
+    return true;
+  }
+
   const char* GetFriendlyName() const override { return "MockDumpProvider"; }
 };
 
@@ -104,21 +128,6 @@ TEST_F(MemoryDumpManagerTest, SingleDumper) {
   TraceLog::GetInstance()->SetDisabled();
 }
 
-TEST_F(MemoryDumpManagerTest, UnregisterDumperWhileTracing) {
-  MockDumpProvider mdp;
-  mdm_->RegisterDumpProvider(&mdp);
-
-  EnableTracing(kTraceCategory);
-  EXPECT_CALL(mdp, DumpInto(_)).Times(1).WillRepeatedly(Return(true));
-  mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED);
-
-  mdm_->UnregisterDumpProvider(&mdp);
-  EXPECT_CALL(mdp, DumpInto(_)).Times(0);
-  mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED);
-
-  DisableTracing();
-}
-
 TEST_F(MemoryDumpManagerTest, MultipleDumpers) {
   MockDumpProvider mdp1;
   MockDumpProvider mdp2;
@@ -149,6 +158,65 @@ TEST_F(MemoryDumpManagerTest, MultipleDumpers) {
   DisableTracing();
 }
 
+// Checks that the MemoryDumpManager respects the thread affinity when a
+// MemoryDumpProvider specifies a task_runner(). The test starts creating 8
+// threads and registering a MemoryDumpProvider on each of them. At each
+// iteration, one thread is removed, to check the live unregistration logic.
+TEST_F(MemoryDumpManagerTest, RespectTaskRunnerAffinity) {
+  const uint32 kNumInitialThreads = 8;
+
+  ScopedVector<Thread> threads;
+  ScopedVector<MockDumpProvider> mdps;
+
+  // Create the threads and setup the expectations. Given that at each iteration
+  // we will pop out one thread/MemoryDumpProvider, each MDP is supposed to be
+  // invoked a number of times equal to its index.
+  for (uint32 i = kNumInitialThreads; i > 0; --i) {
+    threads.push_back(new Thread("test thread"));
+    threads.back()->Start();
+    mdps.push_back(new MockDumpProvider(threads.back()->task_runner()));
+    MockDumpProvider* mdp = mdps.back();
+    mdm_->RegisterDumpProvider(mdp);
+    EXPECT_CALL(*mdp, DumpInto(_))
+        .Times(i)
+        .WillRepeatedly(
+            Invoke(mdp, &MockDumpProvider::DumpIntoAndCheckTaskRunner));
+  }
+
+  EnableTracing(kTraceCategory);
+
+  while (!threads.empty()) {
+    {
+      RunLoop run_loop;
+      MemoryDumpCallback callback =
+          Bind(&MemoryDumpManagerTest::DumpCallbackAdapter, Unretained(this),
+               MessageLoop::current()->task_runner(), run_loop.QuitClosure());
+      mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED, callback);
+      // This nested message loop (|run_loop|) will be quit if and only if
+      // the RequestGlobalDump callback is invoked.
+      run_loop.Run();
+    }
+
+    // Unregister a MDP and destroy one thread at each iteration to check the
+    // live unregistration logic. The unregistration needs to happen on the same
+    // thread the MDP belongs to.
+    {
+      RunLoop run_loop;
+      Closure unregistration =
+          Bind(&MemoryDumpManager::UnregisterDumpProvider,
+               Unretained(mdm_.get()), Unretained(mdps.back()));
+      threads.back()->task_runner()->PostTaskAndReply(FROM_HERE, unregistration,
+                                                      run_loop.QuitClosure());
+      run_loop.Run();
+    }
+    mdps.pop_back();
+    threads.back()->Stop();
+    threads.pop_back();
+  }
+
+  DisableTracing();
+}
+
 // Enable both dump providers, make mdp1 fail and assert that only mdp2 is
 // invoked the 2nd time.
 // FIXME(primiano): remove once crbug.com/461788 gets fixed.
diff --git a/base/trace_event/memory_dump_provider.cc b/base/trace_event/memory_dump_provider.cc
index 9518461..205fd32 100644
--- a/base/trace_event/memory_dump_provider.cc
+++ b/base/trace_event/memory_dump_provider.cc
@@ -5,6 +5,7 @@
 #include "base/trace_event/memory_dump_provider.h"
 
 #include "base/logging.h"
+#include "base/single_thread_task_runner.h"
 
 namespace base {
 namespace trace_event {
@@ -12,6 +13,11 @@ namespace trace_event {
 MemoryDumpProvider::MemoryDumpProvider() {
 }
 
+MemoryDumpProvider::MemoryDumpProvider(
+    const scoped_refptr<SingleThreadTaskRunner>& task_runner)
+    : task_runner_(task_runner) {
+}
+
 MemoryDumpProvider::~MemoryDumpProvider() {
 }
 
diff --git a/base/trace_event/memory_dump_provider.h b/base/trace_event/memory_dump_provider.h
index 62d562c..455eeca 100644
--- a/base/trace_event/memory_dump_provider.h
+++ b/base/trace_event/memory_dump_provider.h
@@ -6,9 +6,13 @@
 #define BASE_TRACE_EVENT_MEMORY_DUMP_PROVIDER_H_
 
 #include "base/base_export.h"
+#include "base/memory/ref_counted.h"
 #include "base/trace_event/memory_allocator_attributes.h"
 
 namespace base {
+
+class SingleThreadTaskRunner;
+
 namespace trace_event {
 
 class ProcessMemoryDump;
@@ -26,8 +30,22 @@ class BASE_EXPORT MemoryDumpProvider {
     return allocator_attributes_;
   }
 
+  // The dump provider can specify an optional thread affinity (in its
+  // base constructor call). If |task_runner| is non empty, all the calls to
+  // DumpInto are guaranteed to be posted to that TaskRunner.
+  const scoped_refptr<SingleThreadTaskRunner>& task_runner() const {
+    return task_runner_;
+  }
+
  protected:
+  // Default ctor: the MDP is not bound to any thread (must be a singleton).
   MemoryDumpProvider();
+
+  // Use this ctor to ensure that DumpInto() is called always on the same thread
+  // specified by |task_runner|.
+  explicit MemoryDumpProvider(
+      const scoped_refptr<SingleThreadTaskRunner>& task_runner);
+
   virtual ~MemoryDumpProvider();
 
   void DeclareAllocatorAttribute(const MemoryAllocatorDeclaredAttribute& attr);
@@ -38,6 +56,9 @@ class BASE_EXPORT MemoryDumpProvider {
   // MemoryDumpProvider will have.
   MemoryAllocatorDeclaredAttributes allocator_attributes_;
 
+  // (Optional) TaskRunner on which the DumpInfo call should be posted.
+  const scoped_refptr<SingleThreadTaskRunner> task_runner_;
+
   DISALLOW_COPY_AND_ASSIGN(MemoryDumpProvider);
 };