From e2340027505c66c24db0bbb2e7955263caf94d1b Mon Sep 17 00:00:00 2001 From: Edwin Wu Date: Thu, 14 May 2026 18:11:32 -0700 Subject: [PATCH] Fixed a go/mobile_tsan error (data_race) found while running //third_party/nearby/internal/platform/implementation/apple/Tests:PlatformTest PiperOrigin-RevId: 915715369 --- .../apple/Mediums/BLE/GNCBLEGATTServer.m | 12 +- .../apple/Tests/GNCMultiThreadExecutorTest.mm | 2 +- .../Tests/GNCSingleThreadExecutorTest.mm | 16 +-- .../apple/Tests/GNCTimerTest.mm | 6 +- .../implementation/apple/Tests/UtilsTest.mm | 5 +- .../Tests/ble_l2cap_server_socket_test.mm | 6 +- .../apple/Tests/ble_medium_test.mm | 12 +- .../apple/Tests/ble_server_socket_test.mm | 6 +- .../implementation/apple/ble_medium.h | 9 +- .../implementation/apple/ble_medium.mm | 108 +++++++++++++----- .../platform/implementation/apple/utils.mm | 8 +- 11 files changed, 124 insertions(+), 66 deletions(-) diff --git a/internal/platform/implementation/apple/Mediums/BLE/GNCBLEGATTServer.m b/internal/platform/implementation/apple/Mediums/BLE/GNCBLEGATTServer.m index 1b9035ad..edf95082 100644 --- a/internal/platform/implementation/apple/Mediums/BLE/GNCBLEGATTServer.m +++ b/internal/platform/implementation/apple/Mediums/BLE/GNCBLEGATTServer.m @@ -64,6 +64,11 @@ static const int kMaxAdvertisementLengthOnIOS = 23; self = [super init]; if (self) { _queue = queue ?: dispatch_queue_create(kGNCBLEGATTServerQueueLabel, DISPATCH_QUEUE_SERIAL); + _services = [[NSMutableDictionary alloc] init]; + _pendingCharacteristics = [[NSMutableDictionary alloc] init]; + _characteristicValues = [[NSMutableDictionary alloc] init]; + _advertisementData = nil; + if (GNCFeatureFlags.sharedPeripheralManagerEnabled) { if (!peripheralManager) { // In shared mode, the peripheral manager must be injected. @@ -76,7 +81,7 @@ static const int kMaxAdvertisementLengthOnIOS = 23; // Legacy mode: Create a new manager if one isn't provided. if (!peripheralManager) { peripheralManager = [[CBPeripheralManager alloc] - initWithDelegate:self + initWithDelegate:nil queue:_queue options:@{CBPeripheralManagerOptionShowPowerAlertKey : @NO}]; } @@ -85,11 +90,6 @@ static const int kMaxAdvertisementLengthOnIOS = 23; // delegate. _peripheralManager.peripheralDelegate = self; } - - _services = [[NSMutableDictionary alloc] init]; - _pendingCharacteristics = [[NSMutableDictionary alloc] init]; - _characteristicValues = [[NSMutableDictionary alloc] init]; - _advertisementData = nil; } return self; } diff --git a/internal/platform/implementation/apple/Tests/GNCMultiThreadExecutorTest.mm b/internal/platform/implementation/apple/Tests/GNCMultiThreadExecutorTest.mm index 886ff646..2d794f65 100644 --- a/internal/platform/implementation/apple/Tests/GNCMultiThreadExecutorTest.mm +++ b/internal/platform/implementation/apple/Tests/GNCMultiThreadExecutorTest.mm @@ -93,7 +93,7 @@ using MultiThreadExecutor = ::nearby::api::SubmittableExecutor; dispatch_queue_t queue = dispatch_get_global_queue(DISPATCH_TARGET_QUEUE_DEFAULT, 0); XCTestExpectation *expectation = [self expectationWithDescription:@"finished"]; - const int kRunnableCount = 1000; + const int kRunnableCount = 100; for (int i = 0; i < kRunnableCount; i++) { executor->Execute([self]() { self.counter++; }); } diff --git a/internal/platform/implementation/apple/Tests/GNCSingleThreadExecutorTest.mm b/internal/platform/implementation/apple/Tests/GNCSingleThreadExecutorTest.mm index e5857cb8..8e5e1dbf 100644 --- a/internal/platform/implementation/apple/Tests/GNCSingleThreadExecutorTest.mm +++ b/internal/platform/implementation/apple/Tests/GNCSingleThreadExecutorTest.mm @@ -76,20 +76,14 @@ using SingleThreadExecutor = ::nearby::api::SubmittableExecutor; // Tests that shutting down an existing task allows to complete. - (void)testShutdownToAllowExistingTaskComplete { std::unique_ptr executor([self executor]); - - dispatch_queue_t queue = dispatch_get_global_queue(DISPATCH_TARGET_QUEUE_DEFAULT, 0); XCTestExpectation *expectation = [self expectationWithDescription:@"finished"]; - - executor->Execute([self]() { self.counter++; }); - - executor->Shutdown(); - - dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(0.2 * NSEC_PER_SEC)), queue, ^{ - XCTAssertEqual(self.counter, 1); + executor->Execute([self, expectation]() { + self.counter++; [expectation fulfill]; }); - - [self waitForExpectationsWithTimeout:0.5 handler:nil]; + executor->Shutdown(); + [self waitForExpectationsWithTimeout:1.0 handler:nil]; + XCTAssertEqual(self.counter, 1); } @end diff --git a/internal/platform/implementation/apple/Tests/GNCTimerTest.mm b/internal/platform/implementation/apple/Tests/GNCTimerTest.mm index 269bdf0f..32c895f1 100644 --- a/internal/platform/implementation/apple/Tests/GNCTimerTest.mm +++ b/internal/platform/implementation/apple/Tests/GNCTimerTest.mm @@ -65,7 +65,7 @@ auto timer = std::make_unique(); std::atomic fireCount = 0; - XCTAssertTrue(timer->Create(10, 10, [&]() { + XCTAssertTrue(timer->Create(100, 100, [&]() { if (fireCount.fetch_add(1) == 1) { dispatch_async(dispatch_get_main_queue(), ^{ [expectation fulfill]; @@ -73,9 +73,9 @@ } })); - [self waitForExpectationsWithTimeout:1.0 handler:nil]; + [self waitForExpectationsWithTimeout:2.0 handler:nil]; XCTAssertTrue(timer->Stop()); - XCTAssertEqual(fireCount.load(), 2); + XCTAssertGreaterThanOrEqual(fireCount.load(), 2); } - (void)testRestart { diff --git a/internal/platform/implementation/apple/Tests/UtilsTest.mm b/internal/platform/implementation/apple/Tests/UtilsTest.mm index cabf5d3d..5c513c1f 100644 --- a/internal/platform/implementation/apple/Tests/UtilsTest.mm +++ b/internal/platform/implementation/apple/Tests/UtilsTest.mm @@ -56,8 +56,9 @@ using ::nearby::ObjCStringFromCppString; - (void)testUUIDStringFromNSUUID { NSString *uuidString = @"E621E1F8-C36C-495A-93FC-0C247A3E6E5F"; NSUUID *uuid = [[NSUUID alloc] initWithUUIDString:uuidString]; - std::string expectedCppString = [uuidString UTF8String]; - XCTAssertEqual(nearby::UUIDStringFromNSUUID(uuid), expectedCppString); + XCTAssert(nearby::UUIDStringFromNSUUID(uuid) == + std::string([uuidString UTF8String], + [uuidString lengthOfBytesUsingEncoding:NSUTF8StringEncoding])); } - (void)testBluetoothUUIDConversions { diff --git a/internal/platform/implementation/apple/Tests/ble_l2cap_server_socket_test.mm b/internal/platform/implementation/apple/Tests/ble_l2cap_server_socket_test.mm index 88b47282..2be79381 100644 --- a/internal/platform/implementation/apple/Tests/ble_l2cap_server_socket_test.mm +++ b/internal/platform/implementation/apple/Tests/ble_l2cap_server_socket_test.mm @@ -50,8 +50,9 @@ - (void)testBleL2capServerSocketAccept { XCTestExpectation *expectation = [self expectationWithDescription:@"accept"]; + nearby::apple::BleL2capServerSocket *serverSocket = _serverSocket.get(); dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ - std::unique_ptr clientSocket = _serverSocket->Accept(); + std::unique_ptr clientSocket = serverSocket->Accept(); XCTAssertNotEqual(clientSocket.get(), nullptr); [expectation fulfill]; }); @@ -76,8 +77,9 @@ - (void)testBleL2capServerSocketClose { XCTestExpectation *expectation = [self expectationWithDescription:@"close"]; + nearby::apple::BleL2capServerSocket *serverSocket = _serverSocket.get(); dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ - std::unique_ptr clientSocket = _serverSocket->Accept(); + std::unique_ptr clientSocket = serverSocket->Accept(); XCTAssertEqual(clientSocket.get(), nullptr); [expectation fulfill]; }); diff --git a/internal/platform/implementation/apple/Tests/ble_medium_test.mm b/internal/platform/implementation/apple/Tests/ble_medium_test.mm index 491ee93f..a8fb6199 100644 --- a/internal/platform/implementation/apple/Tests/ble_medium_test.mm +++ b/internal/platform/implementation/apple/Tests/ble_medium_test.mm @@ -776,18 +776,20 @@ static const char *const kTestServiceID = "TestServiceID"; NSDictionary *serviceData = @{[CBUUID UUIDWithString:kTestServiceUUIDString] : [NSData dataWithBytes:"test" length:4]}; - __block XCTestExpectation *expectation1 = [self expectationWithDescription:@"Callback 1"]; + XCTestExpectation *expectation1 = [self expectationWithDescription:@"Callback 1"]; XCTestExpectation *expectation2 = [self expectationWithDescription:@"Callback 2"]; expectation2.inverted = YES; // Should NOT be called. + auto callback1_fulfilled = std::make_shared>(false); + nearby::api::ble::BleMedium::ScanCallback callback = { .advertisement_found_cb = std::function( - ^(nearby::api::ble::BlePeripheral::UniqueId peripheral_id, - const nearby::api::ble::BleAdvertisementData &advertisement) { - if ([expectation1.description isEqualToString:@"Callback 1"]) { + [callback1_fulfilled, expectation1, expectation2]( + nearby::api::ble::BlePeripheral::UniqueId peripheral_id, + const nearby::api::ble::BleAdvertisementData &advertisement) { + if (!callback1_fulfilled->exchange(true)) { [expectation1 fulfill]; - expectation1 = nil; // Prevent double fulfillment } else { [expectation2 fulfill]; } diff --git a/internal/platform/implementation/apple/Tests/ble_server_socket_test.mm b/internal/platform/implementation/apple/Tests/ble_server_socket_test.mm index 1b887880..a9574f96 100644 --- a/internal/platform/implementation/apple/Tests/ble_server_socket_test.mm +++ b/internal/platform/implementation/apple/Tests/ble_server_socket_test.mm @@ -42,8 +42,9 @@ - (void)testBleServerSocketAccept { XCTestExpectation *expectation = [self expectationWithDescription:@"accept"]; + nearby::apple::BleServerSocket *serverSocket = _serverSocket.get(); dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ - std::unique_ptr clientSocket = _serverSocket->Accept(); + std::unique_ptr clientSocket = serverSocket->Accept(); XCTAssertNotEqual(clientSocket.get(), nullptr); [expectation fulfill]; }); @@ -57,8 +58,9 @@ - (void)testBleServerSocketClose { XCTestExpectation *expectation = [self expectationWithDescription:@"close"]; + nearby::apple::BleServerSocket *serverSocket = _serverSocket.get(); dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ - std::unique_ptr clientSocket = _serverSocket->Accept(); + std::unique_ptr clientSocket = serverSocket->Accept(); XCTAssertEqual(clientSocket.get(), nullptr); [expectation fulfill]; }); diff --git a/internal/platform/implementation/apple/ble_medium.h b/internal/platform/implementation/apple/ble_medium.h index 28438113..34695757 100644 --- a/internal/platform/implementation/apple/ble_medium.h +++ b/internal/platform/implementation/apple/ble_medium.h @@ -245,13 +245,16 @@ class BleMedium : public api::ble::BleMedium { GNSPeripheralServiceManager *socketPeripheralServiceManager_; GNSPeripheralManager *socketPeripheralManager_; - GNSCentralManager *socketCentralManager_; + + absl::Mutex scanning_mutex_; + GNSCentralManager *socketCentralManager_ ABSL_GUARDED_BY(scanning_mutex_); // Used for the blocking version of StartAdvertising and only has an advertisement found callback. - api::ble::BleMedium::ScanCallback scan_cb_; + std::shared_ptr scan_cb_ ABSL_GUARDED_BY(scanning_mutex_); // Used for the async version of StartAdvertising and has both an advertisement found and result // callback. - api::ble::BleMedium::ScanningCallback scanning_cb_; + std::shared_ptr scanning_cb_ + ABSL_GUARDED_BY(scanning_mutex_); // Used for the BleServerSocket. absl::Mutex server_socket_mutex_; diff --git a/internal/platform/implementation/apple/ble_medium.mm b/internal/platform/implementation/apple/ble_medium.mm index dcb1d19f..1120f8b6 100644 --- a/internal/platform/implementation/apple/ble_medium.mm +++ b/internal/platform/implementation/apple/ble_medium.mm @@ -173,11 +173,19 @@ void BleMedium::HandleAdvertisementFound(id peripheral, } #endif - if (scanning_cb_.advertisement_found_cb) { - scanning_cb_.advertisement_found_cb(unique_id, data); + std::shared_ptr scanning_cb; + std::shared_ptr scan_cb; + { + absl::MutexLock lock(&scanning_mutex_); + scanning_cb = scanning_cb_; + scan_cb = scan_cb_; } - if (scan_cb_.advertisement_found_cb) { - scan_cb_.advertisement_found_cb(unique_id, data); + + if (scanning_cb && scanning_cb->advertisement_found_cb) { + scanning_cb->advertisement_found_cb(unique_id, data); + } + if (scan_cb && scan_cb->advertisement_found_cb) { + scan_cb->advertisement_found_cb(unique_id, data); } } @@ -185,7 +193,17 @@ std::unique_ptr BleMedium::StartScanning( const Uuid &service_uuid, api::ble::TxPowerLevel tx_power_level, api::ble::BleMedium::ScanningCallback callback) { CBUUID *serviceUUID = CBUUID128FromCPP(service_uuid); - scanning_cb_ = std::move(callback); + + { + absl::MutexLock lock(&scanning_mutex_); + scanning_cb_ = std::make_shared(std::move(callback)); + + if (central_manager_factory_) { + socketCentralManager_ = central_manager_factory_(serviceUUID); + } else { + socketCentralManager_ = [[GNSCentralManager alloc] initWithSocketServiceUUID:serviceUUID]; + } + } // Clear the map of discovered peripherals only when we are starting a new scan. If we cleared the // map every time we stopped a scan, we would not be able to connect to peripherals that we @@ -193,12 +211,10 @@ std::unique_ptr BleMedium::StartScanning( peripherals_.Clear(); ClearAdvertisementPacketsMap(); - if (central_manager_factory_) { - socketCentralManager_ = central_manager_factory_(serviceUUID); - } else { - socketCentralManager_ = [[GNSCentralManager alloc] initWithSocketServiceUUID:serviceUUID]; + { + absl::MutexLock lock(&scanning_mutex_); + [socketCentralManager_ startNoScanModeWithAdvertisedServiceUUIDs:@[ serviceUUID ]]; } - [socketCentralManager_ startNoScanModeWithAdvertisedServiceUUIDs:@[ serviceUUID ]]; dispatch_semaphore_t semaphore = dispatch_semaphore_create(0); __block NSError *blockError = nil; @@ -211,8 +227,13 @@ std::unique_ptr BleMedium::StartScanning( } completionHandler:^(NSError *error) { blockError = error; - if (scanning_cb_.start_scanning_result) { - scanning_cb_.start_scanning_result( + std::shared_ptr scanning_cb; + { + absl::MutexLock lock(&scanning_mutex_); + scanning_cb = scanning_cb_; + } + if (scanning_cb && scanning_cb->start_scanning_result) { + scanning_cb->start_scanning_result( error == nil ? absl::OkStatus() : absl::InternalError(error.localizedDescription.UTF8String)); } @@ -222,8 +243,13 @@ std::unique_ptr BleMedium::StartScanning( dispatch_time_t timeout = dispatch_time(DISPATCH_TIME_NOW, kApiTimeoutInSeconds * NSEC_PER_SEC); if (dispatch_semaphore_wait(semaphore, timeout) != 0) { GNCLoggerError(@"Start scanning operation timed out."); - if (scanning_cb_.start_scanning_result) { - scanning_cb_.start_scanning_result(absl::DeadlineExceededError("Start scanning timed out")); + std::shared_ptr scanning_cb; + { + absl::MutexLock lock(&scanning_mutex_); + scanning_cb = scanning_cb_; + } + if (scanning_cb && scanning_cb->start_scanning_result) { + scanning_cb->start_scanning_result(absl::DeadlineExceededError("Start scanning timed out")); } return nullptr; } @@ -243,7 +269,17 @@ std::unique_ptr BleMedium::StartScanning( bool BleMedium::StartScanning(const Uuid &service_uuid, api::ble::TxPowerLevel tx_power_level, api::ble::BleMedium::ScanCallback callback) { CBUUID *serviceUUID = CBUUID128FromCPP(service_uuid); - scan_cb_ = std::move(callback); + + { + absl::MutexLock lock(&scanning_mutex_); + scan_cb_ = std::make_shared(std::move(callback)); + + if (central_manager_factory_) { + socketCentralManager_ = central_manager_factory_(serviceUUID); + } else { + socketCentralManager_ = [[GNSCentralManager alloc] initWithSocketServiceUUID:serviceUUID]; + } + } // Clear the map of discovered peripherals only when we are starting a new scan. If we cleared the // map every time we stopped a scan, we would not be able to connect to peripherals that we @@ -251,12 +287,10 @@ bool BleMedium::StartScanning(const Uuid &service_uuid, api::ble::TxPowerLevel t peripherals_.Clear(); ClearAdvertisementPacketsMap(); - if (central_manager_factory_) { - socketCentralManager_ = central_manager_factory_(serviceUUID); - } else { - socketCentralManager_ = [[GNSCentralManager alloc] initWithSocketServiceUUID:serviceUUID]; + { + absl::MutexLock lock(&scanning_mutex_); + [socketCentralManager_ startNoScanModeWithAdvertisedServiceUUIDs:@[ serviceUUID ]]; } - [socketCentralManager_ startNoScanModeWithAdvertisedServiceUUIDs:@[ serviceUUID ]]; dispatch_semaphore_t semaphore = dispatch_semaphore_create(0); __block NSError *blockError = nil; @@ -294,7 +328,16 @@ bool BleMedium::StartMultipleServicesScanning(const std::vector &service_u [serviceUUIDs addObject:CBUUID128FromCPP(service_uuid)]; } - scan_cb_ = std::move(callback); + { + absl::MutexLock lock(&scanning_mutex_); + scan_cb_ = std::make_shared(std::move(callback)); + + if (central_manager_factory_) { + socketCentralManager_ = central_manager_factory_(serviceUUIDs[0]); + } else { + socketCentralManager_ = [[GNSCentralManager alloc] initWithSocketServiceUUID:serviceUUIDs[0]]; + } + } // Clear the map of discovered peripherals only when we are starting a new scan. If we cleared the // map every time we stopped a scan, we would not be able to connect to peripherals that we @@ -302,12 +345,10 @@ bool BleMedium::StartMultipleServicesScanning(const std::vector &service_u peripherals_.Clear(); ClearAdvertisementPacketsMap(); - if (central_manager_factory_) { - socketCentralManager_ = central_manager_factory_(serviceUUIDs[0]); - } else { - socketCentralManager_ = [[GNSCentralManager alloc] initWithSocketServiceUUID:serviceUUIDs[0]]; + { + absl::MutexLock lock(&scanning_mutex_); + [socketCentralManager_ startNoScanModeWithAdvertisedServiceUUIDs:@[ serviceUUIDs[0] ]]; } - [socketCentralManager_ startNoScanModeWithAdvertisedServiceUUIDs:@[ serviceUUIDs[0] ]]; dispatch_semaphore_t semaphore = dispatch_semaphore_create(0); __block NSError *blockError = nil; @@ -333,7 +374,12 @@ bool BleMedium::StartMultipleServicesScanning(const std::vector &service_u } bool BleMedium::StopScanning() { - [socketCentralManager_ stopNoScanMode]; + { + absl::MutexLock lock(&scanning_mutex_); + [socketCentralManager_ stopNoScanMode]; + scan_cb_ = nullptr; + scanning_cb_ = nullptr; + } dispatch_semaphore_t semaphore = dispatch_semaphore_create(0); __block NSError *blockError = nil; @@ -694,8 +740,12 @@ std::unique_ptr BleMedium::Connect( return nullptr; } - GNSCentralPeerManager *updatedCentralPeerManager = - [socketCentralManager_ retrieveCentralPeerWithIdentifier:peripheral.identifier]; + GNSCentralPeerManager *updatedCentralPeerManager; + { + absl::MutexLock lock(&scanning_mutex_); + updatedCentralPeerManager = + [socketCentralManager_ retrieveCentralPeerWithIdentifier:peripheral.identifier]; + } if (!updatedCentralPeerManager) { return nullptr; } diff --git a/internal/platform/implementation/apple/utils.mm b/internal/platform/implementation/apple/utils.mm index 386e1036..e517310d 100644 --- a/internal/platform/implementation/apple/utils.mm +++ b/internal/platform/implementation/apple/utils.mm @@ -27,11 +27,15 @@ bool CppBoolFromObjCBool(BOOL b) { return b ? true : false; } char CharFromNSNumber(NSNumber* n) { return n.charValue; } NSString* ObjCStringFromCppString(absl::string_view s) { - return [NSString stringWithUTF8String:s.data()]; + return [[NSString alloc] initWithBytes:s.data() length:s.size() encoding:NSUTF8StringEncoding]; } std::string CppStringFromObjCString(NSString* s) { - return std::string([s UTF8String], [s lengthOfBytesUsingEncoding:NSUTF8StringEncoding]); + if (!s) return std::string(); + const char* cstr = [s UTF8String]; + if (!cstr) return std::string(); + NSUInteger len = [s lengthOfBytesUsingEncoding:NSUTF8StringEncoding]; + return std::string(cstr, len); } NSData* NSDataFromByteArray(ByteArray byteArray) {