Don't leak ProcessMonitor

The single instance is now owned by
ChromeBrowserMainExtraPartsPerformanceMonitor but it can still be
retrieved by the Get() method.

Bug: 1166820
Change-Id: I27300c2fafb9d5ce04d73397dd791c85efd9393b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2630368
Commit-Queue: Patrick Monette <pmonette@chromium.org>
Reviewed-by: Tommy Nyquist <nyquist@chromium.org>
Reviewed-by: Sébastien Marchand <sebmarchand@chromium.org>
Cr-Commit-Position: refs/heads/master@{#844590}

chrome/browser/chrome_browser_main.cc

@@ -1727,7 +1727,11 @@ bool ChromeBrowserMainParts::MainMessageLoopRun(int* result_code) {
 
   DCHECK(base::CurrentUIThread::IsSet());
 
-  performance_monitor::ProcessMonitor::GetInstance()->StartGatherCycle();
+  // The ProcessMonitor is responsible for gathering important processes
+  // metrics.
+  // TODO(1167290): Can this be moved to PreMainMessageLoopStart() without
+  //                impacting those metrics?
+  performance_monitor::ProcessMonitor::Get()->StartGatherCycle();
 
   g_run_loop->Run();
 

chrome/browser/performance_monitor/chrome_browser_main_extra_parts_performance_monitor.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/performance_monitor/chrome_browser_main_extra_parts_performance_monitor.h"
 
+#include "chrome/browser/performance_monitor/process_monitor.h"
 #include "chrome/browser/performance_monitor/system_monitor.h"
 
 ChromeBrowserMainExtraPartsPerformanceMonitor::
@@ -12,6 +13,10 @@ ChromeBrowserMainExtraPartsPerformanceMonitor::
 ChromeBrowserMainExtraPartsPerformanceMonitor::
     ~ChromeBrowserMainExtraPartsPerformanceMonitor() = default;
 
+void ChromeBrowserMainExtraPartsPerformanceMonitor::PreMainMessageLoopStart() {
+  process_monitor_ = performance_monitor::ProcessMonitor::Create();
+}
+
 void ChromeBrowserMainExtraPartsPerformanceMonitor::PostMainMessageLoopStart() {
   system_monitor_ = performance_monitor::SystemMonitor::Create();
 }

chrome/browser/performance_monitor/chrome_browser_main_extra_parts_performance_monitor.h

@@ -10,6 +10,7 @@
 #include "chrome/browser/chrome_browser_main_extra_parts.h"
 
 namespace performance_monitor {
+class ProcessMonitor;
 class SystemMonitor;
 }  // namespace performance_monitor
 
@@ -25,9 +26,12 @@ class ChromeBrowserMainExtraPartsPerformanceMonitor
       const ChromeBrowserMainExtraPartsPerformanceMonitor&) = delete;
 
   // ChromeBrowserMainExtraParts:
+  void PreMainMessageLoopStart() override;
   void PostMainMessageLoopStart() override;
 
  private:
+  std::unique_ptr<performance_monitor::ProcessMonitor> process_monitor_;
+
   // The system monitor instance, used by some subsystems to collect the system
   // metrics they need.
   std::unique_ptr<performance_monitor::SystemMonitor> system_monitor_;

chrome/browser/performance_monitor/process_monitor.cc

@@ -37,8 +37,8 @@ namespace {
 // collections.
 constexpr base::TimeDelta kGatherInterval = base::TimeDelta::FromSeconds(120);
 
-base::LazyInstance<ProcessMonitor>::DestructorAtExit g_monitor =
-    LAZY_INSTANCE_INITIALIZER;
+// The global instance.
+ProcessMonitor* g_process_monitor = nullptr;
 
 void GatherMetricsForRenderProcess(content::RenderProcessHost* host,
                                    ProcessMetricsMetadata* data) {
@@ -74,13 +74,20 @@ void GatherMetricsForRenderProcess(content::RenderProcessHost* host,
 
 }  // namespace
 
-ProcessMonitor::ProcessMonitor() = default;
-
-ProcessMonitor::~ProcessMonitor() = default;
+// static
+std::unique_ptr<ProcessMonitor> ProcessMonitor::Create() {
+  DCHECK(!g_process_monitor);
+  return base::WrapUnique(new ProcessMonitor());
+}
 
 // static
-ProcessMonitor* ProcessMonitor::GetInstance() {
-  return g_monitor.Pointer();
+ProcessMonitor* ProcessMonitor::Get() {
+  return g_process_monitor;
+}
+
+ProcessMonitor::~ProcessMonitor() {
+  DCHECK(g_process_monitor);
+  g_process_monitor = nullptr;
 }
 
 void ProcessMonitor::StartGatherCycle() {
@@ -89,6 +96,32 @@ void ProcessMonitor::StartGatherCycle() {
                          &ProcessMonitor::GatherMetricsMapOnUIThread);
 }
 
+ProcessMonitor::ProcessMonitor() {
+  DCHECK(!g_process_monitor);
+  g_process_monitor = this;
+}
+
+void ProcessMonitor::MarkProcessAsAlive(
+    const ProcessMetricsMetadata& process_data,
+    int current_update_sequence) {
+  const base::ProcessHandle& handle = process_data.handle;
+  if (handle == base::kNullProcessHandle) {
+    // Process may not be valid yet.
+    return;
+  }
+
+  auto process_metrics_iter = metrics_map_.find(handle);
+  if (process_metrics_iter == metrics_map_.end()) {
+    // If we're not already watching the process, let's initialize it.
+    metrics_map_[handle] = std::make_unique<ProcessMetricsHistory>();
+    metrics_map_[handle]->Initialize(process_data, current_update_sequence);
+  } else {
+    // If we are watching the process, touch it to keep it alive.
+    ProcessMetricsHistory* process_metrics = process_metrics_iter->second.get();
+    process_metrics->set_last_update_sequence(current_update_sequence);
+  }
+}
+
 void ProcessMonitor::GatherMetricsMapOnUIThread() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
@@ -116,27 +149,6 @@ void ProcessMonitor::GatherMetricsMapOnUIThread() {
                      base::Unretained(this), current_update_sequence));
 }
 
-void ProcessMonitor::MarkProcessAsAlive(
-    const ProcessMetricsMetadata& process_data,
-    int current_update_sequence) {
-  const base::ProcessHandle& handle = process_data.handle;
-  if (handle == base::kNullProcessHandle) {
-    // Process may not be valid yet.
-    return;
-  }
-
-  auto process_metrics_iter = metrics_map_.find(handle);
-  if (process_metrics_iter == metrics_map_.end()) {
-    // If we're not already watching the process, let's initialize it.
-    metrics_map_[handle] = std::make_unique<ProcessMetricsHistory>();
-    metrics_map_[handle]->Initialize(process_data, current_update_sequence);
-  } else {
-    // If we are watching the process, touch it to keep it alive.
-    ProcessMetricsHistory* process_metrics = process_metrics_iter->second.get();
-    process_metrics->set_last_update_sequence(current_update_sequence);
-  }
-}
-
 void ProcessMonitor::GatherMetricsMapOnIOThread(int current_update_sequence) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 

chrome/browser/performance_monitor/process_monitor.h

@@ -21,13 +21,19 @@ class ProcessMetricsHistory;
 // ProcessMonitor is a tool which periodically monitors performance metrics
 // of all the Chrome processes for histogram logging and possibly taking action
 // upon noticing serious performance degradation.
-//
-// TODO(sebmarchand): Make this class not a singleton.
 class ProcessMonitor {
  public:
-  // Returns the current ProcessMonitor instance if one exists; otherwise
-  // constructs a new ProcessMonitor.
-  static ProcessMonitor* GetInstance();
+  // Creates and returns the application-wide ProcessMonitor. Can only be called
+  // if no ProcessMonitor instances exists in the current process. The caller
+  // owns the created instance. The current process' instance can be retrieved
+  // with Get().
+  static std::unique_ptr<ProcessMonitor> Create();
+
+  // Returns the application-wide ProcessMonitor if one exists; otherwise
+  // returns nullptr.
+  static ProcessMonitor* Get();
+
+  ~ProcessMonitor();
 
   // Start the cycle of metrics gathering.
   void StartGatherCycle();
@@ -39,7 +45,6 @@ class ProcessMonitor {
       std::map<base::ProcessHandle, std::unique_ptr<ProcessMetricsHistory>>;
 
   ProcessMonitor();
-  ~ProcessMonitor();
 
   // Mark the given process as alive in the current update iteration.
   // This means adding an entry to the map of watched processes if it's not