[tracing] MemoryDumpManager uses TraceConfig to set the periodic dump timers

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 "base/bind_helpers.h"
 #include "base/memory/scoped_vector.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
 #include "base/test/test_io_thread.h"
 #include "base/thread_task_runner_handle.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"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using testing::_;
 using testing::AtMost;
 using testing::Between;
 using testing::Invoke;
 using testing::Return;
 
 namespace base {
 namespace trace_event {
 namespace {
-MemoryDumpArgs high_detail_args = {MemoryDumpArgs::LevelOfDetail::HIGH};
-MemoryDumpArgs low_detail_args = {MemoryDumpArgs::LevelOfDetail::LOW};
-}
+MemoryDumpArgs g_high_detail_args = {MemoryDumpArgs::LevelOfDetail::HIGH};
+MemoryDumpArgs g_low_detail_args = {MemoryDumpArgs::LevelOfDetail::LOW};
+
+const char kMemoryDumpTraceConfigString[] =
+  "{"
+    "\"included_categories\":["
+      "\"disabled-by-default-memory-infra\""

[Primiano Tucci (use gerrit), 2015/08/26 10:25:07]

I don't think you want the "disabled-by-default-memory-infra" here right?

+    "],"
+    "\"memory_dump_config\":{"
+      "\"triggers\":["
+        "{"
+          "\"level_of_detail\":\"low\","
+          "\"periodic_interval_ms\":50"
+        "},"
+        "{"
+          "\"level_of_detail\":\"high\","
+          "\"periodic_interval_ms\":100"
+        "},"
+        "{"
+          "\"level_of_detail\":\"high\","
+          "\"periodic_interval_ms\":150"
+        "}"
+      "]"
+    "}"
+  "}";
+
+}  // namespace
 
 // Testing MemoryDumpManagerDelegate which short-circuits dump requests locally
 // instead of performing IPC dances.
 class MemoryDumpManagerDelegateForTesting : public MemoryDumpManagerDelegate {
  public:
   void RequestGlobalMemoryDump(const MemoryDumpRequestArgs& args,
                                const MemoryDumpCallback& callback) override {
     CreateProcessDump(args, callback);
   }
 
   bool IsCoordinatorProcess() const override { return false; }
   uint64 GetTracingProcessId() const override {
     return MemoryDumpManager::kInvalidTracingProcessId;
   }
 };
 
+class CoordinatorMemoryDumpManagerDelegateForTesting

[Primiano Tucci (use gerrit), 2015/08/26 10:25:07]

Can you Just add a static public boolean to MemoryDumpManagerDelegateForTesting?
it's simpler to read.

So you don't need to change anything and just do

MemoryDumpManagerDelegateForTesting::is_coordinator_process = true below

+    : public MemoryDumpManagerDelegateForTesting {
+  bool IsCoordinatorProcess() const override { return true; }
+};
+
 class MemoryDumpManagerTest : public testing::Test {
  public:
   void SetUp() override {
     last_callback_success_ = false;
     message_loop_.reset(new MessageLoop());
     mdm_.reset(new MemoryDumpManager());
     MemoryDumpManager::SetInstanceForTesting(mdm_.get());
     ASSERT_EQ(mdm_, MemoryDumpManager::GetInstance());
     MemoryDumpManager::GetInstance()->Initialize();
-    MemoryDumpManager::GetInstance()->SetDelegate(&delegate_);
   }
 
   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) {
     last_callback_success_ = success;
     task_runner->PostTask(FROM_HERE, closure);
   }
 
  protected:
   void EnableTracing(const char* category) {

[Primiano Tucci (use gerrit), 2015/08/26 10:25:07]

I think at this point you don't need the char* argument anymore and can make
this just EnableTracing()

+    delegate_.reset(new MemoryDumpManagerDelegateForTesting());
+    MemoryDumpManager::GetInstance()->SetDelegate(delegate_.get());
     TraceLog::GetInstance()->SetEnabled(
         TraceConfig(category, ""), TraceLog::RECORDING_MODE);
   }
 
+  void EnableTracingWithTriggers() {

[Primiano Tucci (use gerrit), 2015/08/26 10:25:07]

Rename this to EnableTracingWithTraceConfig(const char* trace_config)

And I'd remove the delegate_.reset(new
CoordinatorMemoryDumpManagerDelegateForTesting()).
Just in the test fixture below do:
MemoryDumpManagerDelegateForTesting::is_coordinator_process = true;

+    delegate_.reset(new CoordinatorMemoryDumpManagerDelegateForTesting());
+    MemoryDumpManager::GetInstance()->SetDelegate(delegate_.get());
+    TraceConfig tc(kMemoryDumpTraceConfigString);
+    TraceLog::GetInstance()->SetEnabled(tc, TraceLog::RECORDING_MODE);
+  }
+
   void DisableTracing() { TraceLog::GetInstance()->SetDisabled(); }
 
   scoped_ptr<MemoryDumpManager> mdm_;
   bool last_callback_success_;
 
  private:
   scoped_ptr<MessageLoop> message_loop_;
-  MemoryDumpManagerDelegateForTesting delegate_;
+  scoped_ptr<MemoryDumpManagerDelegateForTesting> delegate_;
 
   // We want our singleton torn down after each test.
   ShadowingAtExitManager at_exit_manager_;
 };
 
 class MockDumpProvider : public MemoryDumpProvider {
  public:
   MockDumpProvider()
-      : dump_provider_to_register_or_unregister(nullptr),
-        last_session_state_(nullptr),
-        level_of_detail_(MemoryDumpArgs::LevelOfDetail::HIGH) {}
+      : MockDumpProvider(nullptr, MemoryDumpArgs::LevelOfDetail::HIGH, 0) {}
 
   // Ctor used by the RespectTaskRunnerAffinity test.
   explicit MockDumpProvider(
       const scoped_refptr<SingleThreadTaskRunner>& task_runner)
-      : last_session_state_(nullptr),
-        task_runner_(task_runner),
-        level_of_detail_(MemoryDumpArgs::LevelOfDetail::HIGH) {}
+      : MockDumpProvider(task_runner, MemoryDumpArgs::LevelOfDetail::HIGH, 0) {}
 
   // Ctor used by CheckMemoryDumpArgs test.
   explicit MockDumpProvider(const MemoryDumpArgs::LevelOfDetail level_of_detail)
-      : last_session_state_(nullptr), level_of_detail_(level_of_detail) {}
+      : MockDumpProvider(nullptr, level_of_detail, 0) {}
+
+  // Ctor used by TraceConfigTriggers test.
+  explicit MockDumpProvider(int dump_calls_to_disable_tracing)
+      : MockDumpProvider(nullptr,
+                         MemoryDumpArgs::LevelOfDetail::HIGH,
+                         dump_calls_to_disable_tracing) {}
+
+  MockDumpProvider(const scoped_refptr<SingleThreadTaskRunner>& task_runner,
+                   const MemoryDumpArgs::LevelOfDetail level_of_detail,
+                   int dump_calls_to_disable_tracing)
+      : dump_provider_to_register_or_unregister(nullptr),
+        last_session_state_(nullptr),
+        level_of_detail_(level_of_detail),
+        on_memory_dump_calls_(0),
+        dump_calls_to_disable_tracing_(dump_calls_to_disable_tracing) {}
 
   virtual ~MockDumpProvider() {}
 
   MOCK_METHOD2(OnMemoryDump,
                bool(const MemoryDumpArgs& args, ProcessMemoryDump* pmd));
 
   // OnMemoryDump() override for the RespectTaskRunnerAffinity test.
   bool OnMemoryDump_CheckTaskRunner(const MemoryDumpArgs& args,
                                     ProcessMemoryDump* pmd) {
     EXPECT_TRUE(task_runner_->RunsTasksOnCurrentThread());
@@ skipping 26 matching lines @@
     return true;
   }
 
   // OnMemoryDump() override for the CheckMemoryDumpArgs test.
   bool OnMemoryDump_CheckMemoryDumpArgs(const MemoryDumpArgs& args,
                                         ProcessMemoryDump* pmd) {
     EXPECT_EQ(level_of_detail_, args.level_of_detail);
     return true;
   }
 
+  // OnMemoryDump() override for the TraceConfigTriggers test.
+  bool OnMemoryDump_TraceConfigTriggers(const MemoryDumpArgs& args,
+                                        ProcessMemoryDump* pmd) {
+    if (on_memory_dump_calls_ % 2 == 0 || on_memory_dump_calls_ % 3 == 0)

[Primiano Tucci (use gerrit), 2015/08/26 10:25:06]

I don't think you need all this logic, just use the gtest expectations below.

+      EXPECT_EQ(args.level_of_detail, MemoryDumpArgs::LevelOfDetail::HIGH);
+    else
+      EXPECT_EQ(args.level_of_detail, MemoryDumpArgs::LevelOfDetail::LOW);
+
+    on_memory_dump_calls_++;
+    if (on_memory_dump_calls_ == dump_calls_to_disable_tracing_) {
+      TraceLog::GetInstance()->SetDisabled();
+      MemoryDumpManager::GetInstance()->UnregisterDumpProvider(this);
+    }
+    return true;
+  }
+
   // Used by OnMemoryDump_(Un)RegisterExtraDumpProvider.
   MemoryDumpProvider* dump_provider_to_register_or_unregister;
 
  private:
   MemoryDumpSessionState* last_session_state_;
   scoped_refptr<SingleThreadTaskRunner> task_runner_;
   const MemoryDumpArgs::LevelOfDetail level_of_detail_;
+  int on_memory_dump_calls_;
+  int dump_calls_to_disable_tracing_;
 };
 
 TEST_F(MemoryDumpManagerTest, SingleDumper) {
   MockDumpProvider mdp;
   mdm_->RegisterDumpProvider(&mdp);
 
   // Check that the dumper is not called if the memory category is not enabled.
   EnableTracing("foo-and-bar-but-not-memory");
   EXPECT_CALL(mdp, OnMemoryDump(_, _)).Times(0);
   mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
-                          high_detail_args);
+                          g_high_detail_args);
   DisableTracing();
 
   // Now repeat enabling the memory category and check that the dumper is
   // invoked this time.
   EnableTracing(MemoryDumpManager::kTraceCategory);
   EXPECT_CALL(mdp, OnMemoryDump(_, _)).Times(3).WillRepeatedly(Return(true));
   for (int i = 0; i < 3; ++i)
     mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
-                            high_detail_args);
+                            g_high_detail_args);
   DisableTracing();
 
   mdm_->UnregisterDumpProvider(&mdp);
 
   // Finally check the unregister logic (no calls to the mdp after unregister).
   EnableTracing(MemoryDumpManager::kTraceCategory);
   EXPECT_CALL(mdp, OnMemoryDump(_, _)).Times(0);
   mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
-                          high_detail_args);
+                          g_high_detail_args);
   TraceLog::GetInstance()->SetDisabled();
 }
 
 TEST_F(MemoryDumpManagerTest, CheckMemoryDumpArgs) {
   // Check that requesting dumps with high level of detail actually propagates
   // to OnMemoryDump() call on dump providers.
   MockDumpProvider mdp_high_detail(MemoryDumpArgs::LevelOfDetail::HIGH);
   mdm_->RegisterDumpProvider(&mdp_high_detail);
 
   EnableTracing(MemoryDumpManager::kTraceCategory);
   EXPECT_CALL(mdp_high_detail, OnMemoryDump(_, _))
       .Times(1)
       .WillRepeatedly(
           Invoke(&mdp_high_detail,
                  &MockDumpProvider::OnMemoryDump_CheckMemoryDumpArgs));
   mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
-                          high_detail_args);
+                          g_high_detail_args);
   DisableTracing();
   mdm_->UnregisterDumpProvider(&mdp_high_detail);
 
   // Check that requesting dumps with low level of detail actually propagates to
   // OnMemoryDump() call on dump providers.
   MockDumpProvider mdp_low_detail(MemoryDumpArgs::LevelOfDetail::LOW);
   mdm_->RegisterDumpProvider(&mdp_low_detail);
 
   EnableTracing(MemoryDumpManager::kTraceCategory);
   EXPECT_CALL(mdp_low_detail, OnMemoryDump(_, _))
       .Times(1)
       .WillRepeatedly(
           Invoke(&mdp_low_detail,
                  &MockDumpProvider::OnMemoryDump_CheckMemoryDumpArgs));
   mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
-                          low_detail_args);
+                          g_low_detail_args);
   DisableTracing();
   mdm_->UnregisterDumpProvider(&mdp_low_detail);
 }
 
 TEST_F(MemoryDumpManagerTest, SharedSessionState) {
   MockDumpProvider mdp1;
   MockDumpProvider mdp2;
   mdm_->RegisterDumpProvider(&mdp1);
   mdm_->RegisterDumpProvider(&mdp2);
 
   EnableTracing(MemoryDumpManager::kTraceCategory);
   EXPECT_CALL(mdp1, OnMemoryDump(_, _))
       .Times(2)
       .WillRepeatedly(
           Invoke(&mdp1, &MockDumpProvider::OnMemoryDump_CheckSessionState));
   EXPECT_CALL(mdp2, OnMemoryDump(_, _))
       .Times(2)
       .WillRepeatedly(
           Invoke(&mdp2, &MockDumpProvider::OnMemoryDump_CheckSessionState));
 
   for (int i = 0; i < 2; ++i)
     mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
-                            high_detail_args);
+                            g_high_detail_args);
 
   DisableTracing();
 }
 
 TEST_F(MemoryDumpManagerTest, MultipleDumpers) {
   MockDumpProvider mdp1;
   MockDumpProvider mdp2;
 
   // Enable only mdp1.
   mdm_->RegisterDumpProvider(&mdp1);
   EnableTracing(MemoryDumpManager::kTraceCategory);
   EXPECT_CALL(mdp1, OnMemoryDump(_, _)).Times(1).WillRepeatedly(Return(true));
   EXPECT_CALL(mdp2, OnMemoryDump(_, _)).Times(0);
   mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
-                          high_detail_args);
+                          g_high_detail_args);
   DisableTracing();
 
   // Invert: enable mdp1 and disable mdp2.
   mdm_->UnregisterDumpProvider(&mdp1);
   mdm_->RegisterDumpProvider(&mdp2);
   EnableTracing(MemoryDumpManager::kTraceCategory);
   EXPECT_CALL(mdp1, OnMemoryDump(_, _)).Times(0);
   EXPECT_CALL(mdp2, OnMemoryDump(_, _)).Times(1).WillRepeatedly(Return(true));
   mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
-                          high_detail_args);
+                          g_high_detail_args);
   DisableTracing();
 
   // Enable both mdp1 and mdp2.
   mdm_->RegisterDumpProvider(&mdp1);
   EnableTracing(MemoryDumpManager::kTraceCategory);
   EXPECT_CALL(mdp1, OnMemoryDump(_, _)).Times(1).WillRepeatedly(Return(true));
   EXPECT_CALL(mdp2, OnMemoryDump(_, _)).Times(1).WillRepeatedly(Return(true));
   mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
-                          high_detail_args);
+                          g_high_detail_args);
   DisableTracing();
 }
 
 // Verify that whether OnMemoryDump is called depends only on the current
 // registration state and not on previous registrations and dumps.
 TEST_F(MemoryDumpManagerTest, RegistrationConsistency) {
   MockDumpProvider mdp;
 
   mdm_->RegisterDumpProvider(&mdp);
 
@@ skipping 67 matching lines @@
   EnableTracing(MemoryDumpManager::kTraceCategory);
 
   while (!threads.empty()) {
     last_callback_success_ = false;
     {
       RunLoop run_loop;
       MemoryDumpCallback callback =
           Bind(&MemoryDumpManagerTest::DumpCallbackAdapter, Unretained(this),
                MessageLoop::current()->task_runner(), run_loop.QuitClosure());
       mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
-                              high_detail_args, callback);
+                              g_high_detail_args, callback);
       // This nested message loop (|run_loop|) will be quit if and only if
       // the RequestGlobalDump callback is invoked.
       run_loop.Run();
     }
     EXPECT_TRUE(last_callback_success_);
 
     // 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.
     {
@@ skipping 27 matching lines @@
       .Times(MemoryDumpManager::kMaxConsecutiveFailuresCount)
       .WillRepeatedly(Return(false));
 
   EXPECT_CALL(mdp2, OnMemoryDump(_, _))
       .Times(1 + MemoryDumpManager::kMaxConsecutiveFailuresCount)
       .WillOnce(Return(false))
       .WillRepeatedly(Return(true));
   for (int i = 0; i < 1 + MemoryDumpManager::kMaxConsecutiveFailuresCount;
        i++) {
     mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
-                            high_detail_args);
+                            g_high_detail_args);
   }
 
   DisableTracing();
 }
 
 // Sneakily register an extra memory dump provider while an existing one is
 // dumping and expect it to take part in the already active tracing session.
 TEST_F(MemoryDumpManagerTest, RegisterDumperWhileDumping) {
   MockDumpProvider mdp1;
   MockDumpProvider mdp2;
@@ skipping 10 matching lines @@
       .WillRepeatedly(Return(true));
 
   // Depending on the insertion order (before or after mdp1), mdp2 might be
   // called also immediately after it gets registered.
   EXPECT_CALL(mdp2, OnMemoryDump(_, _))
       .Times(Between(2, 3))
       .WillRepeatedly(Return(true));
 
   for (int i = 0; i < 4; i++) {
     mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
-                            high_detail_args);
+                            g_high_detail_args);
   }
 
   DisableTracing();
 }
 
 // Like the above, but suddenly unregister the dump provider.
 TEST_F(MemoryDumpManagerTest, UnregisterDumperWhileDumping) {
   MockDumpProvider mdp1;
   MockDumpProvider mdp2;
 
@@ skipping 10 matching lines @@
       .WillRepeatedly(Return(true));
 
   // Depending on the insertion order (before or after mdp1), mdp2 might have
   // been already called when OnMemoryDump_UnregisterDumpProvider happens.
   EXPECT_CALL(mdp2, OnMemoryDump(_, _))
       .Times(Between(1, 2))
       .WillRepeatedly(Return(true));
 
   for (int i = 0; i < 4; i++) {
     mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
-                            high_detail_args);
+                            g_high_detail_args);
   }
 
   DisableTracing();
 }
 
 // Verify that the dump does not abort when unregistering a provider while
 // dumping from a different thread than the dumping thread.
 TEST_F(MemoryDumpManagerTest, UnregisterDumperFromThreadWhileDumping) {
   ScopedVector<TestIOThread> threads;
   ScopedVector<MockDumpProvider> mdps;
@@ skipping 55 matching lines @@
   mdm_->RegisterDumpProvider(&mdp1);
   EXPECT_CALL(mdp1, OnMemoryDump(_, _)).Times(0);
 
   last_callback_success_ = true;
   {
     RunLoop run_loop;
     MemoryDumpCallback callback =
         Bind(&MemoryDumpManagerTest::DumpCallbackAdapter, Unretained(this),
              MessageLoop::current()->task_runner(), run_loop.QuitClosure());
     mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
-                            high_detail_args, callback);
+                            g_high_detail_args, callback);
     run_loop.Run();
   }
   EXPECT_FALSE(last_callback_success_);
 }
 
+TEST_F(MemoryDumpManagerTest, TraceConfigTriggers) {

[Primiano Tucci (use gerrit), 2015/08/26 10:25:06]

The name here (TraceConfigTriggers) should reflect what this fixture is testing.
Also please add a comment explaining what this test does.
I'd probably name it SchedulePeriodicDumpsFromTraceConfig

+  MockDumpProvider mdp;
+  mdm_->RegisterDumpProvider(&mdp);
+  const int kTestDumpCount = 10;
+  RunLoop run_loop;
+  EnableTracingWithTriggers();
+  EXPECT_CALL(mdp, OnMemoryDump(_, _))
+      .Times(kTestDumpCount)
+      .WillRepeatedly(
+          Invoke(&mdp, &MockDumpProvider::OnMemoryDump_TraceConfigTriggers));
+
+  // 50 ms is the shortest interval given in the kMemoryDumpTraceConfigString.

[Primiano Tucci (use gerrit), 2015/08/26 10:25:06]

Please don't have tests based on timings as they are extremely flaky.
I'd just make this test as follows:
Make the dump provider post the QuitClosure on the Nth dump, and just check that
you receive the right amount of light and heavy dumps

+  // kTestDumpCount * 50 is time taken for kTestDumpCount "OnMemoryDump" calls.
+  const uint32 test_runtime = kTestDumpCount * 50 + 25;
+  MessageLoop::current()->task_runner()->PostDelayedTask(
+      FROM_HERE, run_loop.QuitClosure(),
+      base::TimeDelta::FromMilliseconds(test_runtime));
+  run_loop.Run();
+}

[Primiano Tucci (use gerrit), 2015/08/26 10:25:07]

Also you should have a test to make sure that if you use the old-style
traceconfig ctor you still get the periodic dumps.

[ssid, 2015/08/28 12:34:48]

This test is not needed here since, all we test here is if we get the correct
TraceConfig and follow it. This test was added in TraceConfig change.

+
 }  // namespace trace_event
 }  // namespace base