Address review comments.

src/darwin/Framework/CHIP/MTRDeviceController.mm

@@ -188,10 +188,6 @@ - (instancetype)initForSubclasses:(BOOL)startSuspended
     _shutdownPending = NO;
     _assertionLock = OS_UNFAIR_LOCK_INIT;
 
-    // All synchronous suspend/resume activity has to be protected by
-    // @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];
@@ -399,13 +395,23 @@ - (void)suspend
 {
     MTR_LOG("%@ suspending", self);
 
+    if (![self isRunning]) {
+        MTR_LOG_ERROR("%@ not running; can't suspend", self);
+        return;
+    }
+
     NSArray * devicesToSuspend;
     {
         std::lock_guard lock(*self.deviceMapLock);
         // Set _suspended under the device map lock.  This guarantees that
         // for any given device exactly one of two things is true:
         // * It is in the snapshot we are creating
         // * It is created after we have changed our _suspended state.
+        if (_suspended) {
+            MTR_LOG("%@ already suspended", self);
+            return;
+        }
+
         _suspended = YES;
         devicesToSuspend = [self.nodeIDToDeviceMap objectEnumerator].allObjects;
     }
@@ -421,19 +427,36 @@ - (void)suspend
     // * CASE sessions in general.
     // * Possibly try to see whether we can change our fabric entry to not advertise and restart advertising.
     [self _notifyDelegatesOfSuspendState];
+
+    [self _controllerSuspended];
+}
+
+- (void)_controllerSuspended
+{
+    // Subclass hook; nothing to do.
 }
 
 - (void)resume
 {
     MTR_LOG("%@ resuming", self);
 
+    if (![self isRunning]) {
+        MTR_LOG_ERROR("%@ not running; can't resume", self);
+        return;
+    }
+
     NSArray * devicesToResume;
     {
         std::lock_guard lock(*self.deviceMapLock);
         // Set _suspended under the device map lock.  This guarantees that
         // for any given device exactly one of two things is true:
         // * It is in the snapshot we are creating
         // * It is created after we have changed our _suspended state.
+        if (!_suspended) {
+            MTR_LOG("%@ already not suspended", self);
+            return;
+        }
+
         _suspended = NO;
         devicesToResume = [self.nodeIDToDeviceMap objectEnumerator].allObjects;
     }
@@ -444,6 +467,13 @@ - (void)resume
     }
 
     [self _notifyDelegatesOfSuspendState];
+
+    [self _controllerResumed];
+}
+
+- (void)_controllerResumed
+{
+    // Subclass hook; nothing to do.
 }
 
 - (BOOL)matchesPendingShutdownControllerWithOperationalCertificate:(nullable MTRCertificateDERBytes)operationalCertificate andRootCertificate:(nullable MTRCertificateDERBytes)rootCertificate

src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm

@@ -86,7 +86,7 @@ @interface MTRDeviceControllerFactoryParams ()
 @end
 
 MTR_DIRECT_MEMBERS
-@interface MTRDeviceControllerFactory () <MTRDeviceControllerDelegate>
+@interface MTRDeviceControllerFactory ()
 - (void)preWarmCommissioningSessionDone;
 @end
 
@@ -559,21 +559,6 @@ - (MTRDeviceController * _Nullable)_startDeviceController:(MTRDeviceController *
         return nil;
     }
 
-    // Listen for changes to suspended state on this controller, so we can
-    // track whether we have any unsuspended controllers.
-    [controller addDeviceControllerDelegate:self queue:_chipWorkQueue];
-
-    if (!startSuspended) {
-        // Async dispatch here is fine: in the unlikely case that someone comes
-        // along and shuts down or suspends controllers so that we have to stop
-        // the browse, and our async dispatch runs after that, we'll just
-        // briefly have a browse that we don't really need, then end up shutting
-        // it down again.  That's what would happen with a sync dispatch anyway.
-        dispatch_async(_chipWorkQueue, ^{
-            self->_operationalBrowser->EnsureBrowse();
-        });
-    }
-
     // Add the controller to _controllers now, so if we fail partway through its
     // startup we will still do the right cleanups.
     os_unfair_lock_lock(&_controllersLock);
@@ -949,12 +934,6 @@ - (void)controllerShuttingDown:(MTRDeviceController *)controller
             self->_groupDataProvider.RemoveGroupKeys(fabricIndex);
         }
 
-        // If all remaining controllers after this one has been removed are
-        // suspended, we should stop our operational browse.
-        if ([self _allControllersSuspended]) {
-            self->_operationalBrowser->StopBrowse();
-        }
-
         // If there are no other controllers left, we can shut down some things.
         // Do this before we shut down the controller itself, because the
         // OtaProviderDelegateBridge uses some services provided by the system
@@ -1260,40 +1239,9 @@ - (PersistentStorageDelegate *)storageDelegate
     return &_groupDataProvider;
 }
 
-#pragma mark - Controller suspension support
-
-- (BOOL)_allControllersSuspended
+- (MTROperationalBrowser *)operationalBrowser
 {
-    auto * controllers = [self getRunningControllers];
-    for (MTRDeviceController * controller in controllers) {
-        if (!controller.suspended) {
-            return NO;
-        }
-    }
-    return YES;
-}
-
-- (void)controller:(MTRDeviceController *)controller
-       isSuspended:(BOOL)suspended
-{
-    // We use the Matter queue as our delegate queue for this notification.
-    assertChipStackLockedByCurrentThread();
-
-    if (!suspended) {
-        // As long as we have one controller that is not suspended, we should
-        // have a running operational browse. Guard against someone resuming a
-        // controller after it has been shut down (at which point it's not in
-        // our "running controllers" list).
-        auto * controllers = [self getRunningControllers];
-        if ([controllers containsObject:controller]) {
-            _operationalBrowser->EnsureBrowse();
-        }
-        return;
-    }
-
-    if ([self _allControllersSuspended]) {
-        _operationalBrowser->StopBrowse();
-    }
+    return _operationalBrowser.get();
 }
 
 @end

src/darwin/Framework/CHIP/MTRDeviceControllerFactory_Internal.h

@@ -30,6 +30,7 @@
 
 #import "MTRDefines_Internal.h"
 #import "MTRDeviceControllerFactory.h"
+#import "MTROperationalBrowser.h"
 
 #include <lib/core/CHIPPersistentStorageDelegate.h>
 #include <lib/core/DataModelTypes.h>
@@ -108,6 +109,7 @@ MTR_DIRECT_MEMBERS
 
 @property (readonly) chip::PersistentStorageDelegate * storageDelegate;
 @property (readonly) chip::Credentials::GroupDataProvider * groupDataProvider;
+@property (readonly, assign) MTROperationalBrowser * operationalBrowser;
 
 @end
 

src/darwin/Framework/CHIP/MTRDeviceController_Concrete.mm

@@ -330,6 +330,15 @@ - (instancetype)initWithFactory:(MTRDeviceControllerFactory *)factory
         self.rootPublicKey = nil;
 
         _storageBehaviorConfiguration = storageBehaviorConfiguration;
+
+        // We let the operational browser know about ourselves here, because
+        // after this point we are guaranteed to have shutDownCppController
+        // called by the factory.
+        if (!startSuspended) {
+            dispatch_async(_chipWorkQueue, ^{
+                factory.operationalBrowser->ControllerActivated();
+            });
+        }
     }
     return self;
 }
@@ -344,6 +353,22 @@ - (BOOL)isRunning
     return _cppCommissioner != nullptr;
 }
 
+- (void)_controllerSuspended
+{
+    MTRDeviceControllerFactory * factory = _factory;
+    dispatch_async(_chipWorkQueue, ^{
+        factory.operationalBrowser->ControllerDeactivated();
+    });
+}
+
+- (void)_controllerResumed
+{
+    MTRDeviceControllerFactory * factory = _factory;
+    dispatch_async(_chipWorkQueue, ^{
+        factory.operationalBrowser->ControllerActivated();
+    });
+}
+
 - (BOOL)matchesPendingShutdownControllerWithOperationalCertificate:(nullable MTRCertificateDERBytes)operationalCertificate andRootCertificate:(nullable MTRCertificateDERBytes)rootCertificate
 {
     if (!operationalCertificate || !rootCertificate) {
@@ -471,6 +496,11 @@ - (void)shutDownCppController
             _operationalCredentialsDelegate->SetDeviceCommissioner(nullptr);
         }
     }
+
+    if (!self.suspended) {
+        _factory.operationalBrowser->ControllerDeactivated();
+    }
+
     _shutdownPending = NO;
 }
 

src/darwin/Framework/CHIP/MTROperationalBrowser.h

@@ -28,14 +28,23 @@ class MTROperationalBrowser
 
     ~MTROperationalBrowser();
 
-    // EnsureBrowse is a no-op if a browse has already been started.
-    void EnsureBrowse();
-    void StopBrowse();
+    // ControllerActivated should be called, on the Matter queue, when a
+    // controller is either started in a non-suspended state or stops being
+    // suspended.
+
+    // ControllerDeactivated should be called, on the Matter queue, when a
+    // controller is either suspended or shut down while in a non-suspended
+    // state.
+    void ControllerActivated();
+    void ControllerDeactivated();
 
 private:
     static void OnBrowse(DNSServiceRef aServiceRef, DNSServiceFlags aFlags, uint32_t aInterfaceId, DNSServiceErrorType aError,
                          const char * aName, const char * aType, const char * aDomain, void * aContext);
 
+    void EnsureBrowse();
+    void StopBrowse();
+
     MTRDeviceControllerFactory * const __weak mDeviceControllerFactory;
     dispatch_queue_t mQueue;
     DNSServiceRef mBrowseRef;
@@ -45,4 +54,8 @@ class MTROperationalBrowser
 
     // If mIsDestroying is true, we're in our destructor, shutting things down.
     bool mIsDestroying = false;
+
+    // Count of controllers that are currently active; we aim to have a browse
+    // going while this is nonzero;
+    size_t mActiveControllerCount = 0;
 };

src/darwin/Framework/CHIP/MTROperationalBrowser.mm

@@ -39,6 +39,27 @@
 {
 }
 
+void MTROperationalBrowser::ControllerActivated()
+{
+    assertChipStackLockedByCurrentThread();
+
+    if (mActiveControllerCount == 0) {
+        EnsureBrowse();
+    }
+    ++mActiveControllerCount;
+}
+
+void MTROperationalBrowser::ControllerDeactivated()
+{
+    assertChipStackLockedByCurrentThread();
+
+    if (mActiveControllerCount == 1) {
+        StopBrowse();
+    }
+
+    --mActiveControllerCount;
+}
+
 void MTROperationalBrowser::EnsureBrowse()
 {
     assertChipStackLockedByCurrentThread();
@@ -117,10 +138,7 @@
     }
 
     ChipLogProgress(Controller, "Notifying controller factory about new operational instance: '%s'", aName);
-    MTRDeviceControllerFactory * strongFactory = self->mDeviceControllerFactory;
-    if (strongFactory) {
-        [strongFactory operationalInstanceAdded:peerId];
-    }
+    [self->mDeviceControllerFactory operationalInstanceAdded:peerId];
 }
 
 MTROperationalBrowser::~MTROperationalBrowser()