arc: Log lmkd and OOM kills from ARCVM

This change implements ArcMetricsService::ReportAppKill, which is
called by the guest to report lmkd and OOM kills. These are passed
on to MemoryKillsMonitor, which manages the corresponding UMA metrics.

BUG=b:165194376
TEST=atest lmkd_unit_test; monitor Chrome log file to verify that
 MemoryKillsMonitor has received kill events.

Change-Id: I55f1800856388e248a29de0478bb983d3f87a4ae
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2463770
Commit-Queue: Willie Koomson <wvk@google.com>
Reviewed-by: Josh Horwich <jhorwich@chromium.org>
Reviewed-by: Cheng-Yu Lee <cylee@chromium.org>
Reviewed-by: Dominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#819119}

chrome/browser/chromeos/arc/metrics/arc_metrics_service_proxy.cc

@@ -8,10 +8,10 @@
 #include "base/metrics/histogram_functions.h"
 #include "chrome/browser/chromeos/arc/arc_util.h"
 #include "chrome/browser/chromeos/arc/session/arc_session_manager.h"
+#include "chrome/browser/memory/memory_kills_monitor.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/ui/app_list/arc/arc_app_list_prefs_factory.h"
 #include "components/arc/arc_browser_context_keyed_service_factory_base.h"
-#include "components/arc/metrics/arc_metrics_service.h"
 
 namespace arc {
 namespace {
@@ -54,11 +54,13 @@ ArcMetricsServiceProxy::ArcMetricsServiceProxy(
       arc_metrics_service_(ArcMetricsService::GetForBrowserContext(context)) {
   arc_app_list_prefs_->AddObserver(this);
   arc::ArcSessionManager::Get()->AddObserver(this);
+  arc_metrics_service_->AddAppKillObserver(this);
 }
 
 void ArcMetricsServiceProxy::Shutdown() {
   arc::ArcSessionManager::Get()->RemoveObserver(this);
   arc_app_list_prefs_->RemoveObserver(this);
+  arc_metrics_service_->RemoveAppKillObserver(this);
 }
 
 void ArcMetricsServiceProxy::OnTaskCreated(int32_t task_id,
@@ -81,4 +83,13 @@ void ArcMetricsServiceProxy::OnArcSessionStopped(ArcStopReason stop_reason) {
   }
 }
 
+void ArcMetricsServiceProxy::OnArcLowMemoryKill() {
+  memory::MemoryKillsMonitor::LogLowMemoryKill("APP", 0);
+}
+
+void ArcMetricsServiceProxy::OnArcOOMKillCount(
+    unsigned long current_oom_kills) {
+  memory::MemoryKillsMonitor::LogArcOOMKill(current_oom_kills);
+}
+
 }  // namespace arc

chrome/browser/chromeos/arc/metrics/arc_metrics_service_proxy.h

@@ -8,6 +8,7 @@
 #include "base/macros.h"
 #include "chrome/browser/chromeos/arc/session/arc_session_manager_observer.h"
 #include "chrome/browser/ui/app_list/arc/arc_app_list_prefs.h"
+#include "components/arc/metrics/arc_metrics_service.h"
 #include "components/keyed_service/core/keyed_service.h"
 
 namespace content {
@@ -17,14 +18,14 @@ class BrowserContext;
 namespace arc {
 
 class ArcBridgeService;
-class ArcMetricsService;
 
 // Proxy to ArcMetricsService for functionalities that depend on code under
 // chrome/browser. Should be merged into ArcMetricsService once dependency
 // issues are cleared. TODO(crbug.com/903048): Remove the proxy.
 class ArcMetricsServiceProxy : public KeyedService,
                                public ArcAppListPrefs::Observer,
-                               public ArcSessionManagerObserver {
+                               public ArcSessionManagerObserver,
+                               public ArcMetricsService::AppKillObserver {
  public:
   static ArcMetricsServiceProxy* GetForBrowserContext(
       content::BrowserContext* context);
@@ -46,6 +47,10 @@ class ArcMetricsServiceProxy : public KeyedService,
   // ArcSessionManagerObserver overrides.
   void OnArcSessionStopped(ArcStopReason stop_reason) override;
 
+  // ArcMetricsService::AppKillObserver overrides.
+  void OnArcLowMemoryKill() override;
+  void OnArcOOMKillCount(unsigned long current_oom_kills) override;
+
  private:
   ArcAppListPrefs* const arc_app_list_prefs_;
   ArcMetricsService* const arc_metrics_service_;

chrome/browser/memory/memory_kills_monitor.cc

@@ -15,6 +15,7 @@
 #include "base/strings/string_split.h"
 #include "base/synchronization/atomic_flag.h"
 #include "chrome/browser/memory/memory_kills_histogram.h"
+#include "chrome/browser/profiles/profile_manager.h"
 #include "content/public/browser/browser_thread.h"
 
 namespace memory {
@@ -47,14 +48,20 @@ void MemoryKillsMonitor::Initialize() {
 }
 
 // static
-void MemoryKillsMonitor::LogLowMemoryKill(
-    const std::string& type, int estimated_freed_kb) {
+void MemoryKillsMonitor::LogLowMemoryKill(const std::string& type,
+                                          int estimated_freed_kb) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   g_memory_kills_monitor_instance.Get().LogLowMemoryKillImpl(
       type, estimated_freed_kb);
 }
 
+// static
+void MemoryKillsMonitor::LogArcOOMKill(unsigned long current_oom_kills) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  g_memory_kills_monitor_instance.Get().LogArcOOMKillImpl(current_oom_kills);
+}
+
 void MemoryKillsMonitor::LoggedInStateChanged() {
   VLOG(2) << "LoggedInStateChanged";
   auto* login_state = chromeos::LoginState::Get();
@@ -107,13 +114,13 @@ void MemoryKillsMonitor::CheckOOMKill() {
 void MemoryKillsMonitor::CheckOOMKillImpl(unsigned long current_oom_kills) {
   DCHECK(monitoring_started_.IsSet());
 
-  unsigned long oom_kills_increased = current_oom_kills - last_oom_kills_count_;
-  if (oom_kills_increased == 0)
+  unsigned long oom_kills_delta = current_oom_kills - last_oom_kills_count_;
+  if (oom_kills_delta == 0)
     return;
 
-  VLOG(1) << "OOM_KILLS " << oom_kills_increased << " times";
+  VLOG(1) << "OOM_KILLS " << oom_kills_delta << " times";
 
-  for (int i = 0; i < oom_kills_increased; ++i) {
+  for (int i = 0; i < oom_kills_delta; ++i) {
     ++oom_kills_count_;
 
     // Report the cumulative count of killed process in one login session. For
@@ -152,6 +159,22 @@ void MemoryKillsMonitor::LogLowMemoryKillImpl(const std::string& type,
   UMA_HISTOGRAM_MEMORY_KB("Arc.LowMemoryKiller.FreedSize", estimated_freed_kb);
 }
 
+void MemoryKillsMonitor::LogArcOOMKillImpl(unsigned long current_oom_kills) {
+  DCHECK(monitoring_started_.IsSet());
+  unsigned long oom_kills_delta = current_oom_kills - last_arc_oom_kills_count_;
+  if (oom_kills_delta == 0)
+    return;
+
+  VLOG(1) << "ARC_OOM_KILLS " << oom_kills_delta << " times";
+
+  for (int i = 0; i < oom_kills_delta; ++i) {
+    ++oom_kills_count_;
+    // Report cumulative count of killed processes in one login session.
+    UMA_HISTOGRAM_CUSTOM_COUNTS("Arc.OOMKills.Count", oom_kills_count_, 1, 1000,
+                                1001);
+  }
+  last_arc_oom_kills_count_ = current_oom_kills;
+}
 MemoryKillsMonitor* MemoryKillsMonitor::GetForTesting() {
   return g_memory_kills_monitor_instance.Pointer();
 }

chrome/browser/memory/memory_kills_monitor.h

@@ -39,6 +39,9 @@ class MemoryKillsMonitor : public chromeos::LoginState::Observer {
   // after StartMonitoring() has been called.
   static void LogLowMemoryKill(const std::string& type, int estimated_freed_kb);
 
+  // A convenient function to log ARCVM OOM kills.
+  static void LogArcOOMKill(unsigned long current_oom_kills);
+
  private:
   FRIEND_TEST_ALL_PREFIXES(MemoryKillsMonitorTest, TestHistograms);
 
@@ -60,6 +63,9 @@ class MemoryKillsMonitor : public chromeos::LoginState::Observer {
   // Split CheckOOMKill and CheckOOMKillImpl for testing.
   void CheckOOMKillImpl(unsigned long current_oom_kills);
 
+  // Logs ARCVM OOM kill.
+  void LogArcOOMKillImpl(unsigned long current_oom_kills);
+
   // A flag set when StartMonitoring() is called to indicate that monitoring has
   // been started.
   base::AtomicFlag monitoring_started_;
@@ -76,6 +82,9 @@ class MemoryKillsMonitor : public chromeos::LoginState::Observer {
   // The last oom kills count from |GetCurrentOOMKills|.
   unsigned long last_oom_kills_count_ = 0;
 
+  // The last ARCVM OOM kills count.
+  unsigned long last_arc_oom_kills_count_ = 0;
+
   base::RepeatingTimer checking_timer_;
 
   DISALLOW_COPY_AND_ASSIGN(MemoryKillsMonitor);

components/arc/metrics/arc_metrics_service.cc

@@ -136,6 +136,12 @@ ArcMetricsService::~ArcMetricsService() {
   arc_bridge_service_->RemoveObserver(&arc_bridge_service_observer_);
 }
 
+void ArcMetricsService::Shutdown() {
+  for (auto& obs : app_kill_observers_)
+    obs.OnArcMetricsServiceDestroyed();
+  app_kill_observers_.Clear();
+}
+
 void ArcMetricsService::SetHistogramNamer(HistogramNamer histogram_namer) {
   histogram_namer_ = histogram_namer;
 }
@@ -273,6 +279,28 @@ void ArcMetricsService::ReportCompanionLibApiUsage(
   UMA_HISTOGRAM_ENUMERATION("Arc.CompanionLibraryApisCounter", api_id);
 }
 
+void ArcMetricsService::ReportAppKill(mojom::AppKillPtr app_kill) {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  switch (app_kill->type) {
+    case mojom::AppKillType::LMKD_KILL:
+      NotifyLowMemoryKill();
+      break;
+    case mojom::AppKillType::OOM_KILL:
+      NotifyOOMKillCount(app_kill->count);
+      break;
+  }
+}
+
+void ArcMetricsService::NotifyLowMemoryKill() {
+  for (auto& obs : app_kill_observers_)
+    obs.OnArcLowMemoryKill();
+}
+
+void ArcMetricsService::NotifyOOMKillCount(unsigned long count) {
+  for (auto& obs : app_kill_observers_)
+    obs.OnArcOOMKillCount(count);
+}
+
 void ArcMetricsService::OnWindowActivated(
     wm::ActivationChangeObserver::ActivationReason reason,
     aura::Window* gained_active,
@@ -315,6 +343,15 @@ void ArcMetricsService::OnTaskDestroyed(int32_t task_id) {
   guest_os_engagement_metrics_.SetBackgroundActive(!task_ids_.empty());
 }
 
+void ArcMetricsService::AddAppKillObserver(AppKillObserver* obs) {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  app_kill_observers_.AddObserver(obs);
+}
+
+void ArcMetricsService::RemoveAppKillObserver(AppKillObserver* obs) {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  app_kill_observers_.RemoveObserver(obs);
+}
 ArcMetricsService::ProcessObserver::ProcessObserver(
     ArcMetricsService* arc_metrics_service)
     : arc_metrics_service_(arc_metrics_service) {}

components/arc/metrics/arc_metrics_service.h

@@ -11,6 +11,8 @@
 
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "base/observer_list.h"
+#include "base/observer_list_types.h"
 #include "base/optional.h"
 #include "base/threading/thread_checker.h"
 #include "base/time/time.h"
@@ -50,6 +52,13 @@ class ArcMetricsService : public KeyedService,
   using HistogramNamer =
       base::RepeatingCallback<std::string(const std::string& base_name)>;
 
+  class AppKillObserver : public base::CheckedObserver {
+   public:
+    virtual void OnArcLowMemoryKill() = 0;
+    virtual void OnArcOOMKillCount(unsigned long count) = 0;
+    virtual void OnArcMetricsServiceDestroyed() {}
+  };
+
   // Returns singleton instance for the given BrowserContext,
   // or nullptr if the browser |context| is not allowed to use ARC.
   static ArcMetricsService* GetForBrowserContext(
@@ -64,6 +73,9 @@ class ArcMetricsService : public KeyedService,
                     ArcBridgeService* bridge_service);
   ~ArcMetricsService() override;
 
+  // KeyedService overrides.
+  void Shutdown() override;
+
   // Sets the histogram namer. Required to not have a dependency on browser
   // codebase.
   void SetHistogramNamer(HistogramNamer histogram_namer);
@@ -77,6 +89,7 @@ class ArcMetricsService : public KeyedService,
                           mojom::BootType boot_type) override;
   void ReportNativeBridge(mojom::NativeBridgeType native_bridge_type) override;
   void ReportCompanionLibApiUsage(mojom::CompanionLibApiId api_id) override;
+  void ReportAppKill(mojom::AppKillPtr app_kill) override;
 
   // wm::ActivationChangeObserver overrides.
   // Records to UMA when a user has interacted with an ARC app window.
@@ -95,6 +108,9 @@ class ArcMetricsService : public KeyedService,
                      const std::string& intent);
   void OnTaskDestroyed(int32_t task_id);
 
+  void AddAppKillObserver(AppKillObserver* obs);
+  void RemoveAppKillObserver(AppKillObserver* obs);
+
  private:
   // Adapter to be able to also observe ProcessInstance events.
   class ProcessObserver : public ConnectionObserver<mojom::ProcessInstance> {
@@ -170,6 +186,10 @@ class ArcMetricsService : public KeyedService,
                                mojom::BootType boot_type,
                                base::Optional<base::TimeTicks> arc_start_time);
 
+  // Notify AppKillObservers.
+  void NotifyLowMemoryKill();
+  void NotifyOOMKillCount(unsigned long count);
+
   THREAD_CHECKER(thread_checker_);
 
   ArcBridgeService* const arc_bridge_service_;  // Owned by ArcServiceManager.
@@ -193,6 +213,8 @@ class ArcMetricsService : public KeyedService,
 
   bool gamepad_interaction_recorded_ = false;
 
+  base::ObserverList<AppKillObserver> app_kill_observers_;
+
   // Always keep this the last member of this class to make sure it's the
   // first thing to be destructed.
   base::WeakPtrFactory<ArcMetricsService> weak_ptr_factory_{this};

components/arc/mojom/metrics.mojom

 // Copyright 2016 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.
-// Next MinVersion: 6
+// Next MinVersion: 7
 
 module arc.mojom;
 
@@ -103,7 +103,23 @@ enum CompanionLibApiId {
   IS_CLIPPING_TO_TASK_DISABLED = 23,
 };
 
-// Next method ID: 3
+// Describes the type of app kill being reported.
+[Extensible]
+enum AppKillType {
+  LMKD_KILL = 0,
+  OOM_KILL = 1,
+};
+
+// Describes an app kill from ARC instance.
+struct AppKill {
+  // Type of kill being reported.
+  AppKillType type;
+  // Number of kills. For LMKD kills, this is always 1. For OOM kills, this is
+  // the total oom_kill count from /proc/vm_stat.
+  uint32 count;
+};
+
+// Next method ID: 4
 interface MetricsHost {
   // Reports boot progress events from ARC instance.
   ReportBootProgress@0(array<BootProgressEvent> events,
@@ -114,6 +130,9 @@ interface MetricsHost {
 
   // Reports api usage by ChromeOS Companion Library.
   [MinVersion=4] ReportCompanionLibApiUsage@2(CompanionLibApiId api_id);
+
+  // Reports LMKD and OOM kills from ARC.
+  [MinVersion=6] ReportAppKill@3(AppKill app_kill);
 };
 
 // Next method ID: 3