device/bluetooth/bluez: track service and advertised uuid separately

BlueZ's device UUIDs property could return either the service UUIDs
or the advertised UUIDs (not both), depends on whether the service
UUIDs are resolved. Unfortunately there is no simple way to detect
which of the two sets of UUIDs are returned.

This CL separates the UUIDs tracking. The advertised UUIDs can be
retrieved by reading the EIR data, therefore we can treat the UUIDs
property as the service UUIDs. In the end, both are merged so a call
to GetUUIDs() would get both the service and advertised UUIDs.

Bug: b:172359553
Change-Id: I7de6f0c47f3799265b833c4b3be3da84bdf0e458
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2560347
Commit-Queue: Archie Pusaka <apusaka@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Reviewed-by: Sonny Sasaka <sonnysasaka@chromium.org>
Cr-Commit-Position: refs/heads/master@{#834606}

device/bluetooth/bluetooth_device.cc

@@ -62,6 +62,15 @@ void BluetoothDevice::DeviceUUIDs::ReplaceServiceUUIDs(
   UpdateDeviceUUIDs();
 }
 
+void BluetoothDevice::DeviceUUIDs::ReplaceServiceUUIDs(
+    UUIDList new_service_uuids) {
+  service_uuids_.clear();
+  for (auto& it : new_service_uuids) {
+    service_uuids_.insert(std::move(it));
+  }
+  UpdateDeviceUUIDs();
+}
+
 void BluetoothDevice::DeviceUUIDs::ClearServiceUUIDs() {
   service_uuids_.clear();
   UpdateDeviceUUIDs();

device/bluetooth/bluetooth_device.h

@@ -672,6 +672,8 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothDevice {
     void ReplaceServiceUUIDs(
         const BluetoothDevice::GattServiceMap& gatt_services);
 
+    void ReplaceServiceUUIDs(UUIDList new_service_uuids);
+
     void ClearServiceUUIDs();
 
     // Returns the union of Advertised UUIDs and Service UUIDs.

device/bluetooth/bluez/bluetooth_adapter_bluez.cc

@@ -775,6 +775,8 @@ void BluetoothAdapterBlueZ::DevicePropertyChanged(
     device_bluez->UpdateManufacturerData();
   else if (property_name == properties->advertising_data_flags.name())
     device_bluez->UpdateAdvertisingDataFlags();
+  else if (property_name == properties->uuids.name())
+    device_bluez->UpdateServiceUUIDs();
 
   if (property_name == properties->bluetooth_class.name() ||
       property_name == properties->appearance.name() ||
@@ -1274,18 +1276,20 @@ void BluetoothAdapterBlueZ::NotifyDeviceAdvertisementReceived(
         base::BindOnce(&BluetoothAdapterBlueZ::OnAdvertisementReceived,
                        weak_ptr_factory_.GetWeakPtr(), device->GetAddress(),
                        device->GetName() ? *(device->GetName()) : std::string(),
-                       rssi, device->GetAppearance());
+                       rssi, device->GetAppearance(), device->object_path());
     ble_scan_parser_->Parse(eir, std::move(callback));
   }
 #endif  // BUILDFLAG(IS_ASH)
 }
 
 #if BUILDFLAG(IS_ASH)
-void BluetoothAdapterBlueZ::OnAdvertisementReceived(std::string device_address,
-                                                    std::string device_name,
-                                                    uint8_t rssi,
-                                                    uint16_t device_appearance,
-                                                    ScanRecordPtr scan_record) {
+void BluetoothAdapterBlueZ::OnAdvertisementReceived(
+    std::string device_address,
+    std::string device_name,
+    uint8_t rssi,
+    uint16_t device_appearance,
+    const dbus::ObjectPath& device_path,
+    ScanRecordPtr scan_record) {
   // Ignore the packet if it could not be parsed successfully.
   if (!scan_record)
     return;
@@ -1299,6 +1303,14 @@ void BluetoothAdapterBlueZ::OnAdvertisementReceived(std::string device_address,
         scan_record->tx_power, device_appearance, scan_record->service_uuids,
         service_data_map, manufacturer_data_map);
   }
+
+  BluetoothDeviceBlueZ* device = GetDeviceWithPath(device_path);
+  if (!device) {
+    BLUETOOTH_LOG(ERROR) << "Device " << device_path.value() << " not found!";
+    return;
+  }
+
+  device->SetAdvertisedUUIDs(scan_record->service_uuids);
 }
 #endif  // BUILDFLAG(IS_ASH)
 

device/bluetooth/bluez/bluetooth_adapter_bluez.h

@@ -198,6 +198,7 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothAdapterBlueZ final
                                std::string device_name,
                                uint8_t rssi,
                                uint16_t device_appearance,
+                               const dbus::ObjectPath& device_path,
                                ScanRecordPtr scan_record);
 #endif  // BUILDFLAG(IS_ASH)
 

device/bluetooth/bluez/bluetooth_bluez_unittest.cc

@@ -36,6 +36,7 @@
 #include "device/bluetooth/dbus/fake_bluetooth_input_client.h"
 #include "device/bluetooth/test/test_bluetooth_adapter_observer.h"
 #include "device/bluetooth/test/test_pairing_delegate.h"
+#include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 
@@ -4587,4 +4588,77 @@ TEST_F(BluetoothBlueZTest, BatteryEvents) {
   EXPECT_FALSE(device->battery_percentage().has_value());
 }
 
+TEST_F(BluetoothBlueZTest, DeviceUUIDsCombinedFromServiceAndAdvertisement) {
+  // Simulate addition and removal of service and advertisement UUIDs.
+  GetAdapter();
+
+  fake_bluetooth_device_client_->CreateDevice(
+      dbus::ObjectPath(bluez::FakeBluetoothAdapterClient::kAdapterPath),
+      dbus::ObjectPath(bluez::FakeBluetoothDeviceClient::kLowEnergyPath));
+  BluetoothDevice* device =
+      adapter_->GetDevice(bluez::FakeBluetoothDeviceClient::kLowEnergyAddress);
+  ASSERT_TRUE(device);
+
+  BluetoothDeviceBlueZ* device_bluez =
+      static_cast<BluetoothDeviceBlueZ*>(device);
+
+  bluez::FakeBluetoothDeviceClient::Properties* properties =
+      fake_bluetooth_device_client_->GetProperties(
+          dbus::ObjectPath(bluez::FakeBluetoothDeviceClient::kLowEnergyPath));
+
+  // Have these 9 UUIDs divided into 3 groups:
+  // -UUIDs which is available from the start, but later is missing
+  // -UUIDs which is not available in the beginning, but later is added
+  // -UUIDs which is always available
+  // In each group, they are further divided into 3 groups:
+  // -advertisement UUIDs
+  // -service UUIDs (from SDP / GATT)
+  // -UUIDs which appear in both
+  BluetoothUUID uuidInitAdv = BluetoothUUID("1111");
+  BluetoothUUID uuidInitServ = BluetoothUUID("2222");
+  BluetoothUUID uuidInitBoth = BluetoothUUID("3333");
+  BluetoothUUID uuidLaterAdv = BluetoothUUID("4444");
+  BluetoothUUID uuidLaterServ = BluetoothUUID("5555");
+  BluetoothUUID uuidLaterBoth = BluetoothUUID("6666");
+  BluetoothUUID uuidAlwaysAdv = BluetoothUUID("7777");
+  BluetoothUUID uuidAlwaysServ = BluetoothUUID("8888");
+  BluetoothUUID uuidAlwaysBoth = BluetoothUUID("9999");
+
+  BluetoothAdapter::UUIDList uuidsInitAdv{uuidInitAdv, uuidInitBoth,
+                                          uuidAlwaysAdv, uuidAlwaysBoth};
+  BluetoothAdapter::UUIDList uuidsLaterAdv{uuidLaterAdv, uuidLaterBoth,
+                                           uuidAlwaysAdv, uuidAlwaysBoth};
+
+  std::vector<std::string> uuidsInitServ{
+      uuidInitServ.canonical_value(), uuidInitBoth.canonical_value(),
+      uuidAlwaysServ.canonical_value(), uuidAlwaysBoth.canonical_value()};
+  std::vector<std::string> uuidsLaterServ{
+      uuidLaterServ.canonical_value(), uuidLaterBoth.canonical_value(),
+      uuidAlwaysServ.canonical_value(), uuidAlwaysBoth.canonical_value()};
+
+  device_bluez->SetAdvertisedUUIDs(uuidsInitAdv);
+  properties->uuids.ReplaceValue(uuidsInitServ);
+
+  // The result should be the union of service and advertisement UUIDs
+  BluetoothDevice::UUIDSet dev_uuids = device->GetUUIDs();
+  EXPECT_THAT(dev_uuids, ::testing::ElementsAre(
+                             uuidInitAdv, uuidInitServ, uuidInitBoth,
+                             uuidAlwaysAdv, uuidAlwaysServ, uuidAlwaysBoth));
+
+  // advertisement UUIDs updated
+  device_bluez->SetAdvertisedUUIDs(uuidsLaterAdv);
+  dev_uuids = device->GetUUIDs();
+  EXPECT_THAT(dev_uuids,
+              ::testing::ElementsAre(uuidInitServ, uuidInitBoth, uuidLaterAdv,
+                                     uuidLaterBoth, uuidAlwaysAdv,
+                                     uuidAlwaysServ, uuidAlwaysBoth));
+
+  // service UUIDs updated
+  properties->uuids.ReplaceValue(uuidsLaterServ);
+  dev_uuids = device->GetUUIDs();
+  EXPECT_THAT(dev_uuids, ::testing::ElementsAre(
+                             uuidLaterAdv, uuidLaterServ, uuidLaterBoth,
+                             uuidAlwaysAdv, uuidAlwaysServ, uuidAlwaysBoth));
+}
+
 }  // namespace bluez

device/bluetooth/bluez/bluetooth_device_bluez.cc

@@ -153,6 +153,7 @@ BluetoothDeviceBlueZ::BluetoothDeviceBlueZ(
   UpdateServiceData();
   UpdateManufacturerData();
   UpdateAdvertisingDataFlags();
+  UpdateServiceUUIDs();
 }
 
 BluetoothDeviceBlueZ::~BluetoothDeviceBlueZ() {
@@ -382,19 +383,7 @@ bool BluetoothDeviceBlueZ::IsConnecting() const {
 }
 
 BluetoothDevice::UUIDSet BluetoothDeviceBlueZ::GetUUIDs() const {
-  bluez::BluetoothDeviceClient::Properties* properties =
-      bluez::BluezDBusManager::Get()->GetBluetoothDeviceClient()->GetProperties(
-          object_path_);
-  DCHECK(properties);
-
-  UUIDSet uuids;
-  const std::vector<std::string>& dbus_uuids = properties->uuids.value();
-  for (const std::string& dbus_uuid : dbus_uuids) {
-    device::BluetoothUUID uuid(dbus_uuid);
-    DCHECK(uuid.IsValid());
-    uuids.insert(std::move(uuid));
-  }
-  return uuids;
+  return device_uuids_.GetUUIDs();
 }
 
 base::Optional<int8_t> BluetoothDeviceBlueZ::GetInquiryRSSI() const {
@@ -750,6 +739,28 @@ void BluetoothDeviceBlueZ::UpdateAdvertisingDataFlags() {
   advertising_data_flags_ = properties->advertising_data_flags.value()[0];
 }
 
+void BluetoothDeviceBlueZ::UpdateServiceUUIDs() {
+  bluez::BluetoothDeviceClient::Properties* properties =
+      bluez::BluezDBusManager::Get()->GetBluetoothDeviceClient()->GetProperties(
+          object_path_);
+  if (!properties)
+    return;
+
+  UUIDList uuids;
+  for (const std::string& dbus_uuid : properties->uuids.value()) {
+    device::BluetoothUUID uuid(dbus_uuid);
+    DCHECK(uuid.IsValid());
+    uuids.push_back(std::move(uuid));
+  }
+
+  device_uuids_.ReplaceServiceUUIDs(uuids);
+}
+
+void BluetoothDeviceBlueZ::SetAdvertisedUUIDs(
+    const BluetoothDevice::UUIDList& uuids) {
+  device_uuids_.ReplaceAdvertisedUUIDs(uuids);
+}
+
 BluetoothPairingBlueZ* BluetoothDeviceBlueZ::BeginPairing(
     BluetoothDevice::PairingDelegate* pairing_delegate) {
   pairing_.reset(new BluetoothPairingBlueZ(this, pairing_delegate));

device/bluetooth/bluez/bluetooth_device_bluez.h

@@ -144,6 +144,21 @@ class DEVICE_BLUETOOTH_EXPORT BluetoothDeviceBlueZ
   // from UpdateServiceData() also applies here.
   void UpdateAdvertisingDataFlags();
 
+  // Called by BluetoothAdapterBlueZ to update device_uuids_ defined in
+  // BluetoothDevice when receiving DevicePropertyChanged event for the UUIDs
+  // property. Note that BlueZ's implementation returns service UUIDs (SDP or
+  // GATT) when they are available, otherwise it contains the EIR or
+  // advertisement UUIDs. However, currently there is no way of knowing which
+  // one we will get. Since the advertised UUIDs can be tracked while we receive
+  // advertisement packets, here we assume it contains the service UUIDs. Both
+  // are merged behind the scenes, so GetUUIDs() would return the expected
+  // result.
+  void UpdateServiceUUIDs();
+
+  // Called by BluetoothAdapterBlueZ to update device_uuids_ defined in
+  // BluetoothDevice when receiving advertisement data.
+  void SetAdvertisedUUIDs(const BluetoothDevice::UUIDList& uuids);
+
   // Creates a pairing object with the given delegate |pairing_delegate| and
   // establishes it as the pairing context for this device. All pairing-related
   // method calls will be forwarded to this object until it is released.