Changed lock to @synchronized because MTRDevice could call isSuspende…

…d while holding the suspension lock
jtung-apple · Sep 7, 2024 · 88d1bed · 88d1bed
1 parent 282c973
commit 88d1bed
Show file tree

Hide file tree

Showing 2 changed files with 26 additions and 35 deletions.
diff --git a/src/darwin/Framework/CHIP/MTRDeviceController.mm b/src/darwin/Framework/CHIP/MTRDeviceController.mm
@@ -134,7 +134,6 @@ @implementation MTRDeviceController {
     MTRP256KeypairBridge _operationalKeypairBridge;
 
     BOOL _suspended;
-    os_unfair_lock _suspensionLock;
 
     // Counters to track assertion status and access controlled by the _assertionLock
     NSUInteger _keepRunningAssertionCounter;
@@ -159,11 +158,11 @@ - (instancetype)initForSubclasses:(BOOL)startSuspended
     _shutdownPending = NO;
     _assertionLock = OS_UNFAIR_LOCK_INIT;
 
-    _suspended = startSuspended;
     // All synchronous suspend/resume activity has to be protected by
-    // _suspensionLock, so that parts of suspend/resume can't interleave with
-    // each other.
-    _suspensionLock = OS_UNFAIR_LOCK_INIT;
+    // @synchronized(self), so that parts of suspend/resume can't
+    // interleave with each other. Using @synchronized here because
+    // MTRDevice may call isSuspended.
+    _suspended = startSuspended;
 
     _nodeIDToDeviceMap = [NSMapTable strongToWeakObjectsMapTable];
 
@@ -210,7 +209,6 @@ - (instancetype)initWithFactory:(MTRDeviceControllerFactory *)factory
         _assertionLock = OS_UNFAIR_LOCK_INIT;
 
         _suspended = startSuspended;
-        _suspensionLock = OS_UNFAIR_LOCK_INIT;
 
         if (storageDelegate != nil) {
             if (storageDelegateQueue == nil) {
@@ -352,51 +350,44 @@ - (BOOL)isRunning
 
 - (BOOL)isSuspended
 {
-    std::lock_guard lock(_suspensionLock);
-    return _suspended;
+    @synchronized (self) {
+        return _suspended;
+    }
 }
 
 - (void)suspend
 {
     MTR_LOG("%@ suspending", self);
 
-    std::lock_guard lock(_suspensionLock);
-
-    _suspended = YES;
+    @synchronized (self) {
+        _suspended = YES;
 
-    NSEnumerator * devices;
-    {
         std::lock_guard lock(*self.deviceMapLock);
-        devices = [self.nodeIDToDeviceMap objectEnumerator];
-    }
+        NSEnumerator * devices = [self.nodeIDToDeviceMap objectEnumerator];
+        for (MTRDevice * device in devices) {
+            [device controllerSuspended];
+        }
 
-    for (MTRDevice * device in devices) {
-        [device controllerSuspended];
+        // TODO: In the concrete class, consider what should happen with:
+        //
+        // * Active commissioning sessions (presumably close them?)
+        // * CASE sessions in general.
+        // * Possibly try to see whether we can change our fabric entry to not advertise and restart advertising.
     }
-
-    // TODO: In the concrete class, consider what should happen with:
-    //
-    // * Active commissioning sessions (presumably close them?)
-    // * CASE sessions in general.
-    // * Possibly try to see whether we can change our fabric entry to not advertise and restart advertising.
 }
 
 - (void)resume
 {
     MTR_LOG("%@ resuming", self);
 
-    std::lock_guard lock(_suspensionLock);
-
-    _suspended = NO;
+    @synchronized (self) {
+        _suspended = NO;
 
-    NSEnumerator * devices;
-    {
         std::lock_guard lock(*self.deviceMapLock);
-        devices = [self.nodeIDToDeviceMap objectEnumerator];
-    }
-
-    for (MTRDevice * device in devices) {
-        [device controllerResumed];
+        NSEnumerator * devices = [self.nodeIDToDeviceMap objectEnumerator];
+        for (MTRDevice * device in devices) {
+            [device controllerResumed];
+        }
     }
 }
 

diff --git a/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm b/src/darwin/Framework/CHIP/MTRDevice_Concrete.mm
@@ -720,7 +720,7 @@ - (BOOL)_subscriptionsAllowed
     os_unfair_lock_assert_owner(&self->_lock);
 
     // We should not allow a subscription for suspended controllers or device controllers over XPC.
-    return _deviceController.suspended == NO && ![_deviceController isKindOfClass:MTRDeviceControllerOverXPC.class];
+    return _deviceController.isSuspended == NO && ![_deviceController isKindOfClass:MTRDeviceControllerOverXPC.class];
 }
 
 - (void)_delegateAdded
@@ -1249,7 +1249,7 @@ - (void)_doHandleSubscriptionReset:(NSNumber * _Nullable)retryDelay
 {
     os_unfair_lock_assert_owner(&_lock);
 
-    if (_deviceController.suspended) {
+    if (_deviceController.isSuspended) {
         MTR_LOG("%@ ignoring expected subscription reset on controller suspend", self);
         return;
     }