Kick ArcIdleManager upon VmResume, to sync idle state

The ARCVM suspend operation changes the same set of status that
are controlled by ARC Idle Manager inside android.  This means
that when CrosVM resumes ARCVM, the status will be out of sync.
This CL re-syncs status upon resume (only necessary if ARCVM
is NOT dozed at that point).

BUG=b:137893026
TEST=manually suspend/resume with and without doze enabled,
     manually suspend/resume in doze with and without arc apps on.

Change-Id: I67225a504cc9e2b1fc9927abd259f9eb9cae038d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4797393
Commit-Queue: Hidehiko Abe <hidehiko@chromium.org>
Auto-Submit: Alexandre Marciano Gimenez <raging@google.com>
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1187046}

ash/components/arc/power/arc_power_bridge.cc

@@ -7,15 +7,13 @@
 #include <algorithm>
 #include <utility>
 
-#include "ash/components/arc/arc_browser_context_keyed_service_factory_base.h"
 #include "ash/components/arc/arc_util.h"
 #include "ash/components/arc/session/arc_bridge_service.h"
 #include "ash/shell.h"
 #include "base/functional/bind.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/memory/raw_ptr.h"
-#include "base/memory/singleton.h"
 #include "base/metrics/histogram_functions.h"
 #include "chromeos/ash/components/dbus/patchpanel/patchpanel_client.h"
 #include "chromeos/dbus/power/power_policy_controller.h"
@@ -33,27 +31,13 @@ namespace {
 // order to prevent spammy brightness updates.
 constexpr base::TimeDelta kNotifyBrightnessDelay = base::Milliseconds(200);
 
-// Singleton factory for ArcPowerBridge.
-class ArcPowerBridgeFactory
-    : public internal::ArcBrowserContextKeyedServiceFactoryBase<
-          ArcPowerBridge,
-          ArcPowerBridgeFactory> {
- public:
-  // Factory name used by ArcBrowserContextKeyedServiceFactoryBase.
-  static constexpr const char* kName = "ArcPowerBridgeFactory";
-
-  static ArcPowerBridgeFactory* GetInstance() {
-    return base::Singleton<ArcPowerBridgeFactory>::get();
-  }
-
- private:
-  friend base::DefaultSingletonTraits<ArcPowerBridgeFactory>;
-  ArcPowerBridgeFactory() = default;
-  ~ArcPowerBridgeFactory() override = default;
-};
-
 }  // namespace
 
+// static
+ArcPowerBridgeFactory* ArcPowerBridgeFactory::GetInstance() {
+  return base::Singleton<ArcPowerBridgeFactory>::get();
+}
+
 // WakeLockRequestor requests a wake lock from the device service in response
 // to wake lock requests of a given type from Android. A count is kept of
 // outstanding Android requests so that only a single actual wake lock is used.
@@ -138,6 +122,9 @@ ArcPowerBridge::ArcPowerBridge(content::BrowserContext* context,
 }
 
 ArcPowerBridge::~ArcPowerBridge() {
+  for (auto& observer : observer_list_) {
+    observer.OnWillDestroyArcPowerBridge();
+  }
   arc_bridge_service_->power()->RemoveObserver(this);
   arc_bridge_service_->power()->SetHost(nullptr);
 }
@@ -261,6 +248,9 @@ void ArcPowerBridge::OnConciergeResumeVmResponse(
     LOG(ERROR) << "Failed to resume arcvm: " << reply.value().failure_reason();
     return;
   }
+  for (auto& observer : observer_list_) {
+    observer.OnVmResumed();
+  }
   DispatchAndroidResume();
 }
 

ash/components/arc/power/arc_power_bridge.h

@@ -9,11 +9,13 @@
 #include <memory>
 #include <string>
 
+#include "ash/components/arc/arc_browser_context_keyed_service_factory_base.h"
 #include "ash/components/arc/mojom/anr.mojom.h"
 #include "ash/components/arc/mojom/power.mojom.h"
 #include "ash/components/arc/session/arc_service_manager.h"
 #include "ash/components/arc/session/connection_observer.h"
 #include "base/memory/raw_ptr.h"
+#include "base/memory/singleton.h"
 #include "base/observer_list.h"
 #include "base/time/time.h"
 #include "base/timer/timer.h"
@@ -33,6 +35,24 @@ class BrowserContext;
 namespace arc {
 
 class ArcBridgeService;
+class ArcPowerBridge;  // So we can declare the factory first.
+
+// Singleton factory for ArcPowerBridge.
+class ArcPowerBridgeFactory
+    : public internal::ArcBrowserContextKeyedServiceFactoryBase<
+          ArcPowerBridge,
+          ArcPowerBridgeFactory> {
+ public:
+  // Factory name used by ArcBrowserContextKeyedServiceFactoryBase.
+  static constexpr const char* kName = "ArcPowerBridgeFactory";
+
+  static ArcPowerBridgeFactory* GetInstance();
+
+ private:
+  friend base::DefaultSingletonTraits<ArcPowerBridgeFactory>;
+  ArcPowerBridgeFactory() = default;
+  ~ArcPowerBridgeFactory() override = default;
+};
 
 // ARC Power Client sets power management policy based on requests from
 // ARC instances.
@@ -47,6 +67,13 @@ class ArcPowerBridge : public KeyedService,
     // Notifies that wakefulness mode is changed.
     virtual void OnWakefulnessChanged(mojom::WakefulnessMode mode) {}
     virtual void OnPreAnr(mojom::AnrType type) {}
+
+    // Notifies about resume state of the underlying VM (ARCVM-exclusive).
+    // ARC Container does not communicate with CrosVM, and it doesn't
+    // instantiate services that care about this event.
+    virtual void OnVmResumed() {}
+
+    virtual void OnWillDestroyArcPowerBridge() {}
   };
 
   // Returns singleton instance for the given BrowserContext,

chrome/browser/ash/arc/idle_manager/arc_idle_manager.cc

@@ -7,7 +7,6 @@
 #include "ash/components/arc/arc_browser_context_keyed_service_factory_base.h"
 #include "ash/components/arc/arc_features.h"
 #include "ash/components/arc/mojom/power.mojom.h"
-#include "ash/components/arc/power/arc_power_bridge.h"
 #include "ash/components/arc/session/arc_bridge_service.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_functions.h"
@@ -52,7 +51,7 @@ class ArcIdleManagerFactory
  private:
   friend class base::NoDestructor<ArcIdleManagerFactory>;
 
-  ArcIdleManagerFactory() = default;
+  ArcIdleManagerFactory() { DependsOn(ArcPowerBridgeFactory::GetInstance()); }
   ~ArcIdleManagerFactory() override = default;
 };
 
@@ -88,8 +87,10 @@ ArcIdleManager::ArcIdleManager(content::BrowserContext* context,
   auto* const power_bridge = ArcPowerBridge::GetForBrowserContext(context);
 
   // This maybe null in unit tests.
-  if (power_bridge)
+  if (power_bridge) {
     power_bridge->DisableAndroidIdleControl();
+    powerbridge_observation_.Observe(power_bridge);
+  }
 
   DCHECK(bridge_);
   bridge_->power()->AddObserver(this);
@@ -109,6 +110,9 @@ void ArcIdleManager::Shutdown() {
   // After this is done, we will no longer get connection notifications.
   bridge_->power()->RemoveObserver(this);
 
+  // No more notifications about VM resumed.
+  powerbridge_observation_.Reset();
+
   // Safeguard against resource leak by observers.
   OnConnectionClosed();
 }
@@ -154,6 +158,21 @@ void ArcIdleManager::ThrottleInstance(bool should_throttle) {
   delegate_->SetInteractiveMode(bridge_, !should_throttle);
 }
 
+void ArcIdleManager::OnVmResumed() {
+  if (!should_throttle()) {
+    // A resume happens because there was a prior suspend.
+    // That earlier suspend counts as first-idle.
+    first_idle_happened_ = true;
+
+    ThrottleInstance(false);
+  }
+}
+
+void ArcIdleManager::OnWillDestroyArcPowerBridge() {
+  // No more notifications about VM resumed.
+  powerbridge_observation_.Reset();
+}
+
 void ArcIdleManager::LogScreenOffTimer(bool toggle_timer) {
   if (toggle_timer) {
     // Start measuring now.

chrome/browser/ash/arc/idle_manager/arc_idle_manager.h

@@ -9,8 +9,10 @@
 #include <string>
 
 #include "ash/components/arc/mojom/power.mojom.h"
+#include "ash/components/arc/power/arc_power_bridge.h"
 #include "ash/components/arc/session/connection_observer.h"
 #include "base/memory/raw_ptr.h"
+#include "base/scoped_observation.h"
 #include "base/sequence_checker.h"
 #include "base/timer/elapsed_timer.h"
 #include "chrome/browser/ash/throttle_service.h"
@@ -22,6 +24,7 @@ class ArcBridgeService;
 // This class holds a number of observers which watch for all conditions that
 // gate the triggering of ARC's Idle (Doze) mode.
 class ArcIdleManager : public KeyedService,
+                       public ArcPowerBridge::Observer,
                        public ash::ThrottleService,
                        public ConnectionObserver<mojom::PowerInstance> {
  public:
@@ -63,6 +66,10 @@ class ArcIdleManager : public KeyedService,
   void OnConnectionReady() override;
   void OnConnectionClosed() override;
 
+  // ArcPowerBridge::Observer
+  void OnVmResumed() override;
+  void OnWillDestroyArcPowerBridge() override;
+
   // Replaces the delegate so we can monitor switches without touching actual
   // power state, for unit test purposes.
   void set_delegate_for_testing(std::unique_ptr<Delegate> delegate) {
@@ -85,6 +92,9 @@ class ArcIdleManager : public KeyedService,
   const raw_ptr<ArcBridgeService, ExperimentalAsh> bridge_;
 
   base::ElapsedTimer interactive_off_span_timer_;
+
+  base::ScopedObservation<ArcPowerBridge, ArcPowerBridge::Observer>
+      powerbridge_observation_{this};
 };
 
 }  // namespace arc

chrome/browser/ash/arc/idle_manager/arc_idle_manager_unittest.cc

@@ -190,9 +190,13 @@ class ArcIdleManagerTest : public testing::Test {
 };
 
 // Tests that ArcIdleManager can be constructed and destructed.
-
 TEST_F(ArcIdleManagerTest, TestConstructDestruct) {}
 
+// Tests that powerbridge early death causes no DCHECKs in observer list.
+TEST_F(ArcIdleManagerTest, TestEarlyPowerBridgeDeath) {
+  arc_idle_manager()->OnWillDestroyArcPowerBridge();
+}
+
 // Tests that ArcIdleManager responds appropriately to various observers.
 TEST_F(ArcIdleManagerTest, TestThrottleInstance) {
   // When no one blocks, it should enable idle;
@@ -250,9 +254,19 @@ TEST_F(ArcIdleManagerTest, TestThrottleInstance) {
   EXPECT_EQ(5U, interactive_enabled_counter());
   EXPECT_EQ(6U, interactive_disabled_counter());
 
+  // ResumeVm event when not idle causes additional idle-disable event.
+  arc_idle_manager()->OnVmResumed();
+  EXPECT_EQ(6U, interactive_enabled_counter());
+  EXPECT_EQ(6U, interactive_disabled_counter());
+
   // Reset.
   arc_window_observer()->SetActive(false);
-  EXPECT_EQ(5U, interactive_enabled_counter());
+  EXPECT_EQ(6U, interactive_enabled_counter());
+  EXPECT_EQ(7U, interactive_disabled_counter());
+
+  // ResumeVm event when idle does not generate switch events.
+  arc_idle_manager()->OnVmResumed();
+  EXPECT_EQ(6U, interactive_enabled_counter());
   EXPECT_EQ(7U, interactive_disabled_counter());
 }
 

chrome/browser/ash/arc/instance_throttle/arc_power_throttle_observer.cc

@@ -35,17 +35,14 @@ void ArcPowerThrottleObserver::StartObserving(
   auto* const power_bridge = ArcPowerBridge::GetForBrowserContext(context);
   // Could be nullptr in unit tests.
   if (power_bridge)
-    power_bridge->AddObserver(this);
+    powerbridge_observation_.Observe(power_bridge);
 }
 
 void ArcPowerThrottleObserver::StopObserving() {
   // Make sure |timer_| is not fired after stopping observing.
   timer_.Stop();
 
-  auto* const power_bridge = ArcPowerBridge::GetForBrowserContext(context());
-  // Could be nullptr in unit tests.
-  if (power_bridge)
-    power_bridge->RemoveObserver(this);
+  powerbridge_observation_.Reset();
 
   ThrottleObserver::StopObserving();
 }
@@ -85,4 +82,9 @@ void ArcPowerThrottleObserver::OnPreAnr(mojom::AnrType type) {
   }
 }
 
+void ArcPowerThrottleObserver::OnWillDestroyArcPowerBridge() {
+  // No more notifications about VM resumed.
+  powerbridge_observation_.Reset();
+}
+
 }  // namespace arc

chrome/browser/ash/arc/instance_throttle/arc_power_throttle_observer.h

@@ -6,6 +6,7 @@
 #define CHROME_BROWSER_ASH_ARC_INSTANCE_THROTTLE_ARC_POWER_THROTTLE_OBSERVER_H_
 
 #include "ash/components/arc/power/arc_power_bridge.h"
+#include "base/scoped_observation.h"
 #include "base/timer/timer.h"
 #include "chrome/browser/ash/throttle_observer.h"
 
@@ -31,9 +32,13 @@ class ArcPowerThrottleObserver : public ash::ThrottleObserver,
 
   // ArcPowerBridge::Observer:
   void OnPreAnr(mojom::AnrType type) override;
+  void OnWillDestroyArcPowerBridge() override;
 
  private:
   base::OneShotTimer timer_;
+
+  base::ScopedObservation<ArcPowerBridge, ArcPowerBridge::Observer>
+      powerbridge_observation_{this};
 };
 
 }  // namespace arc