[Nearby] Use fast advertisement UUID in BLE scanning

A recent uprev of Nearby Connections introduced passing the fast
advertisement service UUID into BleMedium::StartScanning(). (See its
introduction in cl/332425345.) This can eventually be used to filter
incoming Nearby BLE advertisements; the other argument to
StartScanning(), service_id, is just a bare string like "NearbySharing",
and therefore cannot be a filter.

In this CL, we do the following:
(1) Within BleMedium, use fast_advertisement_service_uuid as the actual
    UUID, not service_id.
(2) Because BlePeripheral::GetAdvertisementBytes() still uses service_id
    as its lone parameter, pass the one-to-one map of service_id to
    fast_advertisement_service_uuid. With that mapping, the
    BlePeripheral::GetAdvertisementBytes() can understand which UUID to
    use given an input service_id.

Fixed: 1131974
Change-Id: I7b245176930e803ea0524b9d77a794cd6cfbc504
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2436788
Commit-Queue: Josh Nohle <nohle@chromium.org>
Commit-Queue: Ryan Hansberry <hansberry@chromium.org>
Auto-Submit: Josh Nohle <nohle@chromium.org>
Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#812399}

chrome/services/sharing/nearby/platform_v2/ble_medium.cc

@@ -80,15 +80,25 @@ bool BleMedium::StartScanning(
     const std::string& fast_advertisement_service_uuid,
     api::BleMedium::DiscoveredPeripheralCallback
         discovered_peripheral_callback) {
-  // TODO(https://crbug.com/1132108): |fast_advertisement_service_uuid| is
-  // unused.
+  auto service_uuid = device::BluetoothUUID(fast_advertisement_service_uuid);
+
+  // The ID-to-UUID map should always be in sync with the callbacks map, and we
+  // assume that the ID-UUID mapping is one-to-one.
+  DCHECK_EQ(base::Contains(discovered_peripheral_callbacks_map_, service_uuid),
+            base::Contains(
+                discovery_service_id_to_fast_advertisement_service_uuid_map_,
+                service_id));
 
-  auto service_uuid = device::BluetoothUUID(service_id);
   if (IsScanning() &&
       base::Contains(discovered_peripheral_callbacks_map_, service_uuid)) {
     return true;
   }
 
+  // The ID-to-UUID map should always be in sync with the callbacks map.
+  DCHECK_EQ(
+      discovered_peripheral_callbacks_map_.empty(),
+      discovery_service_id_to_fast_advertisement_service_uuid_map_.empty());
+
   // We only need to start discovery if no other discovery request is active.
   if (discovered_peripheral_callbacks_map_.empty()) {
     discovered_ble_peripherals_map_.clear();
@@ -115,14 +125,40 @@ bool BleMedium::StartScanning(
   }
 
   // A different DiscoveredPeripheralCallback is being passed on each call, so
-  // each must be captured and associated with its |service_id|.
+  // each must be captured and associated with its service UUID.
   discovered_peripheral_callbacks_map_.insert(
       {service_uuid, discovered_peripheral_callback});
+
+  discovery_service_id_to_fast_advertisement_service_uuid_map_.insert(
+      {service_id, service_uuid});
+  for (auto& uuid_peripheral_pair : discovered_ble_peripherals_map_) {
+    uuid_peripheral_pair.second.UpdateIdToUuidMap(
+        discovery_service_id_to_fast_advertisement_service_uuid_map_);
+  }
+
   return true;
 }
 
 bool BleMedium::StopScanning(const std::string& service_id) {
-  discovered_peripheral_callbacks_map_.erase(device::BluetoothUUID(service_id));
+  const auto it =
+      discovery_service_id_to_fast_advertisement_service_uuid_map_.find(
+          service_id);
+  if (it !=
+      discovery_service_id_to_fast_advertisement_service_uuid_map_.end()) {
+    DCHECK(base::Contains(discovered_peripheral_callbacks_map_, it->second));
+    discovered_peripheral_callbacks_map_.erase(it->second);
+    discovery_service_id_to_fast_advertisement_service_uuid_map_.erase(it);
+    for (auto& uuid_peripheral_pair : discovered_ble_peripherals_map_) {
+      uuid_peripheral_pair.second.UpdateIdToUuidMap(
+          discovery_service_id_to_fast_advertisement_service_uuid_map_);
+    }
+  }
+
+  // The ID-to-UUID map should always be in sync with the callbacks map.
+  DCHECK_EQ(
+      discovered_peripheral_callbacks_map_.empty(),
+      discovery_service_id_to_fast_advertisement_service_uuid_map_.empty());
+
   if (!discovered_peripheral_callbacks_map_.empty())
     return true;
 
@@ -202,32 +238,47 @@ void BleMedium::DeviceAdded(bluetooth::mojom::DeviceInfoPtr device) {
   if (device->service_data_map.empty())
     return;
 
-  const std::string& address = device->address;
+  // Add a new or update the existing discovered peripheral. Note: Because
+  // BlePeripherals are passed by reference to NearbyConnections, if a
+  // BlePeripheral already exists with the given address, the reference should
+  // not be invalidated, the update functions should be called instead.
+  std::string address = device->address;
   auto* ble_peripheral = GetDiscoveredBlePeripheral(address);
-  if (ble_peripheral)
+  if (ble_peripheral) {
     ble_peripheral->UpdateDeviceInfo(std::move(device));
-  else
-    discovered_ble_peripherals_map_.emplace(address, std::move(device));
-
-  // Invoking one of the callbacks in |discovered_peripheral_callbacks_map_| may
-  // lead to invalidating one or all elements of
-  // |discovered_peripheral_callbacks_map_|, e.g., triggering StopScanning()
-  // while looping through it. Callbacks are copied to ensure they are not
-  // modified as we loop through them.
-  auto callbacks_map_copy = discovered_peripheral_callbacks_map_;
-  for (auto& it : callbacks_map_copy) {
-    // Must fetch |ble_peripheral| again because it may have been invalidated by
-    // a prior callback in this loop.
-    ble_peripheral = GetDiscoveredBlePeripheral(address);
-    if (!ble_peripheral)
-      break;
+  } else {
+    discovered_ble_peripherals_map_.emplace(
+        address,
+        chrome::BlePeripheral(
+            std::move(device),
+            discovery_service_id_to_fast_advertisement_service_uuid_map_));
+  }
+
+  // Copy the ID-to-UUID map to ensure that elements are not invalidated while
+  // iterating--for example, if StopScanning() is triggered after invoking the
+  // callback in the body of the loop.
+  auto id_uuid_map_copy =
+      discovery_service_id_to_fast_advertisement_service_uuid_map_;
+  for (const auto& id_uuid_pair : id_uuid_map_copy) {
+    // A callback should always be found unless an element was removed while we
+    // were iterating through the IDs.
+    const auto it =
+        discovered_peripheral_callbacks_map_.find(id_uuid_pair.second);
+    if (it == discovered_peripheral_callbacks_map_.end())
+      continue;
 
-    const auto& service_id = it.first.value();
-    if (ble_peripheral->GetAdvertisementBytes(service_id).Empty())
+    // Fetch |ble_peripheral| again because it might have since been invalidated
+    // while we were iterating through IDs.
+    auto* ble_peripheral = GetDiscoveredBlePeripheral(address);
+    if (!ble_peripheral)
       continue;
 
-    it.second.peripheral_discovered_cb(*ble_peripheral, service_id,
-                                       /*fast_advertisement=*/true);
+    // Do not perform any filtering here, for example, by checking if the
+    // peripheral has non-empty advertisement bytes. Unconditionally inform all
+    // callbacks of the discovered device, and rely on the Nearby Connections
+    // library to perform the filtering.
+    it->second.peripheral_discovered_cb(*ble_peripheral, id_uuid_pair.first,
+                                        /*fast_advertisement=*/true);
   }
 }
 
@@ -243,21 +294,26 @@ void BleMedium::DeviceRemoved(bluetooth::mojom::DeviceInfoPtr device) {
   if (!GetDiscoveredBlePeripheral(address))
     return;
 
-  // Invoking one of the callbacks in |discovered_peripheral_callbacks_map_| may
-  // lead to invalidating one or all elements of
-  // |discovered_peripheral_callbacks_map_|, e.g., triggering StopScanning()
-  // while looping through it. Callbacks are copied to ensure they are not
-  // modified as we loop through them.
-  auto callbacks_map_copy = discovered_peripheral_callbacks_map_;
-  for (auto& it : callbacks_map_copy) {
-    // Must fetch |ble_peripheral| again because it may have been invalidated by
-    // a prior callback in this loop.
+  // Copy the ID-to-UUID map to ensure that elements are not invalidated while
+  // iterating--for example, if StopScanning() is triggered after invoking the
+  // callback in the body of the loop.
+  auto id_uuid_map_copy =
+      discovery_service_id_to_fast_advertisement_service_uuid_map_;
+  for (const auto& id_uuid_pair : id_uuid_map_copy) {
+    // A callback should always be found unless an element was removed while we
+    // were iterating through the IDs.
+    const auto it =
+        discovered_peripheral_callbacks_map_.find(id_uuid_pair.second);
+    if (it == discovered_peripheral_callbacks_map_.end())
+      continue;
+
+    // Fetch |ble_peripheral| again because it might have since been invalidated
+    // while we were iterating through IDs.
     auto* ble_peripheral = GetDiscoveredBlePeripheral(address);
     if (!ble_peripheral)
-      break;
+      continue;
 
-    it.second.peripheral_lost_cb(*ble_peripheral,
-                                 /*service_id=*/it.first.value());
+    it->second.peripheral_lost_cb(*ble_peripheral, id_uuid_pair.first);
   }
 
   discovered_ble_peripherals_map_.erase(address);
@@ -269,17 +325,25 @@ void BleMedium::AdvertisementReleased(
 }
 
 bool BleMedium::IsScanning() {
+  DCHECK_EQ(
+      discovered_peripheral_callbacks_map_.empty(),
+      discovery_service_id_to_fast_advertisement_service_uuid_map_.empty());
   return adapter_observer_.is_bound() && discovery_session_.is_bound() &&
          !discovered_peripheral_callbacks_map_.empty();
 }
 
 void BleMedium::StopScanning() {
-  // We cannot simply iterate over |discovered_peripheral_callbacks_map_|
-  // because StopScanning() will erase the provided element.
-  while (!discovered_peripheral_callbacks_map_.empty()) {
-    StopScanning(/*service_id=*/discovered_peripheral_callbacks_map_.begin()
-                     ->first.value());
+  // We cannot simply iterate over
+  // |discovery_service_id_to_fast_advertisement_service_uuid_map_| because
+  // StopScanning() will erase the provided element.
+  while (
+      !discovery_service_id_to_fast_advertisement_service_uuid_map_.empty()) {
+    StopScanning(
+        /*service_id=*/
+        discovery_service_id_to_fast_advertisement_service_uuid_map_.begin()
+            ->first);
   }
+  DCHECK(discovered_peripheral_callbacks_map_.empty());
 }
 
 chrome::BlePeripheral* BleMedium::GetDiscoveredBlePeripheral(

chrome/services/sharing/nearby/platform_v2/ble_medium.h

@@ -87,6 +87,13 @@ class BleMedium : public api::BleMedium,
   std::map<device::BluetoothUUID, mojo::Remote<bluetooth::mojom::Advertisement>>
       registered_advertisements_map_;
 
+  // A map of service IDs (e.g., "NearbySharing") to their respective fast
+  // advertisement UUIDs used to filter incoming BLE advertisements during
+  // scanning. We assume that the mapping is one-to-one. Insertions and
+  // deletions mirror those of |discovered_peripheral_callbacks_map_|.
+  std::map<std::string, device::BluetoothUUID>
+      discovery_service_id_to_fast_advertisement_service_uuid_map_;
+
   // Keyed by requested service UUID. Discovery is active as long as this map is
   // non-empty.
   std::map<device::BluetoothUUID, DiscoveredPeripheralCallback>
@@ -95,7 +102,10 @@ class BleMedium : public api::BleMedium,
   // Only set while discovery is active.
   mojo::Remote<bluetooth::mojom::DiscoverySession> discovery_session_;
 
-  // Keyed by address of advertising remote device.
+  // Keyed by address of advertising remote device. Note: References to these
+  // BlePeripherals are passed to Nearby Connections. This is safe because, for
+  // std::map, insert/emplace do not invalidate references, and the erase
+  // operation only invalidates the reference to the erased element.
   std::map<std::string, chrome::BlePeripheral> discovered_ble_peripherals_map_;
 };
 

chrome/services/sharing/nearby/platform_v2/ble_medium_unittest.cc

@@ -77,14 +77,14 @@ class BleMediumTest : public testing::Test {
   }
 
  protected:
-  void StartScanning(const std::string& service_id) {
+  void StartScanning(const std::string& service_id,
+                     const std::string& fast_advertisement_service_uuid) {
     EXPECT_EQ(!scanning_service_ids_set_.empty(),
               fake_adapter_->IsDiscoverySessionActive());
     scanning_service_ids_set_.insert(service_id);
-    EXPECT_TRUE(ble_medium_->StartScanning(
-        service_id,
-        /*fast_advertisement_service_uuid=*/std::string(),
-        discovered_peripheral_callback_));
+    EXPECT_TRUE(ble_medium_->StartScanning(service_id,
+                                           fast_advertisement_service_uuid,
+                                           discovered_peripheral_callback_));
     EXPECT_TRUE(fake_adapter_->IsDiscoverySessionActive());
   }
 
@@ -272,7 +272,7 @@ TEST_F(BleMediumTest, TestAdvertising) {
 }
 
 TEST_F(BleMediumTest, TestScanning_OneService) {
-  StartScanning(kFastAdvertisementServiceId1);
+  StartScanning(kServiceId1, kFastAdvertisementServiceId1);
 
   base::flat_map<device::BluetoothUUID, std::vector<uint8_t>> service_data_map;
   service_data_map.insert_or_assign(
@@ -285,11 +285,10 @@ TEST_F(BleMediumTest, TestScanning_OneService) {
   auto& last_peripheral_discovered_args = last_peripheral_discovered_args_[0];
   const auto* first_discovered_ble_peripheral =
       last_peripheral_discovered_args.first;
-  EXPECT_EQ(kFastAdvertisementServiceId1,
-            last_peripheral_discovered_args.second);
-  VerifyByteArrayEquals(first_discovered_ble_peripheral->GetAdvertisementBytes(
-                            kFastAdvertisementServiceId1),
-                        kDeviceServiceData1Str);
+  EXPECT_EQ(kServiceId1, last_peripheral_discovered_args.second);
+  VerifyByteArrayEquals(
+      first_discovered_ble_peripheral->GetAdvertisementBytes(kServiceId1),
+      kDeviceServiceData1Str);
 
   // The same information should be returned on a DeviceChanged event, with
   // the same BlePeripheral reference.
@@ -299,11 +298,9 @@ TEST_F(BleMediumTest, TestScanning_OneService) {
   last_peripheral_discovered_args = last_peripheral_discovered_args_[0];
   EXPECT_EQ(first_discovered_ble_peripheral,
             last_peripheral_discovered_args.first);
-  EXPECT_EQ(kFastAdvertisementServiceId1,
-            last_peripheral_discovered_args.second);
+  EXPECT_EQ(kServiceId1, last_peripheral_discovered_args.second);
   VerifyByteArrayEquals(
-      last_peripheral_discovered_args.first->GetAdvertisementBytes(
-          kFastAdvertisementServiceId1),
+      last_peripheral_discovered_args.first->GetAdvertisementBytes(kServiceId1),
       kDeviceServiceData1Str);
 
   // Again, the same BlePeripheral reference should be marked as lost.
@@ -312,14 +309,14 @@ TEST_F(BleMediumTest, TestScanning_OneService) {
   const auto& last_peripheral_lost_args = last_peripheral_lost_args_[0];
   const auto* lost_ble_peripheral = last_peripheral_lost_args.first;
   EXPECT_EQ(first_discovered_ble_peripheral, lost_ble_peripheral);
-  EXPECT_EQ(kFastAdvertisementServiceId1, last_peripheral_lost_args.second);
+  EXPECT_EQ(kServiceId1, last_peripheral_lost_args.second);
 
-  StopScanning(kFastAdvertisementServiceId1);
+  StopScanning(kServiceId1);
 }
 
 TEST_F(BleMediumTest, TestScanning_MultipleServices) {
-  StartScanning(kFastAdvertisementServiceId1);
-  StartScanning(kFastAdvertisementServiceId2);
+  StartScanning(kServiceId1, kFastAdvertisementServiceId1);
+  StartScanning(kServiceId2, kFastAdvertisementServiceId2);
 
   base::flat_map<device::BluetoothUUID, std::vector<uint8_t>> service_data_map;
   service_data_map.insert_or_assign(
@@ -336,18 +333,18 @@ TEST_F(BleMediumTest, TestScanning_MultipleServices) {
   ASSERT_EQ(2u, last_peripheral_discovered_args_.size());
   VerifyByteArrayEquals(
       last_peripheral_discovered_args_[0].first->GetAdvertisementBytes(
-          kFastAdvertisementServiceId1),
+          kServiceId1),
       kDeviceServiceData1Str);
   VerifyByteArrayEquals(
       last_peripheral_discovered_args_[1].first->GetAdvertisementBytes(
-          kFastAdvertisementServiceId2),
+          kServiceId2),
       kDeviceServiceData2Str);
 
   NotifyDeviceRemoved(kDeviceAddress, /*num_expected_peripherals_lost=*/2u);
   ASSERT_EQ(2u, last_peripheral_lost_args_.size());
 
-  StopScanning(kFastAdvertisementServiceId1);
-  StopScanning(kFastAdvertisementServiceId2);
+  StopScanning(kServiceId1);
+  StopScanning(kServiceId2);
 }
 
 TEST_F(BleMediumTest, TestStartAcceptingConnections) {
@@ -357,7 +354,10 @@ TEST_F(BleMediumTest, TestStartAcceptingConnections) {
 }
 
 TEST_F(BleMediumTest, TestConnect) {
-  chrome::BlePeripheral ble_peripheral(bluetooth::mojom::DeviceInfo::New());
+  chrome::BlePeripheral ble_peripheral(
+      bluetooth::mojom::DeviceInfo::New(),
+      /*service_id_to_fast_advertisement_service_uuid_map=*/std::map<
+          std::string, device::BluetoothUUID>());
 
   // Connect() should do nothing and not return a valid api::BleSocket.
   EXPECT_FALSE(ble_medium_->Connect(ble_peripheral, kServiceId1));

chrome/services/sharing/nearby/platform_v2/ble_peripheral.cc

@@ -8,20 +8,33 @@ namespace location {
 namespace nearby {
 namespace chrome {
 
-BlePeripheral::BlePeripheral(bluetooth::mojom::DeviceInfoPtr device_info)
-    : device_info_(std::move(device_info)) {}
+BlePeripheral::BlePeripheral(
+    bluetooth::mojom::DeviceInfoPtr device_info,
+    const std::map<std::string, device::BluetoothUUID>&
+        service_id_to_fast_advertisement_service_uuid_map)
+    : device_info_(std::move(device_info)),
+      service_id_to_fast_advertisement_service_uuid_map_(
+          service_id_to_fast_advertisement_service_uuid_map) {}
 
 BlePeripheral::~BlePeripheral() = default;
 
+BlePeripheral::BlePeripheral(BlePeripheral&&) = default;
+
+BlePeripheral& BlePeripheral::operator=(BlePeripheral&&) = default;
+
 std::string BlePeripheral::GetName() const {
   return device_info_->name_for_display;
 }
 
 ByteArray BlePeripheral::GetAdvertisementBytes(
     const std::string& service_id) const {
-  const auto& service_data_map = device_info_->service_data_map;
+  const auto it_uuid =
+      service_id_to_fast_advertisement_service_uuid_map_.find(service_id);
+  if (it_uuid == service_id_to_fast_advertisement_service_uuid_map_.end())
+    return ByteArray();
 
-  auto it = service_data_map.find(device::BluetoothUUID(service_id));
+  const auto& service_data_map = device_info_->service_data_map;
+  const auto it = service_data_map.find(it_uuid->second);
   if (it == service_data_map.end())
     return ByteArray();
 
@@ -35,6 +48,13 @@ void BlePeripheral::UpdateDeviceInfo(
   device_info_ = std::move(device_info);
 }
 
+void BlePeripheral::UpdateIdToUuidMap(
+    const std::map<std::string, device::BluetoothUUID>&
+        service_id_to_fast_advertisement_service_uuid_map) {
+  service_id_to_fast_advertisement_service_uuid_map_ =
+      service_id_to_fast_advertisement_service_uuid_map;
+}
+
 }  // namespace chrome
 }  // namespace nearby
 }  // namespace location

chrome/services/sharing/nearby/platform_v2/ble_peripheral.h

@@ -5,6 +5,10 @@
 #ifndef CHROME_SERVICES_SHARING_NEARBY_PLATFORM_V2_BLE_PERIPHERAL_H_
 #define CHROME_SERVICES_SHARING_NEARBY_PLATFORM_V2_BLE_PERIPHERAL_H_
 
+#include <map>
+#include <string>
+
+#include "device/bluetooth/public/cpp/bluetooth_uuid.h"
 #include "device/bluetooth/public/mojom/adapter.mojom.h"
 #include "third_party/nearby/src/cpp/platform_v2/api/ble.h"
 
@@ -15,20 +19,28 @@ namespace chrome {
 // Concrete BlePeripheral implementation.
 class BlePeripheral : public api::BlePeripheral {
  public:
-  explicit BlePeripheral(bluetooth::mojom::DeviceInfoPtr device_info);
+  BlePeripheral(bluetooth::mojom::DeviceInfoPtr device_info,
+                const std::map<std::string, device::BluetoothUUID>&
+                    service_id_to_fast_advertisement_service_uuid_map);
   ~BlePeripheral() override;
 
   BlePeripheral(const BlePeripheral&) = delete;
   BlePeripheral& operator=(const BlePeripheral&) = delete;
+  BlePeripheral(BlePeripheral&&);
+  BlePeripheral& operator=(BlePeripheral&&);
 
   // api::BlePeripheral:
   std::string GetName() const override;
   ByteArray GetAdvertisementBytes(const std::string& service_id) const override;
 
   void UpdateDeviceInfo(bluetooth::mojom::DeviceInfoPtr device_info);
+  void UpdateIdToUuidMap(const std::map<std::string, device::BluetoothUUID>&
+                             service_id_to_fast_advertisement_service_uuid_map);
 
  private:
   bluetooth::mojom::DeviceInfoPtr device_info_;
+  std::map<std::string, device::BluetoothUUID>
+      service_id_to_fast_advertisement_service_uuid_map_;
 };
 
 }  // namespace chrome