[SmartLock] Do not trust BluetoothAdapter values shortly after resume.

For a short time window after resuming from suspension, BluetoothAdapter
returns incorrect presence and power values (crbug.com/986896). This CL
is a stopgap measure while we wait for this bug to be resolved. This CL
caches the correct values, and uses them within that time window instead
of directly requesting them from BluetoothAdapter.

This CL also partially addresses crbug.com/890047; it resolves one of
the flickering tooltips.

Bug: 969036, 890047
Change-Id: I26ba8c85d1c3e086ae598aec6abbdae7ee6c5164
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1715632Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Commit-Queue: Ryan Hansberry <hansberry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#681009}

chromeos/components/proximity_auth/BUILD.gn

@@ -121,6 +121,8 @@ source_set("unit_tests") {
     "//chromeos/components/multidevice:test_support",
     "//chromeos/components/multidevice/logging",
     "//chromeos/constants",
+    "//chromeos/dbus/power",
+    "//chromeos/dbus/power:power_manager_proto",
     "//chromeos/services/multidevice_setup/public/cpp:prefs",
     "//chromeos/services/multidevice_setup/public/cpp:test_support",
     "//chromeos/services/secure_channel:test_support",

chromeos/components/proximity_auth/unlock_manager_impl.cc

@@ -12,6 +12,7 @@
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/default_clock.h"
 #include "base/time/time.h"
+#include "base/timer/timer.h"
 #include "chromeos/components/multidevice/logging/logging.h"
 #include "chromeos/components/multidevice/remote_device_ref.h"
 #include "chromeos/components/proximity_auth/messenger.h"
@@ -24,13 +25,22 @@
 namespace proximity_auth {
 namespace {
 
-// The maximum amount of time, in seconds, that the unlock manager can stay in
-// the 'waking up' state after resuming from sleep.
+// The maximum amount of time that the unlock manager can stay in the 'waking
+// up' state after resuming from sleep.
 constexpr base::TimeDelta kWakingUpDuration = base::TimeDelta::FromSeconds(15);
 
-// The limit, in seconds, on the elapsed time for an auth attempt. If an auth
-// attempt exceeds this limit, it will time out and be rejected. This is
-// provided as a failsafe, in case something goes wrong.
+// The maximum amount of time that we wait for the BluetoothAdapter to be
+// fully initialized after resuming from sleep.
+// TODO(crbug.com/986896): This is necessary because the BluetoothAdapter
+// returns incorrect presence and power values directly after resume, and does
+// not return correct values until about 1-2 seconds later. Remove this once
+// the bug is fixed.
+constexpr base::TimeDelta kBluetoothAdapterResumeMaxDuration =
+    base::TimeDelta::FromSeconds(3);
+
+// The limit on the elapsed time for an auth attempt. If an auth attempt exceeds
+// this limit, it will time out and be rejected. This is provided as a failsafe,
+// in case something goes wrong.
 constexpr base::TimeDelta kAuthAttemptTimeout = base::TimeDelta::FromSeconds(5);
 
 constexpr base::TimeDelta kMinGetUnlockableRemoteStatusDuration =
@@ -107,14 +117,9 @@ UnlockManagerImpl::UnlockManagerImpl(
     ProximityAuthSystem::ScreenlockType screenlock_type,
     ProximityAuthClient* proximity_auth_client)
     : screenlock_type_(screenlock_type),
-      life_cycle_(nullptr),
       proximity_auth_client_(proximity_auth_client),
-      is_attempting_auth_(false),
-      is_performing_initial_scan_(false),
-      screenlock_state_(ScreenlockState::INACTIVE),
-      initial_scan_timeout_weak_ptr_factory_(this),
-      reject_auth_attempt_weak_ptr_factory_(this),
-      weak_ptr_factory_(this) {
+      bluetooth_suspension_recovery_timer_(
+          std::make_unique<base::OneShotTimer>()) {
   chromeos::PowerManagerClient::Get()->AddObserver(this);
 
   if (device::BluetoothAdapterFactory::IsBluetoothSupported()) {
@@ -314,15 +319,44 @@ void UnlockManagerImpl::AdapterPoweredChanged(device::BluetoothAdapter* adapter,
   UpdateLockScreen();
 }
 
+void UnlockManagerImpl::SuspendImminent(
+    power_manager::SuspendImminent::Reason reason) {
+  // TODO(crbug.com/986896): For a short time window after resuming from
+  // suspension, BluetoothAdapter returns incorrect presence and power values.
+  // Cache the correct values now, in case we need to check those values during
+  // that time window when the device resumes.
+  was_bluetooth_present_and_powered_before_last_suspend_ =
+      IsBluetoothPresentAndPowered();
+  bluetooth_suspension_recovery_timer_->Stop();
+}
+
 void UnlockManagerImpl::SuspendDone(const base::TimeDelta& sleep_duration) {
+  bluetooth_suspension_recovery_timer_->Start(
+      FROM_HERE, kBluetoothAdapterResumeMaxDuration,
+      base::Bind(&UnlockManagerImpl::UpdateLockScreen,
+                 weak_ptr_factory_.GetWeakPtr()));
+
   SetIsPerformingInitialScan(true /* is_performing_initial_scan */);
 }
 
 bool UnlockManagerImpl::IsBluetoothPresentAndPowered() const {
+  // TODO(crbug.com/986896): If the BluetoothAdapter is still "resuming after
+  // suspension" at this time, it's prone to this bug, meaning we cannot trust
+  // its returned presence and power values. If this is the case, depend on
+  // the cached |was_bluetooth_present_and_powered_before_last_suspend_| to
+  // signal if Bluetooth is enabled; otherwise, directly check request values
+  // from BluetoothAdapter. Remove this check once the bug is fixed.
+  if (IsBluetoothAdapterRecoveringFromSuspend())
+    return was_bluetooth_present_and_powered_before_last_suspend_;
+
   return bluetooth_adapter_ && bluetooth_adapter_->IsPresent() &&
          bluetooth_adapter_->IsPowered();
 }
 
+bool UnlockManagerImpl::IsBluetoothAdapterRecoveringFromSuspend() const {
+  return bluetooth_suspension_recovery_timer_->IsRunning();
+}
+
 void UnlockManagerImpl::AttemptToStartRemoteDeviceLifecycle() {
   if (IsBluetoothPresentAndPowered() && life_cycle_ &&
       life_cycle_->GetState() == RemoteDeviceLifeCycle::State::STOPPED) {
@@ -660,4 +694,9 @@ void UnlockManagerImpl::ResetPerformanceMetricsTimestamps() {
   attempt_get_remote_status_start_time_ = base::Time();
 }
 
+void UnlockManagerImpl::SetBluetoothSuspensionRecoveryTimerForTesting(
+    std::unique_ptr<base::OneShotTimer> timer) {
+  bluetooth_suspension_recovery_timer_ = std::move(timer);
+}
+
 }  // namespace proximity_auth

chromeos/components/proximity_auth/unlock_manager_impl.h

@@ -23,6 +23,10 @@
 #include "chromeos/services/secure_channel/public/mojom/secure_channel.mojom.h"
 #include "device/bluetooth/bluetooth_adapter.h"
 
+namespace base {
+class OneShotTimer;
+}  // namespace base
+
 namespace proximity_auth {
 
 class Messenger;
@@ -57,6 +61,8 @@ class UnlockManagerImpl : public UnlockManager,
       RemoteDeviceLifeCycle* life_cycle);
 
  private:
+  friend class ProximityAuthUnlockManagerImplTest;
+
   // The possible lock screen states for the remote device.
   enum class RemoteScreenlockState {
     UNKNOWN,
@@ -90,6 +96,7 @@ class UnlockManagerImpl : public UnlockManager,
                              bool powered) override;
 
   // chromeos::PowerManagerClient::Observer:
+  void SuspendImminent(power_manager::SuspendImminent::Reason reason) override;
   void SuspendDone(const base::TimeDelta& sleep_duration) override;
 
   // RemoteDeviceLifeCycle::Observer:
@@ -99,6 +106,17 @@ class UnlockManagerImpl : public UnlockManager,
   // Returns true if the BluetoothAdapter is present and powered.
   bool IsBluetoothPresentAndPowered() const;
 
+  // TODO(crbug.com/986896): Waiting a certain time, after resume, before
+  // trusting the presence and power values returned by BluetoothAdapter is
+  // necessary because the BluetoothAdapter returns incorrect values directly
+  // after resume, and does not return correct values until about 1-2 seconds
+  // later. Remove this function once the bug is resolved.
+  //
+  // This function returns true if the BluetoothAdapter is still resuming from
+  // suspension, indicating that its returned presence and power values cannot
+  // yet be trusted.
+  bool IsBluetoothAdapterRecoveringFromSuspend() const;
+
   // If the RemoteDeviceLifeCycle is available, ensure it is started (but only
   // if Bluetooth is available).
   void AttemptToStartRemoteDeviceLifecycle();
@@ -159,27 +177,43 @@ class UnlockManagerImpl : public UnlockManager,
   // Clears the timers for beginning a scan and fetching remote status.
   void ResetPerformanceMetricsTimestamps();
 
+  void SetBluetoothSuspensionRecoveryTimerForTesting(
+      std::unique_ptr<base::OneShotTimer> timer);
+
   // Whether |this| manager is being used for sign-in or session unlock.
   const ProximityAuthSystem::ScreenlockType screenlock_type_;
 
-  // Whether the user is present at the remote device. Unset if no remote status
-  // update has yet been received.
-  std::unique_ptr<RemoteScreenlockState> remote_screenlock_state_;
+  // Used to call into the embedder. Expected to outlive |this| instance.
+  ProximityAuthClient* proximity_auth_client_;
 
-  // Controls the proximity auth flow logic for a remote device. Not owned, and
-  // expcted to outlive |this| instance.
-  RemoteDeviceLifeCycle* life_cycle_;
+  // Starts running after resuming from suspension, and fires once enough time
+  // has elapsed such that the BluetoothAdapter's presence and power values can
+  // be trusted again. To be removed once https://crbug.com/986896 is fixed.
+  std::unique_ptr<base::OneShotTimer> bluetooth_suspension_recovery_timer_;
+
+  // The Bluetooth adapter. Null if there is no adapter present on the local
+  // device.
+  scoped_refptr<device::BluetoothAdapter> bluetooth_adapter_;
 
   // Tracks whether the remote device is currently in close enough proximity to
   // the local device to allow unlocking.
   std::unique_ptr<ProximityMonitor> proximity_monitor_;
 
-  // Used to call into the embedder. Expected to outlive |this| instance.
-  ProximityAuthClient* proximity_auth_client_;
+  // Whether the user is present at the remote device. Unset if no remote status
+  // update has yet been received.
+  std::unique_ptr<RemoteScreenlockState> remote_screenlock_state_;
+
+  // The sign-in secret received from the remote device by decrypting the
+  // sign-in challenge.
+  std::unique_ptr<std::string> sign_in_secret_;
+
+  // Controls the proximity auth flow logic for a remote device. Not owned, and
+  // expcted to outlive |this| instance.
+  RemoteDeviceLifeCycle* life_cycle_ = nullptr;
 
   // True if the manager is currently processing a user-initiated authentication
   // attempt, which is initiated when the user pod is clicked.
-  bool is_attempting_auth_;
+  bool is_attempting_auth_ = false;
 
   // If true, either the lock screen was just shown (after resuming from
   // suspend, or directly locking the screen), or the focused user pod was
@@ -188,18 +222,17 @@ class UnlockManagerImpl : public UnlockManager,
   // point the user visually sees an indication that the phone cannot be found).
   // Though this field becomes false after this timeout, Smart Lock continues
   // to scan for the phone until the user unlocks the screen.
-  bool is_performing_initial_scan_;
-
-  // The Bluetooth adapter. Null if there is no adapter present on the local
-  // device.
-  scoped_refptr<device::BluetoothAdapter> bluetooth_adapter_;
+  bool is_performing_initial_scan_ = false;
 
-  // The sign-in secret received from the remote device by decrypting the
-  // sign-in challenge.
-  std::unique_ptr<std::string> sign_in_secret_;
+  // TODO(crbug.com/986896): For a short time window after resuming from
+  // suspension, BluetoothAdapter returns incorrect presence and power values.
+  // This field acts as a cache in case we need to check those values during
+  // that time window when the device resumes. Remove this field once the bug
+  // is fixed.
+  bool was_bluetooth_present_and_powered_before_last_suspend_ = false;
 
   // The state of the current screen lock UI.
-  ScreenlockState screenlock_state_;
+  ScreenlockState screenlock_state_ = ScreenlockState::INACTIVE;
 
   // The timestamp of when UnlockManager begins to try to establish a secure
   // connection to the requested remote device of the provided
@@ -214,15 +247,16 @@ class UnlockManagerImpl : public UnlockManager,
   // Used to track if the "initial scan" has timed out. See
   // |is_performing_initial_scan_| for more.
   base::WeakPtrFactory<UnlockManagerImpl>
-      initial_scan_timeout_weak_ptr_factory_;
+      initial_scan_timeout_weak_ptr_factory_{this};
 
   // Used to reject auth attempts after a timeout. An in-progress auth attempt
   // blocks the sign-in screen UI, so it's important to prevent the auth attempt
   // from blocking the UI in case a step in the code path hangs.
-  base::WeakPtrFactory<UnlockManagerImpl> reject_auth_attempt_weak_ptr_factory_;
+  base::WeakPtrFactory<UnlockManagerImpl> reject_auth_attempt_weak_ptr_factory_{
+      this};
 
   // Used to vend all other weak pointers.
-  base::WeakPtrFactory<UnlockManagerImpl> weak_ptr_factory_;
+  base::WeakPtrFactory<UnlockManagerImpl> weak_ptr_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(UnlockManagerImpl);
 };

chromeos/components/proximity_auth/unlock_manager_impl_unittest.cc

@@ -12,6 +12,7 @@
 #include "base/test/scoped_feature_list.h"
 #include "base/test/test_simple_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "base/timer/mock_timer.h"
 #include "build/build_config.h"
 #include "chromeos/components/multidevice/logging/logging.h"
 #include "chromeos/components/multidevice/remote_device_test_util.h"
@@ -23,6 +24,9 @@
 #include "chromeos/components/proximity_auth/remote_device_life_cycle.h"
 #include "chromeos/components/proximity_auth/remote_status_update.h"
 #include "chromeos/constants/chromeos_features.h"
+#include "chromeos/dbus/power/fake_power_manager_client.h"
+#include "chromeos/dbus/power/power_manager_client.h"
+#include "chromeos/dbus/power_manager/suspend.pb.h"
 #include "chromeos/services/secure_channel/public/cpp/client/fake_client_channel.h"
 #include "device/bluetooth/bluetooth_adapter_factory.h"
 #include "device/bluetooth/test/mock_bluetooth_adapter.h"
@@ -160,16 +164,16 @@ class ProximityAuthUnlockManagerImplTest : public testing::Test {
   ~ProximityAuthUnlockManagerImplTest() override = default;
 
   void SetUp() override {
+    chromeos::PowerManagerClient::InitializeFake();
+
     ON_CALL(*bluetooth_adapter_, IsPresent()).WillByDefault(Return(true));
     ON_CALL(*bluetooth_adapter_, IsPowered()).WillByDefault(Return(true));
+
     ON_CALL(messenger_, SupportsSignIn()).WillByDefault(Return(true));
     ON_CALL(messenger_, GetChannel())
         .WillByDefault(Return(fake_client_channel_.get()));
-
     life_cycle_.set_messenger(&messenger_);
     life_cycle_.set_channel(fake_client_channel_.get());
-
-    chromeos::PowerManagerClient::InitializeFake();
   }
 
   void TearDown() override {
@@ -189,6 +193,11 @@ class ProximityAuthUnlockManagerImplTest : public testing::Test {
       ProximityAuthSystem::ScreenlockType screenlock_type) {
     unlock_manager_.reset(
         new TestUnlockManager(screenlock_type, &proximity_auth_client_));
+
+    auto mock_timer = std::make_unique<base::MockOneShotTimer>();
+    mock_bluetooth_suspension_recovery_timer_ = mock_timer.get();
+    unlock_manager_->SetBluetoothSuspensionRecoveryTimerForTesting(
+        std::move(mock_timer));
   }
 
   void SimulateUserPresentState() {
@@ -222,6 +231,7 @@ class ProximityAuthUnlockManagerImplTest : public testing::Test {
   NiceMock<MockProximityAuthClient> proximity_auth_client_;
   NiceMock<MockMessenger> messenger_;
   std::unique_ptr<TestUnlockManager> unlock_manager_;
+  base::MockOneShotTimer* mock_bluetooth_suspension_recovery_timer_ = nullptr;
 
  private:
   base::test::ScopedFeatureList scoped_feature_list_;
@@ -528,6 +538,119 @@ TEST_F(ProximityAuthUnlockManagerImplTest, BluetoothAdapterPowerChanges) {
   EXPECT_TRUE(life_cycle_.started());
 }
 
+TEST_F(
+    ProximityAuthUnlockManagerImplTest,
+    CacheBluetoothAdapterStateAfterSuspendAndResume_AttemptConnectionWhileBluetoothAdapterIsStillRecovering) {
+  CreateUnlockManager(ProximityAuthSystem::SESSION_LOCK);
+
+  ASSERT_FALSE(mock_bluetooth_suspension_recovery_timer_->IsRunning());
+
+  chromeos::FakePowerManagerClient::Get()->SendSuspendImminent(
+      power_manager::SuspendImminent_Reason_LID_CLOSED);
+
+  // Simulate https://crbug.com/986896 by returning false for presence and power
+  // directly after resuming, but do not fire
+  // |mock_bluetooth_suspension_recovery_timer_|, simulating that not enough
+  // time has passed for the BluetoothAdapter to recover. It's expected under
+  // these conditions that:
+  // * ProximityAuthClient::UpdateScreenlockState() never be called with
+  //   ScreenlockState::NO_BLUETOOTH.
+  // * ProximityAuthClient::UpdateScreenlockState() only be called once with
+  //   ScreenlockState::BLUETOOTH_CONNECTING, because it should only be called
+  //   on when the ScreenlockState value changes.
+  EXPECT_CALL(proximity_auth_client_,
+              UpdateScreenlockState(ScreenlockState::NO_BLUETOOTH))
+      .Times(0);
+  EXPECT_CALL(proximity_auth_client_,
+              UpdateScreenlockState(ScreenlockState::BLUETOOTH_CONNECTING));
+
+  ON_CALL(*bluetooth_adapter_, IsPresent()).WillByDefault(Return(false));
+  ON_CALL(*bluetooth_adapter_, IsPowered()).WillByDefault(Return(false));
+
+  chromeos::FakePowerManagerClient::Get()->SendSuspendDone();
+  EXPECT_TRUE(mock_bluetooth_suspension_recovery_timer_->IsRunning());
+
+  // Simulate how ProximityAuthSystem, the owner of UnlockManager, reacts to
+  // resume: providing a new RemoteDeviceLifeCycle. This shouldn't trigger a new
+  // call to ProximityAuthClient::UpdateScreenlockState().
+  unlock_manager_->SetRemoteDeviceLifeCycle(&life_cycle_);
+  EXPECT_TRUE(life_cycle_.started());
+
+  EXPECT_TRUE(mock_bluetooth_suspension_recovery_timer_->IsRunning());
+}
+
+TEST_F(
+    ProximityAuthUnlockManagerImplTest,
+    CacheBluetoothAdapterStateAfterSuspendAndResume_AttemptConnectionOnceBluetoothAdapterHasHadTimeToRecover) {
+  CreateUnlockManager(ProximityAuthSystem::SESSION_LOCK);
+
+  ASSERT_FALSE(mock_bluetooth_suspension_recovery_timer_->IsRunning());
+
+  chromeos::FakePowerManagerClient::Get()->SendSuspendImminent(
+      power_manager::SuspendImminent_Reason_LID_CLOSED);
+
+  // Simulate https://crbug.com/986896 by returning false for presence and power
+  // directly after resuming, and then fire
+  // |mock_bluetooth_suspension_recovery_timer_|, simulating that enough time
+  // has passed for the BluetoothAdapter to recover - this means that Bluetooth
+  // is truly off after resume and the user should be visually informed as such.
+  // It's expected under these conditions that:
+  // * ProximityAuthClient::UpdateScreenlockState() only be called once with
+  //   ScreenlockState::NO_BLUETOOTH, but after the timer fires (this is
+  //   impossible to explicitly do in code with mocks, unfortunately).
+  // * ProximityAuthClient::UpdateScreenlockState() only be called once with
+  //   ScreenlockState::BLUETOOTH_CONNECTING, directly after SuspendDone.
+  EXPECT_CALL(proximity_auth_client_,
+              UpdateScreenlockState(ScreenlockState::NO_BLUETOOTH));
+  EXPECT_CALL(proximity_auth_client_,
+              UpdateScreenlockState(ScreenlockState::BLUETOOTH_CONNECTING));
+
+  ON_CALL(*bluetooth_adapter_, IsPresent()).WillByDefault(Return(false));
+  ON_CALL(*bluetooth_adapter_, IsPowered()).WillByDefault(Return(false));
+
+  chromeos::FakePowerManagerClient::Get()->SendSuspendDone();
+  EXPECT_TRUE(mock_bluetooth_suspension_recovery_timer_->IsRunning());
+
+  // Simulate how ProximityAuthSystem, the owner of UnlockManager, reacts to
+  // resume: providing a new RemoteDeviceLifeCycle. This shouldn't trigger a new
+  // call to ProximityAuthClient::UpdateScreenlockState().
+  unlock_manager_->SetRemoteDeviceLifeCycle(&life_cycle_);
+  EXPECT_TRUE(life_cycle_.started());
+
+  ON_CALL(*bluetooth_adapter_, IsPresent()).WillByDefault(Return(false));
+  ON_CALL(*bluetooth_adapter_, IsPowered()).WillByDefault(Return(false));
+
+  EXPECT_TRUE(mock_bluetooth_suspension_recovery_timer_->IsRunning());
+
+  // This leads to ProximityAuthClient::UpdateScreenlockState() being called
+  // with ScreenlockState::NO_BLUETOOTH.
+  mock_bluetooth_suspension_recovery_timer_->Fire();
+}
+
+TEST_F(ProximityAuthUnlockManagerImplTest,
+       BluetoothOffMessagePresentedImmediatelyIfBluetoothWasOffBeforeSuspend) {
+  CreateUnlockManager(ProximityAuthSystem::SESSION_LOCK);
+
+  ON_CALL(*bluetooth_adapter_, IsPresent()).WillByDefault(Return(false));
+  ON_CALL(*bluetooth_adapter_, IsPowered()).WillByDefault(Return(false));
+
+  chromeos::FakePowerManagerClient::Get()->SendSuspendImminent(
+      power_manager::SuspendImminent_Reason_LID_CLOSED);
+
+  EXPECT_CALL(proximity_auth_client_,
+              UpdateScreenlockState(ScreenlockState::NO_BLUETOOTH));
+  EXPECT_CALL(proximity_auth_client_,
+              UpdateScreenlockState(ScreenlockState::BLUETOOTH_CONNECTING))
+      .Times(0);
+
+  chromeos::FakePowerManagerClient::Get()->SendSuspendDone();
+
+  // Simulate how ProximityAuthSystem, the owner of UnlockManager, reacts to
+  // resume: providing a new RemoteDeviceLifeCycle.
+  unlock_manager_->SetRemoteDeviceLifeCycle(&life_cycle_);
+  EXPECT_FALSE(life_cycle_.started());
+}
+
 TEST_F(ProximityAuthUnlockManagerImplTest, StartsProximityMonitor) {
   CreateUnlockManager(ProximityAuthSystem::SESSION_LOCK);
   SimulateUserPresentState();