[MemoryInfra] Support dump providers running on SequencedTaskRunner

base/trace_event/memory_dump_manager_unittest.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 <stdint.h>
 
 #include <vector>
 
 #include "base/bind_helpers.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
 #include "base/strings/stringprintf.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/test/test_io_thread.h"
 #include "base/test/trace_event_analyzer.h"
 #include "base/thread_task_runner_handle.h"
 #include "base/threading/platform_thread.h"
+#include "base/threading/sequenced_worker_pool.h"
 #include "base/threading/thread.h"
 #include "base/trace_event/memory_dump_provider.h"
 #include "base/trace_event/process_memory_dump.h"
 #include "base/trace_event/trace_buffer.h"
 #include "base/trace_event/trace_config_memory_test_util.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using testing::_;
 using testing::AnyNumber;
@@ skipping 11 matching lines @@
 }
 
 MATCHER(IsLightDump, "") {
   return arg.level_of_detail == MemoryDumpLevelOfDetail::LIGHT;
 }
 
 namespace {
 
 void RegisterDumpProvider(
     MemoryDumpProvider* mdp,
+    const scoped_refptr<base::SequencedTaskRunner>& task_runner,
+    const MemoryDumpProvider::Options& options,
+    bool dumps_on_single_thread_task_runner) {
+  MemoryDumpManager* mdm = MemoryDumpManager::GetInstance();
+  mdm->set_dumper_registrations_ignored_for_testing(false);
+  const char* kMDPName = "TestDumpProvider";
+  if (dumps_on_single_thread_task_runner) {
+    mdm->RegisterDumpProvider(
+        mdp, kMDPName,
+        reinterpret_cast<const scoped_refptr<base::SingleThreadTaskRunner>&>(

[Primiano Tucci (use gerrit), 2016/02/18 16:14:37]

Hmm this would be a long conversation but TL;DR I don't think that upcasting
scoped_refptr is guaranteed to be safe.
Let's just be safe, and do an explicit copy here, i.e.:

scoped_refptr<base::SingleThreadTaskRunner> single_thread_task_runner =
static_cast<base::SingleThreadTaskRunner*> task_runner.get();

[ssid, 2016/02/24 19:19:58]

Done.

+            task_runner),
+        options);
+  } else {
+    mdm->RegisterDumpProviderWithSequencedTaskRunner(mdp, kMDPName, task_runner,
+                                                     options);
+  }
+  mdm->set_dumper_registrations_ignored_for_testing(true);
+}
+
+void RegisterDumpProvider(
+    MemoryDumpProvider* mdp,
     const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
     const MemoryDumpProvider::Options& options) {
-  MemoryDumpManager* mdm = MemoryDumpManager::GetInstance();
-  mdm->set_dumper_registrations_ignored_for_testing(false);
-  mdm->RegisterDumpProvider(mdp, "TestDumpProvider", task_runner, options);
-  mdm->set_dumper_registrations_ignored_for_testing(true);
+  RegisterDumpProvider(mdp, task_runner, options,
+                       true /* dumps_on_single_thread_task_runner */);
 }
 
 void RegisterDumpProvider(MemoryDumpProvider* mdp) {
   RegisterDumpProvider(mdp, nullptr, MemoryDumpProvider::Options());
 }
 
+void RegisterDumpProviderWithSequencedTaskRunner(
+    MemoryDumpProvider* mdp,
+    const scoped_refptr<base::SequencedTaskRunner>& task_runner,
+    const MemoryDumpProvider::Options& options) {
+  RegisterDumpProvider(mdp, task_runner, options,
+                       false /* dumps_on_single_thread_task_runner */);
+}
+
 void OnTraceDataCollected(Closure quit_closure,
                           trace_event::TraceResultBuffer* buffer,
                           const scoped_refptr<RefCountedString>& json,
                           bool has_more_events) {
   buffer->AddFragment(json->data());
   if (!has_more_events)
     quit_closure.Run();
 }
 
 }  // namespace
@@ skipping 29 matching lines @@
 
   MockMemoryDumpProvider() : enable_mock_destructor(false) {}
   ~MockMemoryDumpProvider() override {
     if (enable_mock_destructor)
       Destructor();
   }
 
   bool enable_mock_destructor;
 };
 
+class TestSequencedTaskRunner : public SequencedTaskRunner {
+ public:
+  TestSequencedTaskRunner()
+      : worker_pool_(
+            new SequencedWorkerPool(2 /* max_threads */, "Test Task Runner")),
+        disabled_(false),

[Primiano Tucci (use gerrit), 2016/02/18 16:14:37]

why calling it with negative names? Just call it enabled_(true) :)

[ssid, 2016/02/24 19:19:58]

fixed.

+        no_of_post_tasks_(0) {}
+
+  void set_enabled() { disabled_ = false; }

[Primiano Tucci (use gerrit), 2016/02/18 16:14:37]

just set_enabled(bool value) { enabled_ = value; } ?

[ssid, 2016/02/24 19:19:58]

Done.

+  void set_disabled() { disabled_ = true; }
+  unsigned no_of_post_tasks() const { return no_of_post_tasks_; }
+
+  bool PostNonNestableDelayedTask(const tracked_objects::Location& from_here,
+                                  const Closure& task,
+                                  TimeDelta delay) override {
+    NOTREACHED();
+    return false;
+  }
+
+  bool PostDelayedTask(const tracked_objects::Location& from_here,
+                       const Closure& task,
+                       TimeDelta delay) override {
+    EXPECT_EQ(TimeDelta(), delay);

[Primiano Tucci (use gerrit), 2016/02/18 16:14:37]

not sure why you are asserting this.
At most this is a DCHECK not a test expectation.

[ssid, 2016/02/24 19:19:58]

yeah it's not required.

+    no_of_post_tasks_++;
+    if (disabled_)
+      return false;
+    return worker_pool_->PostSequencedWorkerTask(token_, from_here, task);
+  }
+
+  bool RunsTasksOnCurrentThread() const override {
+    return worker_pool_->IsRunningSequenceOnCurrentThread(token_);
+  }
+
+ private:
+  ~TestSequencedTaskRunner() override {}
+
+  scoped_refptr<SequencedWorkerPool> worker_pool_;
+  const SequencedWorkerPool::SequenceToken token_;
+  bool disabled_;
+  unsigned no_of_post_tasks_;

[Primiano Tucci (use gerrit), 2016/02/18 16:14:37]

s/so/num/

[ssid, 2016/02/24 19:19:58]

Done.

+};
+
 class MemoryDumpManagerTest : public testing::Test {
  public:
   MemoryDumpManagerTest() : testing::Test(), kDefaultOptions() {}
 
   void SetUp() override {
     last_callback_success_ = false;
     message_loop_.reset(new MessageLoop());
     mdm_.reset(new MemoryDumpManager());
     MemoryDumpManager::SetInstanceForTesting(mdm_.get());
     ASSERT_EQ(mdm_.get(), MemoryDumpManager::GetInstance());
@@ skipping 252 matching lines @@
   {
     EXPECT_CALL(*delegate_, RequestGlobalMemoryDump(_, _)).Times(1);
     EXPECT_CALL(mdp, OnMemoryDump(_, _)).WillOnce(Return(true));
     EnableTracingWithLegacyCategories(MemoryDumpManager::kTraceCategory);
     RequestGlobalDumpAndWait(MemoryDumpType::EXPLICITLY_TRIGGERED,
                              MemoryDumpLevelOfDetail::DETAILED);
     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.
+// Checks that the MemoryDumpManager respects the task runner affinity when a
+// MemoryDumpProvider specifies task runner. The test starts creating 8 threads
+// and sequenced task runners and registering a MemoryDumpProvider on each of
+// them. At each iteration, one task runner of each kind is removed, to check
+// the live unregistration logic.
 TEST_F(MemoryDumpManagerTest, RespectTaskRunnerAffinity) {
   InitializeMemoryDumpManager(false /* is_coordinator */);
-  const uint32_t kNumInitialThreads = 8;
+  const uint32_t kNumTaskRunners = 8;
 
   std::vector<scoped_ptr<Thread>> threads;
-  std::vector<scoped_ptr<MockMemoryDumpProvider>> mdps;
+  std::vector<scoped_refptr<TestSequencedTaskRunner>> sequenced_task_runners;

[Primiano Tucci (use gerrit), 2016/02/18 16:14:37]

hmm I would keep the tests separate otherwise this TEST_F will be impossible to
debug.
Since you are adding coverage below, I would leave this test as it is

[ssid, 2016/02/24 19:19:58]

Done.

+  std::vector<scoped_ptr<MockMemoryDumpProvider>> threaded_mdps;
+  std::vector<scoped_ptr<MockMemoryDumpProvider>> sequenced_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_t i = kNumInitialThreads; i > 0; --i) {
+  // we will pop out one task runner of each kind and it's MDP, each MDP is
+  // supposed to be invoked a number of times equal to its index.
+  for (uint32_t i = kNumTaskRunners; i > 0; --i) {
     threads.push_back(make_scoped_ptr(new Thread("test thread")));
     auto thread = threads.back().get();
     thread->Start();
-    scoped_refptr<SingleThreadTaskRunner> task_runner = thread->task_runner();
-    mdps.push_back(make_scoped_ptr(new MockMemoryDumpProvider()));
-    auto mdp = mdps.back().get();
-    RegisterDumpProvider(mdp, task_runner, kDefaultOptions);
-    EXPECT_CALL(*mdp, OnMemoryDump(_, _))
+    scoped_refptr<SingleThreadTaskRunner> thread_task_runner =
+        thread->task_runner();
+    sequenced_task_runners.push_back(
+        make_scoped_refptr(new TestSequencedTaskRunner()));
+    auto sequenced_task_runner = sequenced_task_runners.back().get();
+
+    threaded_mdps.push_back(make_scoped_ptr(new MockMemoryDumpProvider()));
+    auto thread_mdp = threaded_mdps.back().get();
+    sequenced_mdps.push_back(make_scoped_ptr(new MockMemoryDumpProvider()));
+    auto sequenced_runner_mdp = sequenced_mdps.back().get();
+
+    RegisterDumpProvider(thread_mdp, thread_task_runner, kDefaultOptions);
+    RegisterDumpProviderWithSequencedTaskRunner(
+        sequenced_runner_mdp, sequenced_task_runner, kDefaultOptions);
+
+    EXPECT_CALL(*thread_mdp, OnMemoryDump(_, _))
         .Times(i)
-        .WillRepeatedly(Invoke(
-            [task_runner](const MemoryDumpArgs&, ProcessMemoryDump*) -> bool {
-              EXPECT_TRUE(task_runner->RunsTasksOnCurrentThread());
+        .WillRepeatedly(
+            Invoke([thread_task_runner](const MemoryDumpArgs&,
+                                        ProcessMemoryDump*) -> bool {
+              EXPECT_TRUE(thread_task_runner->RunsTasksOnCurrentThread());
               return true;
             }));
+
+    EXPECT_CALL(*sequenced_runner_mdp, OnMemoryDump(_, _))
+        .Times(i)
+        .WillRepeatedly(Invoke([sequenced_task_runner](
+                                   const MemoryDumpArgs&,
+                                   ProcessMemoryDump* pmd) -> bool {
+          EXPECT_TRUE(
+              SequencedWorkerPool::GetSequenceTokenForCurrentThread().Equals(
+                  SequencedWorkerPool::SequenceToken()));
+          EXPECT_TRUE(sequenced_task_runner->RunsTasksOnCurrentThread());
+          return true;
+        }));
   }
   EnableTracingWithLegacyCategories(MemoryDumpManager::kTraceCategory);
 
-  while (!threads.empty()) {
+  for (uint32_t i = 1; i <= kNumTaskRunners; ++i) {
     last_callback_success_ = false;
     EXPECT_CALL(*delegate_, RequestGlobalMemoryDump(_, _)).Times(1);
     RequestGlobalDumpAndWait(MemoryDumpType::EXPLICITLY_TRIGGERED,
                              MemoryDumpLevelOfDetail::DETAILED);
     EXPECT_TRUE(last_callback_success_);
+    // Check that each dump call was posted for the last sequenced task runner.
+    EXPECT_EQ(i, sequenced_task_runners.back()->no_of_post_tasks());
 
-    // 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.
+    // Destroy one thread and one sequenced task runner and their corresponding
+    // MDPs at each iteration to check the live unregistration logic. The
+    // unregistration needs to happen on the same task runner the MDP belongs
+    // to.
     {
       RunLoop run_loop;
-      Closure unregistration =
+      Closure unregistration1 =
           Bind(&MemoryDumpManager::UnregisterDumpProvider,
-               Unretained(mdm_.get()), Unretained(mdps.back().get()));
-      threads.back()->task_runner()->PostTaskAndReply(FROM_HERE, unregistration,
-                                                      run_loop.QuitClosure());
+               Unretained(mdm_.get()), Unretained(threaded_mdps.back().get()));
+      Closure unregistration2 =
+          Bind(&MemoryDumpManager::UnregisterDumpProvider,
+               Unretained(mdm_.get()), Unretained(sequenced_mdps.back().get()));
+      threads.back()->task_runner()->PostTask(FROM_HERE, unregistration1);
+      sequenced_task_runners.back()->PostTaskAndReply(
+          FROM_HERE, unregistration2, run_loop.QuitClosure());
       run_loop.Run();
     }
-    mdps.pop_back();
+
+    threaded_mdps.pop_back();
+    sequenced_mdps.pop_back();
     threads.back()->Stop();
+    sequenced_task_runners.pop_back();
     threads.pop_back();
   }
 
   DisableTracing();
 }
 
+// Check that the memory dump calls are always posted on task runner for
+// SequencedTaskRunner case and that the dump provider gets disabled when
+// PostTask fails, but the dump still succeeds.
+TEST_F(MemoryDumpManagerTest, PostTaskForSequencedTaskRunner) {
+  InitializeMemoryDumpManager(false /* is_coordinator */);
+  std::vector<MockMemoryDumpProvider> mdps(3);
+  scoped_refptr<TestSequencedTaskRunner> task_runner1(
+      make_scoped_refptr(new TestSequencedTaskRunner()));
+  scoped_refptr<TestSequencedTaskRunner> task_runner2(
+      make_scoped_refptr(new TestSequencedTaskRunner()));
+  RegisterDumpProviderWithSequencedTaskRunner(&mdps[0], task_runner1,
+                                              kDefaultOptions);
+  RegisterDumpProviderWithSequencedTaskRunner(&mdps[1], task_runner2,
+                                              kDefaultOptions);
+  RegisterDumpProviderWithSequencedTaskRunner(&mdps[2], task_runner2,
+                                              kDefaultOptions);
+  task_runner1->set_disabled();
+  EXPECT_CALL(mdps[0], OnMemoryDump(_, _)).Times(0);
+  EXPECT_CALL(mdps[1], OnMemoryDump(_, _)).Times(2);
+  EXPECT_CALL(mdps[2], OnMemoryDump(_, _)).Times(2);
+  EXPECT_CALL(*delegate_, RequestGlobalMemoryDump(_, _)).Times(2);
+
+  EnableTracingWithLegacyCategories(MemoryDumpManager::kTraceCategory);
+  last_callback_success_ = false;
+  RequestGlobalDumpAndWait(MemoryDumpType::EXPLICITLY_TRIGGERED,
+                           MemoryDumpLevelOfDetail::DETAILED);
+  // Tasks should be posted even if |mdps[1]| and |mdps[2]| belong to same task

[Primiano Tucci (use gerrit), 2016/02/18 16:14:37]

s/posted/individually posted/

[ssid, 2016/02/24 19:19:59]

Done.

+  // runner.
+  EXPECT_EQ(1u, task_runner1->no_of_post_tasks());
+  EXPECT_EQ(2u, task_runner2->no_of_post_tasks());
+  EXPECT_TRUE(last_callback_success_);
+
+  // Check that |mdps[0]|'s OnMemoryDump is not called even if the task runner
+  // is enabled since |mdps[0]| was disabled previously.
+  task_runner1->set_enabled();
+  last_callback_success_ = false;
+  RequestGlobalDumpAndWait(MemoryDumpType::EXPLICITLY_TRIGGERED,
+                           MemoryDumpLevelOfDetail::DETAILED);
+  EXPECT_EQ(2u, task_runner1->no_of_post_tasks());

[Primiano Tucci (use gerrit), 2016/02/18 16:14:37]

Hmm this I don't follow: how does task_runner1 get 2 post task? According to
your comment shouldn't it get 0?

[ssid, 2016/02/24 19:19:58]

The comment is at wrong place.
The post task happens even for disabled provider, but OnMemoryDump is not
called. Fixed the comment.

+  EXPECT_EQ(4u, task_runner2->no_of_post_tasks());
+  EXPECT_TRUE(last_callback_success_);
+  DisableTracing();
+}
+
 // Checks that providers get disabled after 3 consecutive failures, but not
 // otherwise (e.g., if interleaved).
 TEST_F(MemoryDumpManagerTest, DisableFailingDumpers) {
   InitializeMemoryDumpManager(false /* is_coordinator */);
   MockMemoryDumpProvider mdp1;
   MockMemoryDumpProvider mdp2;
 
   RegisterDumpProvider(&mdp1);
   RegisterDumpProvider(&mdp2);
   EnableTracingWithLegacyCategories(MemoryDumpManager::kTraceCategory);
@@ skipping 519 matching lines @@
   EXPECT_CALL(*delegate_, RequestGlobalMemoryDump(_, _)).Times(2);
   for (int i = 0; i < 2; ++i) {
     RequestGlobalDumpAndWait(MemoryDumpType::EXPLICITLY_TRIGGERED,
                              MemoryDumpLevelOfDetail::DETAILED);
   }
   DisableTracing();
 }
 
 }  // namespace trace_event
 }  // namespace base