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

base/trace_event/memory_dump_manager_unittest.cc

Index: base/trace_event/memory_dump_manager_unittest.cc
diff --git a/base/trace_event/memory_dump_manager_unittest.cc b/base/trace_event/memory_dump_manager_unittest.cc
index 4d0372fd3243286fca9fbca439a89108970be143..fb20b27ca15fa1e4dffca90f541d6da7b85d99ea 100644
--- a/base/trace_event/memory_dump_manager_unittest.cc
+++ b/base/trace_event/memory_dump_manager_unittest.cc
@@ -25,25 +25,48 @@ 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/09/03 08:39:51]

I'd probably move this as an inline string in  the TEST_F
SchedulePeriodicDumpsFromTraceConfig, so the test becomes more readable
(and make EnableTracingWithTraceConfig take a const char* arg)

[ssid, 2015/09/03 16:30:18]

Done.

+    "],"
+    "\"memory_dump_config\":{"
+      "\"triggers\":["
+        "{"
+          "\"mode\":\"light\","
+          "\"periodic_interval_ms\":1"

[Primiano Tucci (use gerrit), 2015/09/03 08:39:51]

Hmm I'm scared that this is too fast. There is the risk that some dumps will be
rejected (and the test fail) if the memory dump manager has not finished the
previous dump.
I'd probably make these 100 and 300 ms

[ssid, 2015/09/03 16:30:18]

I asked sami about this and he suggested this is the usual time that is set for
periodic timer tests. Also, if the test takes longer people might revert the
test since the unittests should be very short. He suggested 1ms should be good
enough.

[Sami, 2015/09/03 16:41:52]

Right, the idea was that we should try to keep unit test execution time on the
order of milliseconds instead of hundreds of ms. For instance in this test it
seems like we don't really care about the actual data in the dump so maybe
there's a way to mock that out.

+        "},"
+        "{"
+          "\"mode\":\"detailed\","
+          "\"periodic_interval_ms\":3"
+        "}"
+      "]"
+    "}"
+  "}";
+
+}  // namespace
 
 // Testing MemoryDumpManagerDelegate which short-circuits dump requests locally
 // instead of performing IPC dances.
 class MemoryDumpManagerDelegateForTesting : public MemoryDumpManagerDelegate {
  public:
+  static bool is_coordinator_process;
   void RequestGlobalMemoryDump(const MemoryDumpRequestArgs& args,
                                const MemoryDumpCallback& callback) override {
     CreateProcessDump(args, callback);
   }
 
-  bool IsCoordinatorProcess() const override { return false; }
+  bool IsCoordinatorProcess() const override { return is_coordinator_process; }
   uint64 GetTracingProcessId() const override {
     return MemoryDumpManager::kInvalidTracingProcessId;
   }
 };
 
+bool MemoryDumpManagerDelegateForTesting::is_coordinator_process = false;
+
 class MemoryDumpManagerTest : public testing::Test {
  public:
   void SetUp() override {
@@ -73,10 +96,17 @@ class MemoryDumpManagerTest : public testing::Test {
 
  protected:
   void EnableTracing(const char* category) {

[Primiano Tucci (use gerrit), 2015/09/03 08:39:51]

Please add a comment here saying that this enalbes tracing using the legacy
category filter string.

[ssid, 2015/09/03 16:30:18]

Done.

+    delegate_.is_coordinator_process = false;
     TraceLog::GetInstance()->SetEnabled(
         TraceConfig(category, ""), TraceLog::RECORDING_MODE);
   }
 
+  void EnableTracingWithTraceConfig() {
+    delegate_.is_coordinator_process = true;
+    TraceConfig tc(kMemoryDumpTraceConfigString);
+    TraceLog::GetInstance()->SetEnabled(tc, TraceLog::RECORDING_MODE);
+  }
+
   void DisableTracing() { TraceLog::GetInstance()->SetDisabled(); }
 
   scoped_ptr<MemoryDumpManager> mdm_;
@@ -162,6 +192,35 @@ class MockDumpProvider : public MemoryDumpProvider {
   const MemoryDumpArgs::LevelOfDetail level_of_detail_;
 };
 
+// Provider used by SchedulePeriodicDumpsFromTraceConfig.
+class MockDumpProviderForPeriodicDump : public MockDumpProvider {
+ public:
+  MockDumpProviderForPeriodicDump(int dump_calls_to_quit,
+                                  base::Closure quit_closure)
+      : dump_call_count_(0),
+        dump_calls_to_quit_(dump_calls_to_quit),
+        quit_closure_(quit_closure) {}
+  ~MockDumpProviderForPeriodicDump() override {}
+
+  // OnMemoryDump() override for the SchedulePeriodicDumpsFromTraceConfig test.
+  // Quits the loop after specified OnMemoryDump calls.
+  bool OnMemoryDump_SchedulePeriodicDumpsFromTraceConfig(
+      const MemoryDumpArgs& args,
+      ProcessMemoryDump* pmd) {
+    if (++dump_call_count_ == dump_calls_to_quit_) {
+      TraceLog::GetInstance()->SetDisabled();
+      MemoryDumpManager::GetInstance()->UnregisterDumpProvider(this);
+      MessageLoop::current()->task_runner()->PostTask(FROM_HERE, quit_closure_);

[Primiano Tucci (use gerrit), 2015/09/03 08:39:51]

s/MessageLoop::current()->task_runner()/TaskRunnerHandle::get()->PostTask/

[ssid, 2015/09/03 16:30:18]

Done.

+    }
+    return true;
+  }
+
+ private:
+  int dump_call_count_;
+  int dump_calls_to_quit_;
+  base::Closure quit_closure_;
+};
+
 TEST_F(MemoryDumpManagerTest, SingleDumper) {
   MockDumpProvider mdp;
   mdm_->RegisterDumpProvider(&mdp);
@@ -170,7 +229,7 @@ TEST_F(MemoryDumpManagerTest, SingleDumper) {
   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
@@ -179,7 +238,7 @@ TEST_F(MemoryDumpManagerTest, SingleDumper) {
   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);
@@ -188,7 +247,7 @@ TEST_F(MemoryDumpManagerTest, SingleDumper) {
   EnableTracing(MemoryDumpManager::kTraceCategory);
   EXPECT_CALL(mdp, OnMemoryDump(_, _)).Times(0);
   mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
-                          high_detail_args);
+                          g_high_detail_args);
   TraceLog::GetInstance()->SetDisabled();
 }
 
@@ -205,7 +264,7 @@ TEST_F(MemoryDumpManagerTest, CheckMemoryDumpArgs) {
           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);
 
@@ -221,7 +280,7 @@ TEST_F(MemoryDumpManagerTest, CheckMemoryDumpArgs) {
           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);
 }
@@ -244,7 +303,7 @@ TEST_F(MemoryDumpManagerTest, SharedSessionState) {
 
   for (int i = 0; i < 2; ++i)
     mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
-                            high_detail_args);
+                            g_high_detail_args);
 
   DisableTracing();
 }
@@ -259,7 +318,7 @@ TEST_F(MemoryDumpManagerTest, MultipleDumpers) {
   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.
@@ -269,7 +328,7 @@ TEST_F(MemoryDumpManagerTest, MultipleDumpers) {
   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.
@@ -278,7 +337,7 @@ TEST_F(MemoryDumpManagerTest, MultipleDumpers) {
   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();
 }
 
@@ -293,7 +352,7 @@ TEST_F(MemoryDumpManagerTest, RegistrationConsistency) {
     EXPECT_CALL(mdp, OnMemoryDump(_, _)).Times(1);
     EnableTracing(MemoryDumpManager::kTraceCategory);
     mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
-                            high_detail_args);
+                            g_high_detail_args);
     DisableTracing();
   }
 
@@ -303,7 +362,7 @@ TEST_F(MemoryDumpManagerTest, RegistrationConsistency) {
     EXPECT_CALL(mdp, OnMemoryDump(_, _)).Times(0);
     EnableTracing(MemoryDumpManager::kTraceCategory);
     mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
-                            high_detail_args);
+                            g_high_detail_args);
     DisableTracing();
   }
 
@@ -314,7 +373,7 @@ TEST_F(MemoryDumpManagerTest, RegistrationConsistency) {
     EXPECT_CALL(mdp, OnMemoryDump(_, _)).Times(0);
     EnableTracing(MemoryDumpManager::kTraceCategory);
     mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
-                            high_detail_args);
+                            g_high_detail_args);
     DisableTracing();
   }
 
@@ -326,7 +385,7 @@ TEST_F(MemoryDumpManagerTest, RegistrationConsistency) {
     EXPECT_CALL(mdp, OnMemoryDump(_, _)).Times(1);
     EnableTracing(MemoryDumpManager::kTraceCategory);
     mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
-                            high_detail_args);
+                            g_high_detail_args);
     DisableTracing();
   }
 }
@@ -366,7 +425,7 @@ TEST_F(MemoryDumpManagerTest, RespectTaskRunnerAffinity) {
           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();
@@ -414,7 +473,7 @@ TEST_F(MemoryDumpManagerTest, DisableFailingDumpers) {
   for (int i = 0; i < 1 + MemoryDumpManager::kMaxConsecutiveFailuresCount;
        i++) {
     mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
-                            high_detail_args);
+                            g_high_detail_args);
   }
 
   DisableTracing();
@@ -445,7 +504,7 @@ TEST_F(MemoryDumpManagerTest, RegisterDumperWhileDumping) {
 
   for (int i = 0; i < 4; i++) {
     mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
-                            high_detail_args);
+                            g_high_detail_args);
   }
 
   DisableTracing();
@@ -476,7 +535,7 @@ TEST_F(MemoryDumpManagerTest, UnregisterDumperWhileDumping) {
 
   for (int i = 0; i < 4; i++) {
     mdm_->RequestGlobalDump(MemoryDumpType::EXPLICITLY_TRIGGERED,
-                            high_detail_args);
+                            g_high_detail_args);
   }
 
   DisableTracing();
@@ -526,7 +585,7 @@ TEST_F(MemoryDumpManagerTest, UnregisterDumperFromThreadWhileDumping) {
 
   EnableTracing(MemoryDumpManager::kTraceCategory);
   MemoryDumpRequestArgs request_args = {0, MemoryDumpType::EXPLICITLY_TRIGGERED,
-                                        high_detail_args};
+                                        g_high_detail_args};
   mdm_->CreateProcessDump(request_args, callback);
 
   run_loop.Run();
@@ -552,11 +611,33 @@ TEST_F(MemoryDumpManagerTest, CallbackCalledOnFailure) {
         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);

[Primiano Tucci (use gerrit), 2015/09/03 08:39:51]

Thanks for fixing these :)

     run_loop.Run();
   }
   EXPECT_FALSE(last_callback_success_);
 }
 
+TEST_F(MemoryDumpManagerTest, SchedulePeriodicDumpsFromTraceConfig) {
+  RunLoop run_loop;
+  MockDumpProviderForPeriodicDump mdp(10, run_loop.QuitClosure());

[Primiano Tucci (use gerrit), 2015/09/03 08:39:51]

either make this 10 a constant (i.e. const int kDumpCallsToQuit = 10)
or just add aninline comment (i.e. 10 /* dump_calls_to_quit */)

[ssid, 2015/09/03 16:30:18]

Done.

+  mdm_->RegisterDumpProvider(&mdp);
+  EnableTracingWithTraceConfig();
+
+  DCHECK(g_high_detail_args == g_high_detail_args);

[Primiano Tucci (use gerrit), 2015/09/03 08:39:51]

uh?

[ssid, 2015/09/03 16:30:18]

sorry, removed!

+  EXPECT_CALL(mdp, OnMemoryDump(g_high_detail_args, _))
+      .Times(4)

[Primiano Tucci (use gerrit), 2015/09/03 08:39:51]

just make these 2+3 = 5 (instead of 6+4 = 10) so the test will be faster and
have the same coverage.

[ssid, 2015/09/03 16:30:18]

Done.

+      .WillRepeatedly(
+          Invoke(&mdp, &MockDumpProviderForPeriodicDump::
+                           OnMemoryDump_SchedulePeriodicDumpsFromTraceConfig));
+
+  EXPECT_CALL(mdp, OnMemoryDump(g_low_detail_args, _))
+      .Times(6)
+      .WillRepeatedly(
+          Invoke(&mdp, &MockDumpProviderForPeriodicDump::
+                           OnMemoryDump_SchedulePeriodicDumpsFromTraceConfig));
+
+  run_loop.Run();
+}
+
 }  // namespace trace_event
 }  // namespace base