Fixing a reconnection bug in ProximityAuthBleSystem

BluetoothLowEnergyConnectionFinder was unable to reconnect to a BLE
device immediatelly after (< 1s) after successfully disconnecting from
it.

This was caused by a race condition with |BluetoothDevice::Disconnect()|.
Basically, the last call to |BluetoothDevice::Disconnect()| was not
completed before the next |BluetoothDevice::CreateGattConnection()|
was done. So, |BluetoothDevice::Disconnect()| would complete after, and
interrupt the later call to |BluetoothDevice::CreateGattConnection()|.

The use of |BluetoothDevice::Disconnect()| was incorrect in this case,
as it effectively kills all connections to the given device. We should
use |BluetoothGattConnection::Disconnect()| instead, as it only kills
a given |BluetoothGattConnection| instance. This change solves the
problem.

BUG=506633

Review URL: https://codereview.chromium.org/1216413005

Cr-Commit-Position: refs/heads/master@{#337397}

6 files changed, 38 insertions, 35 deletions

diff --git a/components/proximity_auth/ble/bluetooth_low_energy_characteristics_finder.cc b/components/proximity_auth/ble/bluetooth_low_energy_characteristics_finder.cc
index 838ee6e..60d15c5 100644
--- a/components/proximity_auth/ble/bluetooth_low_energy_characteristics_finder.cc
+++ b/components/proximity_auth/ble/bluetooth_low_energy_characteristics_finder.cc
@@ -85,15 +85,22 @@ void BluetoothLowEnergyCharacteristicsFinder::GattDiscoveryCompleteForService(
 void BluetoothLowEnergyCharacteristicsFinder::ScanRemoteCharacteristics(
     BluetoothDevice* device,
     const BluetoothUUID& service_uuid) {
+  PA_LOG(INFO) << "Scanning remote characteristics.";
   if (device) {
     std::vector<BluetoothGattService*> services = device->GetGattServices();
     for (const auto& service : services) {
       if (service->GetUUID() == service_uuid) {
+        PA_LOG(INFO) << "Service " << service_uuid.canonical_value()
+                     << " found.";
         // Right service found, now scaning its characteristics.
         std::vector<device::BluetoothGattCharacteristic*> characteristics =
             service->GetCharacteristics();
-        for (const auto& characteristic : characteristics)
+        for (const auto& characteristic : characteristics) {
+          PA_LOG(INFO) << "Char: "
+                       << characteristic->GetUUID().canonical_value()
+                       << " found.";
           HandleCharacteristicUpdate(characteristic);
+        }
         break;
       }
     }
diff --git a/components/proximity_auth/ble/bluetooth_low_energy_connection.cc b/components/proximity_auth/ble/bluetooth_low_energy_connection.cc
index 42c7655..11b4a2f 100644
--- a/components/proximity_auth/ble/bluetooth_low_energy_connection.cc
+++ b/components/proximity_auth/ble/bluetooth_low_energy_connection.cc
@@ -53,7 +53,7 @@ BluetoothLowEnergyConnection::BluetoothLowEnergyConnection(
       remote_service_({remote_service_uuid, ""}),
       to_peripheral_char_({to_peripheral_char_uuid, ""}),
       from_peripheral_char_({from_peripheral_char_uuid, ""}),
-      connection_(gatt_connection.Pass()),
+      gatt_connection_(gatt_connection.Pass()),
       sub_status_(SubStatus::DISCONNECTED),
       receiving_bytes_(false),
       write_remote_characteristic_pending_(false),
@@ -76,8 +76,8 @@ BluetoothLowEnergyConnection::~BluetoothLowEnergyConnection() {
 }
 
 void BluetoothLowEnergyConnection::Connect() {
-  if (connection_ && connection_->IsConnected()) {
-    OnGattConnectionCreated(connection_.Pass());
+  if (gatt_connection_ && gatt_connection_->IsConnected()) {
+    OnGattConnectionCreated(gatt_connection_.Pass());
     return;
   }
 
@@ -97,17 +97,12 @@ void BluetoothLowEnergyConnection::Disconnect() {
     ClearWriteRequestsQueue();
     StopNotifySession();
     SetSubStatus(SubStatus::DISCONNECTED);
-    if (connection_) {
-      connection_.reset();
-      BluetoothDevice* device = GetRemoteDevice();
-      if (device) {
-        PA_LOG(INFO) << "Disconnect from device " << device->GetAddress();
-        device->Disconnect(
-            base::Bind(&BluetoothLowEnergyConnection::OnDisconnected,
-                       weak_ptr_factory_.GetWeakPtr()),
-            base::Bind(&BluetoothLowEnergyConnection::OnDisconnectError,
-                       weak_ptr_factory_.GetWeakPtr()));
-      }
+    if (gatt_connection_) {
+      PA_LOG(INFO) << "Disconnect from device "
+                   << gatt_connection_->GetDeviceAddress();
+
+      // Destroying BluetoothGattConnection also disconnects it.
+      gatt_connection_.reset();
     }
   }
 }
@@ -273,7 +268,7 @@ void BluetoothLowEnergyConnection::OnCreateGattConnectionError(
 
 void BluetoothLowEnergyConnection::OnGattConnectionCreated(
     scoped_ptr<device::BluetoothGattConnection> gatt_connection) {
-  connection_ = gatt_connection.Pass();
+  gatt_connection_ = gatt_connection.Pass();
   SetSubStatus(SubStatus::WAITING_CHARACTERISTICS);
   characteristic_finder_.reset(CreateCharacteristicsFinder(
       base::Bind(&BluetoothLowEnergyConnection::OnCharacteristicsFound,
diff --git a/components/proximity_auth/ble/bluetooth_low_energy_connection.h b/components/proximity_auth/ble/bluetooth_low_energy_connection.h
index 648b199..9a57608 100644
--- a/components/proximity_auth/ble/bluetooth_low_energy_connection.h
+++ b/components/proximity_auth/ble/bluetooth_low_energy_connection.h
@@ -244,7 +244,7 @@ class BluetoothLowEnergyConnection : public Connection,
   // The Bluetooth adapter over which the Bluetooth connection will be made.
   scoped_refptr<device::BluetoothAdapter> adapter_;
 
-  // Remote service the |connection_| was established with.
+  // Remote service the |gatt_connection_| was established with.
   RemoteAttribute remote_service_;
 
   // Characteristic used to send data to the remote device.
@@ -254,7 +254,7 @@ class BluetoothLowEnergyConnection : public Connection,
   RemoteAttribute from_peripheral_char_;
 
   // The GATT connection with the remote device.
-  scoped_ptr<device::BluetoothGattConnection> connection_;
+  scoped_ptr<device::BluetoothGattConnection> gatt_connection_;
 
   // The characteristics finder for remote device.
   scoped_ptr<BluetoothLowEnergyCharacteristicsFinder> characteristic_finder_;
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 c2e0991..37276e8 100644
--- a/components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc
+++ b/components/proximity_auth/ble/bluetooth_low_energy_connection_finder.cc
@@ -310,14 +310,9 @@ void BluetoothLowEnergyConnectionFinder::CreateGattConnection(
 void BluetoothLowEnergyConnectionFinder::CloseGattConnection(
     scoped_ptr<device::BluetoothGattConnection> gatt_connection) {
   DCHECK(gatt_connection);
-  std::string device_address = gatt_connection->GetDeviceAddress();
+
+  // Destroying the BluetoothGattConnection also disconnects it.
   gatt_connection.reset();
-  BluetoothDevice* device = adapter_->GetDevice(device_address);
-  if (device) {
-    PA_LOG(INFO) << "Disconnect from device " << device->GetAddress();
-    device->Disconnect(base::Bind(&base::DoNothing),
-                       base::Bind(&base::DoNothing));
-  }
 }
 
 scoped_ptr<Connection> BluetoothLowEnergyConnectionFinder::CreateConnection(
@@ -358,9 +353,8 @@ void BluetoothLowEnergyConnectionFinder::RestartDiscoverySessionWhenReady() {
   // |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_);
+  if ((!gatt_connection_ || !gatt_connection_->IsConnected()) &&
+      !discovery_session_) {
     connection_.reset();
     connected_ = false;
     StartDiscoverySession();
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 e639246..7d0310e 100644
--- a/components/proximity_auth/ble/bluetooth_low_energy_connection_finder.h
+++ b/components/proximity_auth/ble/bluetooth_low_energy_connection_finder.h
@@ -41,10 +41,6 @@ class BluetoothLowEnergyConnectionFinder
   // Finds a connection to the remote device. Only the first one is functional.
   void Find(const ConnectionCallback& connection_callback) override;
 
-  // Closes the connection and forgets the device.
-  void CloseGattConnection(
-      scoped_ptr<device::BluetoothGattConnection> gatt_connection);
-
   // proximity_auth::ConnectionObserver:
   void OnConnectionStatusChanged(Connection* connection,
                                  Connection::Status old_status,
@@ -61,6 +57,10 @@ class BluetoothLowEnergyConnectionFinder
                      device::BluetoothDevice* device) override;
 
  protected:
+  // Closes the GATT connection. Virtual for testing.
+  virtual void CloseGattConnection(
+      scoped_ptr<device::BluetoothGattConnection> gatt_connection);
+
   // Creates a proximity_auth::Connection based on |gatt_connection|. Exposed
   // for testing.
   virtual scoped_ptr<Connection> CreateConnection(
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 75981cc..86be60e 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
@@ -92,12 +92,21 @@ class MockBluetoothLowEnergyConnectionFinder
     return connection_alias;
   }
 
+  MOCK_METHOD0(CloseGattConnectionProxy, void(void));
+
  protected:
   scoped_ptr<Connection> CreateConnection(
       scoped_ptr<device::BluetoothGattConnection> gatt_connection) override {
     return make_scoped_ptr(CreateConnectionProxy());
   }
 
+  void CloseGattConnection(
+      scoped_ptr<device::BluetoothGattConnection> gatt_connection) override {
+    BluetoothLowEnergyConnectionFinder::CloseGattConnection(
+        gatt_connection.Pass());
+    CloseGattConnectionProxy();
+  }
+
  private:
   DISALLOW_COPY_AND_ASSIGN(MockBluetoothLowEnergyConnectionFinder);
 };
@@ -324,9 +333,7 @@ TEST_F(ProximityAuthBluetoothLowEnergyConnectionFinderTest,
   run_loop.RunUntilIdle();
 
   // The second device should be forgetten.
-  EXPECT_CALL(*adapter_, GetDevice(std::string(kOtherBluetoothAddress)))
-      .WillOnce(Return(&other_device));
-  EXPECT_CALL(other_device, Disconnect(_, _));
+  EXPECT_CALL(connection_finder, CloseGattConnectionProxy());
   other_gatt_connection_callback.Run(
       make_scoped_ptr(new NiceMock<device::MockBluetoothGattConnection>(
           kOtherBluetoothAddress)));