diff options
author | sacomoto <sacomoto@chromium.org> | 2015-06-26 01:10:08 -0700 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2015-06-26 08:10:39 +0000 |
commit | deae010542cc2d5eeaf7523b29ce9f4d7209753d (patch) | |
tree | bf7e4da424192fbab93ec8c95f6f71ef53f05c16 /components/proximity_auth | |
parent | aa9eebf8777f5a9f787dcb4c715d8dc6c831d8c7 (diff) | |
download | chromium_src-deae010542cc2d5eeaf7523b29ce9f4d7209753d.zip chromium_src-deae010542cc2d5eeaf7523b29ce9f4d7209753d.tar.gz chromium_src-deae010542cc2d5eeaf7523b29ce9f4d7209753d.tar.bz2 |
Properly handling an adapter removal event in BLE connection finder.
This CL modifies the proximity_auth::BluetoothLowEnergyConnectionFinder
class to handle the case where the bluetooth adapter is removed and
re-added.
This is precisely what happens when a BluetoothLowEnergyConnectionFinder
instance is running and the CrOS goes to sleep and is waken up later.
BUG=503924
Review URL: https://codereview.chromium.org/1207543006
Cr-Commit-Position: refs/heads/master@{#336343}
Diffstat (limited to 'components/proximity_auth')
4 files changed, 171 insertions, 50 deletions
diff --git a/components/proximity_auth/ble/bluetooth_low_energy_connection.h b/components/proximity_auth/ble/bluetooth_low_energy_connection.h index 29baf42..648b199 100644 --- a/components/proximity_auth/ble/bluetooth_low_energy_connection.h +++ b/components/proximity_auth/ble/bluetooth_low_energy_connection.h @@ -90,7 +90,7 @@ class BluetoothLowEnergyConnection : public Connection, ~BluetoothLowEnergyConnection() override; - // proximity_auth::Connection + // proximity_auth::Connection: void Connect() override; void Disconnect() override; @@ -106,12 +106,12 @@ class BluetoothLowEnergyConnection : public Connection, const BluetoothLowEnergyCharacteristicsFinder::ErrorCallback& error_callback); - // proximity_auth::Connection + // proximity_auth::Connection: void SendMessageImpl(scoped_ptr<WireMessage> message) override; scoped_ptr<WireMessage> DeserializeWireMessage( bool* is_incomplete_message) override; - // device::BluetoothAdapter::Observer + // device::BluetoothAdapter::Observer: void DeviceChanged(device::BluetoothAdapter* adapter, device::BluetoothDevice* device) override; void DeviceRemoved(device::BluetoothAdapter* adapter, @@ -189,15 +189,13 @@ class BluetoothLowEnergyConnection : public Connection, // accordingly the following flow: // 1) |request| is enqueued; // 2) |request| will be processed by ProcessNextWriteRequest() when there is - // no pending - // write request; + // no pending write request; // 3) |request| will be dequeued when it's successfully processed // (OnRemoteCharacteristicWritten()); // 4) |request| is not dequeued if it fails - // (OnWriteRemoteCharacteristicError()), - // it remains on the queue and will be retried. - // |request| will remain on the queue until it succeeds or it triggers a - // Disconnect() call (after |max_number_of_tries_|). + // (OnWriteRemoteCharacteristicError()), it remains on the queue and will be + // retried. |request| will remain on the queue until it succeeds or it + // triggers a Disconnect() call (after |max_number_of_tries_|). void WriteRemoteCharacteristic(WriteRequest request); // Processes the next request in |write_requests_queue_|. diff --git a/components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc b/components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc index 005b22d..93858e0 100644 --- a/components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc +++ b/components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc @@ -77,19 +77,50 @@ void BluetoothLowEnergyConnectionFinder::Find( weak_ptr_factory_.GetWeakPtr())); } +// It's not necessary to observe |AdapterPresentChanged| too. When |adapter_| is +// present, but not powered, it's not possible to scan for new devices. +void BluetoothLowEnergyConnectionFinder::AdapterPoweredChanged( + BluetoothAdapter* adapter, + bool powered) { + DCHECK_EQ(adapter_.get(), adapter); + VLOG(1) << "Adapter powered: " << powered; + + // Important: do not rely on |adapter->IsDiscoverying()| to verify if there is + // an active discovery session. We need to create our own with an specific + // filter. + if (powered && (!discovery_session_ || !discovery_session_->IsActive())) + StartDiscoverySession(); +} + void BluetoothLowEnergyConnectionFinder::DeviceAdded(BluetoothAdapter* adapter, BluetoothDevice* device) { + DCHECK_EQ(adapter_.get(), adapter); DCHECK(device); VLOG(1) << "Device added: " << device->GetAddress(); - HandleDeviceUpdated(device); + + // Note: Only consider |device| when it was actually added/updated during a + // scanning, otherwise the device is stale and the GATT connection will fail. + // For instance, when |adapter_| change status from unpowered to powered, + // |DeviceAdded| is called for each paired |device|. + if (adapter_->IsPowered() && discovery_session_ && + discovery_session_->IsActive()) + HandleDeviceUpdated(device); } void BluetoothLowEnergyConnectionFinder::DeviceChanged( BluetoothAdapter* adapter, BluetoothDevice* device) { + DCHECK_EQ(adapter_.get(), adapter); DCHECK(device); VLOG(1) << "Device changed: " << device->GetAddress(); - HandleDeviceUpdated(device); + + // Note: Only consider |device| when it was actually added/updated during a + // scanning, otherwise the device is stale and the GATT connection will fail. + // For instance, when |adapter_| change status from unpowered to powered, + // |DeviceAdded| is called for each paired |device|. + if (adapter_->IsPowered() && discovery_session_ && + discovery_session_->IsActive()) + HandleDeviceUpdated(device); } void BluetoothLowEnergyConnectionFinder::HandleDeviceUpdated( @@ -223,6 +254,13 @@ void BluetoothLowEnergyConnectionFinder::OnCreateGattConnectionError( BluetoothDevice::ConnectErrorCode error_code) { VLOG(1) << "Error creating connection to device " << device_address << " : error code = " << error_code; + + BluetoothDevice* device = GetDevice(device_address); + const auto& i = pending_connections_.find(device); + if (i != pending_connections_.end()) { + VLOG(1) << "Remove pending connection to " << device->GetAddress(); + pending_connections_.erase(i); + } } void BluetoothLowEnergyConnectionFinder::OnGattConnectionCreated( @@ -315,26 +353,13 @@ void BluetoothLowEnergyConnectionFinder::OnConnectionStatusChanged( } void BluetoothLowEnergyConnectionFinder::RestartDiscoverySessionWhenReady() { - // It's not possible to simply use - // |adapter_->GetDevice(GetRemoteDeviceAddress())| to find the device with MAC - // address |GetRemoteDeviceAddress()|. For paired devices, - // BluetoothAdapter::GetDevice(XXX) searches for the temporary MAC address - // XXX, whereas |remote_device_.bluetooth_address| is the real MAC address. - // This is a bug in the way device::BluetoothAdapter is storing the devices - // (see crbug.com/497841). - BluetoothDevice* device = nullptr; - std::vector<BluetoothDevice*> devices = adapter_->GetDevices(); - for (const auto& dev : devices) { - if (dev->GetAddress() == remote_device_.bluetooth_address) - device = dev; - } - // To restart scanning for devices, it's necessary to ensure that: // (i) the GATT connection to |remove_device_| is closed; // (ii) there is no pending call to // |device::BluetoothDiscoverySession::Stop()|. // The second condition is satisfied when |OnDiscoveryStopped| is called and // |discovery_session_| is reset. + BluetoothDevice* device = GetDevice(remote_device_.bluetooth_address); if ((!device || !device->IsConnected()) && !discovery_session_) { DCHECK(!gatt_connection_); connection_.reset(); @@ -353,4 +378,21 @@ void BluetoothLowEnergyConnectionFinder::SetDelayForTesting( delay_after_gatt_connection_ = delay; } +BluetoothDevice* BluetoothLowEnergyConnectionFinder::GetDevice( + std::string device_address) { + // It's not possible to simply use + // |adapter_->GetDevice(GetRemoteDeviceAddress())| to find the device with MAC + // address |GetRemoteDeviceAddress()|. For paired devices, + // BluetoothAdapter::GetDevice(XXX) searches for the temporary MAC address + // XXX, whereas |remote_device_.bluetooth_address| is the real MAC address. + // This is a bug in the way device::BluetoothAdapter is storing the devices + // (see crbug.com/497841). + std::vector<BluetoothDevice*> devices = adapter_->GetDevices(); + for (const auto& device : devices) { + if (device->GetAddress() == device_address) + return device; + } + return nullptr; +} + } // namespace proximity_auth diff --git a/components/proximity_auth/ble/bluetooth_low_energy_connection_finder.h b/components/proximity_auth/ble/bluetooth_low_energy_connection_finder.h index 7b727b3..e639246 100644 --- a/components/proximity_auth/ble/bluetooth_low_energy_connection_finder.h +++ b/components/proximity_auth/ble/bluetooth_low_energy_connection_finder.h @@ -51,6 +51,8 @@ class BluetoothLowEnergyConnectionFinder Connection::Status new_status) override; // device::BluetoothAdapter::Observer: + void AdapterPoweredChanged(device::BluetoothAdapter* adapter, + bool powered) override; void DeviceAdded(device::BluetoothAdapter* adapter, device::BluetoothDevice* device) override; void DeviceChanged(device::BluetoothAdapter* adapter, @@ -117,6 +119,9 @@ class BluetoothLowEnergyConnectionFinder // Restarts the discovery session after creating |connection_| fails. void RestartDiscoverySessionWhenReady(); + // Returns the device with |device_address|. + device::BluetoothDevice* GetDevice(std::string device_address); + // The uuid of the service it looks for to establish a GattConnection. device::BluetoothUUID remote_service_uuid_; diff --git a/components/proximity_auth/ble/bluetooth_low_energy_connection_finder_unittest.cc b/components/proximity_auth/ble/bluetooth_low_energy_connection_finder_unittest.cc index 4465290..21c2039 100644 --- a/components/proximity_auth/ble/bluetooth_low_energy_connection_finder_unittest.cc +++ b/components/proximity_auth/ble/bluetooth_low_energy_connection_finder_unittest.cc @@ -118,7 +118,8 @@ class ProximityAuthBluetoothLowEnergyConnectionFinderTest kDeviceName, kBluetoothAddress, false, - false)) { + false)), + last_discovery_session_alias_(nullptr) { device::BluetoothAdapterFactory::SetAdapterForTesting(adapter_); ON_CALL(*adapter_, IsPresent()).WillByDefault(Return(true)); @@ -134,23 +135,22 @@ class ProximityAuthBluetoothLowEnergyConnectionFinderTest device::BluetoothAdapter::DiscoverySessionCallback discovery_callback; scoped_ptr<device::MockBluetoothDiscoverySession> discovery_session( new NiceMock<device::MockBluetoothDiscoverySession>()); - device::MockBluetoothDiscoverySession* discovery_session_alias = - discovery_session.get(); + last_discovery_session_alias_ = discovery_session.get(); // Starting a discovery session. StartDiscoveryWithFilterRaw is a proxy for // StartDiscoveryWithFilter. EXPECT_CALL(*adapter_, StartDiscoverySessionWithFilterRaw(_, _, _)) .WillOnce(SaveArg<1>(&discovery_callback)); EXPECT_CALL(*adapter_, AddObserver(_)); - ON_CALL(*discovery_session_alias, IsActive()).WillByDefault(Return(true)); + ON_CALL(*last_discovery_session_alias_, IsActive()) + .WillByDefault(Return(true)); connection_finder.Find(connection_callback_); ASSERT_FALSE(discovery_callback.is_null()); discovery_callback.Run(discovery_session.Pass()); + } - // Cleaning up - EXPECT_CALL(*discovery_session_alias, Stop(_, _)) - .Times(AtLeast(1)) - .WillRepeatedly(SaveArg<0>(&last_stop_discovery_session_callback_)); + void ExpectStopDiscoveryAndRemoveObserver() { + EXPECT_CALL(*last_discovery_session_alias_, Stop(_, _)).Times(AtLeast(1)); EXPECT_CALL(*adapter_, RemoveObserver(_)).Times(AtLeast(1)); } @@ -174,14 +174,15 @@ class ProximityAuthBluetoothLowEnergyConnectionFinderTest // Prepare expectations to add/change a wrong device. void PrepareForNewWrongDevice(const std::string& uuid) { PrepareDevice(uuid); + ON_CALL(*device_, IsPaired()).WillByDefault(Return(true)); EXPECT_CALL(*device_, CreateGattConnection(_, _)).Times(0); } - base::Closure last_stop_discovery_session_callback_; scoped_refptr<device::MockBluetoothAdapter> adapter_; ConnectionFinder::ConnectionCallback connection_callback_; scoped_ptr<device::MockBluetoothDevice> device_; scoped_ptr<Connection> last_found_connection_; + device::MockBluetoothDiscoverySession* last_discovery_session_alias_; private: base::MessageLoop message_loop_; @@ -239,6 +240,7 @@ TEST_F(ProximityAuthBluetoothLowEnergyConnectionFinderTest, kMaxNumberOfAttempts); device::BluetoothDevice::GattConnectionCallback gatt_connection_callback; FindAndExpectStartDiscovery(connection_finder); + ExpectStopDiscoveryAndRemoveObserver(); PrepareForNewRightDevice(kServiceUUID, gatt_connection_callback); connection_finder.DeviceAdded(adapter_.get(), device_.get()); @@ -251,6 +253,7 @@ TEST_F(ProximityAuthBluetoothLowEnergyConnectionFinderTest, kServiceUUID, kToPeripheralCharUUID, kFromPeripheralCharUUID, kMaxNumberOfAttempts); FindAndExpectStartDiscovery(connection_finder); + ExpectStopDiscoveryAndRemoveObserver(); PrepareForNewWrongDevice(kOtherUUID); connection_finder.DeviceAdded(adapter_.get(), device_.get()); @@ -263,6 +266,7 @@ TEST_F(ProximityAuthBluetoothLowEnergyConnectionFinderTest, kMaxNumberOfAttempts); device::BluetoothDevice::GattConnectionCallback gatt_connection_callback; FindAndExpectStartDiscovery(connection_finder); + ExpectStopDiscoveryAndRemoveObserver(); PrepareForNewRightDevice(kServiceUUID, gatt_connection_callback); connection_finder.DeviceChanged(adapter_.get(), device_.get()); @@ -275,6 +279,7 @@ TEST_F(ProximityAuthBluetoothLowEnergyConnectionFinderTest, kServiceUUID, kToPeripheralCharUUID, kFromPeripheralCharUUID, kMaxNumberOfAttempts); FindAndExpectStartDiscovery(connection_finder); + ExpectStopDiscoveryAndRemoveObserver(); PrepareForNewWrongDevice(kOtherUUID); connection_finder.DeviceChanged(adapter_.get(), device_.get()); @@ -284,6 +289,7 @@ TEST_F(ProximityAuthBluetoothLowEnergyConnectionFinderTest, Find_CreatesTwoGattConnections) { StrictMock<MockBluetoothLowEnergyConnectionFinder> connection_finder; FindAndExpectStartDiscovery(connection_finder); + ExpectStopDiscoveryAndRemoveObserver(); connection_finder.ExpectCreateConnection(); // Prepare to add |device_|. @@ -314,7 +320,7 @@ TEST_F(ProximityAuthBluetoothLowEnergyConnectionFinderTest, new NiceMock<device::MockBluetoothGattConnection>(kBluetoothAddress))); run_loop.RunUntilIdle(); - // The second device should be forgetten + // The second device should be forgetten. EXPECT_CALL(*adapter_, GetDevice(std::string(kOtherBluetoothAddress))) .WillOnce(Return(&other_device)); EXPECT_CALL(other_device, Disconnect(_, _)); @@ -327,15 +333,16 @@ TEST_F(ProximityAuthBluetoothLowEnergyConnectionFinderTest, Find_ConnectionSucceeds) { StrictMock<MockBluetoothLowEnergyConnectionFinder> connection_finder; - // Starting discovery + // Starting discovery. FindAndExpectStartDiscovery(connection_finder); + ExpectStopDiscoveryAndRemoveObserver(); - // Finding and creating a GATT connection to the right device + // Finding and creating a GATT connection to the right device. device::BluetoothDevice::GattConnectionCallback gatt_connection_callback; PrepareForNewRightDevice(kServiceUUID, gatt_connection_callback); connection_finder.DeviceAdded(adapter_.get(), device_.get()); - // Creating a connection + // Creating a connection. MockConnection* connection = connection_finder.ExpectCreateConnection(); ASSERT_FALSE(gatt_connection_callback.is_null()); base::RunLoop run_loop; @@ -352,16 +359,19 @@ TEST_F(ProximityAuthBluetoothLowEnergyConnectionFinderTest, Find_ConnectionFails_RestartDiscoveryAndConnectionSucceeds) { StrictMock<MockBluetoothLowEnergyConnectionFinder> connection_finder; - // Starting discovery + // Starting discovery. FindAndExpectStartDiscovery(connection_finder); + base::Closure stop_discovery_session_callback; + EXPECT_CALL(*last_discovery_session_alias_, Stop(_, _)) + .WillOnce(SaveArg<0>(&stop_discovery_session_callback)); - // Finding and creating a GATT connection to the right device + // Preparing to create a GATT connection to the right device. device::BluetoothDevice::GattConnectionCallback gatt_connection_callback; PrepareForNewRightDevice(kServiceUUID, gatt_connection_callback); - connection_finder.DeviceAdded(adapter_.get(), device_.get()); - - // Trying to create a connection MockConnection* connection = connection_finder.ExpectCreateConnection(); + + // Trying to create a connection. + connection_finder.DeviceAdded(adapter_.get(), device_.get()); ASSERT_FALSE(gatt_connection_callback.is_null()); base::RunLoop run_loop; gatt_connection_callback.Run(make_scoped_ptr( @@ -370,37 +380,103 @@ TEST_F(ProximityAuthBluetoothLowEnergyConnectionFinderTest, ASSERT_FALSE(last_found_connection_); connection->SetStatus(Connection::IN_PROGRESS); - ASSERT_FALSE(last_stop_discovery_session_callback_.is_null()); - last_stop_discovery_session_callback_.Run(); + // Stopping the discovery session. + ASSERT_FALSE(stop_discovery_session_callback.is_null()); + stop_discovery_session_callback.Run(); - // Connection fails + // Preparing to restart the discovery session. device::BluetoothAdapter::DiscoverySessionCallback discovery_callback; std::vector<const device::BluetoothDevice*> devices; ON_CALL(*adapter_, GetDevices()).WillByDefault(Return(devices)); EXPECT_CALL(*adapter_, StartDiscoverySessionWithFilterRaw(_, _, _)) .WillOnce(SaveArg<1>(&discovery_callback)); + + // Connection fails. connection->SetStatus(Connection::DISCONNECTED); + // Restarting the discovery session. scoped_ptr<device::MockBluetoothDiscoverySession> discovery_session( new NiceMock<device::MockBluetoothDiscoverySession>()); + last_discovery_session_alias_ = discovery_session.get(); + ON_CALL(*last_discovery_session_alias_, IsActive()) + .WillByDefault(Return(true)); ASSERT_FALSE(discovery_callback.is_null()); discovery_callback.Run(discovery_session.Pass()); - // Finding and creating a GATT connection to the right device + // Preparing to create a GATT connection to the right device. PrepareForNewRightDevice(kServiceUUID, gatt_connection_callback); - connection_finder.DeviceAdded(adapter_.get(), device_.get()); - - // Creating a connection connection = connection_finder.ExpectCreateConnection(); + + // Trying to create a connection. + connection_finder.DeviceAdded(adapter_.get(), device_.get()); + EXPECT_CALL(*last_discovery_session_alias_, Stop(_, _)).Times(AtLeast(1)); ASSERT_FALSE(gatt_connection_callback.is_null()); base::RunLoop other_run_loop; gatt_connection_callback.Run(make_scoped_ptr( new NiceMock<device::MockBluetoothGattConnection>(kBluetoothAddress))); other_run_loop.RunUntilIdle(); + + // Completing the connection. EXPECT_FALSE(last_found_connection_); connection->SetStatus(Connection::IN_PROGRESS); connection->SetStatus(Connection::CONNECTED); EXPECT_TRUE(last_found_connection_); } +TEST_F(ProximityAuthBluetoothLowEnergyConnectionFinderTest, + Find_AdapterRemoved_RestartDiscoveryAndConnectionSucceeds) { + StrictMock<MockBluetoothLowEnergyConnectionFinder> connection_finder; + + // Starting discovery. + FindAndExpectStartDiscovery(connection_finder); + + // Removing the adapter. + ON_CALL(*adapter_, IsPresent()).WillByDefault(Return(false)); + ON_CALL(*adapter_, IsPowered()).WillByDefault(Return(false)); + ON_CALL(*last_discovery_session_alias_, IsActive()) + .WillByDefault(Return(false)); + connection_finder.AdapterPoweredChanged(adapter_.get(), false); + connection_finder.AdapterPresentChanged(adapter_.get(), false); + + // Adding the adapter. + ON_CALL(*adapter_, IsPresent()).WillByDefault(Return(true)); + ON_CALL(*adapter_, IsPowered()).WillByDefault(Return(true)); + + device::BluetoothAdapter::DiscoverySessionCallback discovery_callback; + scoped_ptr<device::MockBluetoothDiscoverySession> discovery_session( + new NiceMock<device::MockBluetoothDiscoverySession>()); + last_discovery_session_alias_ = discovery_session.get(); + + // Restarting the discovery session. + EXPECT_CALL(*adapter_, StartDiscoverySessionWithFilterRaw(_, _, _)) + .WillOnce(SaveArg<1>(&discovery_callback)); + connection_finder.AdapterPresentChanged(adapter_.get(), true); + connection_finder.AdapterPoweredChanged(adapter_.get(), true); + ON_CALL(*last_discovery_session_alias_, IsActive()) + .WillByDefault(Return(true)); + + ASSERT_FALSE(discovery_callback.is_null()); + discovery_callback.Run(discovery_session.Pass()); + + // Preparing to create a GATT connection to the right device. + device::BluetoothDevice::GattConnectionCallback gatt_connection_callback; + PrepareForNewRightDevice(kServiceUUID, gatt_connection_callback); + MockConnection* connection = connection_finder.ExpectCreateConnection(); + + // Trying to create a connection. + connection_finder.DeviceAdded(adapter_.get(), device_.get()); + EXPECT_CALL(*last_discovery_session_alias_, Stop(_, _)).Times(AtLeast(1)); + ASSERT_FALSE(gatt_connection_callback.is_null()); + base::RunLoop run_loop; + gatt_connection_callback.Run(make_scoped_ptr( + new NiceMock<device::MockBluetoothGattConnection>(kBluetoothAddress))); + run_loop.RunUntilIdle(); + + // Completing the connection. + ASSERT_FALSE(last_found_connection_); + connection->SetStatus(Connection::IN_PROGRESS); + connection->SetStatus(Connection::CONNECTED); + EXPECT_TRUE(last_found_connection_); +} + } // namespace proximity_auth |