Stop persistent operational browse when all controllers are suspended.

src/darwin/Framework/CHIP/MTRDeviceControllerFactory.mm

@@ -86,7 +86,7 @@ @interface MTRDeviceControllerFactoryParams ()
 @end
 
 MTR_DIRECT_MEMBERS
-@interface MTRDeviceControllerFactory ()
+@interface MTRDeviceControllerFactory () <MTRDeviceControllerDelegate>
 - (void)preWarmCommissioningSessionDone;
 @end
 
@@ -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,9 +559,18 @@ - (MTRDeviceController * _Nullable)_startDeviceController:(MTRDeviceController *
         return nil;
     }
 
-    if ([_controllers count] == 0) {
-        dispatch_sync(_chipWorkQueue, ^{
-            self->_operationalBrowser = new MTROperationalBrowser(self, self->_chipWorkQueue);
+    // 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();
         });
     }
 
@@ -938,14 +949,17 @@ - (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
         // state without retaining it.
         if (_controllers.count == 0) {
-            delete self->_operationalBrowser;
-            self->_operationalBrowser = nullptr;
-
             if (_otaProviderDelegateBridge) {
                 _otaProviderDelegateBridge->Shutdown();
                 _otaProviderDelegateBridge.reset();
@@ -1246,6 +1260,42 @@ - (PersistentStorageDelegate *)storageDelegate
     return &_groupDataProvider;
 }
 
+#pragma mark - Controller suspension support
+
+- (BOOL)_allControllersSuspended
+{
+    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();
+    }
+}
+
 @end
 
 @interface MTRDummyStorage : NSObject <MTRStorage>

src/darwin/Framework/CHIP/MTROperationalBrowser.h

@@ -23,19 +23,20 @@
 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();
 
+    // EnsureBrowse is a no-op if a browse has already been started.
+    void EnsureBrowse();
+    void StopBrowse();
+
 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();
-
-    MTRDeviceControllerFactory * const mDeviceControllerFactory;
+    MTRDeviceControllerFactory * const __weak mDeviceControllerFactory;
     dispatch_queue_t mQueue;
     DNSServiceRef mBrowseRef;
 

src/darwin/Framework/CHIP/MTROperationalBrowser.mm

@@ -37,26 +37,28 @@
     : 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::EnsureBrowse()
 {
     assertChipStackLockedByCurrentThread();
 
-    ChipLogProgress(Controller, "Trying to start persistent operational browse");
+    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 +69,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 +93,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;
     }
@@ -107,7 +117,10 @@
     }
 
     ChipLogProgress(Controller, "Notifying controller factory about new operational instance: '%s'", aName);
-    [self->mDeviceControllerFactory operationalInstanceAdded:peerId];
+    MTRDeviceControllerFactory * strongFactory = self->mDeviceControllerFactory;
+    if (strongFactory) {
+        [strongFactory operationalInstanceAdded:peerId];
+    }
 }
 
 MTROperationalBrowser::~MTROperationalBrowser()
@@ -116,7 +129,5 @@
 
     mIsDestroying = true;
 
-    if (mInitialized) {
-        DNSServiceRefDeallocate(mBrowseRef);
-    }
+    StopBrowse();
 }

src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m

@@ -76,6 +76,30 @@ - (void)controller:(MTRDeviceController *)controller commissioningComplete:(NSEr
 
 @end
 
+@interface MTRPerControllerStorageTestsSuspensionDelegate : NSObject <MTRDeviceControllerDelegate>
+@property (nonatomic, strong) XCTestExpectation * expectation;
+@property (nonatomic, assign) BOOL expectedSuspensionState;
+@end
+
+@implementation MTRPerControllerStorageTestsSuspensionDelegate
+- (id)initWithExpectation:(XCTestExpectation *)expectation expectedSuspensionState:(BOOL)expectedSuspensionState
+{
+    self = [super init];
+    if (self) {
+        _expectation = expectation;
+        _expectedSuspensionState = expectedSuspensionState;
+    }
+    return self;
+}
+
+- (void)controller:(MTRDeviceController *)controller
+       isSuspended:(BOOL)suspended
+{
+    XCTAssertEqual(suspended, self.expectedSuspensionState);
+    [self.expectation fulfill];
+}
+@end
+
 @interface MTRPerControllerStorageTestsCertificateIssuer : NSObject <MTROperationalCertificateIssuer>
 - (instancetype)initWithRootCertificate:(MTRCertificateDERBytes)rootCertificate
                 intermediateCertificate:(MTRCertificateDERBytes _Nullable)intermediateCertificate
@@ -1713,6 +1737,17 @@ - (void)test013_suspendDevices
         [becameUnreachableExpectation fulfill];
     };
 
+    XCTestExpectation * suspendedExpectation = [self expectationWithDescription:@"Controller has been suspended"];
+    __auto_type * suspensionDelegate = [[MTRPerControllerStorageTestsSuspensionDelegate alloc] initWithExpectation:suspendedExpectation expectedSuspensionState:YES];
+    [controller addDeviceControllerDelegate:suspensionDelegate queue:queue];
+
+    XCTestExpectation * browseStoppedExpectation = [self expectationWithDescription:@"Operational browse has stopped"];
+    MTRSetLogCallback(MTRLogTypeProgress, ^(MTRLogType type, NSString * moduleName, NSString * message) {
+        if ([message containsString:@"stopping persistent operational browse"]) {
+            [browseStoppedExpectation fulfill];
+        }
+    });
+
     [controller suspend];
     XCTAssertTrue(controller.suspended);
 
@@ -1723,7 +1758,7 @@ - (void)test013_suspendDevices
         [toggle2Expectation fulfill];
     }];
 
-    [self waitForExpectations:@[ becameUnreachableExpectation, toggle2Expectation ] timeout:kTimeoutInSeconds];
+    [self waitForExpectations:@[ becameUnreachableExpectation, toggle2Expectation, suspendedExpectation, browseStoppedExpectation ] timeout:kTimeoutInSeconds];
 
     XCTestExpectation * newSubscriptionExpectation = [self expectationWithDescription:@"Subscription has been set up again"];
     XCTestExpectation * newReachableExpectation = [self expectationWithDescription:@"Device became reachable again"];
@@ -1735,10 +1770,23 @@ - (void)test013_suspendDevices
         [newSubscriptionExpectation fulfill];
     };
 
+    XCTestExpectation * resumedExpectation = [self expectationWithDescription:@"Controller has been resumed"];
+    suspensionDelegate.expectation = resumedExpectation;
+    suspensionDelegate.expectedSuspensionState = NO;
+
+    XCTestExpectation * browseRestartedExpectation = [self expectationWithDescription:@"Operational browse has re-started"];
+    MTRSetLogCallback(MTRLogTypeProgress, ^(MTRLogType type, NSString * moduleName, NSString * message) {
+        if ([message containsString:@"trying to start persistent operational browse"]) {
+            [browseRestartedExpectation fulfill];
+        }
+    });
+
     [controller resume];
     XCTAssertFalse(controller.suspended);
 
-    [self waitForExpectations:@[ newSubscriptionExpectation, newReachableExpectation ] timeout:kSubscriptionTimeoutInSeconds];
+    [self waitForExpectations:@[ newSubscriptionExpectation, newReachableExpectation, resumedExpectation, browseRestartedExpectation ] timeout:kSubscriptionTimeoutInSeconds];
+
+    MTRSetLogCallback(MTRLogTypeProgress, nil);
 
     // Test that sending a command works again.  Clear the delegate's onReportEnd
     // first, so reports from the command don't trigger it.