Make sure to cancel the timeout timer whenever we remove an MTRDownload from the downloads list. (#38123)

bzbarsky-apple · web-flow · commit 012dfbcdfc1a · 2025-03-25T21:01:16.000Z
Fixes a few issues:

1) abortDownloadsForController used to not actually cancel the BDX transfer, as
   far as I can tell.  Maybe that worked because controller shutdown would close
   the session/exchange, but better to cancel the transfer explicitly.
2) abortDownloadsForController could remove an MTRDownload from the list without
   canceling its timeout timer.  Then when the timer fired it might use a
   now-deleted MTRDownload pointer.

The fix is to move the timer management into MTRDownload and ensure that the
timer is always canceled on remove:.
diff --git a/src/darwin/Framework/CHIP/MTRDiagnosticLogsDownloader.mm b/src/darwin/Framework/CHIP/MTRDiagnosticLogsDownloader.mm
@@ -57,6 +57,7 @@ @interface MTRDownload : NSObject
 - (instancetype)initWithType:(MTRDiagnosticLogType)type
                  fabricIndex:(NSNumber *)fabricIndex
                       nodeID:(NSNumber *)nodeID
+                     timeout:(NSTimeInterval)timeout
                        queue:(dispatch_queue_t)queue
                   completion:(void (^)(NSURL * _Nullable url, NSError * _Nullable error))completion
                         done:(void (^)(MTRDownload * finishedDownload))done;
@@ -69,8 +70,14 @@ - (BOOL)matches:(NSString *)fileDesignator
 
 - (void)checkInteractionModelResponse:(MTRDiagnosticLogsClusterRetrieveLogsResponseParams * _Nullable)response error:(NSError * _Nullable)error;
 
+// TODO: The lifetime management for these objects is very odd.  Nothing
+// _really_ prevents a BDX transfer starting after a failure: call, for example.
+// We should move more of the state into a single place that will know the exact
+// state of the object.
 - (void)success;
-- (void)failure:(NSError * _Nullable)error;
+- (void)failure:(NSError *)error;
+- (void)cancelTimeoutTimer;
+- (void)abort:(NSError *)error;
 @end
 
 @interface MTRDownloads : NSObject
@@ -83,6 +90,7 @@ - (MTRDownload * _Nullable)get:(NSString *)fileDesignator
 - (MTRDownload * _Nullable)add:(MTRDiagnosticLogType)type
                    fabricIndex:(NSNumber *)fabricIndex
                         nodeID:(NSNumber *)nodeID
+                       timeout:(NSTimeInterval)timeout
                          queue:(dispatch_queue_t)queue
                     completion:(void (^)(NSURL * _Nullable url, NSError * _Nullable error))completion
                           done:(void (^)(MTRDownload * finishedDownload))done;
@@ -141,22 +149,38 @@ - (void)handleBDXTransferSessionEndForFileDesignator:(NSString *)fileDesignator
     CHIP_ERROR OnTransferEnd(chip::bdx::BDXTransferProxy * transfer, CHIP_ERROR error) override;
     CHIP_ERROR OnTransferData(chip::bdx::BDXTransferProxy * transfer, const chip::ByteSpan & data) override;
 
-    CHIP_ERROR StartBDXTransferTimeout(MTRDownload * download, uint16_t timeoutInSeconds);
-    void CancelBDXTransferTimeout(MTRDownload * download);
-
 private:
-    static void OnTransferTimeout(chip::System::Layer * layer, void * context);
     MTRDiagnosticLogsDownloader * __weak mDelegate;
 };
 
 @implementation MTRDownload
+
+static void OnTransferTimeout(chip::System::Layer * layer, void * context)
+{
+    assertChipStackLockedByCurrentThread();
+
+    auto * download = (__bridge MTRDownload *) context;
+    VerifyOrReturn(nil != download);
+
+    auto * controller = [[MTRDeviceControllerFactory sharedInstance] runningControllerForFabricIndex:download.fabricIndex.unsignedCharValue];
+
+    MTR_LOG("%@ Diagnostic log transfer timed out for %016llX-%016llX (%llu), abortHandler: %@", download,
+        controller.compressedFabricID.unsignedLongLongValue, download.nodeID.unsignedLongLongValue,
+        download.nodeID.unsignedLongLongValue, download.abortHandler);
+
+    [download abort:[MTRError errorForCHIPErrorCode:CHIP_ERROR_TIMEOUT]];
+}
+
 - (instancetype)initWithType:(MTRDiagnosticLogType)type
                  fabricIndex:(NSNumber *)fabricIndex
                       nodeID:(NSNumber *)nodeID
+                     timeout:(NSTimeInterval)timeout
                        queue:(dispatch_queue_t)queue
                   completion:(void (^)(NSURL * _Nullable url, NSError * _Nullable error))completion
                         done:(void (^)(MTRDownload * finishedDownload))done;
 {
+    assertChipStackLockedByCurrentThread();
+
     self = [super init];
     if (self) {
         auto * fileDesignator = [self _toFileDesignatorString:type nodeID:nodeID];
@@ -184,6 +208,22 @@ - (instancetype)initWithType:(MTRDiagnosticLogType)type
         _fileURL = fileURL;
         _fileHandle = nil;
         _finalize = bdxTransferDone;
+
+        if (timeout <= 0) {
+            timeout = 0;
+        } else if (timeout > UINT16_MAX) {
+            MTR_LOG("Warning: timeout is too large. It will be truncated to UINT16_MAX.");
+            timeout = UINT16_MAX;
+        }
+
+        if (timeout > 0) {
+            CHIP_ERROR timerStartErr = chip::DeviceLayer::SystemLayer().StartTimer(chip::System::Clock::Seconds16(static_cast<uint16_t>(timeout)),
+                OnTransferTimeout, (__bridge void *) self);
+            if (timerStartErr != CHIP_NO_ERROR) {
+                MTR_LOG_ERROR("Failed to start timer for diagnostic log download timeout");
+                return nil;
+            }
+        }
     }
     return self;
 }
@@ -259,7 +299,7 @@ - (BOOL)matches:(NSString *)fileDesignator
     return [_fileDesignator isEqualToString:fileDesignator] && [_fabricIndex isEqualToNumber:fabricIndex] && [_nodeID isEqualToNumber:nodeID];
 }
 
-- (void)failure:(NSError * _Nullable)error
+- (void)failure:(NSError *)error
 {
     MTR_LOG("%@ Diagnostic log transfer failure: %@", self, error);
     _finalize(error);
@@ -270,6 +310,29 @@ - (void)success
     _finalize(nil);
 }
 
+- (void)cancelTimeoutTimer
+{
+    assertChipStackLockedByCurrentThread();
+    chip::DeviceLayer::SystemLayer().CancelTimer(OnTransferTimeout, (__bridge void *) self);
+}
+
+- (void)abort:(NSError *)error
+{
+    assertChipStackLockedByCurrentThread();
+
+    [self cancelTimeoutTimer];
+
+    // If there is no abortHandler, it means that the BDX transfer has not
+    // started, so we can just call failure: directly.
+    //
+    // If there is an abortHandler, we need to call it to abort the transfer.
+    if (self.abortHandler == nil) {
+        [self failure:error];
+    } else {
+        self.abortHandler(error);
+    }
+}
+
 - (NSURL *)_toFileURL:(MTRDiagnosticLogType)type nodeID:(NSNumber *)nodeID
 {
     auto * dateFormatter = [[NSDateFormatter alloc] init];
@@ -344,13 +407,14 @@ - (MTRDownload * _Nullable)get:(NSString *)fileDesignator fabricIndex:(NSNumber
 - (MTRDownload * _Nullable)add:(MTRDiagnosticLogType)type
                    fabricIndex:(NSNumber *)fabricIndex
                         nodeID:(NSNumber *)nodeID
+                       timeout:(NSTimeInterval)timeout
                          queue:(dispatch_queue_t)queue
                     completion:(void (^)(NSURL * _Nullable url, NSError * _Nullable error))completion
                           done:(void (^)(MTRDownload * finishedDownload))done
 {
     assertChipStackLockedByCurrentThread();
 
-    auto download = [[MTRDownload alloc] initWithType:type fabricIndex:fabricIndex nodeID:nodeID queue:queue completion:completion done:done];
+    auto download = [[MTRDownload alloc] initWithType:type fabricIndex:fabricIndex nodeID:nodeID timeout:timeout queue:queue completion:completion done:done];
     VerifyOrReturnValue(nil != download, nil);
 
     [_downloads addObject:download];
@@ -367,7 +431,10 @@ - (void)abortDownloadsForController:(MTRDeviceController_Concrete *)controller
             continue;
         }
 
-        [download failure:[MTRError errorForCHIPErrorCode:CHIP_ERROR_CANCELLED]];
+        [download abort:[MTRError errorForCHIPErrorCode:CHIP_ERROR_CANCELLED]];
+        // Remove directly instead of waiting for the async bits to catch up,
+        // since those async bits might run after controller shutdown finishes
+        // and then not be able to dispatch the async task to do the removal.
         [self remove:download];
     }
 }
@@ -376,6 +443,7 @@ - (void)remove:(MTRDownload *)download
 {
     assertChipStackLockedByCurrentThread();
 
+    [download cancelTimeoutTimer];
     [_downloads removeObject:download];
 }
 @end
@@ -423,29 +491,15 @@ - (void)downloadLogFromNodeWithID:(NSNumber *)nodeID
     // any existing ones so we can start this new one.
     [self abortDownloadsForController:controller];
 
-    uint16_t timeoutInSeconds = 0;
-    if (timeout <= 0) {
-        timeoutInSeconds = 0;
-    } else if (timeout > UINT16_MAX) {
-        MTR_LOG("Warning: timeout is too large. It will be truncated to UINT16_MAX.");
-        timeoutInSeconds = UINT16_MAX;
-    } else {
-        timeoutInSeconds = static_cast<uint16_t>(timeout);
-    }
-
     // This block is always called when a download is finished.
     auto done = ^(MTRDownload * finishedDownload) {
         [controller asyncDispatchToMatterQueue:^() {
             [self->_downloads remove:finishedDownload];
-
-            if (timeoutInSeconds > 0) {
-                self->_bridge->CancelBDXTransferTimeout(finishedDownload);
-            }
         } errorHandler:nil];
     };
 
     auto fabricIndex = @(controller.fabricIndex);
-    auto download = [_downloads add:type fabricIndex:fabricIndex nodeID:nodeID queue:queue completion:completion done:done];
+    auto download = [_downloads add:type fabricIndex:fabricIndex nodeID:nodeID timeout:timeout queue:queue completion:completion done:done];
     VerifyOrReturn(nil != download,
         dispatch_async(queue, ^{ completion(nil, [MTRError errorForCHIPErrorCode:CHIP_ERROR_INTERNAL]); }));
 
@@ -463,11 +517,6 @@ - (void)downloadLogFromNodeWithID:(NSNumber *)nodeID
 
     [cluster retrieveLogsRequestWithParams:params expectedValues:nil expectedValueInterval:nil completion:interactionModelDone];
 
-    if (timeoutInSeconds > 0) {
-        auto err = _bridge->StartBDXTransferTimeout(download, timeoutInSeconds);
-        VerifyOrReturn(CHIP_NO_ERROR == err, [download failure:[MTRError errorForCHIPErrorCode:err]]);
-    }
-
     MTR_LOG("%@ Started log download attempt for node %016llX-%016llX (%llu)", download,
         controller.compressedFabricID.unsignedLongLongValue, nodeID.unsignedLongLongValue, nodeID.unsignedLongLongValue);
 }
@@ -648,40 +697,3 @@ - (void)handleBDXTransferSessionEndForFileDesignator:(NSString *)fileDesignator
                                                   completion:completionHandler];
     return CHIP_NO_ERROR;
 }
-
-CHIP_ERROR DiagnosticLogsDownloaderBridge::StartBDXTransferTimeout(MTRDownload * download, uint16_t timeoutInSeconds)
-{
-    assertChipStackLockedByCurrentThread();
-    return chip::DeviceLayer::SystemLayer().StartTimer(chip::System::Clock::Seconds16(timeoutInSeconds), OnTransferTimeout, (__bridge void *) download);
-}
-
-void DiagnosticLogsDownloaderBridge::CancelBDXTransferTimeout(MTRDownload * download)
-{
-    assertChipStackLockedByCurrentThread();
-    chip::DeviceLayer::SystemLayer().CancelTimer(OnTransferTimeout, (__bridge void *) download);
-}
-
-void DiagnosticLogsDownloaderBridge::OnTransferTimeout(chip::System::Layer * layer, void * context)
-{
-    assertChipStackLockedByCurrentThread();
-
-    auto * download = (__bridge MTRDownload *) context;
-    VerifyOrReturn(nil != download);
-
-    auto * controller = [[MTRDeviceControllerFactory sharedInstance] runningControllerForFabricIndex:download.fabricIndex.unsignedCharValue];
-
-    MTR_LOG("%@ Diagnostic log transfer timed out for %016llX-%016llX (%llu), abortHandler: %@", download,
-        controller.compressedFabricID.unsignedLongLongValue, download.nodeID.unsignedLongLongValue,
-        download.nodeID.unsignedLongLongValue, download.abortHandler);
-
-    // If there is no abortHandler, it means that the BDX transfer has not started.
-    // When a BDX transfer has started we need to abort the transfer and we would error out
-    // at next poll. We would end up calling OnTransferEnd and eventually [download failure:error].
-    // But if the transfer has not started we would stop right now.
-    auto error = [MTRError errorForCHIPErrorCode:CHIP_ERROR_TIMEOUT];
-    if (download.abortHandler == nil) {
-        [download failure:error];
-    } else {
-        download.abortHandler(error);
-    }
-}
