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}

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