[memory-infra] MemoryDumpScheduler follows config for time between dumps

base/trace_event/memory_dump_scheduler.cc

Index: base/trace_event/memory_dump_scheduler.cc
diff --git a/base/trace_event/memory_dump_scheduler.cc b/base/trace_event/memory_dump_scheduler.cc
index 14c0f7e188f45b3178c185741b8788c088ee1cf1..56cab18afd1cadbfd05a942f031b85db227aaf09 100644
--- a/base/trace_event/memory_dump_scheduler.cc
+++ b/base/trace_event/memory_dump_scheduler.cc
@@ -42,16 +42,16 @@ void MemoryDumpScheduler::AddTrigger(MemoryDumpType trigger_type,
                                      uint32_t min_time_between_dumps_ms) {
   if (trigger_type == MemoryDumpType::PEAK_MEMORY_USAGE) {
     DCHECK(!periodic_state_.is_configured);
-    DCHECK(!polling_state_.is_configured);
+    DCHECK_EQ(PollingTriggerState::DISABLED, polling_state_.current_state);
     DCHECK_NE(0u, min_time_between_dumps_ms);
 
     polling_state_.level_of_detail = level_of_detail;
     polling_state_.min_polls_between_dumps =
         (min_time_between_dumps_ms + polling_state_.polling_interval_ms - 1) /
         polling_state_.polling_interval_ms;
-    polling_state_.is_configured = true;
+    polling_state_.current_state = PollingTriggerState::CONFIGURED;
   } else if (trigger_type == MemoryDumpType::PERIODIC_INTERVAL) {
-    DCHECK(!polling_state_.is_configured);
+    DCHECK_EQ(PollingTriggerState::DISABLED, polling_state_.current_state);
     periodic_state_.is_configured = true;
     DCHECK_NE(0u, min_time_between_dumps_ms);
     switch (level_of_detail) {
@@ -92,21 +92,30 @@ void MemoryDumpScheduler::NotifyPeriodicTriggerSupported() {
 }
 
 void MemoryDumpScheduler::NotifyPollingSupported() {
-  if (!polling_state_.is_configured || polling_state_.is_polling_enabled)
+  if (polling_state_.current_state != PollingTriggerState::CONFIGURED)
     return;
-  polling_state_.is_polling_enabled = true;
-  for (uint32_t i = 0;
-       i < PollingTriggerState::kNumTotalsTrackedForPeakDetection; ++i) {
-    polling_state_.last_memory_totals_kb[i] = 0;
-  }
-  polling_state_.last_memory_totals_kb_index = 0;
-  polling_state_.num_polls_from_last_dump = 0;
-  polling_state_.last_dump_memory_total = 0;
+  polling_state_.current_state = PollingTriggerState::ENABLED;
+
+  polling_state_.ResetTotals();
   polling_state_.polling_task_runner->PostTask(
       FROM_HERE,
       Bind(&MemoryDumpScheduler::PollMemoryOnPollingThread, Unretained(this)));
 }
 
+void MemoryDumpScheduler::NotifyDumpTriggered() {
+  if (polling_state_.polling_task_runner &&
+      ThreadTaskRunnerHandle::Get() != polling_state_.polling_task_runner) {

[Primiano Tucci (use gerrit), 2017/03/10 10:35:39]

do you really want to compare the TaskRunnerHandles? That should be a pretty
rare need.
by looking at the code it seems to be you just want to use:

polling_state_.polling_task_runner.RunsTasksOnCurrentThread()

In general you should not rely on the fact that two calls to
ThreadTaskRunnerHandle::Get() o the same thread returns the same handle. task
runner handles are kind of proxy objects. 

Sorry I just realized that there is a similar instance below and I didn't spot
in the previous review.

[ssid, 2017/03/10 23:51:10]

Yes my bad. Fixed it.

+    polling_state_.polling_task_runner->PostTask(
+        FROM_HERE,
+        Bind(&MemoryDumpScheduler::NotifyDumpTriggered, Unretained(this)));

[Primiano Tucci (use gerrit), 2017/03/10 10:35:39]

Also I just realized that these Unretained aren't really safe. The MDScheduler
is reset OnTraceLogDisabled, so these post task might end up causing
Use-After-Free. I think you might need a weakptr here, or figure out some better
ownership semantic.

[ssid, 2017/03/10 14:57:37]

The reason why the unretained should be safe here is because of the thread.Stop
called by MDM before destroying the scheduler. There could an issue with the
task runner itself being null while posting, but that will again be handled by
MDM which takes lock while accessing scheduler, and does not call it after trace
log disabled takes away the scheduler. Only after the |dump_schefuler| is null
we call DisableTriggers which then makes the task runner null.

+    return;
+  }
+  if (polling_state_.current_state != PollingTriggerState::ENABLED)
+    return;
+
+  polling_state_.ResetTotals();
+}
+
 void MemoryDumpScheduler::DisableAllTriggers() {
   if (periodic_state_.timer.IsRunning())
     periodic_state_.timer.Stop();
@@ -120,8 +129,7 @@ void MemoryDumpScheduler::DisablePolling() {
             Bind(&MemoryDumpScheduler::DisablePolling, Unretained(this))))
       return;
   }
-  polling_state_.is_polling_enabled = false;
-  polling_state_.is_configured = false;
+  polling_state_.current_state = PollingTriggerState::DISABLED;
   polling_state_.polling_task_runner = nullptr;
 }
 
@@ -148,7 +156,7 @@ void MemoryDumpScheduler::RequestPeriodicGlobalDump() {
 }
 
 void MemoryDumpScheduler::PollMemoryOnPollingThread() {
-  if (!polling_state_.is_configured)
+  if (polling_state_.current_state != PollingTriggerState::ENABLED)
     return;
 
   uint64_t polled_memory = 0;
@@ -198,15 +206,8 @@ bool MemoryDumpScheduler::ShouldTriggerDump(uint64_t current_memory_total) {
   should_dump |=
       increase_from_last_dump > polling_state_.memory_increase_threshold;
   should_dump |= IsCurrentSamplePeak(current_memory_total);
-  if (should_dump) {
-    polling_state_.last_dump_memory_total = current_memory_total;
-    polling_state_.num_polls_from_last_dump = 0;
-    for (uint32_t i = 0;
-         i < PollingTriggerState::kNumTotalsTrackedForPeakDetection; ++i) {
-      polling_state_.last_memory_totals_kb[i] = 0;
-    }
-    polling_state_.last_memory_totals_kb_index = 0;
-  }
+  if (should_dump)
+    polling_state_.ResetTotals();
   return should_dump;
 }
 
@@ -267,8 +268,7 @@ MemoryDumpScheduler::PeriodicTriggerState::~PeriodicTriggerState() {
 
 MemoryDumpScheduler::PollingTriggerState::PollingTriggerState(
     scoped_refptr<SingleThreadTaskRunner> polling_task_runner)
-    : is_configured(false),
-      is_polling_enabled(false),
+    : current_state(DISABLED),
       level_of_detail(MemoryDumpLevelOfDetail::FIRST),
       polling_task_runner(polling_task_runner),
       polling_interval_ms(g_polling_interval_ms_for_testing
@@ -284,5 +284,16 @@ MemoryDumpScheduler::PollingTriggerState::~PollingTriggerState() {
   DCHECK(!polling_task_runner);
 }
 
+void MemoryDumpScheduler::PollingTriggerState::ResetTotals() {
+  if (last_memory_totals_kb_index) {
+    last_dump_memory_total =
+        last_memory_totals_kb[last_memory_totals_kb_index] * 1024;
+  }
+  num_polls_from_last_dump = 0;
+  for (uint32_t i = 0; i < kNumTotalsTrackedForPeakDetection; ++i)
+    last_memory_totals_kb[i] = 0;
+  last_memory_totals_kb_index = 0;
+}
+
 }  // namespace trace_event
 }  // namespace base