Fix reference cycle between controller and data store in Matter.framework. (#33263)
* Fix reference cycle between controller and data store in Matter.framework.
Also adds some leak detection to unit tests in CI that would have caught this.
* Address review comment.
* Disable the leak checks for now, because we are getting unrelated positives.
diff --git a/.github/workflows/darwin.yaml b/.github/workflows/darwin.yaml
index dcc025b..62a8d08 100644
--- a/.github/workflows/darwin.yaml
+++ b/.github/workflows/darwin.yaml
@@ -83,6 +83,9 @@
-enableUndefinedBehaviorSanitizer YES
- flavor: tsan
arguments: -enableThreadSanitizer YES
+ # "leaks" does not seem to be very compatible with asan or tsan
+ - flavor: leaks
+ defines: ENABLE_LEAK_DETECTION=1
steps:
- name: Checkout
uses: actions/checkout@v4
diff --git a/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm b/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm
index 38f0bbc..1f515f2 100644
--- a/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm
+++ b/src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm
@@ -22,6 +22,7 @@
#include <lib/core/CASEAuthTag.h>
#include <lib/core/NodeId.h>
+#include <lib/support/CodeUtils.h>
#include <lib/support/SafeInt.h>
// FIXME: Are these good key strings? https://github.com/project-chip/connectedhomeip/issues/28973
@@ -106,7 +107,8 @@
@implementation MTRDeviceControllerDataStore {
id<MTRDeviceControllerStorageDelegate> _storageDelegate;
dispatch_queue_t _storageDelegateQueue;
- MTRDeviceController * _controller;
+ // Controller owns us, so we have to make sure to not keep it alive.
+ __weak MTRDeviceController * _controller;
// Array of nodes with resumption info, oldest-stored first.
NSMutableArray<NSNumber *> * _nodesWithResumptionInfo;
}
@@ -126,7 +128,9 @@
__block id resumptionNodeList;
dispatch_sync(_storageDelegateQueue, ^{
@autoreleasepool {
- resumptionNodeList = [_storageDelegate controller:_controller
+ // NOTE: controller, not our weak ref, since we know it's still
+ // valid under this sync dispatch.
+ resumptionNodeList = [_storageDelegate controller:controller
valueForKey:sResumptionNodeListKey
securityLevel:MTRStorageSecurityLevelSecure
sharingType:MTRStorageSharingTypeNotShared];
@@ -154,9 +158,12 @@
- (void)fetchAttributeDataForAllDevices:(MTRDeviceControllerDataStoreClusterDataHandler)clusterDataHandler
{
__block NSDictionary<NSString *, id> * dataStoreSecureLocalValues = nil;
+ MTRDeviceController * controller = _controller;
+ VerifyOrReturn(controller != nil); // No way to call delegate without controller.
+
dispatch_sync(_storageDelegateQueue, ^{
if ([self->_storageDelegate respondsToSelector:@selector(valuesForController:securityLevel:sharingType:)]) {
- dataStoreSecureLocalValues = [self->_storageDelegate valuesForController:self->_controller securityLevel:MTRStorageSecurityLevelSecure sharingType:MTRStorageSharingTypeNotShared];
+ dataStoreSecureLocalValues = [self->_storageDelegate valuesForController:controller securityLevel:MTRStorageSecurityLevelSecure sharingType:MTRStorageSharingTypeNotShared];
}
});
@@ -177,24 +184,27 @@
- (void)storeResumptionInfo:(MTRCASESessionResumptionInfo *)resumptionInfo
{
+ MTRDeviceController * controller = _controller;
+ VerifyOrReturn(controller != nil); // No way to call delegate without controller.
+
auto * oldInfo = [self findResumptionInfoByNodeID:resumptionInfo.nodeID];
dispatch_sync(_storageDelegateQueue, ^{
if (oldInfo != nil) {
// Remove old resumption id key. No need to do that for the
// node id, because we are about to overwrite it.
- [_storageDelegate controller:_controller
+ [_storageDelegate controller:controller
removeValueForKey:ResumptionByResumptionIDKey(oldInfo.resumptionID)
securityLevel:MTRStorageSecurityLevelSecure
sharingType:MTRStorageSharingTypeNotShared];
[_nodesWithResumptionInfo removeObject:resumptionInfo.nodeID];
}
- [_storageDelegate controller:_controller
+ [_storageDelegate controller:controller
storeValue:resumptionInfo
forKey:ResumptionByNodeIDKey(resumptionInfo.nodeID)
securityLevel:MTRStorageSecurityLevelSecure
sharingType:MTRStorageSharingTypeNotShared];
- [_storageDelegate controller:_controller
+ [_storageDelegate controller:controller
storeValue:resumptionInfo
forKey:ResumptionByResumptionIDKey(resumptionInfo.resumptionID)
securityLevel:MTRStorageSecurityLevelSecure
@@ -202,7 +212,7 @@
// Update our resumption info node list.
[_nodesWithResumptionInfo addObject:resumptionInfo.nodeID];
- [_storageDelegate controller:_controller
+ [_storageDelegate controller:controller
storeValue:[_nodesWithResumptionInfo copy]
forKey:sResumptionNodeListKey
securityLevel:MTRStorageSecurityLevelSecure
@@ -212,17 +222,20 @@
- (void)clearAllResumptionInfo
{
+ MTRDeviceController * controller = _controller;
+ VerifyOrReturn(controller != nil); // No way to call delegate without controller.
+
// Can we do less dispatch? We would need to have a version of
// _findResumptionInfoWithKey that assumes we are already on the right queue.
for (NSNumber * nodeID in _nodesWithResumptionInfo) {
auto * oldInfo = [self findResumptionInfoByNodeID:nodeID];
if (oldInfo != nil) {
dispatch_sync(_storageDelegateQueue, ^{
- [_storageDelegate controller:_controller
+ [_storageDelegate controller:controller
removeValueForKey:ResumptionByResumptionIDKey(oldInfo.resumptionID)
securityLevel:MTRStorageSecurityLevelSecure
sharingType:MTRStorageSharingTypeNotShared];
- [_storageDelegate controller:_controller
+ [_storageDelegate controller:controller
removeValueForKey:ResumptionByNodeIDKey(oldInfo.nodeID)
securityLevel:MTRStorageSecurityLevelSecure
sharingType:MTRStorageSharingTypeNotShared];
@@ -235,9 +248,12 @@
- (CHIP_ERROR)storeLastLocallyUsedNOC:(MTRCertificateTLVBytes)noc
{
+ MTRDeviceController * controller = _controller;
+ VerifyOrReturnError(controller != nil, CHIP_ERROR_PERSISTED_STORAGE_FAILED); // No way to call delegate without controller.
+
__block BOOL ok;
dispatch_sync(_storageDelegateQueue, ^{
- ok = [_storageDelegate controller:_controller
+ ok = [_storageDelegate controller:controller
storeValue:noc
forKey:sLastLocallyUsedNOCKey
securityLevel:MTRStorageSecurityLevelSecure
@@ -248,10 +264,13 @@
- (MTRCertificateTLVBytes _Nullable)fetchLastLocallyUsedNOC
{
+ MTRDeviceController * controller = _controller;
+ VerifyOrReturnValue(controller != nil, nil); // No way to call delegate without controller.
+
__block id data;
dispatch_sync(_storageDelegateQueue, ^{
@autoreleasepool {
- data = [_storageDelegate controller:_controller
+ data = [_storageDelegate controller:controller
valueForKey:sLastLocallyUsedNOCKey
securityLevel:MTRStorageSecurityLevelSecure
sharingType:MTRStorageSharingTypeNotShared];
@@ -271,6 +290,9 @@
- (nullable MTRCASESessionResumptionInfo *)_findResumptionInfoWithKey:(nullable NSString *)key
{
+ MTRDeviceController * controller = _controller;
+ VerifyOrReturnValue(controller != nil, nil); // No way to call delegate without controller.
+
// key could be nil if [NSString stringWithFormat] returns nil for some reason.
if (key == nil) {
return nil;
@@ -279,7 +301,7 @@
__block id resumptionInfo;
dispatch_sync(_storageDelegateQueue, ^{
@autoreleasepool {
- resumptionInfo = [_storageDelegate controller:_controller
+ resumptionInfo = [_storageDelegate controller:controller
valueForKey:key
securityLevel:MTRStorageSecurityLevelSecure
sharingType:MTRStorageSharingTypeNotShared];
@@ -318,9 +340,12 @@
- (id)_fetchAttributeCacheValueForKey:(NSString *)key expectedClass:(Class)expectedClass;
{
+ MTRDeviceController * controller = _controller;
+ VerifyOrReturnValue(controller != nil, nil); // No way to call delegate without controller.
+
id data;
@autoreleasepool {
- data = [_storageDelegate controller:_controller
+ data = [_storageDelegate controller:controller
valueForKey:key
securityLevel:MTRStorageSecurityLevelSecure
sharingType:MTRStorageSharingTypeNotShared];
@@ -338,7 +363,10 @@
- (BOOL)_storeAttributeCacheValue:(id)value forKey:(NSString *)key
{
- return [_storageDelegate controller:_controller
+ MTRDeviceController * controller = _controller;
+ VerifyOrReturnValue(controller != nil, NO); // No way to call delegate without controller.
+
+ return [_storageDelegate controller:controller
storeValue:value
forKey:key
securityLevel:MTRStorageSecurityLevelSecure
@@ -347,7 +375,10 @@
- (BOOL)_bulkStoreAttributeCacheValues:(NSDictionary<NSString *, id<NSSecureCoding>> *)values
{
- return [_storageDelegate controller:_controller
+ MTRDeviceController * controller = _controller;
+ VerifyOrReturnValue(controller != nil, NO); // No way to call delegate without controller.
+
+ return [_storageDelegate controller:controller
storeValues:values
securityLevel:MTRStorageSecurityLevelSecure
sharingType:MTRStorageSharingTypeNotShared];
@@ -355,7 +386,10 @@
- (BOOL)_removeAttributeCacheValueForKey:(NSString *)key
{
- return [_storageDelegate controller:_controller
+ MTRDeviceController * controller = _controller;
+ VerifyOrReturnValue(controller != nil, NO); // No way to call delegate without controller.
+
+ return [_storageDelegate controller:controller
removeValueForKey:key
securityLevel:MTRStorageSecurityLevelSecure
sharingType:MTRStorageSharingTypeNotShared];
diff --git a/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m b/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m
index cce918d..3c6ebfd 100644
--- a/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m
+++ b/src/darwin/Framework/CHIPTests/MTRPerControllerStorageTests.m
@@ -16,14 +16,12 @@
#import <Matter/Matter.h>
-// system dependencies
-#import <XCTest/XCTest.h>
-
#import "MTRDeviceControllerLocalTestStorage.h"
#import "MTRDeviceTestDelegate.h"
#import "MTRDevice_Internal.h"
#import "MTRErrorTestUtils.h"
#import "MTRFabricInfoChecker.h"
+#import "MTRTestCase.h"
#import "MTRTestDeclarations.h"
#import "MTRTestKeys.h"
#import "MTRTestPerControllerStorage.h"
@@ -177,7 +175,7 @@
@end
-@interface MTRPerControllerStorageTests : XCTestCase
+@interface MTRPerControllerStorageTests : MTRTestCase
@end
@implementation MTRPerControllerStorageTests {
@@ -353,6 +351,8 @@
- (void)test001_BasicControllerStartup
{
+ self.detectLeaks = YES;
+
__auto_type * factory = [MTRDeviceControllerFactory sharedInstance];
XCTAssertNotNil(factory);
@@ -401,6 +401,8 @@
- (void)test002_TryStartingTwoControllersWithSameNodeID
{
+ self.detectLeaks = YES;
+
__auto_type * rootKeys = [[MTRTestKeys alloc] init];
XCTAssertNotNil(rootKeys);
diff --git a/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestCase.h b/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestCase.h
new file mode 100644
index 0000000..d12ea0a
--- /dev/null
+++ b/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestCase.h
@@ -0,0 +1,28 @@
+/**
+ * Copyright (c) 2024 Project CHIP Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#import <XCTest/XCTest.h>
+
+NS_ASSUME_NONNULL_BEGIN
+
+@interface MTRTestCase : XCTestCase
+// It would be nice to do the leak-detection automatically, but running "leaks"
+// on every single sub-test is slow, and some of our tests seem to have leaks
+// outside Matter.framework. So have it be opt-in for now, and improve later.
+@property (nonatomic) BOOL detectLeaks;
+@end
+
+NS_ASSUME_NONNULL_END
diff --git a/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestCase.mm b/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestCase.mm
new file mode 100644
index 0000000..b4cf92b
--- /dev/null
+++ b/src/darwin/Framework/CHIPTests/TestHelpers/MTRTestCase.mm
@@ -0,0 +1,44 @@
+/**
+ * Copyright (c) 2024 Project CHIP Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <stdlib.h>
+#include <unistd.h>
+
+#import "MTRTestCase.h"
+
+@implementation MTRTestCase
+
+/**
+ * Unfortunately, doing this in "+ (void)tearDown" (the global suite teardown)
+ * does not trigger a test failure even if the XCTAssertEqual fails.
+ */
+- (void)tearDown
+{
+#if 0
+#if defined(ENABLE_LEAK_DETECTION) && ENABLE_LEAK_DETECTION
+ if (_detectLeaks) {
+ int pid = getpid();
+ __auto_type * cmd = [NSString stringWithFormat:@"leaks %d", pid];
+ int ret = system(cmd.UTF8String);
+ XCTAssertEqual(ret, 0, "LEAKS DETECTED");
+ }
+#endif
+#endif
+
+ [super tearDown];
+}
+
+@end
diff --git a/src/darwin/Framework/Matter.xcodeproj/project.pbxproj b/src/darwin/Framework/Matter.xcodeproj/project.pbxproj
index 03b0b10..be17129 100644
--- a/src/darwin/Framework/Matter.xcodeproj/project.pbxproj
+++ b/src/darwin/Framework/Matter.xcodeproj/project.pbxproj
@@ -139,6 +139,7 @@
512431282BA0C8BF000BC136 /* SetMRPParametersCommand.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5124311C2BA0C09A000BC136 /* SetMRPParametersCommand.mm */; };
512431292BA0C8BF000BC136 /* ResetMRPParametersCommand.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5124311A2BA0C09A000BC136 /* ResetMRPParametersCommand.mm */; };
5129BCFD26A9EE3300122DDF /* MTRError.h in Headers */ = {isa = PBXBuildFile; fileRef = 5129BCFC26A9EE3300122DDF /* MTRError.h */; settings = {ATTRIBUTES = (Public, ); }; };
+ 5131BF662BE2E1B000D5D6BC /* MTRTestCase.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5131BF642BE2E1B000D5D6BC /* MTRTestCase.mm */; };
51339B1F2A0DA64D00C798C1 /* MTRCertificateValidityTests.m in Sources */ = {isa = PBXBuildFile; fileRef = 51339B1E2A0DA64D00C798C1 /* MTRCertificateValidityTests.m */; };
5136661328067D550025EDAE /* MTRDeviceController_Internal.h in Headers */ = {isa = PBXBuildFile; fileRef = 5136660F28067D540025EDAE /* MTRDeviceController_Internal.h */; };
5136661428067D550025EDAE /* MTRDeviceControllerFactory.mm in Sources */ = {isa = PBXBuildFile; fileRef = 5136661028067D540025EDAE /* MTRDeviceControllerFactory.mm */; };
@@ -549,6 +550,8 @@
5124311B2BA0C09A000BC136 /* SetMRPParametersCommand.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = SetMRPParametersCommand.h; sourceTree = "<group>"; };
5124311C2BA0C09A000BC136 /* SetMRPParametersCommand.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = SetMRPParametersCommand.mm; sourceTree = "<group>"; };
5129BCFC26A9EE3300122DDF /* MTRError.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = MTRError.h; sourceTree = "<group>"; };
+ 5131BF642BE2E1B000D5D6BC /* MTRTestCase.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = MTRTestCase.mm; sourceTree = "<group>"; };
+ 5131BF652BE2E1B000D5D6BC /* MTRTestCase.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MTRTestCase.h; sourceTree = "<group>"; };
51339B1E2A0DA64D00C798C1 /* MTRCertificateValidityTests.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = MTRCertificateValidityTests.m; sourceTree = "<group>"; };
5136660F28067D540025EDAE /* MTRDeviceController_Internal.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = MTRDeviceController_Internal.h; sourceTree = "<group>"; };
5136661028067D540025EDAE /* MTRDeviceControllerFactory.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = MTRDeviceControllerFactory.mm; sourceTree = "<group>"; };
@@ -1119,6 +1122,8 @@
51189FC72A33ACE900184508 /* TestHelpers */ = {
isa = PBXGroup;
children = (
+ 5131BF652BE2E1B000D5D6BC /* MTRTestCase.h */,
+ 5131BF642BE2E1B000D5D6BC /* MTRTestCase.mm */,
D437613F285BDC0D0051FEA2 /* MTRTestKeys.h */,
51C8E3F72825CDB600D47D00 /* MTRTestKeys.m */,
D4376140285BDC0D0051FEA2 /* MTRTestStorage.h */,
@@ -1974,6 +1979,7 @@
buildActionMask = 2147483647;
files = (
51742B4E29CB6B88009974FE /* MTRPairingTests.m in Sources */,
+ 5131BF662BE2E1B000D5D6BC /* MTRTestCase.mm in Sources */,
51669AF02913204400F4AA36 /* MTRBackwardsCompatTests.m in Sources */,
51D10D2E2808E2CA00E8CA3D /* MTRTestStorage.m in Sources */,
51D0B12A2B61766F006E3511 /* MTRServerEndpointTests.m in Sources */,
