Reland 'Smart Lock: Clear stale ProximityMonitor on disconnect.'
The original commit (crrev.com/c/1479941) was reverted
(crrev.com/c/1480309) because of usage of unitialized memory.
Specifically, the MockProximityMonitor object had been deleted but was
then derefenced.
Patch 1 is the original commit, Patch 2 is the fix.
Original commit message:
UnlockManager was previously holding onto a stale
ProximityMonitor from its first connection. That meant
that if a disconnection and subsequent new connection
occurred, the old ProximityMonitor, with an old reference
to the previous connection (represented as a ClientChannel
object) was used. This always resulted in a Smart Lock
failure for a second connection, and occasionally, a crash.
In order to correctly reset the ProximityMonitor object,
the way that it is called had to be refactored (remove
completely unneeded ScreenLockBridge logic), in order to
consolidate calls to one place.
Bug: 931929
Change-Id: If8d7b48efe0055b8d0afc2f4cd3954114c0b7a71
Reviewed-on: https://chromium-review.googlesource.com/c/1481613
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Commit-Queue: Ryan Hansberry <hansberry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#634330}diff --git a/chromeos/components/proximity_auth/unlock_manager_impl.cc b/chromeos/components/proximity_auth/unlock_manager_impl.cc
index ce7ae90f..d545594 100644
--- a/chromeos/components/proximity_auth/unlock_manager_impl.cc
+++ b/chromeos/components/proximity_auth/unlock_manager_impl.cc
@@ -115,17 +115,12 @@
life_cycle_(nullptr),
proximity_auth_client_(proximity_auth_client),
pref_manager_(pref_manager),
- is_locked_(false),
is_attempting_auth_(false),
is_waking_up_(false),
screenlock_state_(ScreenlockState::INACTIVE),
clear_waking_up_state_weak_ptr_factory_(this),
reject_auth_attempt_weak_ptr_factory_(this),
weak_ptr_factory_(this) {
- ScreenlockBridge* screenlock_bridge = ScreenlockBridge::Get();
- screenlock_bridge->AddObserver(this);
- OnScreenLockedOrUnlocked(screenlock_bridge->IsLocked());
-
DBusThreadManager::Get()->GetPowerManagerClient()->AddObserver(this);
SetWakingUpState(true /* is_waking_up */);
@@ -144,8 +139,6 @@
if (proximity_monitor_)
proximity_monitor_->RemoveObserver(this);
- ScreenlockBridge::Get()->RemoveObserver(this);
-
DBusThreadManager::Get()->GetPowerManagerClient()->RemoveObserver(this);
if (bluetooth_adapter_)
@@ -196,11 +189,16 @@
if (!proximity_monitor_) {
proximity_monitor_ = CreateProximityMonitor(life_cycle_, pref_manager_);
proximity_monitor_->AddObserver(this);
+ proximity_monitor_->Start();
}
GetMessenger()->AddObserver(this);
attempt_get_remote_status_start_time_ =
base::DefaultClock::GetInstance()->Now();
+ } else if (proximity_monitor_) {
+ proximity_monitor_->RemoveObserver(this);
+ proximity_monitor_->Stop();
+ proximity_monitor_.reset();
}
if (state == RemoteDeviceLifeCycle::State::AUTHENTICATION_FAILED)
@@ -302,26 +300,6 @@
UpdateLockScreen();
}
-void UnlockManagerImpl::OnScreenDidLock(
- ScreenlockBridge::LockHandler::ScreenType screen_type) {
- OnScreenLockedOrUnlocked(true);
-}
-
-void UnlockManagerImpl::OnScreenDidUnlock(
- ScreenlockBridge::LockHandler::ScreenType screen_type) {
- OnScreenLockedOrUnlocked(false);
-}
-
-void UnlockManagerImpl::OnFocusedUserChanged(const AccountId& account_id) {}
-
-void UnlockManagerImpl::OnScreenLockedOrUnlocked(bool is_locked) {
- if (is_locked && IsBluetoothPresentAndPowered() && life_cycle_)
- SetWakingUpState(true /* is_waking_up */);
-
- is_locked_ = is_locked;
- UpdateProximityMonitorState();
-}
-
void UnlockManagerImpl::OnBluetoothAdapterInitialized(
scoped_refptr<device::BluetoothAdapter> adapter) {
bluetooth_adapter_ = adapter;
@@ -515,8 +493,6 @@
void UnlockManagerImpl::UpdateLockScreen() {
AttemptToStartRemoteDeviceLifecycle();
- UpdateProximityMonitorState();
-
ScreenlockState new_state = GetScreenlockState();
if (screenlock_state_ == new_state)
return;
@@ -531,19 +507,6 @@
screenlock_state_ = new_state;
}
-void UnlockManagerImpl::UpdateProximityMonitorState() {
- if (!proximity_monitor_)
- return;
-
- if (is_locked_ && life_cycle_ &&
- life_cycle_->GetState() ==
- RemoteDeviceLifeCycle::State::SECURE_CHANNEL_ESTABLISHED) {
- proximity_monitor_->Start();
- } else {
- proximity_monitor_->Stop();
- }
-}
-
void UnlockManagerImpl::SetWakingUpState(bool is_waking_up) {
is_waking_up_ = is_waking_up;
diff --git a/chromeos/components/proximity_auth/unlock_manager_impl.h b/chromeos/components/proximity_auth/unlock_manager_impl.h
index 0da19963..497913e 100644
--- a/chromeos/components/proximity_auth/unlock_manager_impl.h
+++ b/chromeos/components/proximity_auth/unlock_manager_impl.h
@@ -35,7 +35,6 @@
class UnlockManagerImpl : public UnlockManager,
public MessengerObserver,
public ProximityMonitorObserver,
- public ScreenlockBridge::Observer,
chromeos::PowerManagerClient::Observer,
public device::BluetoothAdapter::Observer {
public:
@@ -79,13 +78,6 @@
// ProximityMonitorObserver:
void OnProximityStateChanged() override;
- // ScreenlockBridge::Observer
- void OnScreenDidLock(
- ScreenlockBridge::LockHandler::ScreenType screen_type) override;
- void OnScreenDidUnlock(
- ScreenlockBridge::LockHandler::ScreenType screen_type) override;
- void OnFocusedUserChanged(const AccountId& account_id) override;
-
// Called when the screenlock state changes.
void OnScreenLockedOrUnlocked(bool is_locked);
@@ -184,9 +176,6 @@
// Used to access the common prefs. Expected to outlive |this| instance.
ProximityAuthPrefManager* pref_manager_;
- // Whether the screen is currently locked.
- bool is_locked_;
-
// 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_;
diff --git a/chromeos/components/proximity_auth/unlock_manager_impl_unittest.cc b/chromeos/components/proximity_auth/unlock_manager_impl_unittest.cc
index 81d9606..ac049bd1 100644
--- a/chromeos/components/proximity_auth/unlock_manager_impl_unittest.cc
+++ b/chromeos/components/proximity_auth/unlock_manager_impl_unittest.cc
@@ -22,7 +22,6 @@
#include "chromeos/components/proximity_auth/proximity_monitor.h"
#include "chromeos/components/proximity_auth/remote_device_life_cycle.h"
#include "chromeos/components/proximity_auth/remote_status_update.h"
-#include "chromeos/components/proximity_auth/screenlock_bridge.h"
#include "chromeos/constants/chromeos_features.h"
#include "chromeos/dbus/dbus_thread_manager.h"
#include "chromeos/services/secure_channel/public/cpp/client/fake_client_channel.h"
@@ -76,22 +75,22 @@
class MockProximityMonitor : public ProximityMonitor {
public:
- MockProximityMonitor() : started_(false), stopped_(false) {
+ MockProximityMonitor(base::OnceClosure destroy_callback)
+ : destroy_callback_(std::move(destroy_callback)), started_(false) {
ON_CALL(*this, IsUnlockAllowed()).WillByDefault(Return(true));
}
- ~MockProximityMonitor() override {}
+ ~MockProximityMonitor() override { std::move(destroy_callback_).Run(); }
void Start() override { started_ = true; }
- void Stop() override { stopped_ = true; }
+ void Stop() override {}
MOCK_CONST_METHOD0(IsUnlockAllowed, bool());
MOCK_METHOD0(RecordProximityMetricsOnAuthSuccess, void());
bool started() { return started_; }
- bool stopped() { return stopped_; }
private:
+ base::OnceClosure destroy_callback_;
bool started_;
- bool stopped_;
DISALLOW_COPY_AND_ASSIGN(MockProximityMonitor);
};
@@ -100,8 +99,7 @@
public:
TestUnlockManager(ProximityAuthSystem::ScreenlockType screenlock_type,
ProximityAuthClient* proximity_auth_client)
- : UnlockManagerImpl(screenlock_type, proximity_auth_client, nullptr),
- proximity_monitor_(nullptr) {}
+ : UnlockManagerImpl(screenlock_type, proximity_auth_client, nullptr) {}
~TestUnlockManager() override {}
using UnlockManager::OnAuthAttempted;
@@ -110,24 +108,29 @@
using MessengerObserver::OnDecryptResponse;
using MessengerObserver::OnUnlockResponse;
using MessengerObserver::OnDisconnected;
- using ScreenlockBridge::Observer::OnScreenDidLock;
- using ScreenlockBridge::Observer::OnScreenDidUnlock;
- using ScreenlockBridge::Observer::OnFocusedUserChanged;
MockProximityMonitor* proximity_monitor() { return proximity_monitor_; }
+ bool proximity_monitor_destroyed() { return proximity_monitor_destroyed_; }
private:
std::unique_ptr<ProximityMonitor> CreateProximityMonitor(
RemoteDeviceLifeCycle* life_cycle,
ProximityAuthPrefManager* pref_manager) override {
std::unique_ptr<MockProximityMonitor> proximity_monitor(
- new NiceMock<MockProximityMonitor>());
+ new NiceMock<MockProximityMonitor>(
+ base::BindOnce(&TestUnlockManager::OnProximityMonitorDestroyed,
+ base::Unretained(this))));
+ proximity_monitor_destroyed_ = false;
+
proximity_monitor_ = proximity_monitor.get();
return std::move(proximity_monitor);
}
+ void OnProximityMonitorDestroyed() { proximity_monitor_destroyed_ = true; }
+
// Owned by the super class.
- MockProximityMonitor* proximity_monitor_;
+ MockProximityMonitor* proximity_monitor_ = nullptr;
+ bool proximity_monitor_destroyed_ = false;
DISALLOW_COPY_AND_ASSIGN(TestUnlockManager);
};
@@ -170,8 +173,6 @@
unlock_manager_.reset();
chromeos::DBusThreadManager::Shutdown();
-
- ScreenlockBridge::Get()->SetLockHandler(nullptr);
}
void SetUp() override {
@@ -183,7 +184,6 @@
life_cycle_.set_messenger(&messenger_);
life_cycle_.set_channel(fake_client_channel_.get());
- ScreenlockBridge::Get()->SetLockHandler(&lock_handler_);
chromeos::DBusThreadManager::Initialize();
}
@@ -208,6 +208,11 @@
return unlock_manager_ ? unlock_manager_->proximity_monitor() : nullptr;
}
+ bool proximity_monitor_destroyed() {
+ return unlock_manager_ ? unlock_manager_->proximity_monitor_destroyed()
+ : false;
+ }
+
protected:
chromeos::multidevice::RemoteDeviceRef remote_device_;
chromeos::multidevice::RemoteDeviceRef local_device_;
@@ -443,7 +448,7 @@
unlock_manager_->OnLifeCycleStateChanged();
life_cycle_.ChangeState(RemoteDeviceLifeCycle::State::FINDING_CONNECTION);
unlock_manager_->OnLifeCycleStateChanged();
- EXPECT_TRUE(proximity_monitor()->stopped());
+ EXPECT_TRUE(proximity_monitor_destroyed());
}
TEST_F(
@@ -459,6 +464,34 @@
EXPECT_TRUE(proximity_monitor()->started());
}
+// Regression test for crbug.com/931929. Capture the case where the phone is
+// connected to, connection is lost, and then a new connection is made shortly
+// after.
+TEST_F(ProximityAuthUnlockManagerImplTest,
+ SetRemoteDeviceLifeCycle_TwiceConnectedRemoteDeviceLifeCycle) {
+ CreateUnlockManager(ProximityAuthSystem::SESSION_LOCK);
+
+ unlock_manager_->SetRemoteDeviceLifeCycle(&life_cycle_);
+ unlock_manager_->OnLifeCycleStateChanged();
+
+ life_cycle_.ChangeState(
+ RemoteDeviceLifeCycle::State::SECURE_CHANNEL_ESTABLISHED);
+ unlock_manager_->OnLifeCycleStateChanged();
+ EXPECT_TRUE(proximity_monitor()->started());
+
+ // Simulate the phone connection being lost. The ProximityMonitor is stale
+ // and should have been destroyed.
+ life_cycle_.ChangeState(RemoteDeviceLifeCycle::State::FINDING_CONNECTION);
+ unlock_manager_->OnLifeCycleStateChanged();
+ EXPECT_TRUE(proximity_monitor_destroyed());
+
+ life_cycle_.ChangeState(
+ RemoteDeviceLifeCycle::State::SECURE_CHANNEL_ESTABLISHED);
+ unlock_manager_->OnLifeCycleStateChanged();
+ EXPECT_FALSE(proximity_monitor_destroyed());
+ EXPECT_TRUE(proximity_monitor()->started());
+}
+
TEST_F(ProximityAuthUnlockManagerImplTest, BluetoothAdapterNotPresent) {
ON_CALL(*bluetooth_adapter_, IsPresent()).WillByDefault(Return(false));
@@ -513,7 +546,7 @@
unlock_manager_->OnLifeCycleStateChanged();
life_cycle_.ChangeState(RemoteDeviceLifeCycle::State::AUTHENTICATION_FAILED);
unlock_manager_->OnLifeCycleStateChanged();
- EXPECT_TRUE(proximity_monitor()->stopped());
+ EXPECT_TRUE(proximity_monitor_destroyed());
}
TEST_F(ProximityAuthUnlockManagerImplTest,
@@ -562,44 +595,6 @@
}
TEST_F(ProximityAuthUnlockManagerImplTest,
- OnScreenDidUnlock_StopsProximityMonitor) {
- CreateUnlockManager(ProximityAuthSystem::SESSION_LOCK);
- SimulateUserPresentState();
-
- unlock_manager_.get()->OnScreenDidUnlock(
- ScreenlockBridge::LockHandler::LOCK_SCREEN);
- EXPECT_TRUE(proximity_monitor()->stopped());
-}
-
-TEST_F(ProximityAuthUnlockManagerImplTest,
- OnScreenDidLock_StartsProximityMonitor) {
- CreateUnlockManager(ProximityAuthSystem::SESSION_LOCK);
- unlock_manager_->SetRemoteDeviceLifeCycle(&life_cycle_);
-
- unlock_manager_.get()->OnScreenDidLock(
- ScreenlockBridge::LockHandler::LOCK_SCREEN);
-
- life_cycle_.ChangeState(
- RemoteDeviceLifeCycle::State::SECURE_CHANNEL_ESTABLISHED);
- unlock_manager_->OnLifeCycleStateChanged();
- EXPECT_TRUE(proximity_monitor()->started());
-}
-
-TEST_F(ProximityAuthUnlockManagerImplTest, OnScreenDidLock_SetsWakingUpState) {
- CreateUnlockManager(ProximityAuthSystem::SESSION_LOCK);
-
- EXPECT_CALL(proximity_auth_client_,
- UpdateScreenlockState(ScreenlockState::BLUETOOTH_CONNECTING));
-
- life_cycle_.ChangeState(RemoteDeviceLifeCycle::State::FINDING_CONNECTION);
- unlock_manager_->SetRemoteDeviceLifeCycle(&life_cycle_);
- unlock_manager_.get()->OnScreenDidLock(
- ScreenlockBridge::LockHandler::LOCK_SCREEN);
-
- unlock_manager_->OnLifeCycleStateChanged();
-}
-
-TEST_F(ProximityAuthUnlockManagerImplTest,
OnDecryptResponse_NoAuthAttemptInProgress) {
CreateUnlockManager(ProximityAuthSystem::SESSION_LOCK);
SimulateUserPresentState();
