[Bluetooth][WinRT] Fix Gatt Disconnection

This change fixes BluetoothDeviceWinrt::DiscoonectGatt. It does so by
working around a quirk in the UWP APIs, that requires dropping all
internal references to a given device. See this GitHub issue for further
discussion: https://github.com/jasongin/noble-uwp/issues/20

Bug: 821766
Change-Id: I5bf0e4e0ce0e3b43d9cbb10e3b11e378de05e6b3
Reviewed-on: https://chromium-review.googlesource.com/1183234Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Commit-Queue: Jan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585799}

device/bluetooth/bluetooth_device_unittest.cc

@@ -1159,7 +1159,7 @@ TEST_F(BluetoothTest, MAYBE_DisconnectionNotifiesDeviceChanged) {
   EXPECT_TRUE(device->IsConnected());
   EXPECT_TRUE(device->IsGattConnected());
 
-  SimulateGattDisconnection(device);
+  SimulateDeviceBreaksConnection(device);
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(2, observer.device_changed_count());
   EXPECT_FALSE(device->IsConnected());
@@ -1859,7 +1859,7 @@ TEST_F(BluetoothTest, MAYBE_GattServicesDiscovered_AfterDisconnection) {
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(1, gatt_discovery_attempts_);
 
-  SimulateGattDisconnection(device);
+  SimulateDeviceBreaksConnection(device);
   base::RunLoop().RunUntilIdle();
 
   SimulateGattServicesDiscovered(

device/bluetooth/bluetooth_device_winrt.cc

@@ -450,7 +450,20 @@ void BluetoothDeviceWinrt::CreateGattConnectionImpl() {
 }
 
 void BluetoothDeviceWinrt::DisconnectGatt() {
+  // Closing the device and disposing of all references will trigger a Gatt
+  // Disconnection after a short timeout. Since the Gatt Services store a
+  // reference to |ble_device_| as well, we need to clear them to drop all
+  // remaining references, so that the OS disconnects.
+  // Reference:
+  // - https://docs.microsoft.com/en-us/windows/uwp/devices-sensors/gatt-client
   CloseDevice(ble_device_);
+  ClearGattServices();
+
+  // Stop any pending Gatt Discovery sessions and report an error. This will
+  // destroy |gatt_discoverer_| and release remaining references the discoverer
+  // might have hold.
+  if (gatt_discoverer_)
+    OnGattDiscoveryComplete(false);
 }
 
 HRESULT BluetoothDeviceWinrt::GetBluetoothLEDeviceStaticsActivationFactory(
@@ -517,9 +530,7 @@ void BluetoothDeviceWinrt::OnConnectionStatusChanged(
     DidConnectGatt();
   } else {
     gatt_discoverer_.reset();
-    gatt_services_.clear();
-    device_uuids_.ClearServiceUUIDs();
-    SetGattServicesDiscoveryComplete(false);
+    ClearGattServices();
     DidDisconnectGatt();
   }
 }
@@ -527,6 +538,8 @@ void BluetoothDeviceWinrt::OnConnectionStatusChanged(
 void BluetoothDeviceWinrt::OnGattServicesChanged(IBluetoothLEDevice* ble_device,
                                                  IInspectable* object) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  // Note: We don't clear out |gatt_services_| here, as we don't want to break
+  // existing references to Gatt Services that did not change.
   device_uuids_.ClearServiceUUIDs();
   SetGattServicesDiscoveryComplete(false);
   adapter_->NotifyDeviceChanged(this);
@@ -581,4 +594,10 @@ void BluetoothDeviceWinrt::OnGattDiscoveryComplete(bool success) {
   gatt_discoverer_.reset();
 }
 
+void BluetoothDeviceWinrt::ClearGattServices() {
+  gatt_services_.clear();
+  device_uuids_.ClearServiceUUIDs();
+  SetGattServicesDiscoveryComplete(false);
+}
+
 }  // namespace device

device/bluetooth/bluetooth_device_winrt.h

@@ -119,11 +119,7 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothDeviceWinrt : public BluetoothDevice {
 
   void OnGattDiscoveryComplete(bool success);
 
-  void OnPairingRequested(
-      ABI::Windows::Devices::Enumeration::IDeviceInformationCustomPairing*
-          custom_pairing,
-      ABI::Windows::Devices::Enumeration::IDevicePairingRequestedEventArgs*
-          event_args);
+  void ClearGattServices();
 
   uint64_t raw_address_;
   std::string address_;

device/bluetooth/bluetooth_remote_gatt_characteristic_unittest.cc

@@ -585,7 +585,7 @@ TEST_F(BluetoothRemoteGattCharacteristicTest,
 #endif
 
   ASSERT_EQ(1u, adapter_->GetDevices().size());
-  SimulateGattDisconnection(adapter_->GetDevices()[0]);
+  SimulateDeviceBreaksConnection(adapter_->GetDevices()[0]);
   base::RunLoop().RunUntilIdle();
 
   EXPECT_EQ(BluetoothRemoteGattService::GATT_ERROR_FAILED,
@@ -677,7 +677,7 @@ TEST_F(BluetoothRemoteGattCharacteristicTest,
 #endif  // defined(OS_ANDROID)
 
   ASSERT_EQ(1u, adapter_->GetDevices().size());
-  SimulateGattDisconnection(adapter_->GetDevices()[0]);
+  SimulateDeviceBreaksConnection(adapter_->GetDevices()[0]);
   base::RunLoop().RunUntilIdle();
 
   EXPECT_EQ(BluetoothRemoteGattService::GATT_ERROR_FAILED,
@@ -3321,7 +3321,7 @@ TEST_F(
   characteristic1_->WriteRemoteCharacteristic(
       std::vector<uint8_t>(), GetCallback(Call::NOT_EXPECTED),
       GetGattErrorCallback(Call::EXPECTED));
-  SimulateGattDisconnection(device_);
+  SimulateDeviceBreaksConnection(adapter_->GetDevices()[0]);
 
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(BluetoothRemoteGattService::GATT_ERROR_FAILED,

device/bluetooth/test/bluetooth_test.cc

@@ -110,6 +110,11 @@ BluetoothDevice* BluetoothTestBase::SimulateClassicDevice() {
   return nullptr;
 }
 
+void BluetoothTestBase::SimulateDeviceBreaksConnection(
+    BluetoothDevice* device) {
+  SimulateGattDisconnection(device);
+}
+
 bool BluetoothTestBase::SimulateLocalGattCharacteristicNotificationsRequest(
     BluetoothLocalGattCharacteristic* characteristic,
     bool start) {

device/bluetooth/test/bluetooth_test.h

@@ -272,6 +272,10 @@ class BluetoothTestBase : public testing::Test {
   // Simulates GattConnection disconnecting.
   virtual void SimulateGattDisconnection(BluetoothDevice* device) {}
 
+  // Simulates an event where the OS breaks the Gatt connection. Defaults to
+  // SimulateGattDisconnection(device).
+  virtual void SimulateDeviceBreaksConnection(BluetoothDevice* device);
+
   // Simulates success of discovering services. |uuids| is used to create a
   // service for each UUID string. Multiple UUIDs with the same value produce
   // multiple service instances.

device/bluetooth/test/bluetooth_test_win.cc

@@ -718,6 +718,14 @@ void BluetoothTestWinrt::SimulateGattDisconnection(BluetoothDevice* device) {
   ble_device->SimulateGattDisconnection();
 }
 
+void BluetoothTestWinrt::SimulateDeviceBreaksConnection(
+    BluetoothDevice* device) {
+  auto* const ble_device =
+      static_cast<TestBluetoothDeviceWinrt*>(device)->ble_device();
+  DCHECK(ble_device);
+  ble_device->SimulateDeviceBreaksConnection();
+}
+
 void BluetoothTestWinrt::SimulateGattServicesDiscovered(
     BluetoothDevice* device,
     const std::vector<std::string>& uuids) {

device/bluetooth/test/bluetooth_test_win.h

@@ -130,6 +130,7 @@ class BluetoothTestWinrt : public BluetoothTestWin,
       BluetoothDevice* device,
       BluetoothDevice::ConnectErrorCode error_code) override;
   void SimulateGattDisconnection(BluetoothDevice* device) override;
+  void SimulateDeviceBreaksConnection(BluetoothDevice* device) override;
   void SimulateGattServicesDiscovered(
       BluetoothDevice* device,
       const std::vector<std::string>& uuids) override;

device/bluetooth/test/fake_bluetooth_le_device_winrt.cc

@@ -183,10 +183,20 @@ HRESULT FakeBluetoothLEDeviceWinrt::GetGattServicesForUuidWithCacheModeAsync(
 }
 
 HRESULT FakeBluetoothLEDeviceWinrt::Close() {
+  --reference_count_;
+  fake_services_.clear();
   bluetooth_test_winrt_->OnFakeBluetoothGattDisconnect();
   return S_OK;
 }
 
+void FakeBluetoothLEDeviceWinrt::AddReference() {
+  ++reference_count_;
+}
+
+void FakeBluetoothLEDeviceWinrt::RemoveReference() {
+  --reference_count_;
+}
+
 void FakeBluetoothLEDeviceWinrt::SimulateDevicePaired(bool is_paired) {
   device_information_ = Make<FakeDeviceInformationWinrt>(
       Make<FakeDeviceInformationPairingWinrt>(is_paired));
@@ -221,6 +231,24 @@ void FakeBluetoothLEDeviceWinrt::SimulateGattDisconnection() {
     return;
   }
 
+  // Simulate production UWP behavior that only really disconnects once all
+  // references to a device are dropped.
+  if (reference_count_ == 0u) {
+    status_ = BluetoothConnectionStatus_Disconnected;
+    connection_status_changed_handler_->Invoke(this, nullptr);
+  }
+}
+
+void FakeBluetoothLEDeviceWinrt::SimulateDeviceBreaksConnection() {
+  if (status_ == BluetoothConnectionStatus_Disconnected) {
+    DCHECK(gatt_services_callback_);
+    std::move(gatt_services_callback_)
+        .Run(Make<FakeGattDeviceServicesResultWinrt>(
+            GattCommunicationStatus_Unreachable));
+    return;
+  }
+
+  // Simulate a Gatt Disconnecion regardless of the reference count.
   status_ = BluetoothConnectionStatus_Disconnected;
   connection_status_changed_handler_->Invoke(this, nullptr);
 }
@@ -231,8 +259,9 @@ void FakeBluetoothLEDeviceWinrt::SimulateGattServicesDiscovered(
     // Attribute handles need to be unique for a given BLE device. Increasing by
     // a large number ensures enough address space for the contained
     // characteristics and descriptors.
-    fake_services_.push_back(Make<FakeGattDeviceServiceWinrt>(
-        bluetooth_test_winrt_, uuid, service_attribute_handle_ += 0x0400));
+    fake_services_.push_back(
+        Make<FakeGattDeviceServiceWinrt>(bluetooth_test_winrt_, this, uuid,
+                                         service_attribute_handle_ += 0x0400));
   }
 
   DCHECK(gatt_services_callback_);

device/bluetooth/test/fake_bluetooth_le_device_winrt.h

@@ -114,12 +114,19 @@ class FakeBluetoothLEDeviceWinrt
   // IClosable:
   IFACEMETHODIMP Close() override;
 
+  // We perform explicit reference counting, to be able to query the ref count
+  // ourselves. This is required to simulate Gatt disconnection behavior
+  // exhibited by the production UWP APIs.
+  void AddReference();
+  void RemoveReference();
+
   void SimulateDevicePaired(bool is_paired);
   void SimulatePairingPinCode(std::string pin_code);
   void SimulateGattConnection();
   void SimulateGattConnectionError(
       BluetoothDevice::ConnectErrorCode error_code);
   void SimulateGattDisconnection();
+  void SimulateDeviceBreaksConnection();
   void SimulateGattServicesDiscovered(const std::vector<std::string>& uuids);
   void SimulateGattServicesChanged();
   void SimulateGattServiceRemoved(BluetoothRemoteGattService* service);
@@ -132,6 +139,7 @@ class FakeBluetoothLEDeviceWinrt
 
  private:
   BluetoothTestWinrt* bluetooth_test_winrt_ = nullptr;
+  uint32_t reference_count_ = 1u;
 
   ABI::Windows::Devices::Bluetooth::BluetoothConnectionStatus status_ =
       ABI::Windows::Devices::Bluetooth::BluetoothConnectionStatus_Disconnected;

device/bluetooth/test/fake_gatt_device_service_winrt.cc

@@ -14,6 +14,7 @@
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/win/async_operation.h"
 #include "device/bluetooth/bluetooth_uuid.h"
+#include "device/bluetooth/test/fake_bluetooth_le_device_winrt.h"
 #include "device/bluetooth/test/fake_gatt_characteristic_winrt.h"
 #include "device/bluetooth/test/fake_gatt_characteristics_result_winrt.h"
 
@@ -38,20 +39,27 @@ using ABI::Windows::Devices::Enumeration::DeviceAccessStatus;
 using ABI::Windows::Devices::Enumeration::IDeviceAccessInformation;
 using ABI::Windows::Foundation::Collections::IVectorView;
 using ABI::Windows::Foundation::IAsyncOperation;
+using Microsoft::WRL::ComPtr;
 using Microsoft::WRL::Make;
 
 }  // namespace
 
 FakeGattDeviceServiceWinrt::FakeGattDeviceServiceWinrt(
     BluetoothTestWinrt* bluetooth_test_winrt,
+    ComPtr<FakeBluetoothLEDeviceWinrt> fake_device,
     base::StringPiece uuid,
     uint16_t attribute_handle)
     : bluetooth_test_winrt_(bluetooth_test_winrt),
+      fake_device_(std::move(fake_device)),
       uuid_(BluetoothUUID::GetCanonicalValueAsGUID(uuid)),
       attribute_handle_(attribute_handle),
-      characteristic_attribute_handle_(attribute_handle_) {}
+      characteristic_attribute_handle_(attribute_handle_) {
+  fake_device_->AddReference();
+}
 
-FakeGattDeviceServiceWinrt::~FakeGattDeviceServiceWinrt() = default;
+FakeGattDeviceServiceWinrt::~FakeGattDeviceServiceWinrt() {
+  fake_device_->RemoveReference();
+}
 
 HRESULT FakeGattDeviceServiceWinrt::GetCharacteristics(
     GUID characteristic_uuid,

device/bluetooth/test/fake_gatt_device_service_winrt.h

@@ -19,6 +19,7 @@
 namespace device {
 
 class BluetoothTestWinrt;
+class FakeBluetoothLEDeviceWinrt;
 class FakeGattCharacteristicWinrt;
 
 class FakeGattDeviceServiceWinrt
@@ -30,9 +31,11 @@ class FakeGattDeviceServiceWinrt
           ABI::Windows::Devices::Bluetooth::GenericAttributeProfile::
               IGattDeviceService3> {
  public:
-  FakeGattDeviceServiceWinrt(BluetoothTestWinrt* bluetooth_test_winrt,
-                             base::StringPiece uuid,
-                             uint16_t attribute_handle);
+  FakeGattDeviceServiceWinrt(
+      BluetoothTestWinrt* bluetooth_test_winrt,
+      Microsoft::WRL::ComPtr<FakeBluetoothLEDeviceWinrt> fake_device,
+      base::StringPiece uuid,
+      uint16_t attribute_handle);
   ~FakeGattDeviceServiceWinrt() override;
 
   // IGattDeviceService:
@@ -115,6 +118,7 @@ class FakeGattDeviceServiceWinrt
 
  private:
   BluetoothTestWinrt* bluetooth_test_winrt_;
+  Microsoft::WRL::ComPtr<FakeBluetoothLEDeviceWinrt> fake_device_;
   GUID uuid_;
   uint16_t attribute_handle_;