From 85d1a42e5a101b336dce608e16a8a7013d22ec9a Mon Sep 17 00:00:00 2001
From: Vivien Nicolas <vnicolas@apple.com>
Date: Tue, 7 Mar 2023 13:40:00 +0100
Subject: [PATCH] [darwin] Race when BleConnection stop is called

---
 .../Darwin/BleConnectionDelegateImpl.mm       | 129 +++++++++++-------
 1 file changed, 80 insertions(+), 49 deletions(-)

diff --git a/src/platform/Darwin/BleConnectionDelegateImpl.mm b/src/platform/Darwin/BleConnectionDelegateImpl.mm
index d69623006d84ab..69973669318a62 100644
--- a/src/platform/Darwin/BleConnectionDelegateImpl.mm
+++ b/src/platform/Darwin/BleConnectionDelegateImpl.mm
@@ -32,6 +32,7 @@
 #include <lib/support/logging/CHIPLogging.h>
 #include <platform/CHIPDeviceLayer.h>
 #include <platform/Darwin/BleConnectionDelegate.h>
+#include <platform/LockTracker.h>
 #include <setup_payload/SetupPayload.h>
 
 #import "UUIDHelper.h"
@@ -40,11 +41,12 @@
 
 constexpr uint64_t kScanningWithDiscriminatorTimeoutInSeconds = 60;
 constexpr uint64_t kScanningWithoutDiscriminatorTimeoutInSeconds = 120;
+constexpr const char * kBleWorkQueueName = "org.csa-iot.matter.framework.ble.workqueue";
 
 @interface BleConnection : NSObject <CBCentralManagerDelegate, CBPeripheralDelegate>
 
-@property (strong, nonatomic) dispatch_queue_t workQueue;
 @property (strong, nonatomic) dispatch_queue_t chipWorkQueue;
+@property (strong, nonatomic) dispatch_queue_t workQueue;
 @property (strong, nonatomic) CBCentralManager * centralManager;
 @property (strong, nonatomic) CBPeripheral * peripheral;
 @property (strong, nonatomic) CBUUID * shortServiceUUID;
@@ -58,7 +60,8 @@ @interface BleConnection : NSObject <CBCentralManagerDelegate, CBPeripheralDeleg
 @property (unsafe_unretained, nonatomic) BleConnectionDelegate::OnConnectionErrorFunct onConnectionError;
 @property (unsafe_unretained, nonatomic) chip::Ble::BleLayer * mBleLayer;
 
-- (id)initWithDiscriminator:(const chip::SetupDiscriminator &)deviceDiscriminator;
+- (id)initWithQueue:(dispatch_queue_t)queue;
+- (id)initWithDiscriminator:(const chip::SetupDiscriminator &)deviceDiscriminator queue:(dispatch_queue_t)queue;
 - (void)setBleLayer:(chip::Ble::BleLayer *)bleLayer;
 - (void)start;
 - (void)stop;
@@ -72,57 +75,80 @@ - (void)update;
 namespace DeviceLayer {
     namespace Internal {
         BleConnection * ble;
+        dispatch_queue_t bleWorkQueue;
 
         void BleConnectionDelegateImpl::NewConnection(
             Ble::BleLayer * bleLayer, void * appState, const SetupDiscriminator & deviceDiscriminator)
         {
+            assertChipStackLockedByCurrentThread();
+
             ChipLogProgress(Ble, "%s", __FUNCTION__);
+            if (!bleWorkQueue) {
+                bleWorkQueue = dispatch_queue_create(kBleWorkQueueName, DISPATCH_QUEUE_SERIAL_WITH_AUTORELEASE_POOL);
+            }
+
+            dispatch_async(bleWorkQueue, ^{
+                // If the previous connection delegate was a scan without a discriminator, just reuse it instead of
+                // creating a brand new connection but update the discriminator and the ble layer members.
+                if (ble and ![ble hasDiscriminator]) {
+                    [ble setBleLayer:bleLayer];
+                    ble.appState = appState;
+                    ble.onConnectionComplete = OnConnectionComplete;
+                    ble.onConnectionError = OnConnectionError;
+                    [ble updateWithDiscriminator:deviceDiscriminator];
+                    return;
+                }
 
-            // If the previous connection delegate was a scan without a discriminator, just reuse it instead of
-            // creating a brand new connection but update the discriminator and the ble layer members.
-            if (ble and ![ble hasDiscriminator]) {
+                [ble stop];
+                ble = [[BleConnection alloc] initWithDiscriminator:deviceDiscriminator queue:bleWorkQueue];
                 [ble setBleLayer:bleLayer];
                 ble.appState = appState;
                 ble.onConnectionComplete = OnConnectionComplete;
                 ble.onConnectionError = OnConnectionError;
-                [ble updateWithDiscriminator:deviceDiscriminator];
-                return;
-            }
-
-            CancelConnection();
-            ble = [[BleConnection alloc] initWithDiscriminator:deviceDiscriminator];
-            [ble setBleLayer:bleLayer];
-            ble.appState = appState;
-            ble.onConnectionComplete = OnConnectionComplete;
-            ble.onConnectionError = OnConnectionError;
-            ble.centralManager = [ble.centralManager initWithDelegate:ble queue:ble.workQueue];
+                ble.centralManager = [ble.centralManager initWithDelegate:ble queue:bleWorkQueue];
+            });
         }
 
         void BleConnectionDelegateImpl::PrepareConnection()
         {
-            ChipLogProgress(Ble, "%s", __FUNCTION__);
+            assertChipStackLockedByCurrentThread();
 
-            // If the previous connection delegate was a scan without a discriminator, just reuse it instead of
-            // creating a brand new connection but clear the cache and reset the timer.
-            if (ble and ![ble hasDiscriminator]) {
-                [ble update];
-                return;
+            ChipLogProgress(Ble, "%s", __FUNCTION__);
+            if (!bleWorkQueue) {
+                bleWorkQueue = dispatch_queue_create(kBleWorkQueueName, DISPATCH_QUEUE_SERIAL_WITH_AUTORELEASE_POOL);
             }
 
-            CancelConnection();
-            ble = [[BleConnection alloc] init];
-            ble.onConnectionComplete = OnConnectionComplete;
-            ble.onConnectionError = OnConnectionError;
-            ble.centralManager = [ble.centralManager initWithDelegate:ble queue:ble.workQueue];
+            dispatch_async(bleWorkQueue, ^{
+                // If the previous connection delegate was a scan without a discriminator, just reuse it instead of
+                // creating a brand new connection but clear the cache and reset the timer.
+                if (ble and ![ble hasDiscriminator]) {
+                    [ble update];
+                    return;
+                }
+
+                [ble stop];
+                ble = [[BleConnection alloc] initWithQueue:bleWorkQueue];
+                ble.onConnectionComplete = OnConnectionComplete;
+                ble.onConnectionError = OnConnectionError;
+                ble.centralManager = [ble.centralManager initWithDelegate:ble queue:bleWorkQueue];
+            });
         }
 
         CHIP_ERROR BleConnectionDelegateImpl::CancelConnection()
         {
+            assertChipStackLockedByCurrentThread();
+
             ChipLogProgress(Ble, "%s", __FUNCTION__);
-            if (ble) {
+            if (bleWorkQueue == nil) {
+                return CHIP_NO_ERROR;
+            }
+
+            dispatch_async(bleWorkQueue, ^{
                 [ble stop];
                 ble = nil;
-            }
+            });
+
+            bleWorkQueue = nil;
             return CHIP_NO_ERROR;
         }
     } // namespace Internal
@@ -134,15 +160,14 @@ @interface BleConnection ()
 
 @implementation BleConnection
 
-- (id)init
+- (id)initWithQueue:(dispatch_queue_t)queue
 {
     self = [super init];
     if (self) {
         self.shortServiceUUID = [UUIDHelper GetShortestServiceUUID:&chip::Ble::CHIP_BLE_SVC_ID];
-        _workQueue
-            = dispatch_queue_create("org.csa-iot.matter.framework.ble.workqueue", DISPATCH_QUEUE_SERIAL_WITH_AUTORELEASE_POOL);
         _chipWorkQueue = chip::DeviceLayer::PlatformMgrImpl().GetWorkQueue();
-        _timer = dispatch_source_create(DISPATCH_SOURCE_TYPE_TIMER, 0, 0, _workQueue);
+        _workQueue = queue;
+        _timer = dispatch_source_create(DISPATCH_SOURCE_TYPE_TIMER, 0, 0, queue);
         _centralManager = [CBCentralManager alloc];
         _found = false;
         _cachedPeripherals = [[NSMutableDictionary alloc] init];
@@ -159,9 +184,9 @@ - (id)init
     return self;
 }
 
-- (id)initWithDiscriminator:(const chip::SetupDiscriminator &)deviceDiscriminator
+- (id)initWithDiscriminator:(const chip::SetupDiscriminator &)deviceDiscriminator queue:(dispatch_queue_t)queue
 {
-    self = [self init];
+    self = [self initWithQueue:queue];
     if (self) {
         _deviceDiscriminator = deviceDiscriminator;
         _hasDeviceDiscriminator = true;
@@ -409,12 +434,28 @@ - (void)start
 - (void)stop
 {
     [self stopScanning];
-    [self disconnect];
     [_cachedPeripherals removeAllObjects];
     _cachedPeripherals = nil;
-    _centralManager.delegate = nil;
-    _centralManager = nil;
-    _peripheral = nil;
+
+    if (!_centralManager || !_peripheral) {
+        return;
+    }
+
+    // Properly closing the underlying ble connections needs to happens
+    // on the chip work queue. At the same time the SDK is trying to
+    // properly unsubscribe and shutdown the connection, so if we nullify
+    // the centralManager and the peripheral members too early it won't be
+    // able to reach those.
+    // This is why closing connections happens as 2 async steps.
+    dispatch_async(_chipWorkQueue, ^{
+        _mBleLayer->CloseAllBleConnections();
+
+        dispatch_async(_workQueue, ^{
+            _centralManager.delegate = nil;
+            _centralManager = nil;
+            _peripheral = nil;
+        });
+    });
 }
 
 - (void)startScanning
@@ -445,16 +486,6 @@ - (void)connect:(CBPeripheral *)peripheral
     [_centralManager connectPeripheral:peripheral options:nil];
 }
 
-- (void)disconnect
-{
-    if (!_centralManager || !_peripheral) {
-        return;
-    }
-
-    _mBleLayer->CloseAllBleConnections();
-    _peripheral = nil;
-}
-
 - (void)update
 {
     [_cachedPeripherals removeAllObjects];