[CrOS MultiDevice] Fix BLE scan result handling.

Previously, we would attempt a BLE scan any time that we got a
DeviceChanged() or DeviceAdded() callback. However, the DeviceChanged()
callback was invoked when a device was in the process of disconnecting,
meaning that we would see a disconnecting device, then try to start a
connection immediately. This resulted in spurious connection requests
which could sometime cause a deadlock in underlying Bluetooth code.
Additionally, we would rely on the DeviceChanged() and DeviceRemoved()
callbacks to listen for Bluetooth disconnections, which also resulted in
error-prone disconnections.

This CL updates both call sites to use the updated
DeviceAdvertisementReceived() and DeviceConnectedStateChanged()
callbacks instead.

Bug: 898334
Change-Id: I743e51e6e47194649ab6852966b9d892c3ef334d
Reviewed-on: https://chromium-review.googlesource.com/c/1359478
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613358}

chromeos/services/secure_channel/ble_scanner_impl.cc

@@ -88,14 +88,11 @@ void BleScannerImpl::HandleScanFilterChange() {
   UpdateDiscoveryStatus();
 }
 
-void BleScannerImpl::DeviceAdded(device::BluetoothAdapter* adapter,
-                                 device::BluetoothDevice* bluetooth_device) {
-  DCHECK_EQ(adapter_.get(), adapter);
-  HandleDeviceUpdated(bluetooth_device);
-}
-
-void BleScannerImpl::DeviceChanged(device::BluetoothAdapter* adapter,
-                                   device::BluetoothDevice* bluetooth_device) {
+void BleScannerImpl::DeviceAdvertisementReceived(
+    device::BluetoothAdapter* adapter,
+    device::BluetoothDevice* bluetooth_device,
+    int16_t rssi,
+    const std::vector<uint8_t>& eir) {
   DCHECK_EQ(adapter_.get(), adapter);
   HandleDeviceUpdated(bluetooth_device);
 }

chromeos/services/secure_channel/ble_scanner_impl.h

@@ -69,10 +69,10 @@ class BleScannerImpl : public BleScanner,
   void HandleScanFilterChange() override;
 
   // device::BluetoothAdapter::Observer:
-  void DeviceAdded(device::BluetoothAdapter* adapter,
-                   device::BluetoothDevice* bluetooth_device) override;
-  void DeviceChanged(device::BluetoothAdapter* adapter,
-                     device::BluetoothDevice* bluetooth_device) override;
+  void DeviceAdvertisementReceived(device::BluetoothAdapter* adapter,
+                                   device::BluetoothDevice* device,
+                                   int16_t rssi,
+                                   const std::vector<uint8_t>& eir) override;
 
   void UpdateDiscoveryStatus();
   bool IsDiscoverySessionActive();

chromeos/services/secure_channel/ble_scanner_impl_unittest.cc

@@ -141,19 +141,17 @@ class SecureChannelBleScannerImplTest : public testing::Test {
   }
 
   void ProcessScanResultAndVerifyNoDeviceIdentified(
-      const std::string& service_data,
-      bool is_new_device) {
+      const std::string& service_data) {
     const FakeBleScannerDelegate::ScannedResultList& results =
         fake_delegate_->handled_scan_results();
 
     size_t num_results_before_call = results.size();
-    SimulateScanResult(service_data, is_new_device);
+    SimulateScanResult(service_data);
     EXPECT_EQ(num_results_before_call, results.size());
   }
 
   void ProcessScanResultAndVerifyDevice(
       const std::string& service_data,
-      bool is_new_device,
       multidevice::RemoteDeviceRef expected_remote_device,
       bool is_background_advertisement) {
     const FakeBleScannerDelegate::ScannedResultList& results =
@@ -164,7 +162,7 @@ class SecureChannelBleScannerImplTest : public testing::Test {
 
     size_t num_results_before_call = results.size();
     FakeBluetoothDevice* fake_bluetooth_device =
-        SimulateScanResult(service_data, is_new_device);
+        SimulateScanResult(service_data);
     EXPECT_EQ(num_results_before_call + 1u, results.size());
 
     EXPECT_EQ(expected_remote_device, std::get<0>(results.back()));
@@ -216,11 +214,10 @@ class SecureChannelBleScannerImplTest : public testing::Test {
   }
 
  private:
-  // Scan results come in as the result of either a new device or a change on an
-  // existing device. If |is_new_device| is true, a new device change will be
-  // simulated; otherwise, an existing device change will be simulated.
-  FakeBluetoothDevice* SimulateScanResult(const std::string& service_data,
-                                          bool is_new_device) {
+  FakeBluetoothDevice* SimulateScanResult(const std::string& service_data) {
+    static const int16_t kFakeRssi = -70;
+    static const std::vector<uint8_t> kFakeEir;
+
     // Scan result should not be received if there is no active discovery
     // session.
     EXPECT_TRUE(fake_discovery_session_);
@@ -228,17 +225,12 @@ class SecureChannelBleScannerImplTest : public testing::Test {
     auto fake_bluetooth_device = std::make_unique<FakeBluetoothDevice>(
         service_data, mock_adapter_.get());
 
-    BleScannerImpl* ble_scanner_derived =
-        static_cast<BleScannerImpl*>(ble_scanner_.get());
-
     // Note: MockBluetoothAdapter provides no way to notify observers, so the
     // observer callback must be invoked directly.
-    if (is_new_device) {
-      ble_scanner_derived->DeviceAdded(mock_adapter_.get(),
-                                       fake_bluetooth_device.get());
-    } else {
-      ble_scanner_derived->DeviceChanged(mock_adapter_.get(),
-                                         fake_bluetooth_device.get());
+    for (auto& observer : mock_adapter_->GetObservers()) {
+      observer.DeviceAdvertisementReceived(mock_adapter_.get(),
+                                           fake_bluetooth_device.get(),
+                                           kFakeRssi, kFakeEir);
     }
 
     return fake_bluetooth_device.get();
@@ -271,10 +263,7 @@ TEST_F(SecureChannelBleScannerImplTest, UnrelatedScanResults) {
   InvokeStartDiscoveryCallback(true /* success */, 0u /* command_index */);
   EXPECT_TRUE(fake_discovery_session());
 
-  ProcessScanResultAndVerifyNoDeviceIdentified("unrelatedServiceData",
-                                               true /* is_new_device */);
-  ProcessScanResultAndVerifyNoDeviceIdentified("unrelatedServiceData",
-                                               false /* is_new_device */);
+  ProcessScanResultAndVerifyNoDeviceIdentified("unrelatedServiceData");
 
   RemoveScanFilter(filter);
   InvokeStopDiscoveryCallback(true /* success */, 1u /* command_index */);
@@ -296,17 +285,14 @@ TEST_F(SecureChannelBleScannerImplTest, IncorrectRole) {
       "wrongRoleServiceData", test_devices()[0],
       false /* is_background_advertisement */);
 
-  ProcessScanResultAndVerifyNoDeviceIdentified("wrongRoleServiceData",
-                                               true /* is_new_device */);
-  ProcessScanResultAndVerifyNoDeviceIdentified("wrongRoleServiceData",
-                                               false /* is_new_device */);
+  ProcessScanResultAndVerifyNoDeviceIdentified("wrongRoleServiceData");
 
   RemoveScanFilter(filter);
   InvokeStopDiscoveryCallback(true /* success */, 1u /* command_index */);
   EXPECT_FALSE(fake_discovery_session());
 }
 
-TEST_F(SecureChannelBleScannerImplTest, IdentifyDevice_NewDevice_Background) {
+TEST_F(SecureChannelBleScannerImplTest, IdentifyDevice_Background) {
   BleScanner::ScanFilter filter(DeviceIdPair(test_devices()[0].GetDeviceId(),
                                              test_devices()[1].GetDeviceId()),
                                 ConnectionRole::kListenerRole);
@@ -315,9 +301,7 @@ TEST_F(SecureChannelBleScannerImplTest, IdentifyDevice_NewDevice_Background) {
   InvokeStartDiscoveryCallback(true /* success */, 0u /* command_index */);
   EXPECT_TRUE(fake_discovery_session());
 
-  // is_new_device == true, is_background_advertisement == true
-  ProcessScanResultAndVerifyDevice("device0ServiceData",
-                                   true /* is_new_device */, test_devices()[0],
+  ProcessScanResultAndVerifyDevice("device0ServiceData", test_devices()[0],
                                    true /* is_background_advertisement */);
 
   RemoveScanFilter(filter);
@@ -325,8 +309,7 @@ TEST_F(SecureChannelBleScannerImplTest, IdentifyDevice_NewDevice_Background) {
   EXPECT_FALSE(fake_discovery_session());
 }
 
-TEST_F(SecureChannelBleScannerImplTest,
-       IdentifyDevice_ExistingDevice_Foreground) {
+TEST_F(SecureChannelBleScannerImplTest, IdentifyDevice_Foreground) {
   BleScanner::ScanFilter filter(DeviceIdPair(test_devices()[0].GetDeviceId(),
                                              test_devices()[1].GetDeviceId()),
                                 ConnectionRole::kInitiatorRole);
@@ -335,9 +318,7 @@ TEST_F(SecureChannelBleScannerImplTest,
   InvokeStartDiscoveryCallback(true /* success */, 0u /* command_index */);
   EXPECT_TRUE(fake_discovery_session());
 
-  // is_new_device == false, is_background_advertisement == false
-  ProcessScanResultAndVerifyDevice("device0ServiceData",
-                                   false /* is_new_device */, test_devices()[0],
+  ProcessScanResultAndVerifyDevice("device0ServiceData", test_devices()[0],
                                    false /* is_background_advertisement */);
 
   RemoveScanFilter(filter);
@@ -359,8 +340,7 @@ TEST_F(SecureChannelBleScannerImplTest, IdentifyDevice_MultipleScans) {
   EXPECT_TRUE(fake_discovery_session());
 
   // Identify device 0.
-  ProcessScanResultAndVerifyDevice("device0ServiceData",
-                                   false /* is_new_device */, test_devices()[0],
+  ProcessScanResultAndVerifyDevice("device0ServiceData", test_devices()[0],
                                    false /* is_background_advertisement */);
 
   // Remove the identified device from the list of scan filters.
@@ -383,8 +363,7 @@ TEST_F(SecureChannelBleScannerImplTest, IdentifyDevice_MultipleScans) {
   EXPECT_TRUE(fake_discovery_session());
 
   // Identify device 2.
-  ProcessScanResultAndVerifyDevice("device2ServiceData",
-                                   false /* is_new_device */, test_devices()[2],
+  ProcessScanResultAndVerifyDevice("device2ServiceData", test_devices()[2],
                                    false /* is_background_advertisement */);
 
   // Remove the scan filter, and verify that the scan stopped.

components/cryptauth/ble/bluetooth_low_energy_weave_client_connection.cc

@@ -392,9 +392,10 @@ void BluetoothLowEnergyWeaveClientConnection::SendMessageImpl(
   ProcessNextWriteRequest();
 }
 
-void BluetoothLowEnergyWeaveClientConnection::DeviceChanged(
+void BluetoothLowEnergyWeaveClientConnection::DeviceConnectedStateChanged(
     device::BluetoothAdapter* adapter,
-    device::BluetoothDevice* device) {
+    device::BluetoothDevice* device,
+    bool is_now_connected) {
   // Ignore updates about other devices.
   if (device->GetAddress() != GetDeviceAddress())
     return;
@@ -408,7 +409,7 @@ void BluetoothLowEnergyWeaveClientConnection::DeviceChanged(
 
   // If a connection has already occurred and |device| is still connected, there
   // is nothing to do.
-  if (device->IsConnected())
+  if (is_now_connected)
     return;
 
   PA_LOG(WARNING) << "GATT connection to " << GetDeviceInfoLogString()
@@ -417,19 +418,6 @@ void BluetoothLowEnergyWeaveClientConnection::DeviceChanged(
                         BLE_WEAVE_CONNECTION_RESULT_ERROR_CONNECTION_DROPPED);
 }
 
-void BluetoothLowEnergyWeaveClientConnection::DeviceRemoved(
-    device::BluetoothAdapter* adapter,
-    device::BluetoothDevice* device) {
-  // Ignore updates about other devices.
-  if (device->GetAddress() != GetDeviceAddress())
-    return;
-
-  PA_LOG(WARNING) << "Device has been lost: " << GetDeviceInfoLogString()
-                  << ".";
-  DestroyConnection(
-      BleWeaveConnectionResult::BLE_WEAVE_CONNECTION_RESULT_ERROR_DEVICE_LOST);
-}
-
 void BluetoothLowEnergyWeaveClientConnection::GattCharacteristicValueChanged(
     device::BluetoothAdapter* adapter,
     device::BluetoothRemoteGattCharacteristic* characteristic,

components/cryptauth/ble/bluetooth_low_energy_weave_client_connection.h

@@ -152,10 +152,9 @@ class BluetoothLowEnergyWeaveClientConnection
   void OnDidSendMessage(const WireMessage& message, bool success) override;
 
   // device::BluetoothAdapter::Observer:
-  void DeviceChanged(device::BluetoothAdapter* adapter,
-                     device::BluetoothDevice* device) override;
-  void DeviceRemoved(device::BluetoothAdapter* adapter,
-                     device::BluetoothDevice* device) override;
+  void DeviceConnectedStateChanged(device::BluetoothAdapter* adapter,
+                                   device::BluetoothDevice* device,
+                                   bool is_now_connected) override;
   void GattCharacteristicValueChanged(
       device::BluetoothAdapter* adapter,
       device::BluetoothRemoteGattCharacteristic* characteristic,
@@ -175,6 +174,8 @@ class BluetoothLowEnergyWeaveClientConnection
                            ConnectSuccess);
   FRIEND_TEST_ALL_PREFIXES(CryptAuthBluetoothLowEnergyWeaveClientConnectionTest,
                            ConnectSuccessDisconnect);
+  FRIEND_TEST_ALL_PREFIXES(CryptAuthBluetoothLowEnergyWeaveClientConnectionTest,
+                           ConnectThenBluetoothDisconnects);
   FRIEND_TEST_ALL_PREFIXES(CryptAuthBluetoothLowEnergyWeaveClientConnectionTest,
                            DisconnectCalledTwice);
   FRIEND_TEST_ALL_PREFIXES(CryptAuthBluetoothLowEnergyWeaveClientConnectionTest,

components/cryptauth/ble/bluetooth_low_energy_weave_client_connection_unittest.cc

@@ -759,6 +759,22 @@ TEST_F(CryptAuthBluetoothLowEnergyWeaveClientConnectionTest,
           BLE_WEAVE_CONNECTION_RESULT_CLOSED_NORMALLY);
 }
 
+TEST_F(CryptAuthBluetoothLowEnergyWeaveClientConnectionTest,
+       ConnectThenBluetoothDisconnects) {
+  std::unique_ptr<TestBluetoothLowEnergyWeaveClientConnection> connection(
+      CreateConnection(true /* should_set_low_connection_latency */));
+  InitializeConnection(connection.get(), kDefaultMaxPacketSize);
+  EXPECT_EQ(connection->sub_status(), SubStatus::CONNECTED_AND_IDLE);
+
+  connection->DeviceConnectedStateChanged(adapter_.get(),
+                                          mock_bluetooth_device_.get(),
+                                          false /* is_now_connected */);
+
+  VerifyBleWeaveConnectionResult(
+      BluetoothLowEnergyWeaveClientConnection::BleWeaveConnectionResult::
+          BLE_WEAVE_CONNECTION_RESULT_ERROR_CONNECTION_DROPPED);
+}
+
 TEST_F(CryptAuthBluetoothLowEnergyWeaveClientConnectionTest,
        DisconnectCalledTwice) {
   std::unique_ptr<TestBluetoothLowEnergyWeaveClientConnection> connection(