Resolve unique_ptr being thrown away and potential deadlock issue

PiperOrigin-RevId: 487979164
2026-09-16 15:36:12 -04:00 · 2022-11-11 20:30:27 -08:00
parent 0a88efb1de
commit b7de5f4097
3 changed files with 145 additions and 74 deletions
@@ -16,7 +16,9 @@
 #define THIRD_PARTY_NEARBY_PRESENCE_SCAN_CALLBACK_H_

 #include <functional>
+#include <utility>

+#include "absl/functional/any_invocable.h"
 #include "presence/presence_device.h"
 #include "presence/status.h"

@@ -30,8 +32,8 @@ class ScanSession {
  ScanSession()
      : stop_scan_callback_(
            []() { return Status{Status::Value::kNotImplemented}; }) {}
-  explicit ScanSession(std::function<Status(void)> stop_scan_callback)
-      : stop_scan_callback_(stop_scan_callback) {}
+  explicit ScanSession(absl::AnyInvocable<Status(void)> stop_scan_callback)
+      : stop_scan_callback_(std::move(stop_scan_callback)) {}

  Status StopScan() {
    return stop_scan_callback_();
@@ -41,7 +43,7 @@ class ScanSession {
  // Nearby library would provide the implementation of this callback in
  // runtime. Assigning with a default value NotImplemented to surface potential
  // issue where library failed to provide the implementation.
-  std::function<Status(void)> stop_scan_callback_;
+  absl::AnyInvocable<Status(void)> stop_scan_callback_;
 };

 // Callers would provide the implementation of these callbacks. If callers
@@ -19,6 +19,7 @@
 #include <memory>
 #include <string>
 #include <utility>
+#include <vector>

 #include "absl/random/random.h"
 #include "absl/random/uniform_int_distribution.h"
@@ -76,14 +77,18 @@ ScanSession ScanManager::StartScan(ScanRequest scan_request, ScanCallback cb) {
  std::unique_ptr<ScanningSession> scanning_session =
      mediums_->GetBle().StartScanning(scan_request, std::move(callback));
  auto modified_scanning_session = ScanSession(
-      [scanning_session_cb = std::move(scanning_session->stop_scanning), this,
-       id]() {
-        absl::MutexLock lock(&mutex_);
-        int erased = absl::erase_if(
-            scanning_callbacks_,
-            [id](const auto& entry) { return id == entry.first; });
-        if (erased == 0) return Status{.value = Status::Value::kError};
-        BleOperationStatus st = scanning_session_cb();
+      /*stop_scan_callback=*/[scanning_session_internal =
+                                  std::move(scanning_session),
+                              this, id]() {
+        {
+          absl::MutexLock lock(&mutex_);
+          int erased = absl::erase_if(
+              scanning_callbacks_,
+              [id](const auto& entry) { return id == entry.first; });
+          if (erased == 0) return Status{.value = Status::Value::kError};
+          // Unlock mutex since we don't need to access the list anymore.
+        }
+        BleOperationStatus st = scanning_session_internal->stop_scanning();
        if (st != BleOperationStatus::kSucceeded) {
          return Status{.value = Status::Value::kError};
        }
@@ -94,22 +99,28 @@ ScanSession ScanManager::StartScan(ScanRequest scan_request, ScanCallback cb) {

 void ScanManager::NotifyFoundBle(BleAdvertisementData data,
                                 const BlePeripheral& peripheral) {
-  absl::MutexLock lock(&mutex_);
-  auto advertisement_data =
-      data.service_data[kPresenceServiceUuid].AsStringView();
-  for (const auto& entry : scanning_callbacks_) {
-    auto candidate = entry.second;
-    auto advert = candidate.decoder.DecodeAdvertisement(advertisement_data);
-    if (!advert.ok()) {
-      // This advertisement is not relevant to the current element, skip.
-      continue;
-    }
-    if (candidate.decoder.MatchesScanFilter(advert.value())) {
-      // TODO(b/256913915): Provide more information in PresenceDevice once
-      // fully implemented
-      candidate.callback.on_discovered_cb(PresenceDevice());
+  std::vector<ScanCallback> callbacks;
+  {
+    absl::MutexLock lock(&mutex_);
+    auto advertisement_data =
+        data.service_data[kPresenceServiceUuid].AsStringView();
+    for (const auto& entry : scanning_callbacks_) {
+      auto candidate = entry.second;
+      auto advert = candidate.decoder.DecodeAdvertisement(advertisement_data);
+      if (!advert.ok()) {
+        // This advertisement is not relevant to the current element, skip.
+        continue;
+      }
+      if (candidate.decoder.MatchesScanFilter(advert.value())) {
+        callbacks.push_back(candidate.callback);
+      }
    }
  }
+  // TODO(b/256913915): Provide more information in PresenceDevice once fully
+  // implemented
+  for (const auto& callback : callbacks) {
+    callback.on_discovered_cb(PresenceDevice());
+  }
 }

 }  // namespace presence
@@ -44,6 +44,8 @@ using AdvertisingSession =
 using AdvertisingCallback =
    ::location::nearby::api::ble_v2::BleMedium::AdvertisingCallback;

+using CountDownLatch = location::nearby::CountDownLatch;
+
 class ScanManagerTest : public testing::Test {
 protected:
  void SetUp() override { env_.Start(); }
@@ -52,7 +54,7 @@ class ScanManagerTest : public testing::Test {
  std::unique_ptr<AdvertisingSession> StartAdvertisingOn(Ble& ble) {
    PresenceBroadcast::BroadcastSection section = {
        .identity = internal::IDENTITY_TYPE_PUBLIC,
-        .extended_properties = extended_properties_,
+        .extended_properties = MakeDefaultExtendedProperties(),
        .account_name = "Test account"};
    PresenceBroadcast presence_request = {.sections = {section}};
    BroadcastRequest input = {.tx_power = 30, .variant = presence_request};
@@ -71,43 +73,47 @@ class ScanManagerTest : public testing::Test {
    return session;
  }

-  std::vector<nearby::internal::IdentityType> identity_types_ = {
-      nearby::internal::IdentityType::IDENTITY_TYPE_PUBLIC,
-  };
-  std::vector<DataElement> extended_properties_ = {
-      DataElement(ActionBit::kPresenceManagerAction)};
-  std::vector<absl::variant<PresenceScanFilter, LegacyPresenceScanFilter>>
-      filters_ = {PresenceScanFilter{
-          .scan_type = ScanType::kPresenceScan,
-          .extended_properties = extended_properties_,
-      }};
-  ScanRequest scan_request_ = {
-      .account_name = "Test account",
-      .identity_types = identity_types_,
-      .scan_filters = filters_,
-      .use_ble = true,
-      .scan_type = ScanType::kPresenceScan,
-      .power_mode = PowerMode::kBalanced,
-      .scan_only_when_screen_on = true,
-  };
+  ScanRequest MakeDefaultScanRequest() {
+    std::vector<absl::variant<PresenceScanFilter, LegacyPresenceScanFilter>>
+        filters = {PresenceScanFilter{
+            .scan_type = ScanType::kPresenceScan,
+            .extended_properties = MakeDefaultExtendedProperties(),
+        }};
+    return {
+        .account_name = "Test account",
+        .identity_types = MakeDefaultIdentityTypes(),
+        .scan_filters = filters,
+        .use_ble = true,
+        .scan_type = ScanType::kPresenceScan,
+        .power_mode = PowerMode::kBalanced,
+        .scan_only_when_screen_on = true,
+    };
+  }
+
+  ScanCallback MakeDefaultScanCallback() {
+    return {.start_scan_cb =
+                [this](Status status) {
+                  if (status.Ok()) {
+                    start_latch_.CountDown();
+                  }
+                },
+            .on_discovered_cb =
+                [this](PresenceDevice pd) { found_latch_.CountDown(); }};
+  }
+
+  std::vector<nearby::internal::IdentityType> MakeDefaultIdentityTypes() {
+    return {
+        nearby::internal::IdentityType::IDENTITY_TYPE_PUBLIC,
+    };
+  }
+  std::vector<DataElement> MakeDefaultExtendedProperties() {
+    return {DataElement(ActionBit::kPresenceManagerAction)};
+  }
  CredentialManagerImpl credential_manager_;
  location::nearby::MediumEnvironment& env_ = {
      location::nearby::MediumEnvironment::Instance()};
-  location::nearby::CountDownLatch start_latch_{1};
-  location::nearby::CountDownLatch found_latch_{1};
-  ScanCallback scanning_callback_ = {.start_scan_cb =
-                                         [this](Status status) {
-                                           NEARBY_LOGS(INFO) << "Scan started "
-                                                             << status.Ok();
-                                           if (status.Ok()) {
-                                             start_latch_.CountDown();
-                                           }
-                                         },
-                                     .on_discovered_cb =
-                                         [this](PresenceDevice pd) {
-                                           NEARBY_LOGS(INFO) << "Device found";
-                                           found_latch_.CountDown();
-                                         }};
+  CountDownLatch start_latch_{1};
+  CountDownLatch found_latch_{1};
 };

 TEST_F(ScanManagerTest, CanStartThenStopScanning) {
@@ -121,9 +127,8 @@ TEST_F(ScanManagerTest, CanStartThenStopScanning) {

  // Start scanning
  ScanSession scan_session =
-      manager.StartScan(scan_request_, scanning_callback_);
+      manager.StartScan(MakeDefaultScanRequest(), MakeDefaultScanCallback());
  EXPECT_EQ(manager.ScanningCallbacksLengthForTest(), 1);
-  ASSERT_TRUE(mediums.GetBle().IsAvailable());
  EXPECT_TRUE(start_latch_.Await().Ok());
  EXPECT_TRUE(found_latch_.Await().Ok());
  EXPECT_TRUE(scan_session.StopScan().Ok());
@@ -134,7 +139,8 @@ TEST_F(ScanManagerTest, CannotStopScanTwice) {
  Mediums mediums;
  ScanManager manager(mediums, credential_manager_);

-  auto scan_session = manager.StartScan(scan_request_, scanning_callback_);
+  auto scan_session =
+      manager.StartScan(MakeDefaultScanRequest(), MakeDefaultScanCallback());

  NEARBY_LOGS(INFO) << "Start scan";
  EXPECT_TRUE(start_latch_.Await().Ok());
@@ -156,25 +162,77 @@ TEST_F(ScanManagerTest, TestNoFilter) {
      StartAdvertisingOn(ble2);

  // Start scanning
-  ScanRequest scan_request = {
-      .account_name = "Test account",
-      .identity_types = identity_types_,
-      .scan_filters = {},
-      .use_ble = true,
-      .scan_type = ScanType::kPresenceScan,
-      .power_mode = PowerMode::kBalanced,
-      .scan_only_when_screen_on = true,
-  };
+  ScanRequest scan_request_no_filter = MakeDefaultScanRequest();
+  scan_request_no_filter.scan_filters.clear();
  ScanSession scan_session =
-      manager.StartScan(scan_request, scanning_callback_);
+      manager.StartScan(scan_request_no_filter, MakeDefaultScanCallback());

-  EXPECT_EQ(manager.ScanningCallbacksLengthForTest(), 1);
+  ASSERT_EQ(manager.ScanningCallbacksLengthForTest(), 1);
  ASSERT_TRUE(mediums.GetBle().IsAvailable());
  EXPECT_TRUE(start_latch_.Await().Ok());
  EXPECT_TRUE(found_latch_.Await().Ok());
  EXPECT_TRUE(scan_session.StopScan().Ok());
  EXPECT_EQ(manager.ScanningCallbacksLengthForTest(), 0);
 }
+
+TEST_F(ScanManagerTest, StopOneSessionFromAnotherDeadlock) {
+  Mediums mediums;
+  ScanManager manager(mediums, credential_manager_);
+  CountDownLatch start_latch2{1};
+  CountDownLatch found_latch2{1};
+
+  // Start scanning
+  std::vector<DataElement> extended_properties_mismatch = {
+      DataElement(ActionBit::kInstantTetheringAction)};
+  std::vector<absl::variant<PresenceScanFilter, LegacyPresenceScanFilter>>
+      mismatch_filters = {PresenceScanFilter{
+          .scan_type = ScanType::kPresenceScan,
+          .extended_properties = extended_properties_mismatch,
+      }};
+  ScanRequest scan_request_mismatch = {
+      .account_name = "Test account",
+      .identity_types = MakeDefaultIdentityTypes(),
+      .scan_filters = mismatch_filters,
+      .use_ble = true,
+      .scan_type = ScanType::kPresenceScan,
+      .power_mode = PowerMode::kBalanced,
+      .scan_only_when_screen_on = true,
+  };
+  // we use scan_request_mismatch so this session's discovery doesn't get
+  // triggered.
+  ScanSession scan_session =
+      manager.StartScan(scan_request_mismatch, MakeDefaultScanCallback());
+  ScanCallback scanning_callback2 = {
+      .start_scan_cb =
+          [&start_latch2](Status status) {
+            if (status.Ok()) {
+              start_latch2.CountDown();
+            }
+          },
+      .on_discovered_cb =
+          [&found_latch2, &scan_session](PresenceDevice pd) {
+            found_latch2.CountDown();
+            scan_session.StopScan();
+          }};
+  ScanSession scan_session2 = manager.StartScan(MakeDefaultScanRequest(),
+                                                std::move(scanning_callback2));
+
+  ASSERT_EQ(manager.ScanningCallbacksLengthForTest(), 2);
+
+  // Set up advertiser
+  location::nearby::BluetoothAdapter server_adapter;
+  Ble ble2(server_adapter);
+  std::unique_ptr<AdvertisingSession> advertising_session =
+      StartAdvertisingOn(ble2);
+
+  EXPECT_TRUE(found_latch2.Await(absl::Milliseconds(1500)).result());
+  EXPECT_FALSE(found_latch_.Await(absl::Milliseconds(1500)).result());
+  // Session was stopped before, this should not be able to stop successfully.
+  EXPECT_FALSE(scan_session.StopScan().Ok());
+  EXPECT_EQ(manager.ScanningCallbacksLengthForTest(), 1);
+  EXPECT_TRUE(scan_session2.StopScan().Ok());
+  EXPECT_EQ(manager.ScanningCallbacksLengthForTest(), 0);
+}
 }  // namespace
 }  // namespace presence
 }  // namespace nearby