memlog: Simplify threading in background upload

Previously, the background trigger logic attempted to hop between the
IO and UI threads. This caused problems with task invalidation. In the
new logic, the background trigger logic only runs on the UI thread
which allows cancellation using WeakPtrFactory.

Bug: 751321
Change-Id: Ibccf6f5f898ce53ffa2d52f11ebba00118d025f2
Reviewed-on: https://chromium-review.googlesource.com/773113
Commit-Queue: Albert J. Wong <ajwong@chromium.org>
Reviewed-by: Albert J. Wong <ajwong@chromium.org>
Reviewed-by: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517205}

chrome/browser/profiling_host/background_profiling_triggers.cc

@@ -16,7 +16,7 @@ namespace profiling {
 namespace {
 // Check memory usage every hour. Trigger slow report if needed.
 const int kRepeatingCheckMemoryDelayInHours = 1;
-const int kSecondReportRepeatingCheckMemoryDelayInHours = 12;
+const int kThrottledReportRepeatingCheckMemoryDelayInHours = 12;
 
 #if defined(OS_ANDROID)
 const size_t kBrowserProcessMallocTriggerKb = 100 * 1024;    // 100 MB
@@ -67,6 +67,8 @@ BackgroundProfilingTriggers::BackgroundProfilingTriggers(
 BackgroundProfilingTriggers::~BackgroundProfilingTriggers() {}
 
 void BackgroundProfilingTriggers::StartTimer() {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
   // Register a repeating timer to check memory usage periodically.
   timer_.Start(
       FROM_HERE, base::TimeDelta::FromHours(kRepeatingCheckMemoryDelayInHours),
@@ -97,14 +99,8 @@ bool BackgroundProfilingTriggers::IsOverTriggerThreshold(
 }
 
 void BackgroundProfilingTriggers::PerformMemoryUsageChecks() {
-  content::BrowserThread::PostTask(
-      content::BrowserThread::IO, FROM_HERE,
-      base::BindOnce(
-          &BackgroundProfilingTriggers::PerformMemoryUsageChecksOnIOThread,
-          weak_ptr_factory_.GetWeakPtr()));
-}
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
 
-void BackgroundProfilingTriggers::PerformMemoryUsageChecksOnIOThread() {
   memory_instrumentation::MemoryInstrumentation::GetInstance()
       ->RequestGlobalDump(
           base::Bind(&BackgroundProfilingTriggers::OnReceivedMemoryDump,
@@ -114,37 +110,38 @@ void BackgroundProfilingTriggers::PerformMemoryUsageChecksOnIOThread() {
 void BackgroundProfilingTriggers::OnReceivedMemoryDump(
     bool success,
     memory_instrumentation::mojom::GlobalMemoryDumpPtr dump) {
-  if (!success)
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  if (!success) {
     return;
+  }
 
+  bool report_was_sent = false;
   for (const auto& proc : dump->process_dumps) {
     if (IsOverTriggerThreshold(GetContentProcessType(proc->process_type),
                                proc->os_dump->private_footprint_kb)) {
       TriggerMemoryReportForProcess(proc->pid);
+      report_was_sent = true;
     }
   }
-}
 
-void BackgroundProfilingTriggers::TriggerMemoryReportForProcess(
-    base::ProcessId pid) {
-  content::BrowserThread::GetTaskRunnerForThread(content::BrowserThread::UI)
-      ->PostTask(FROM_HERE,
-                 base::BindOnce(&BackgroundProfilingTriggers::
-                                    TriggerMemoryReportForProcessOnUIThread,
-                                weak_ptr_factory_.GetWeakPtr(), pid));
+  if (report_was_sent) {
+    // If a report was sent, throttle the memory data collection rate to
+    // kThrottledReportRepeatingCheckMemoryDelayInHours to avoid sending too
+    // many reports from a known problematic client.
+    timer_.Start(
+        FROM_HERE,
+        base::TimeDelta::FromHours(
+            kThrottledReportRepeatingCheckMemoryDelayInHours),
+        base::Bind(&BackgroundProfilingTriggers::PerformMemoryUsageChecks,
+                   weak_ptr_factory_.GetWeakPtr()));
+  }
 }
 
-void BackgroundProfilingTriggers::TriggerMemoryReportForProcessOnUIThread(
+void BackgroundProfilingTriggers::TriggerMemoryReportForProcess(
     base::ProcessId pid) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   host_->RequestProcessReport(pid, "MEMLOG_BACKGROUND_TRIGGER");
-
-  // Reset the timer to avoid uploading too many reports.
-  timer_.Start(
-      FROM_HERE,
-      base::TimeDelta::FromHours(kSecondReportRepeatingCheckMemoryDelayInHours),
-      base::Bind(&BackgroundProfilingTriggers::PerformMemoryUsageChecks,
-                 weak_ptr_factory_.GetWeakPtr()));
 }
 
 }  // namespace profiling

chrome/browser/profiling_host/background_profiling_triggers.h

@@ -37,7 +37,6 @@ class BackgroundProfilingTriggers {
 
   // Check the current memory usage and send a slow-report if needed.
   void PerformMemoryUsageChecks();
-  void PerformMemoryUsageChecksOnIOThread();
 
   // Called when the memory dump is received. Performs
   // checks on memory usage and trigger a memory report with
@@ -48,11 +47,11 @@ class BackgroundProfilingTriggers {
 
   // Virtual for testing. Called when a memory report needs to be send.
   virtual void TriggerMemoryReportForProcess(base::ProcessId pid);
-  void TriggerMemoryReportForProcessOnUIThread(base::ProcessId pid);
+
+  ProfilingProcessHost* host_;
 
   // Timer to periodically check memory consumption and upload a slow-report.
   base::RepeatingTimer timer_;
-  ProfilingProcessHost* host_;
 
   base::WeakPtrFactory<BackgroundProfilingTriggers> weak_ptr_factory_;
 

chrome/browser/profiling_host/background_profiling_triggers_unittest.cc

@@ -8,8 +8,10 @@
 #include <utility>
 #include <vector>
 
+#include "base/test/scoped_task_environment.h"
 #include "base/time/time.h"
 #include "chrome/browser/profiling_host/profiling_process_host.h"
+#include "content/public/test/test_browser_thread_bundle.h"
 #include "services/resource_coordinator/public/cpp/memory_instrumentation/coordinator.h"
 #include "services/resource_coordinator/public/interfaces/memory_instrumentation/memory_instrumentation.mojom.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -91,11 +93,17 @@ class FakeBackgroundProfilingTriggers : public BackgroundProfilingTriggers {
 
 class BackgroundProfilingTriggersTest : public testing::Test {
  public:
-  BackgroundProfilingTriggersTest() : triggers_(&host_) {}
+  BackgroundProfilingTriggersTest()
+      : scoped_task_environment_(
+            base::test::ScopedTaskEnvironment::MainThreadType::UI),
+        triggers_(&host_) {}
 
   void SetMode(ProfilingProcessHost::Mode mode) { host_.SetMode(mode); }
 
  protected:
+  base::test::ScopedTaskEnvironment scoped_task_environment_;
+  content::TestBrowserThreadBundle thread_bundle;
+
   ProfilingProcessHost host_;
   FakeBackgroundProfilingTriggers triggers_;
 };