[bluetooth/winrt] Ignore services that cannot be opened

It appears that in addition to returning "AccessDenied" when attempting
to enumerate GATT characteristics Windows can also return "AccessDenied"
when trying to open a service. This patch avoids failing service
discovery entirely by simply ignoring such inaccessible services.

Bug: 1058849
Change-Id: If4cf178bb30a12305cdadbc379922302c14671c9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2099762
Commit-Queue: Reilly Grant <reillyg@chromium.org>
Auto-Submit: Reilly Grant <reillyg@chromium.org>
Reviewed-by: James Hollyer <jameshollyer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#819726}

device/bluetooth/bluetooth_device_unittest.cc

@@ -2020,6 +2020,36 @@ TEST_F(BluetoothTest, MAYBE_GetGattServices_DiscoveryError) {
   EXPECT_EQ(0u, device->GetGattServices().size());
 }
 
+#if defined(OS_WIN)
+TEST_P(BluetoothTestWinrtOnly, GattServicesDiscovered_SomeServicesBlocked) {
+#else
+TEST_F(BluetoothTest, DISABLED_GattServicesDiscovered_SomeServicesBlocked) {
+#endif
+  if (!PlatformSupportsLowEnergy()) {
+    LOG(WARNING) << "Low Energy Bluetooth unavailable, skipping unit test.";
+    return;
+  }
+  InitWithFakeAdapter();
+  StartLowEnergyDiscoverySession();
+  TestBluetoothAdapterObserver observer(adapter_);
+  BluetoothDevice* device = SimulateLowEnergyDevice(3);
+  ResetEventCounts();
+  ASSERT_TRUE(ConnectGatt(device));
+  EXPECT_EQ(1, gatt_discovery_attempts_);
+  EXPECT_EQ(0, observer.gatt_services_discovered_count());
+
+  SimulateGattServicesDiscovered(
+      device,
+      /*uuids=*/{kTestUUIDGenericAccess, kTestUUIDHeartRate},
+      /*blocked_uuids=*/{kTestUUIDU2f});
+  base::RunLoop().RunUntilIdle();
+
+  EXPECT_TRUE(device->IsGattServicesDiscoveryComplete());
+  EXPECT_EQ(1, observer.gatt_services_discovered_count());
+  // Even though some services are blocked they should still appear in the list.
+  EXPECT_EQ(3u, device->GetGattServices().size());
+}
+
 #if defined(OS_CHROMEOS) || defined(OS_LINUX)
 TEST_F(BluetoothTest, GetDeviceTransportType) {
   if (!PlatformSupportsLowEnergy()) {

device/bluetooth/bluetooth_gatt_discoverer_winrt.cc

@@ -11,6 +11,7 @@
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/stl_util.h"
+#include "base/strings/stringprintf.h"
 #include "base/win/post_async_results.h"
 #include "components/device_event_log/device_event_log.h"
 #include "device/bluetooth/bluetooth_remote_gatt_service_winrt.h"
@@ -65,6 +66,46 @@ using ABI::Windows::Foundation::IReference;
 using ABI::Windows::Foundation::Collections::IVectorView;
 using Microsoft::WRL::ComPtr;
 
+std::string GattCommunicationStatusToString(GattCommunicationStatus status) {
+  switch (status) {
+    case GattCommunicationStatus_Success:
+      return "Success";
+    case ABI::Windows::Devices::Bluetooth::GenericAttributeProfile::
+        GattCommunicationStatus_Unreachable:
+      return "Unreachable";
+    case ABI::Windows::Devices::Bluetooth::GenericAttributeProfile::
+        GattCommunicationStatus_ProtocolError:
+      return "ProtocolError";
+    case GattCommunicationStatus_AccessDenied:
+      return "AccessDenied";
+    default:
+      return base::StringPrintf("Unknown (%d)", status);
+  }
+}
+
+std::string GattOpenStatusToString(GattOpenStatus status) {
+  switch (status) {
+    case ABI::Windows::Devices::Bluetooth::GenericAttributeProfile::
+        GattOpenStatus_Unspecified:
+      return "Unspecified";
+    case GattOpenStatus_Success:
+      return "Success";
+    case GattOpenStatus_AlreadyOpened:
+      return "AlreadyOpened";
+    case ABI::Windows::Devices::Bluetooth::GenericAttributeProfile::
+        GattOpenStatus_NotFound:
+      return "NotFound";
+    case ABI::Windows::Devices::Bluetooth::GenericAttributeProfile::
+        GattOpenStatus_SharingViolation:
+      return "SharingViolation";
+    case ABI::Windows::Devices::Bluetooth::GenericAttributeProfile::
+        GattOpenStatus_AccessDenied:
+      return "AccessDenied";
+    default:
+      return base::StringPrintf("Unknown (%d)", status);
+  }
+}
+
 template <typename IGattResult>
 bool CheckCommunicationStatus(IGattResult* gatt_result,
                               bool allow_access_denied = false) {
@@ -85,7 +126,8 @@ bool CheckCommunicationStatus(IGattResult* gatt_result,
     if (status == GattCommunicationStatus_AccessDenied) {
       BLUETOOTH_LOG(DEBUG) << "GATT access denied error";
     } else {
-      BLUETOOTH_LOG(DEBUG) << "Unexpected GattCommunicationStatus: " << status;
+      BLUETOOTH_LOG(DEBUG) << "Unexpected GattCommunicationStatus: "
+                           << GattCommunicationStatusToString(status);
     }
     BLUETOOTH_LOG(DEBUG)
         << "GATT Error Code: "
@@ -263,13 +305,16 @@ void BluetoothGattDiscovererWinrt::OnServiceOpen(
     GattOpenStatus status) {
   if (status != GattOpenStatus_Success &&
       status != GattOpenStatus_AlreadyOpened) {
-    BLUETOOTH_LOG(DEBUG) << "Failed to open service "
-                         << service_attribute_handle << ": " << status;
-    std::move(callback_).Run(false);
+    BLUETOOTH_LOG(DEBUG) << "Ignoring failure to open service "
+                         << service_attribute_handle << ": "
+                         << GattOpenStatusToString(status);
+
+    // Enumerate no characteristics on services the browser is unable to access.
+    service_to_characteristics_map_.insert({service_attribute_handle, {}});
+    RunCallbackIfDone();
     return;
   }
 
-
   ComPtr<IAsyncOperation<GattCharacteristicsResult*>> get_characteristics_op;
   HRESULT hr = gatt_service_3->GetCharacteristicsAsync(&get_characteristics_op);
   if (FAILED(hr)) {
@@ -297,7 +342,8 @@ void BluetoothGattDiscovererWinrt::OnGetCharacteristics(
     ComPtr<IGattCharacteristicsResult> characteristics_result) {
   // A few GATT services like HID over GATT (short UUID 0x1812) are protected
   // by the OS, leading to an access denied error.
-  if (!CheckCommunicationStatus(characteristics_result.Get(), true)) {
+  if (!CheckCommunicationStatus(characteristics_result.Get(),
+                                /*allow_access_denied=*/true)) {
     BLUETOOTH_LOG(DEBUG) << "Failed to get characteristics for service "
                          << service_attribute_handle << ".";
     std::move(callback_).Run(false);

device/bluetooth/test/bluetooth_test.h

@@ -357,12 +357,14 @@ class BluetoothTestBase : public testing::Test {
   // Simulates a connection status change to disconnect.
   virtual void SimulateStatusChangeToDisconnect(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.
+  // Simulates success of discovering services. |uuids| and |blocked_uuids| are
+  // used to create a service for each UUID string. Multiple UUIDs with the same
+  // value produce multiple service instances. UUIDs in the |blocked_uuids| list
+  // create services which cannot be accessed (WinRT-only).
   virtual void SimulateGattServicesDiscovered(
       BluetoothDevice* device,
-      const std::vector<std::string>& uuids) {}
+      const std::vector<std::string>& uuids,
+      const std::vector<std::string>& blocked_uuids = {}) {}
 
   // Simulates a GATT Services changed event.
   virtual void SimulateGattServicesChanged(BluetoothDevice* device) {}

device/bluetooth/test/bluetooth_test_android.cc

@@ -163,7 +163,9 @@ void BluetoothTestAndroid::SimulateGattDisconnection(BluetoothDevice* device) {
 
 void BluetoothTestAndroid::SimulateGattServicesDiscovered(
     BluetoothDevice* device,
-    const std::vector<std::string>& uuids) {
+    const std::vector<std::string>& uuids,
+    const std::vector<std::string>& blocked_uuids) {
+  DCHECK(blocked_uuids.empty()) << "Setting blocked_uuids unsupported.";
   BluetoothDeviceAndroid* device_android = nullptr;
   if (device) {
     device_android = static_cast<BluetoothDeviceAndroid*>(device);

device/bluetooth/test/bluetooth_test_android.h

@@ -36,7 +36,8 @@ class BluetoothTestAndroid : public BluetoothTestBase {
   void SimulateGattDisconnection(BluetoothDevice* device) override;
   void SimulateGattServicesDiscovered(
       BluetoothDevice* device,
-      const std::vector<std::string>& uuids) override;
+      const std::vector<std::string>& uuids,
+      const std::vector<std::string>& blocked_uuids = {}) override;
   void SimulateGattServicesDiscoveryError(BluetoothDevice* device) override;
   void SimulateGattCharacteristic(BluetoothRemoteGattService* service,
                                   const std::string& uuid,

device/bluetooth/test/bluetooth_test_mac.h

@@ -54,7 +54,8 @@ class BluetoothTestMac : public BluetoothTestBase {
   void SimulateGattDisconnectionError(BluetoothDevice* device) override;
   void SimulateGattServicesDiscovered(
       BluetoothDevice* device,
-      const std::vector<std::string>& uuids) override;
+      const std::vector<std::string>& uuids,
+      const std::vector<std::string>& blocked_uuids = {}) override;
   void SimulateGattServicesChanged(BluetoothDevice* device) override;
   void SimulateGattServiceRemoved(BluetoothRemoteGattService* service) override;
   void SimulateGattCharacteristic(BluetoothRemoteGattService* service,

device/bluetooth/test/bluetooth_test_mac.mm

@@ -344,7 +344,9 @@ void BluetoothTestMac::SimulateGattDisconnectionError(BluetoothDevice* device) {
 
 void BluetoothTestMac::SimulateGattServicesDiscovered(
     BluetoothDevice* device,
-    const std::vector<std::string>& uuids) {
+    const std::vector<std::string>& uuids,
+    const std::vector<std::string>& blocked_uuids) {
+  DCHECK(blocked_uuids.empty()) << "Setting blocked_uuids unsupported.";
   AddServicesToDeviceMac(device, uuids);
   [GetMockCBPeripheral(device) mockDidDiscoverEvents];
 }

device/bluetooth/test/bluetooth_test_win.cc

@@ -351,7 +351,9 @@ void BluetoothTestWin::SimulateStatusChangeToDisconnect(
 
 void BluetoothTestWin::SimulateGattServicesDiscovered(
     BluetoothDevice* device,
-    const std::vector<std::string>& uuids) {
+    const std::vector<std::string>& uuids,
+    const std::vector<std::string>& blocked_uuids) {
+  DCHECK(blocked_uuids.empty());
   std::string address =
       device ? device->GetAddress() : remembered_device_address_;
 
@@ -988,14 +990,16 @@ void BluetoothTestWinrt::SimulateDeviceBreaksConnection(
 
 void BluetoothTestWinrt::SimulateGattServicesDiscovered(
     BluetoothDevice* device,
-    const std::vector<std::string>& uuids) {
+    const std::vector<std::string>& uuids,
+    const std::vector<std::string>& blocked_uuids) {
   if (!UsesNewBleImplementation() || !PlatformSupportsLowEnergy())
-    return BluetoothTestWin::SimulateGattServicesDiscovered(device, uuids);
+    return BluetoothTestWin::SimulateGattServicesDiscovered(device, uuids,
+                                                            blocked_uuids);
 
   auto* const ble_device =
       static_cast<TestBluetoothDeviceWinrt*>(device)->ble_device();
   DCHECK(ble_device);
-  ble_device->SimulateGattServicesDiscovered(uuids);
+  ble_device->SimulateGattServicesDiscovered(uuids, blocked_uuids);
 }
 
 void BluetoothTestWinrt::SimulateGattServicesChanged(BluetoothDevice* device) {

device/bluetooth/test/bluetooth_test_win.h

@@ -44,7 +44,8 @@ class BluetoothTestWin : public BluetoothTestBase,
   void SimulateStatusChangeToDisconnect(BluetoothDevice* device) override;
   void SimulateGattServicesDiscovered(
       BluetoothDevice* device,
-      const std::vector<std::string>& uuids) override;
+      const std::vector<std::string>& uuids,
+      const std::vector<std::string>& blocked_uuids = {}) override;
   void SimulateGattServiceRemoved(BluetoothRemoteGattService* service) override;
   void SimulateGattCharacteristic(BluetoothRemoteGattService* service,
                                   const std::string& uuid,
@@ -204,7 +205,8 @@ class BluetoothTestWinrt
   void SimulateStatusChangeToDisconnect(BluetoothDevice* device) override;
   void SimulateGattServicesDiscovered(
       BluetoothDevice* device,
-      const std::vector<std::string>& uuids) override;
+      const std::vector<std::string>& uuids,
+      const std::vector<std::string>& blocked_uuids = {}) override;
   void SimulateGattServicesChanged(BluetoothDevice* device) override;
   void SimulateGattServiceRemoved(BluetoothRemoteGattService* service) override;
   void SimulateGattServicesDiscoveryError(BluetoothDevice* device) override;

device/bluetooth/test/fake_bluetooth_le_device_winrt.cc

@@ -325,14 +325,23 @@ void FakeBluetoothLEDeviceWinrt::SimulateGattNameChange(
 }
 
 void FakeBluetoothLEDeviceWinrt::SimulateGattServicesDiscovered(
-    const std::vector<std::string>& uuids) {
+    const std::vector<std::string>& uuids,
+    const std::vector<std::string>& blocked_uuids) {
   for (const auto& uuid : uuids) {
     // 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_, this, uuid,
-                                         service_attribute_handle_ += 0x0400));
+    fake_services_.push_back(Make<FakeGattDeviceServiceWinrt>(
+        bluetooth_test_winrt_, this, uuid, service_attribute_handle_ += 0x0400,
+        /*allowed=*/true));
+  }
+  for (const auto& uuid : blocked_uuids) {
+    // 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_, this, uuid, service_attribute_handle_ += 0x0400,
+        /*allowed=*/false));
   }
 
   DCHECK(gatt_services_callback_);

device/bluetooth/test/fake_bluetooth_le_device_winrt.h

@@ -134,7 +134,9 @@ class FakeBluetoothLEDeviceWinrt
   void SimulateGattDisconnection();
   void SimulateDeviceBreaksConnection();
   void SimulateGattNameChange(const std::string& new_name);
-  void SimulateGattServicesDiscovered(const std::vector<std::string>& uuids);
+  void SimulateGattServicesDiscovered(
+      const std::vector<std::string>& uuids,
+      const std::vector<std::string>& blocked_uuids);
   void SimulateGattServicesChanged();
   void SimulateStatusChangeToDisconnect();
   void SimulateGattServiceRemoved(BluetoothRemoteGattService* service);

device/bluetooth/test/fake_gatt_device_service_winrt.cc

@@ -33,6 +33,8 @@ using ABI::Windows::Devices::Bluetooth::GenericAttributeProfile::
 using ABI::Windows::Devices::Bluetooth::GenericAttributeProfile::
     GattDeviceServicesResult;
 using ABI::Windows::Devices::Bluetooth::GenericAttributeProfile::GattOpenStatus;
+using ABI::Windows::Devices::Bluetooth::GenericAttributeProfile::
+    GattOpenStatus_AccessDenied;
 using ABI::Windows::Devices::Bluetooth::GenericAttributeProfile::
     GattOpenStatus_AlreadyOpened;
 using ABI::Windows::Devices::Bluetooth::GenericAttributeProfile::
@@ -55,11 +57,13 @@ FakeGattDeviceServiceWinrt::FakeGattDeviceServiceWinrt(
     BluetoothTestWinrt* bluetooth_test_winrt,
     ComPtr<FakeBluetoothLEDeviceWinrt> fake_device,
     base::StringPiece uuid,
-    uint16_t attribute_handle)
+    uint16_t attribute_handle,
+    bool allowed)
     : bluetooth_test_winrt_(bluetooth_test_winrt),
       fake_device_(std::move(fake_device)),
       uuid_(BluetoothUUID::GetCanonicalValueAsGUID(uuid)),
       attribute_handle_(attribute_handle),
+      allowed_(allowed),
       characteristic_attribute_handle_(attribute_handle_) {
   fake_device_->AddReference();
 }
@@ -118,9 +122,13 @@ HRESULT FakeGattDeviceServiceWinrt::OpenAsync(
   if (sharing_mode != GattSharingMode_SharedReadAndWrite)
     return E_NOTIMPL;
 
-  GattOpenStatus status =
-      opened_ ? GattOpenStatus_AlreadyOpened : GattOpenStatus_Success;
-  opened_ = true;
+  GattOpenStatus status;
+  if (allowed_) {
+    status = opened_ ? GattOpenStatus_AlreadyOpened : GattOpenStatus_Success;
+    opened_ = true;
+  } else {
+    status = GattOpenStatus_AccessDenied;
+  }
 
   auto async_op = Make<base::win::AsyncOperation<GattOpenStatus>>();
   base::ThreadTaskRunnerHandle::Get()->PostTask(

device/bluetooth/test/fake_gatt_device_service_winrt.h

@@ -35,7 +35,8 @@ class FakeGattDeviceServiceWinrt
       BluetoothTestWinrt* bluetooth_test_winrt,
       Microsoft::WRL::ComPtr<FakeBluetoothLEDeviceWinrt> fake_device,
       base::StringPiece uuid,
-      uint16_t attribute_handle);
+      uint16_t attribute_handle,
+      bool allowed);
   ~FakeGattDeviceServiceWinrt() override;
 
   // IGattDeviceService:
@@ -117,10 +118,11 @@ class FakeGattDeviceServiceWinrt
   void SimulateGattCharacteristic(base::StringPiece uuid, int proporties);
 
  private:
-  BluetoothTestWinrt* bluetooth_test_winrt_;
-  Microsoft::WRL::ComPtr<FakeBluetoothLEDeviceWinrt> fake_device_;
-  GUID uuid_;
-  uint16_t attribute_handle_;
+  BluetoothTestWinrt* const bluetooth_test_winrt_;
+  const Microsoft::WRL::ComPtr<FakeBluetoothLEDeviceWinrt> fake_device_;
+  const GUID uuid_;
+  const uint16_t attribute_handle_;
+  const bool allowed_;
   bool opened_ = false;
 
   std::vector<Microsoft::WRL::ComPtr<FakeGattCharacteristicWinrt>>