Skip to content

Commit

Permalink
Fix leak in MTRBaseDevice tests.
Browse files Browse the repository at this point in the history 
Our test code in deregisterReportHandlersWithQueue would directly delete the
ReadClient, but that never called OnDone, and hence never deleted the
BufferedReadClientCallback<MTRDataValueDictionaryDecodableType> 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.
  • Loading branch information
@bzbarsky-apple
bzbarsky-apple committed May 2, 2024
1 parent 3fa70cf commit 4dedcf4
Show file tree
Hide file tree
Showing 3 changed files with 53 additions and 100 deletions.
112 changes: 23 additions & 89 deletions src/darwin/Framework/CHIP/MTRBaseDevice.mm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,12 +91,18 @@
NSString * const MTREventIsHistoricalKey = @"eventIsHistorical";

class MTRDataValueDictionaryCallbackBridge;
class MTRDataValueDictionaryDecodableType;
template <typename DecodableValueType>
class BufferedReadClientCallback;

@interface MTRReadClientContainer : NSObject
@property (nonatomic, readwrite) app::ReadClient * readClientPtr;
@property (nonatomic, readwrite) BufferedReadClientCallback<MTRDataValueDictionaryDecodableType> * callback;
@property (nonatomic, readwrite) app::AttributePathParams * pathParams;
@property (nonatomic, readwrite) app::EventPathParams * eventPathParams;
@property (nonatomic, readwrite) uint64_t deviceID;

- (void)cleanup;
- (void)onDone;
@end

Expand Down Expand Up @@ -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<AttributePathParams>::value,
"AttributePathParams destructors won't get run");
Platform::MemoryFree(container.pathParams);
container.pathParams = nullptr;
}
if (container.eventPathParams) {
static_assert(
std::is_trivially_destructible<EventPathParams>::value, "EventPathParams destructors won't get run");
Platform::MemoryFree(container.eventPathParams);
container.eventPathParams = nullptr;
}
[container cleanup];
}
[listToDelete removeAllObjects];
if (completion) {
Expand Down Expand Up @@ -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<MTRReadClientContainer *> * 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<NSString *, id> * 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);
Expand All @@ -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<AttributePathParams>::value, "AttributePathParams destructors won't get run");
Platform::MemoryFree(_pathParams);
_pathParams = nullptr;
}
if (_eventPathParams) {
static_assert(std::is_trivially_destructible<EventPathParams>::value, "EventPathParams destructors won't get run");
Platform::MemoryFree(_eventPathParams);
_eventPathParams = nullptr;
}
[self cleanup];

PurgeCompletedReadClientContainers(_deviceID);
}

@end

@implementation MTRBaseDevice
Expand Down Expand Up @@ -1730,6 +1680,7 @@ - (void)subscribeToAttributePaths:(NSArray<MTRAttributeRequestPath *> * _Nullabl
dispatch_async(queue, ^{
reportHandler(nil, [MTRError errorForCHIPErrorCode:CHIP_ERROR_NO_MEMORY]);
});
[container cleanup];
return;
}
for (MTRAttributeRequestPath * attribute in attributes) {
Expand All @@ -1744,6 +1695,7 @@ - (void)subscribeToAttributePaths:(NSArray<MTRAttributeRequestPath *> * _Nullabl
dispatch_async(queue, ^{
reportHandler(nil, [MTRError errorForCHIPErrorCode:CHIP_ERROR_NO_MEMORY]);
});
[container cleanup];
return;
}
for (MTREventRequestPath * event in events) {
Expand All @@ -1763,9 +1715,6 @@ - (void)subscribeToAttributePaths:(NSArray<MTRAttributeRequestPath *> * _Nullabl

auto onDone = [container](BufferedReadClientCallback<MTRDataValueDictionaryDecodableType> * 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<BufferedReadClientCallback<MTRDataValueDictionaryDecodableType>>(
Expand All @@ -1775,6 +1724,9 @@ - (void)subscribeToAttributePaths:(NSArray<MTRAttributeRequestPath *> * _Nullabl
auto readClient = Platform::New<app::ReadClient>(
engine, exchangeManager, callback->GetBufferedCallback(), chip::app::ReadClient::InteractionType::Subscribe);

container.readClientPtr = readClient;
container.callback = callback.release();

if (!params.resubscribeAutomatically) {
err = readClient->SendRequest(readParams);
} else {
Expand All @@ -1785,23 +1737,12 @@ - (void)subscribeToAttributePaths:(NSArray<MTRAttributeRequestPath *> * _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();
}];
}

Expand Down Expand Up @@ -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
{
Expand Down
39 changes: 30 additions & 9 deletions src/darwin/Framework/CHIPTests/MTRDeviceTests.m
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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];
}
};

Expand Down Expand Up @@ -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
Expand All @@ -1124,7 +1145,7 @@ - (void)test012_SubscriptionError
}];
[self waitForExpectations:@[ deregisterExpectation ] timeout:kTimeoutInSeconds];
}
#endif
#endif // DEBUG

- (void)test013_ReuseChipClusterObject
{
Expand Down
2 changes: 0 additions & 2 deletions src/darwin/Framework/CHIPTests/TestHelpers/MTRTestDeclarations.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 4dedcf4

Please sign in to comment.