Stop persistent operational browse when all controllers are suspended. (

project-chip#35541)

* Stop persistent operational browse when all controllers are suspended.

* 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

@@ -111,7 +111,7 @@ @implementation MTRDeviceControllerFactory {
     MTRSessionResumptionStorageBridge * _sessionResumptionStorage;
     PersistentStorageOperationalKeystore * _keystore;
     Credentials::PersistentStorageOpCertStore * _opCertStore;
-    MTROperationalBrowser * _operationalBrowser;
+    std::unique_ptr<MTROperationalBrowser> _operationalBrowser;
 
     // productAttestationAuthorityCertificates and certificationDeclarationCertificates are just copied
     // from MTRDeviceControllerFactoryParams.
@@ -223,6 +223,8 @@ - (instancetype)init
     _serverEndpointsLock = OS_UNFAIR_LOCK_INIT;
     _serverEndpoints = [[NSMutableArray alloc] init];
 
+    _operationalBrowser = std::make_unique<MTROperationalBrowser>(self, self->_chipWorkQueue);
+
     return self;
 }
 
@@ -557,12 +559,6 @@ - (MTRDeviceController * _Nullable)_startDeviceController:(MTRDeviceController *
         return nil;
     }
 
-    if ([_controllers count] == 0) {
-        dispatch_sync(_chipWorkQueue, ^{
-            self->_operationalBrowser = new MTROperationalBrowser(self, self->_chipWorkQueue);
-        });
-    }
-
     // 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);
@@ -943,9 +939,6 @@ - (void)controllerShuttingDown:(MTRDeviceController *)controller
         // OtaProviderDelegateBridge uses some services provided by the system
         // state without retaining it.
         if (_controllers.count == 0) {
-            delete self->_operationalBrowser;
-            self->_operationalBrowser = nullptr;
-
             if (_otaProviderDelegateBridge) {
                 _otaProviderDelegateBridge->Shutdown();
                 _otaProviderDelegateBridge.reset();
@@ -1246,6 +1239,11 @@ - (PersistentStorageDelegate *)storageDelegate
     return &_groupDataProvider;
 }
 
+- (MTROperationalBrowser *)operationalBrowser
+{
+    return _operationalBrowser.get();
+}
+
 @end
 
 @interface MTRDummyStorage : NSObject <MTRStorage>

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

@@ -23,19 +23,29 @@
 class MTROperationalBrowser
 {
 public:
-    // Should be created at a point when the factory starts up the event loop,
-    // and destroyed when the event loop is stopped.
+    // Should be created at a point when the dispatch queue is available.
     MTROperationalBrowser(MTRDeviceControllerFactory * aFactory, dispatch_queue_t aQueue);
 
     ~MTROperationalBrowser();
 
+    // 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 TryToStartBrowse();
+    void EnsureBrowse();
+    void StopBrowse();
 
-    MTRDeviceControllerFactory * const mDeviceControllerFactory;
+    MTRDeviceControllerFactory * const __weak mDeviceControllerFactory;
     dispatch_queue_t mQueue;
     DNSServiceRef mBrowseRef;
 
@@ -44,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

@@ -37,26 +37,49 @@
     : mDeviceControllerFactory(aFactory)
     , mQueue(aQueue)
 {
-    // If we fail to start a browse, there's nothing our consumer would do
-    // differently, so we might as well do this in the constructor.
-    TryToStartBrowse();
 }
 
-void MTROperationalBrowser::TryToStartBrowse()
+void MTROperationalBrowser::ControllerActivated()
 {
     assertChipStackLockedByCurrentThread();
 
-    ChipLogProgress(Controller, "Trying to start persistent operational browse");
+    if (mActiveControllerCount == 0) {
+        EnsureBrowse();
+    }
+    ++mActiveControllerCount;
+}
+
+void MTROperationalBrowser::ControllerDeactivated()
+{
+    assertChipStackLockedByCurrentThread();
+
+    if (mActiveControllerCount == 1) {
+        StopBrowse();
+    }
+
+    --mActiveControllerCount;
+}
+
+void MTROperationalBrowser::EnsureBrowse()
+{
+    assertChipStackLockedByCurrentThread();
+
+    if (mInitialized) {
+        ChipLogProgress(Controller, "%p already has a persistent operational browse running", this);
+        return;
+    }
+
+    ChipLogProgress(Controller, "%p trying to start persistent operational browse", this);
 
     auto err = DNSServiceCreateConnection(&mBrowseRef);
     if (err != kDNSServiceErr_NoError) {
-        ChipLogError(Controller, "Failed to create connection for persistent operational browse: %" PRId32, err);
+        ChipLogError(Controller, "%p failed to create connection for persistent operational browse: %" PRId32, this, err);
         return;
     }
 
     err = DNSServiceSetDispatchQueue(mBrowseRef, mQueue);
     if (err != kDNSServiceErr_NoError) {
-        ChipLogError(Controller, "Failed to set up dispatch queue properly for persistent operational browse: %" PRId32, err);
+        ChipLogError(Controller, "%p failed to set up dispatch queue properly for persistent operational browse: %" PRId32, this, err);
         DNSServiceRefDeallocate(mBrowseRef);
         return;
     }
@@ -67,11 +90,20 @@
         auto browseRef = mBrowseRef; // Mandatory copy because of kDNSServiceFlagsShareConnection.
         err = DNSServiceBrowse(&browseRef, kDNSServiceFlagsShareConnection, kDNSServiceInterfaceIndexAny, kOperationalType, domain, OnBrowse, this);
         if (err != kDNSServiceErr_NoError) {
-            ChipLogError(Controller, "Failed to start persistent operational browse for \"%s\" domain: %" PRId32, StringOrNullMarker(domain), err);
+            ChipLogError(Controller, "%p failed to start persistent operational browse for \"%s\" domain: %" PRId32, this, StringOrNullMarker(domain), err);
         }
     }
 }
 
+void MTROperationalBrowser::StopBrowse()
+{
+    ChipLogProgress(Controller, "%p stopping persistent operational browse", this);
+    if (mInitialized) {
+        DNSServiceRefDeallocate(mBrowseRef);
+        mInitialized = false;
+    }
+}
+
 void MTROperationalBrowser::OnBrowse(DNSServiceRef aServiceRef, DNSServiceFlags aFlags, uint32_t aInterfaceId,
     DNSServiceErrorType aError, const char * aName, const char * aType, const char * aDomain, void * aContext)
 {
@@ -82,14 +114,13 @@
     // We only expect to get notified about our type/domain.
     if (aError != kDNSServiceErr_NoError) {
         ChipLogError(Controller, "Operational browse failure: %" PRId32, aError);
-        DNSServiceRefDeallocate(self->mBrowseRef);
-        self->mInitialized = false;
+        self->StopBrowse();
 
         // We shouldn't really get callbacks under our destructor, but guard
         // against it just in case.
         if (!self->mIsDestroying) {
             // Try to start a new browse, so we have one going.
-            self->TryToStartBrowse();
+            self->EnsureBrowse();
         }
         return;
     }
@@ -116,7 +147,5 @@
 
     mIsDestroying = true;
 
-    if (mInitialized) {
-        DNSServiceRefDeallocate(mBrowseRef);
-    }
+    StopBrowse();
 }