Use FastPairDeviceRepository in FastPairSevice

PiperOrigin-RevId: 535386036

fastpair/BUILD

@@ -95,6 +95,7 @@ cc_library(
         ":fast_pair_plugin",
         ":fast_pair_seeker",
         "//fastpair/internal",
+        "//fastpair/repository:device_repository",
         "//internal/platform:types",
         "@com_google_absl//absl/container:flat_hash_map",
         "@com_google_absl//absl/status",

fastpair/dart/fast_pair_wrapper_impl.h

@@ -40,7 +40,7 @@ class FastPairWrapperImpl : public FastPairWrapper {
 
  private:
   SingleThreadExecutor executor_;
-  FastPairDeviceRepository devices_;
+  FastPairDeviceRepository devices_{&executor_};
   std::unique_ptr<ScannerBroker> scanner_broker_;
 
   // True if we are currently scanning for remote devices.

fastpair/fast_pair_service.cc

@@ -33,14 +33,8 @@ constexpr absl::Duration kTimeout = absl::Seconds(3);
 }
 
 FastPairService::FastPairService() {
-  seeker_ =
-      std::make_unique<FastPairSeekerImpl>(FastPairSeekerImpl::ServiceCallbacks{
-          .on_device_added =
-              [this](std::unique_ptr<FastPairDevice> device) {
-                AddDevice(std::move(device));
-              },
-          .on_device_lost =
-              [this](const FastPairDevice& device) { RemoveDevice(&device); },
+  seeker_ = std::make_unique<FastPairSeekerImpl>(
+      FastPairSeekerImpl::ServiceCallbacks{
           .on_initial_discovery =
               [this](const FastPairDevice& device,
                      InitialDiscoveryEvent event) {
@@ -66,7 +60,8 @@ FastPairService::FastPairService() {
           .on_ring_event =
               [this](const FastPairDevice& device, RingEvent event) {
                 OnRingEvent(device, std::move(event));
-              }});
+              }},
+      &executor_, &devices_);
 }
 
 absl::Status FastPairService::RegisterPluginProvider(
@@ -101,25 +96,6 @@ absl::Status FastPairService::UnregisterPluginProvider(absl::string_view name) {
                      : absl::DeadlineExceededError("Unregister plugin timeout");
 }
 
-void FastPairService::AddDevice(std::unique_ptr<FastPairDevice> device) {
-  NEARBY_LOGS(INFO) << "Add device " << *device;
-  executor_.Execute("add-device", [this, device = std::move(device)]() mutable {
-    devices_.push_back(std::move(device));
-  });
-}
-
-void FastPairService::RemoveDevice(const FastPairDevice* device) {
-  NEARBY_LOGS(INFO) << "Remove device " << *device;
-  executor_.Execute("remove-device", [this, device]() {
-    devices_.erase(
-        std::remove_if(devices_.begin(), devices_.end(),
-                       [&](const std::unique_ptr<FastPairDevice>& item) {
-                         return item.get() == device;
-                       }),
-        devices_.end());
-  });
-}
-
 void FastPairService::OnInitialDiscoveryEvent(const FastPairDevice& device,
                                               InitialDiscoveryEvent event) {
   executor_.Execute("on-initial-discovery", [this, device = &device,

fastpair/fast_pair_service.h

@@ -25,6 +25,7 @@
 #include "absl/strings/string_view.h"
 #include "fastpair/fast_pair_plugin.h"
 #include "fastpair/fast_pair_seeker.h"
+#include "fastpair/repository/fast_pair_device_repository.h"
 #include "internal/platform/single_thread_executor.h"
 
 namespace nearby {
@@ -51,8 +52,6 @@ class FastPairService {
   FastPairSeeker* GetSeeker() const { return seeker_.get(); }
 
  private:
-  void AddDevice(std::unique_ptr<FastPairDevice> device);
-  void RemoveDevice(const FastPairDevice* device);
   void OnInitialDiscoveryEvent(const FastPairDevice& device,
                                InitialDiscoveryEvent event);
   void OnSubsequentDiscoveryEvent(const FastPairDevice& device,
@@ -65,7 +64,7 @@ class FastPairService {
   std::unique_ptr<FastPairSeeker> seeker_;
   absl::flat_hash_map<std::string, std::unique_ptr<FastPairPluginProvider>>
       providers_;
-  std::vector<std::unique_ptr<FastPairDevice>> devices_;
+  FastPairDeviceRepository devices_{&executor_};
 };
 
 }  // namespace fastpair

fastpair/internal/BUILD

@@ -13,6 +13,8 @@ cc_library(
     deps = [
         "//fastpair:fast_pair_events",
         "//fastpair:fast_pair_seeker",
+        "//fastpair/repository:device_repository",
+        "//internal/platform:types",
         "@com_google_absl//absl/status",
     ],
 )

fastpair/internal/fast_pair_seeker_impl.cc

@@ -44,15 +44,14 @@ absl::Status FastPairSeekerImpl::StartFastPairScan() {
   // TODO(jsobczak): Replace with actual implementation
   auto device = std::make_unique<FastPairDevice>(
       "model_id", "11:22:33:44:55:66", Protocol::kFastPairInitialPairing);
-  test_device_ = device.get();
-  callbacks_.on_device_added(std::move(device));
+  test_device_ = devices_->AddDevice(std::move(device));
   callbacks_.on_initial_discovery(*test_device_, {});
   return absl::OkStatus();
 }
 
 absl::Status FastPairSeekerImpl::StopFastPairScan() {
   // TODO(jsobczak): Replace with actual implementation
-  callbacks_.on_device_lost(*test_device_);
+  devices_->RemoveDevice(test_device_);
   return absl::OkStatus();
 }
 

fastpair/internal/fast_pair_seeker_impl.h

@@ -20,6 +20,8 @@
 
 #include "fastpair/fast_pair_events.h"
 #include "fastpair/fast_pair_seeker.h"
+#include "fastpair/repository/fast_pair_device_repository.h"
+#include "internal/platform/single_thread_executor.h"
 
 namespace nearby {
 namespace fastpair {
@@ -35,9 +37,6 @@ class FastPairSeekerExt : public FastPairSeeker {
 class FastPairSeekerImpl : public FastPairSeekerExt {
  public:
   struct ServiceCallbacks {
-    absl::AnyInvocable<void(std::unique_ptr<FastPairDevice>)> on_device_added;
-    absl::AnyInvocable<void(const FastPairDevice&)> on_device_lost;
-
     absl::AnyInvocable<void(const FastPairDevice&, InitialDiscoveryEvent)>
         on_initial_discovery;
     absl::AnyInvocable<void(const FastPairDevice&, SubsequentDiscoveryEvent)>
@@ -50,8 +49,11 @@ class FastPairSeekerImpl : public FastPairSeekerExt {
     absl::AnyInvocable<void(const FastPairDevice&, RingEvent)> on_ring_event;
   };
 
-  explicit FastPairSeekerImpl(ServiceCallbacks callbacks)
-      : callbacks_(std::move(callbacks)) {}
+  FastPairSeekerImpl(ServiceCallbacks callbacks, SingleThreadExecutor* executor,
+                     FastPairDeviceRepository* devices)
+      : callbacks_(std::move(callbacks)),
+        executor_(executor),
+        devices_(devices) {}
 
   // From FastPairSeeker.
   absl::Status StartInitialPairing(FastPairDevice& device,
@@ -74,6 +76,9 @@ class FastPairSeekerImpl : public FastPairSeekerExt {
 
  private:
   ServiceCallbacks callbacks_;
+  SingleThreadExecutor* executor_;
+  FastPairDeviceRepository* devices_;
+
   FastPairDevice* test_device_ = nullptr;
 };
 

fastpair/repository/BUILD

@@ -39,7 +39,11 @@ cc_library(
     ],
     compatible_with = ["//buildenv/target:non_prod"],
     visibility = ["//fastpair:__subpackages__"],
-    deps = ["//fastpair/common"],
+    deps = [
+        "//fastpair/common",
+        "//internal/platform:logging",
+        "//internal/platform:types",
+    ],
 )
 
 cc_test(
@@ -50,6 +54,7 @@ cc_test(
     deps = [
         ":device_repository",
         "//fastpair/common",
+        "//internal/platform:types",
         "//internal/platform/implementation/g3",  # build_cleaner: keep
         "@com_github_protobuf_matchers//protobuf-matchers",
         "@com_google_googletest//:gtest_main",

fastpair/repository/fast_pair_device_repository.cc

@@ -19,12 +19,16 @@
 #include <optional>
 #include <utility>
 
+#include "internal/platform/logging.h"
+#include "internal/platform/mutex_lock.h"
+
 namespace nearby {
 namespace fastpair {
 
 FastPairDevice* FastPairDeviceRepository::AddDevice(
     std::unique_ptr<FastPairDevice> device) {
   const auto& id = device->GetUniqueId();
+  MutexLock lock(&mutex_);
   for (auto& item : devices_) {
     if (item->GetUniqueId() == id) {
       // Overwrite the existing object.
@@ -38,16 +42,19 @@ FastPairDevice* FastPairDeviceRepository::AddDevice(
 }
 
 void FastPairDeviceRepository::RemoveDevice(const FastPairDevice* device) {
-  devices_.erase(
-      std::remove_if(devices_.begin(), devices_.end(),
-                     [&](const std::unique_ptr<FastPairDevice>& item) {
-                       return item.get() == device;
-                     }),
-      devices_.end());
+  std::unique_ptr<FastPairDevice> fast_pair_device = ExtractDevice(device);
+  if (fast_pair_device == nullptr) return;
+  // Tasks running in the background may still be referencing `device`. Defering
+  // the destruction to the background thread should prevent use-after-free
+  // errors.
+  executor_->Execute([fast_pair_device = std::move(fast_pair_device)]() {
+    NEARBY_LOGS(VERBOSE) << "Destroyed FP device: " << fast_pair_device;
+  });
 }
 
 std::optional<FastPairDevice*> FastPairDeviceRepository::FindDevice(
     absl::string_view mac_address) {
+  MutexLock lock(&mutex_);
   auto it = std::find_if(devices_.begin(), devices_.end(),
                          [&](const std::unique_ptr<FastPairDevice>& device) {
                            return device->GetBleAddress() == mac_address ||
@@ -60,5 +67,18 @@ std::optional<FastPairDevice*> FastPairDeviceRepository::FindDevice(
   }
 }
 
+std::unique_ptr<FastPairDevice> FastPairDeviceRepository::ExtractDevice(
+    const FastPairDevice* device) {
+  MutexLock lock(&mutex_);
+  auto it = std::find_if(devices_.begin(), devices_.end(),
+                         [&](const std::unique_ptr<FastPairDevice>& item) {
+                           return item.get() == device;
+                         });
+  if (it == devices_.end()) return nullptr;
+  std::unique_ptr<FastPairDevice> fast_pair_device = std::move(*it);
+  devices_.erase(it);
+  return fast_pair_device;
+}
+
 }  // namespace fastpair
 }  // namespace nearby

fastpair/repository/fast_pair_device_repository.h

@@ -20,6 +20,8 @@
 #include <vector>
 
 #include "fastpair/common/fast_pair_device.h"
+#include "internal/platform/mutex.h"
+#include "internal/platform/single_thread_executor.h"
 
 namespace nearby {
 namespace fastpair {
@@ -27,6 +29,9 @@ namespace fastpair {
 // Owner of `FastPairDevice` instances.
 class FastPairDeviceRepository {
  public:
+  explicit FastPairDeviceRepository(SingleThreadExecutor* executor)
+      : executor_(executor) {}
+
   // Adds device to the repository and takes over ownership.
   // If a device with the same MAC address is already in the repository, it is
   // replaced.
@@ -42,7 +47,11 @@ class FastPairDeviceRepository {
   std::optional<FastPairDevice*> FindDevice(absl::string_view mac_address);
 
  private:
-  std::vector<std::unique_ptr<FastPairDevice>> devices_;
+  // Removes `device` from `devices_`.
+  std::unique_ptr<FastPairDevice> ExtractDevice(const FastPairDevice* device);
+  Mutex mutex_;
+  SingleThreadExecutor* executor_;
+  std::vector<std::unique_ptr<FastPairDevice>> devices_ ABSL_GUARDED_BY(mutex_);
 };
 
 }  // namespace fastpair

fastpair/repository/fast_pair_device_repository_test.cc

@@ -22,6 +22,7 @@
 #include "gtest/gtest.h"
 #include "fastpair/common/fast_pair_device.h"
 #include "fastpair/common/protocol.h"
+#include "internal/platform/single_thread_executor.h"
 
 namespace nearby {
 namespace fastpair {
@@ -32,7 +33,8 @@ constexpr absl::string_view kBleAddress = "AA:BB:CC:DD:EE:FF";
 constexpr absl::string_view kBtAddress = "12:34:56:78:90:AB";
 
 TEST(FastPairDeviceRepositoryTest, AddDevice) {
-  FastPairDeviceRepository repo;
+  SingleThreadExecutor executor;
+  FastPairDeviceRepository repo(&executor);
 
   FastPairDevice* device = repo.AddDevice(std::make_unique<FastPairDevice>(
       kModelId, kBleAddress, Protocol::kFastPairInitialPairing));
@@ -42,7 +44,8 @@ TEST(FastPairDeviceRepositoryTest, AddDevice) {
 }
 
 TEST(FastPairDeviceRepositoryTest, FindDeviceByBleAddress) {
-  FastPairDeviceRepository repo;
+  SingleThreadExecutor executor;
+  FastPairDeviceRepository repo(&executor);
   repo.AddDevice(std::make_unique<FastPairDevice>(
       kModelId, kBleAddress, Protocol::kFastPairInitialPairing));
 
@@ -55,7 +58,8 @@ TEST(FastPairDeviceRepositoryTest, FindDeviceByBleAddress) {
 }
 
 TEST(FastPairDeviceRepositoryTest, FindDeviceByBtAddress) {
-  FastPairDeviceRepository repo;
+  SingleThreadExecutor executor;
+  FastPairDeviceRepository repo(&executor);
   auto fast_pair_device =
       std::make_unique<FastPairDevice>(Protocol::kFastPairInitialPairing);
   fast_pair_device->SetPublicAddress(kBtAddress);
@@ -70,7 +74,8 @@ TEST(FastPairDeviceRepositoryTest, FindDeviceByBtAddress) {
 }
 
 TEST(FastPairDeviceRepositoryTest, RemoveDevice) {
-  FastPairDeviceRepository repo;
+  SingleThreadExecutor executor;
+  FastPairDeviceRepository repo(&executor);
   FastPairDevice* device = repo.AddDevice(std::make_unique<FastPairDevice>(
       kModelId, kBleAddress, Protocol::kFastPairInitialPairing));
 
@@ -79,6 +84,22 @@ TEST(FastPairDeviceRepositoryTest, RemoveDevice) {
   EXPECT_FALSE(repo.FindDevice(kBleAddress).has_value());
 }
 
+TEST(FastPairDeviceRepositoryTest, RemovingNonRegisteredDeviceIsSafe) {
+  SingleThreadExecutor executor;
+  FastPairDeviceRepository repo(&executor);
+  FastPairDevice* device = repo.AddDevice(std::make_unique<FastPairDevice>(
+      kModelId, kBleAddress, Protocol::kFastPairInitialPairing));
+  FastPairDevice other_device(Protocol::kFastPairInitialPairing);
+  repo.RemoveDevice(device);
+
+  // `device` already removed.
+  repo.RemoveDevice(device);
+  // `other_device` was never added.
+  repo.RemoveDevice(&other_device);
+
+  EXPECT_FALSE(repo.FindDevice(kBleAddress).has_value());
+}
+
 }  // namespace
 
 }  // namespace fastpair

fastpair/scanning/fastpair/fast_pair_discoverable_scanner_impl_test.cc

@@ -76,7 +76,7 @@ TEST(FastPairDiscoverableScannerImplTest, ValidModelId) {
   auto scanner = std::make_unique<FakeFastPairScanner>();
   auto repository = std::make_unique<FakeFastPairRepository>();
   SingleThreadExecutor executor;
-  FastPairDeviceRepository devices;
+  FastPairDeviceRepository devices(&executor);
   proto::Device metadata;
   metadata.set_device_type(proto::DeviceType::TRUE_WIRELESS_HEADPHONES);
   repository->SetFakeMetadata(kValidModelId, metadata);
@@ -103,7 +103,7 @@ TEST(FastPairDiscoverableScannerImplTest, InvalidModelId) {
   auto scanner = std::make_unique<FakeFastPairScanner>();
   auto repository = std::make_unique<FakeFastPairRepository>();
   SingleThreadExecutor executor;
-  FastPairDeviceRepository devices;
+  FastPairDeviceRepository devices(&executor);
   proto::Device metadata;
   metadata.set_device_type(proto::DeviceType::TRUE_WIRELESS_HEADPHONES);
   repository->SetFakeMetadata(kValidModelId, metadata);
@@ -130,7 +130,7 @@ TEST(FastPairDiscoverableScannerImplTest, NoServiceData) {
   auto scanner = std::make_unique<FakeFastPairScanner>();
   auto repository = std::make_unique<FakeFastPairRepository>();
   SingleThreadExecutor executor;
-  FastPairDeviceRepository devices;
+  FastPairDeviceRepository devices(&executor);
   proto::Device metadata;
   metadata.set_device_type(proto::DeviceType::TRUE_WIRELESS_HEADPHONES);
   repository->SetFakeMetadata(kValidModelId, metadata);
@@ -157,7 +157,7 @@ TEST(FastPairDiscoverableScannerImplTest, UnsupportedDeviceType) {
   auto scanner = std::make_unique<FakeFastPairScanner>();
   auto repository = std::make_unique<FakeFastPairRepository>();
   SingleThreadExecutor executor;
-  FastPairDeviceRepository devices;
+  FastPairDeviceRepository devices(&executor);
   proto::Device metadata;
   metadata.set_device_type(proto::DeviceType::AUTOMOTIVE);
   repository->SetFakeMetadata(kValidModelId, metadata);
@@ -184,7 +184,7 @@ TEST(FastPairDiscoverableScannerImplTest, UnsupportedNotifictionType) {
   auto scanner = std::make_unique<FakeFastPairScanner>();
   auto repository = std::make_unique<FakeFastPairRepository>();
   SingleThreadExecutor executor;
-  FastPairDeviceRepository devices;
+  FastPairDeviceRepository devices(&executor);
   proto::Device metadata;
   metadata.set_device_type(proto::DeviceType::HEADPHONES);
   metadata.set_notification_type(proto::NotificationType::APP_LAUNCH);
@@ -215,7 +215,7 @@ TEST(FastPairDiscoverableScannerImplTest, UnspecifiedNotificationType) {
   // the notification to be safe.
   auto repository = std::make_unique<FakeFastPairRepository>();
   SingleThreadExecutor executor;
-  FastPairDeviceRepository devices;
+  FastPairDeviceRepository devices(&executor);
   proto::Device metadata;
   metadata.set_device_type(proto::DeviceType::DEVICE_TYPE_UNSPECIFIED);
   metadata.set_notification_type(
@@ -246,7 +246,7 @@ TEST(FastPairDiscoverableScannerImplTest, V1NotificationType) {
   // type and a notification type of FAST_PAIR_ONE.
   auto repository = std::make_unique<FakeFastPairRepository>();
   SingleThreadExecutor executor;
-  FastPairDeviceRepository devices;
+  FastPairDeviceRepository devices(&executor);
   proto::Device metadata;
   metadata.set_device_type(proto::DeviceType::DEVICE_TYPE_UNSPECIFIED);
   metadata.set_notification_type(proto::NotificationType::FAST_PAIR_ONE);
@@ -276,7 +276,7 @@ TEST(FastPairDiscoverableScannerImplTest, V2NotificationType) {
   // type of TRUE_WIRELESS_HEADPHONES and a notification type of FAST_PAIR.
   auto repository = std::make_unique<FakeFastPairRepository>();
   SingleThreadExecutor executor;
-  FastPairDeviceRepository devices;
+  FastPairDeviceRepository devices(&executor);
   proto::Device metadata;
   metadata.set_device_type(proto::DeviceType::TRUE_WIRELESS_HEADPHONES);
   metadata.set_notification_type(proto::NotificationType::FAST_PAIR);
@@ -304,7 +304,7 @@ TEST(FastPairDiscoverableScannerImplTest, NearbyShareModelId) {
   auto scanner = std::make_unique<FakeFastPairScanner>();
   auto repository = std::make_unique<FakeFastPairRepository>();
   SingleThreadExecutor executor;
-  FastPairDeviceRepository devices;
+  FastPairDeviceRepository devices(&executor);
   proto::Device metadata;
   metadata.set_device_type(proto::DeviceType::TRUE_WIRELESS_HEADPHONES);
   repository->SetFakeMetadata(kValidModelId, metadata);
@@ -331,7 +331,7 @@ TEST(FastPairDiscoverableScannerImplTest,
   auto scanner = std::make_unique<FakeFastPairScanner>();
   auto repository = std::make_unique<FakeFastPairRepository>();
   SingleThreadExecutor executor;
-  FastPairDeviceRepository devices;
+  FastPairDeviceRepository devices(&executor);
   proto::Device metadata;
   metadata.set_device_type(proto::DeviceType::TRUE_WIRELESS_HEADPHONES);
   repository->SetFakeMetadata(kValidModelId, metadata);

fastpair/scanning/scanner_broker_impl_test.cc

@@ -75,7 +75,7 @@ TEST_F(ScannerBrokerImplTest, CanStartScanning) {
   std::string decoded_key;
   absl::Base64Unescape(kPublicAntiSpoof, &decoded_key);
   SingleThreadExecutor executor;
-  FastPairDeviceRepository devices;
+  FastPairDeviceRepository devices(&executor);
   proto::Device metadata;
   auto repository_ = std::make_unique<FakeFastPairRepository>();
   metadata.mutable_anti_spoofing_key_pair()->set_public_key(decoded_key);