Fix random failure in MTRPerControllerStorageTests testSubscriptionPool (#38803)

Two fixes here:

1. We were not consistently dispatching the unitTestSubscriptionPoolWorkComplete
   notification, because when _clearSubscriptionPoolWork was called from dealloc
   we had already cleared our delegate list before the call.  This was the
   proximate cause of the random failure: when we got deallocated before the
   work item for our subscription pool work got dequeued, we would end up with
   subscriptionRunningCount too high (since it would increment on enqueue but we
   would miss the decrement.

2. The fake device is not consistent about whether it actually dispatches the
   unitTestSubscriptionPoolDequeue notification: usually it does not (because it
   was dealloced before that happened), but sometimes it does.  When it does, we
   were ending up with more dequeue notifications than expected in the test. The
   fix for this is to not track the dequeue/complete bits in the test for the
   fake device at all.

Fixes #38797

Author: bzbarsky-apple

src/darwin/Framework/CHIP/MTRDevice_Concrete.mm

@@ -518,9 +518,9 @@ - (void)dealloc
 
     [[NSNotificationCenter defaultCenter] removeObserver:_systemTimeChangeObserverToken];
 
+    __block id testDelegate = nil;
 #ifdef DEBUG
     // Save the first delegate for testing
-    __block id testDelegate = nil;
     for (MTRDeviceDelegateInfo * delegateInfo in _delegates) {
         testDelegate = delegateInfo.delegate;
         break;
@@ -542,7 +542,7 @@ - (void)dealloc
 
     // Clear this device from subscription pool and persist cached data to storage as needed.
     std::lock_guard lock(_lock);
-    [self _clearSubscriptionPoolWork];
+    [self _clearSubscriptionPoolWorkWithProvidedDelegate:testDelegate];
     [self _doPersistClusterData];
 }
 
@@ -1381,16 +1381,29 @@ - (BOOL)_deviceUsesThread
 }
 
 - (void)_clearSubscriptionPoolWork
+{
+    [self _clearSubscriptionPoolWorkWithProvidedDelegate:nil];
+}
+
+// We provide a delegate to notify if we are doing
+// _clearSubscriptionPoolWorkWithProvidedDelegate after clearing our delegates
+// in dealloc.
+- (void)_clearSubscriptionPoolWorkWithProvidedDelegate:(nullable id)providedDelegate
 {
     os_unfair_lock_assert_owner(&self->_lock);
     MTRAsyncWorkCompletionBlock completion = self->_subscriptionPoolWorkCompletionBlock;
     if (completion) {
 #ifdef DEBUG
-        [self _callDelegatesWithBlock:^(id testDelegate) {
+        auto notificationBlock = ^(id testDelegate) {
             if ([testDelegate respondsToSelector:@selector(unitTestSubscriptionPoolWorkComplete:)]) {
                 [testDelegate unitTestSubscriptionPoolWorkComplete:self];
             }
-        }];
+        };
+        if (providedDelegate != nil) {
+            notificationBlock(providedDelegate);
+        } else {
+            [self _callDelegatesWithBlock:notificationBlock];
+        }
 #endif
         self->_subscriptionPoolWorkCompletionBlock = nil;
         completion(MTRAsyncWorkComplete);

src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m

@@ -2811,13 +2811,6 @@ - (void)doTestSubscriptionPoolWithSize:(NSInteger)subscriptionPoolSize deviceOnb
 
     NSArray<NSNumber *> * orderedDeviceIDs = [deviceOnboardingPayloads allKeys];
 
-    // We have one fake device, which we use an invalid node ID for, that we use
-    // to test what happens if a device is deallocated while its subscription
-    // work item is queued.
-    NSMutableArray<NSNumber *> * orderedDeviceIDsIncludingFake = [orderedDeviceIDs mutableCopy];
-    NSNumber * fakeDeviceID = @(0xFFFFFFFFFFFFFFFF);
-    [orderedDeviceIDsIncludingFake insertObject:fakeDeviceID atIndex:0];
-
     // Commission 5 devices
     for (NSNumber * deviceID in orderedDeviceIDs) {
         certificateIssuer.nextNodeID = deviceID;
@@ -2851,7 +2844,7 @@ - (void)doTestSubscriptionPoolWithSize:(NSInteger)subscriptionPoolSize deviceOnb
     }
 
     NSMutableDictionary<NSNumber *, MTRDeviceTestDelegate *> * deviceDelegates = [NSMutableDictionary dictionary];
-    for (NSNumber * deviceID in orderedDeviceIDsIncludingFake) {
+    for (NSNumber * deviceID in orderedDeviceIDs) {
         deviceDelegates[deviceID] = [[MTRDeviceTestDelegate alloc] init];
     }
 
@@ -2861,7 +2854,7 @@ - (void)doTestSubscriptionPoolWithSize:(NSInteger)subscriptionPoolSize deviceOnb
     __block NSUInteger subscriptionDequeueCount = 0;
     __block BOOL baseDeviceReadCompleted = NO;
 
-    for (NSNumber * deviceID in orderedDeviceIDsIncludingFake) {
+    for (NSNumber * deviceID in orderedDeviceIDs) {
         MTRDeviceTestDelegate * delegate = deviceDelegates[deviceID];
         delegate.pretendThreadEnabled = YES;
 
@@ -2890,17 +2883,17 @@ - (void)doTestSubscriptionPoolWithSize:(NSInteger)subscriptionPoolSize deviceOnb
         };
         __weak __auto_type weakDelegate = delegate;
         delegate.onReportEnd = ^{
-            // We don't have a subscriptionExpectation for the fake device.
-            XCTestExpectation * subscriptionExpectation = subscriptionExpectations[deviceID];
-            if (subscriptionExpectation) {
-                [subscriptionExpectation fulfill];
-            }
+            [subscriptionExpectations[deviceID] fulfill];
             // reset callback so expectation not fulfilled twice, given the run time of this can be long due to subscription pool
             __strong __auto_type strongDelegate = weakDelegate;
             strongDelegate.onReportEnd = nil;
         };
     }
 
+    // We have one fake device, which we use an invalid node ID for, that we use
+    // to test what happens if a device is deallocated while its subscription
+    // work item is queued.
+    //
     // Schedule a job in the controller's pool that will not complete until the
     // fake device is deallocated.
     __auto_type * fakeDeviceDeletedExpectation = [self expectationWithDescription:@"Fake device deleted"];
@@ -2911,18 +2904,24 @@ - (void)doTestSubscriptionPoolWithSize:(NSInteger)subscriptionPoolSize deviceOnb
     }];
     [controller.concurrentSubscriptionPool enqueueWorkItem:blockingWorkItem description:@"Waiting for fake device deletion"];
 
+    // Make sure the delegate for the fake device does not go away before the
+    // fake device does, so keep it out of the @autoreleasepool block.
+    MTRDeviceTestDelegate * fakeDeviceDelegate = [[MTRDeviceTestDelegate alloc] init];
+    fakeDeviceDelegate.pretendThreadEnabled = YES;
+
     @autoreleasepool {
         // Create our fake device and have it dealloc before the blocking WorkItem
         // completes and hence before its subscription work item gets a chance
         // to run (in the width-1 case).
-        MTRDeviceTestDelegate * delegate = deviceDelegates[fakeDeviceID];
+
         // onSubscriptionCallbackDelete is called from dealloc
-        delegate.onSubscriptionCallbackDelete = ^{
+        fakeDeviceDelegate.onSubscriptionCallbackDelete = ^{
             [fakeDeviceDeletedExpectation fulfill];
         };
 
+        NSNumber * fakeDeviceID = @(0xFFFFFFFFFFFFFFFF);
         __auto_type * device = [MTRDevice deviceWithNodeID:fakeDeviceID controller:controller];
-        [device setDelegate:delegate queue:queue];
+        [device setDelegate:fakeDeviceDelegate queue:queue];
     }
 
     for (NSNumber * deviceID in orderedDeviceIDs) {