From 4dedcf4b59d23fdf4ce36918143ce7b2732929f3 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 2 May 2024 10:14:18 -0400 Subject: [PATCH] Fix leak in MTRBaseDevice tests. Our test code in deregisterReportHandlersWithQueue would directly delete the ReadClient, but that never called OnDone, and hence never deleted the BufferedReadClientCallback that we had also allocated. The fix is to put all the things we need to deallocate in the MTRReadClientContainer and consistently use it to clean those things up, including on all the various failure paths we have. Removes the CauseReadClientFailure function which did not in fact cause a failure, and instead just has the relevant test trigger a subscription drop. --- src/darwin/Framework/CHIP/MTRBaseDevice.mm | 112 ++++-------------- .../Framework/CHIPTests/MTRDeviceTests.m | 39 ++++-- .../TestHelpers/MTRTestDeclarations.h | 2 - 3 files changed, 53 insertions(+), 100 deletions(-) diff --git a/src/darwin/Framework/CHIP/MTRBaseDevice.mm b/src/darwin/Framework/CHIP/MTRBaseDevice.mm index 356e607ad86d4b..0712e779c525e6 100644 --- a/src/darwin/Framework/CHIP/MTRBaseDevice.mm +++ b/src/darwin/Framework/CHIP/MTRBaseDevice.mm @@ -91,12 +91,18 @@ NSString * const MTREventIsHistoricalKey = @"eventIsHistorical"; class MTRDataValueDictionaryCallbackBridge; +class MTRDataValueDictionaryDecodableType; +template +class BufferedReadClientCallback; @interface MTRReadClientContainer : NSObject @property (nonatomic, readwrite) app::ReadClient * readClientPtr; +@property (nonatomic, readwrite) BufferedReadClientCallback * callback; @property (nonatomic, readwrite) app::AttributePathParams * pathParams; @property (nonatomic, readwrite) app::EventPathParams * eventPathParams; @property (nonatomic, readwrite) uint64_t deviceID; + +- (void)cleanup; - (void)onDone; @end @@ -157,22 +163,7 @@ static void PurgeReadClientContainers( [controller asyncDispatchToMatterQueue:^() { for (MTRReadClientContainer * container in listToDelete) { - if (container.readClientPtr) { - Platform::Delete(container.readClientPtr); - container.readClientPtr = nullptr; - } - if (container.pathParams) { - static_assert(std::is_trivially_destructible::value, - "AttributePathParams destructors won't get run"); - Platform::MemoryFree(container.pathParams); - container.pathParams = nullptr; - } - if (container.eventPathParams) { - static_assert( - std::is_trivially_destructible::value, "EventPathParams destructors won't get run"); - Platform::MemoryFree(container.eventPathParams); - container.eventPathParams = nullptr; - } + [container cleanup]; } [listToDelete removeAllObjects]; if (completion) { @@ -204,47 +195,13 @@ static void PurgeCompletedReadClientContainers(uint64_t deviceId) [readClientContainersLock unlock]; } -#ifdef DEBUG -// This function is for unit testing only. This function closes all read clients. -static void CauseReadClientFailure( - MTRDeviceController * controller, uint64_t deviceId, dispatch_queue_t queue, void (^_Nullable completion)(void)) -{ - InitializeReadClientContainers(); - - NSMutableArray * listToFail; - NSNumber * key = [NSNumber numberWithUnsignedLongLong:deviceId]; - [readClientContainersLock lock]; - listToFail = readClientContainers[key]; - [readClientContainers removeObjectForKey:key]; - [readClientContainersLock unlock]; - - [controller - asyncDispatchToMatterQueue:^() { - for (MTRReadClientContainer * container in listToFail) { - // Send auto resubscribe request again by read clients, which must fail. - chip::app::ReadPrepareParams readParams; - if (container.readClientPtr) { - container.readClientPtr->SendAutoResubscribeRequest(std::move(readParams)); - } - } - if (completion) { - dispatch_async(queue, completion); - } - } - errorHandler:^(NSError * error) { - // Can't fail things. Just put them back. - ReinstateReadClientList(listToFail, key, queue, completion); - }]; -} -#endif - static bool CheckMemberOfType(NSDictionary * responseValue, NSString * memberName, Class expectedClass, NSString * errorMessage, NSError * __autoreleasing * error); static void LogStringAndReturnError(NSString * errorStr, CHIP_ERROR errorCode, NSError * __autoreleasing * error); static void LogStringAndReturnError(NSString * errorStr, MTRErrorCode errorCode, NSError * __autoreleasing * error); @implementation MTRReadClientContainer -- (void)onDone +- (void)cleanup { if (_readClientPtr) { Platform::Delete(_readClientPtr); @@ -260,26 +217,19 @@ - (void)onDone Platform::MemoryFree(_eventPathParams); _eventPathParams = nullptr; } - PurgeCompletedReadClientContainers(_deviceID); + if (_callback) { + Platform::Delete(_callback); + _callback = nullptr; + } } -- (void)dealloc +- (void)onDone { - if (_readClientPtr) { - Platform::Delete(_readClientPtr); - _readClientPtr = nullptr; - } - if (_pathParams) { - static_assert(std::is_trivially_destructible::value, "AttributePathParams destructors won't get run"); - Platform::MemoryFree(_pathParams); - _pathParams = nullptr; - } - if (_eventPathParams) { - static_assert(std::is_trivially_destructible::value, "EventPathParams destructors won't get run"); - Platform::MemoryFree(_eventPathParams); - _eventPathParams = nullptr; - } + [self cleanup]; + + PurgeCompletedReadClientContainers(_deviceID); } + @end @implementation MTRBaseDevice @@ -1730,6 +1680,7 @@ - (void)subscribeToAttributePaths:(NSArray * _Nullabl dispatch_async(queue, ^{ reportHandler(nil, [MTRError errorForCHIPErrorCode:CHIP_ERROR_NO_MEMORY]); }); + [container cleanup]; return; } for (MTRAttributeRequestPath * attribute in attributes) { @@ -1744,6 +1695,7 @@ - (void)subscribeToAttributePaths:(NSArray * _Nullabl dispatch_async(queue, ^{ reportHandler(nil, [MTRError errorForCHIPErrorCode:CHIP_ERROR_NO_MEMORY]); }); + [container cleanup]; return; } for (MTREventRequestPath * event in events) { @@ -1763,9 +1715,6 @@ - (void)subscribeToAttributePaths:(NSArray * _Nullabl auto onDone = [container](BufferedReadClientCallback * callback) { [container onDone]; - // Make sure we delete callback last, because doing that actually destroys our - // lambda, so we can't access captured values after that. - chip::Platform::Delete(callback); }; auto callback = chip::Platform::MakeUnique>( @@ -1775,6 +1724,9 @@ - (void)subscribeToAttributePaths:(NSArray * _Nullabl auto readClient = Platform::New( engine, exchangeManager, callback->GetBufferedCallback(), chip::app::ReadClient::InteractionType::Subscribe); + container.readClientPtr = readClient; + container.callback = callback.release(); + if (!params.resubscribeAutomatically) { err = readClient->SendRequest(readParams); } else { @@ -1785,23 +1737,12 @@ - (void)subscribeToAttributePaths:(NSArray * _Nullabl dispatch_async(queue, ^{ reportHandler(nil, [MTRError errorForCHIPErrorCode:err]); }); - Platform::Delete(readClient); - if (container.pathParams != nullptr) { - Platform::MemoryFree(container.pathParams); - } - - if (container.eventPathParams != nullptr) { - Platform::MemoryFree(container.eventPathParams); - } - container.pathParams = nullptr; - container.eventPathParams = nullptr; + [container cleanup]; return; } // Read clients will be purged when deregistered. - container.readClientPtr = readClient; AddReadClientContainer(container.deviceID, container); - callback.release(); }]; } @@ -2024,13 +1965,6 @@ - (void)openCommissioningWindowWithDiscriminator:(NSNumber *)discriminator } #ifdef DEBUG -// This method is for unit testing only -- (void)failSubscribers:(dispatch_queue_t)queue completion:(void (^)(void))completion -{ - MTR_LOG_DEBUG("Causing failure in subscribers on purpose"); - CauseReadClientFailure(self.deviceController, self.nodeID, queue, completion); -} - // The following method is for unit testing purpose only + (id)CHIPEncodeAndDecodeNSObject:(id)object { diff --git a/src/darwin/Framework/CHIPTests/MTRDeviceTests.m b/src/darwin/Framework/CHIPTests/MTRDeviceTests.m index 2dc01461826b40..7a7a3d82043699 100644 --- a/src/darwin/Framework/CHIPTests/MTRDeviceTests.m +++ b/src/darwin/Framework/CHIPTests/MTRDeviceTests.m @@ -1071,8 +1071,8 @@ - (void)test012_SubscriptionError XCTAssertTrue([result[@"data"] isKindOfClass:[NSDictionary class]]); XCTAssertTrue([result[@"data"][@"type"] isEqualToString:@"Boolean"]); if ([result[@"data"][@"value"] boolValue] == YES) { - [reportExpectation fulfill]; globalReportHandler = nil; + [reportExpectation fulfill]; } }; @@ -1109,13 +1109,34 @@ - (void)test012_SubscriptionError // Wait for report [self waitForExpectations:[NSArray arrayWithObject:reportExpectation] timeout:kTimeoutInSeconds]; - // Trigger reader failure - XCTestExpectation * failureExpectation = [self expectationWithDescription:@"failed on purpose"]; - [device failSubscribers:queue - completion:^{ - [failureExpectation fulfill]; - }]; - [self waitForExpectations:@[ failureExpectation ] timeout:kTimeoutInSeconds]; + XCTestExpectation * errorExpectation1 = [self expectationWithDescription:@"First subscription errored out"]; + XCTestExpectation * errorExpectation2 = [self expectationWithDescription:@"Second subscription errored out"]; + + globalReportHandler = ^(id _Nullable values, NSError * _Nullable error) { + XCTAssertNil(values); + XCTAssertNotNil(error); + globalReportHandler = nil; + [errorExpectation1 fulfill]; + }; + + // Try to create a second subscription, which will cancel the first + // subscription. We can use a non-existent path here to cut down on the + // work that gets done. + params.replaceExistingSubscriptions = YES; + [device subscribeToAttributesWithEndpointID:@10000 + clusterID:@6 + attributeID:@0 + params:params + queue:queue + reportHandler:^(id _Nullable values, NSError * _Nullable error) { + XCTAssertNil(values); + XCTAssertNotNil(error); + [errorExpectation2 fulfill]; + } + subscriptionEstablished:^() { + XCTFail("Did not expect this subscription to succeed"); + }]; + [self waitForExpectations:@[ errorExpectation1, errorExpectation2 ] timeout:60]; deregisterExpectation = [self expectationWithDescription:@"Report handler deregistered"]; [device deregisterReportHandlersWithQueue:queue @@ -1124,7 +1145,7 @@ - (void)test012_SubscriptionError }]; [self waitForExpectations:@[ deregisterExpectation ] timeout:kTimeoutInSeconds]; } -#endif +#endif // DEBUG - (void)test013_ReuseChipClusterObject { diff --git a/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestDeclarations.h b/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestDeclarations.h index ba0d94e1c341dc..97819f8c99ce07 100644 --- a/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestDeclarations.h +++ b/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestDeclarations.h @@ -54,8 +54,6 @@ NS_ASSUME_NONNULL_BEGIN @end @interface MTRBaseDevice (TestDebug) -- (void)failSubscribers:(dispatch_queue_t)queue completion:(void (^)(void))completion; - // Test function for whitebox testing + (id)CHIPEncodeAndDecodeNSObject:(id)object; @end