Skip to content

Commit b299671

Browse files
bzbarsky-applebzbarsky-apple
committed
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.
1 parent 335ae19 commit b299671

31 files changed

+238
-99
lines changed

‎.github/workflows/darwin.yaml

+3
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,9 @@ jobs:
8383
-enableUndefinedBehaviorSanitizer YES
8484
- flavor: tsan
8585
arguments: -enableThreadSanitizer YES
86+
# "leaks" does not seem to be very compatible with asan or tsan
87+
- flavor: leaks
88+
defines: ENABLE_LEAK_DETECTION=1
8689
steps:
8790
- name: Checkout
8891
uses: actions/checkout@v4

‎src/darwin/Framework/CHIP/MTRDeviceControllerDataStore.mm

+79-16
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,8 @@ static bool IsValidCATNumber(id _Nullable value)
106106
@implementation MTRDeviceControllerDataStore {
107107
id<MTRDeviceControllerStorageDelegate> _storageDelegate;
108108
dispatch_queue_t _storageDelegateQueue;
109-
MTRDeviceController * _controller;
109+
// Controller owns us, so we have to make sure to not keep it alive.
110+
__weak MTRDeviceController * _controller;
110111
// Array of nodes with resumption info, oldest-stored first.
111112
NSMutableArray<NSNumber *> * _nodesWithResumptionInfo;
112113
}
@@ -126,7 +127,9 @@ - (nullable instancetype)initWithController:(MTRDeviceController *)controller
126127
__block id resumptionNodeList;
127128
dispatch_sync(_storageDelegateQueue, ^{
128129
@autoreleasepool {
129-
resumptionNodeList = [_storageDelegate controller:_controller
130+
// NOTE: controller, not our weak ref, since we know it's still
131+
// valid under this sync dispatch.
132+
resumptionNodeList = [_storageDelegate controller:controller
130133
valueForKey:sResumptionNodeListKey
131134
securityLevel:MTRStorageSecurityLevelSecure
132135
sharingType:MTRStorageSharingTypeNotShared];
@@ -154,9 +157,15 @@ - (nullable instancetype)initWithController:(MTRDeviceController *)controller
154157
- (void)fetchAttributeDataForAllDevices:(MTRDeviceControllerDataStoreClusterDataHandler)clusterDataHandler
155158
{
156159
__block NSDictionary<NSString *, id> * dataStoreSecureLocalValues = nil;
160+
MTRDeviceController * controller = _controller;
161+
if (controller == nil) {
162+
// Not expected; no way to call delegate without controller.
163+
return;
164+
}
165+
157166
dispatch_sync(_storageDelegateQueue, ^{
158167
if ([self->_storageDelegate respondsToSelector:@selector(valuesForController:securityLevel:sharingType:)]) {
159-
dataStoreSecureLocalValues = [self->_storageDelegate valuesForController:self->_controller securityLevel:MTRStorageSecurityLevelSecure sharingType:MTRStorageSharingTypeNotShared];
168+
dataStoreSecureLocalValues = [self->_storageDelegate valuesForController:controller securityLevel:MTRStorageSecurityLevelSecure sharingType:MTRStorageSharingTypeNotShared];
160169
}
161170
});
162171

@@ -177,32 +186,38 @@ - (nullable MTRCASESessionResumptionInfo *)findResumptionInfoByResumptionID:(NSD
177186

178187
- (void)storeResumptionInfo:(MTRCASESessionResumptionInfo *)resumptionInfo
179188
{
189+
MTRDeviceController * controller = _controller;
190+
if (controller == nil) {
191+
// Not expected; no way to call delegate without controller.
192+
return;
193+
}
194+
180195
auto * oldInfo = [self findResumptionInfoByNodeID:resumptionInfo.nodeID];
181196
dispatch_sync(_storageDelegateQueue, ^{
182197
if (oldInfo != nil) {
183198
// Remove old resumption id key. No need to do that for the
184199
// node id, because we are about to overwrite it.
185-
[_storageDelegate controller:_controller
200+
[_storageDelegate controller:controller
186201
removeValueForKey:ResumptionByResumptionIDKey(oldInfo.resumptionID)
187202
securityLevel:MTRStorageSecurityLevelSecure
188203
sharingType:MTRStorageSharingTypeNotShared];
189204
[_nodesWithResumptionInfo removeObject:resumptionInfo.nodeID];
190205
}
191206

192-
[_storageDelegate controller:_controller
207+
[_storageDelegate controller:controller
193208
storeValue:resumptionInfo
194209
forKey:ResumptionByNodeIDKey(resumptionInfo.nodeID)
195210
securityLevel:MTRStorageSecurityLevelSecure
196211
sharingType:MTRStorageSharingTypeNotShared];
197-
[_storageDelegate controller:_controller
212+
[_storageDelegate controller:controller
198213
storeValue:resumptionInfo
199214
forKey:ResumptionByResumptionIDKey(resumptionInfo.resumptionID)
200215
securityLevel:MTRStorageSecurityLevelSecure
201216
sharingType:MTRStorageSharingTypeNotShared];
202217

203218
// Update our resumption info node list.
204219
[_nodesWithResumptionInfo addObject:resumptionInfo.nodeID];
205-
[_storageDelegate controller:_controller
220+
[_storageDelegate controller:controller
206221
storeValue:[_nodesWithResumptionInfo copy]
207222
forKey:sResumptionNodeListKey
208223
securityLevel:MTRStorageSecurityLevelSecure
@@ -212,17 +227,23 @@ - (void)storeResumptionInfo:(MTRCASESessionResumptionInfo *)resumptionInfo
212227

213228
- (void)clearAllResumptionInfo
214229
{
230+
MTRDeviceController * controller = _controller;
231+
if (controller == nil) {
232+
// Not expected; no way to call delegate without controller.
233+
return;
234+
}
235+
215236
// Can we do less dispatch? We would need to have a version of
216237
// _findResumptionInfoWithKey that assumes we are already on the right queue.
217238
for (NSNumber * nodeID in _nodesWithResumptionInfo) {
218239
auto * oldInfo = [self findResumptionInfoByNodeID:nodeID];
219240
if (oldInfo != nil) {
220241
dispatch_sync(_storageDelegateQueue, ^{
221-
[_storageDelegate controller:_controller
242+
[_storageDelegate controller:controller
222243
removeValueForKey:ResumptionByResumptionIDKey(oldInfo.resumptionID)
223244
securityLevel:MTRStorageSecurityLevelSecure
224245
sharingType:MTRStorageSharingTypeNotShared];
225-
[_storageDelegate controller:_controller
246+
[_storageDelegate controller:controller
226247
removeValueForKey:ResumptionByNodeIDKey(oldInfo.nodeID)
227248
securityLevel:MTRStorageSecurityLevelSecure
228249
sharingType:MTRStorageSharingTypeNotShared];
@@ -235,9 +256,15 @@ - (void)clearAllResumptionInfo
235256

236257
- (CHIP_ERROR)storeLastLocallyUsedNOC:(MTRCertificateTLVBytes)noc
237258
{
259+
MTRDeviceController * controller = _controller;
260+
if (controller == nil) {
261+
// Not expected; no way to call delegate without controller.
262+
return CHIP_ERROR_PERSISTED_STORAGE_FAILED;
263+
}
264+
238265
__block BOOL ok;
239266
dispatch_sync(_storageDelegateQueue, ^{
240-
ok = [_storageDelegate controller:_controller
267+
ok = [_storageDelegate controller:controller
241268
storeValue:noc
242269
forKey:sLastLocallyUsedNOCKey
243270
securityLevel:MTRStorageSecurityLevelSecure
@@ -248,10 +275,16 @@ - (CHIP_ERROR)storeLastLocallyUsedNOC:(MTRCertificateTLVBytes)noc
248275

249276
- (MTRCertificateTLVBytes _Nullable)fetchLastLocallyUsedNOC
250277
{
278+
MTRDeviceController * controller = _controller;
279+
if (controller == nil) {
280+
// Not expected; no way to call delegate without controller.
281+
return nil;
282+
}
283+
251284
__block id data;
252285
dispatch_sync(_storageDelegateQueue, ^{
253286
@autoreleasepool {
254-
data = [_storageDelegate controller:_controller
287+
data = [_storageDelegate controller:controller
255288
valueForKey:sLastLocallyUsedNOCKey
256289
securityLevel:MTRStorageSecurityLevelSecure
257290
sharingType:MTRStorageSharingTypeNotShared];
@@ -271,6 +304,12 @@ - (MTRCertificateTLVBytes _Nullable)fetchLastLocallyUsedNOC
271304

272305
- (nullable MTRCASESessionResumptionInfo *)_findResumptionInfoWithKey:(nullable NSString *)key
273306
{
307+
MTRDeviceController * controller = _controller;
308+
if (controller == nil) {
309+
// Not expected; no way to call delegate without controller.
310+
return nil;
311+
}
312+
274313
// key could be nil if [NSString stringWithFormat] returns nil for some reason.
275314
if (key == nil) {
276315
return nil;
@@ -279,7 +318,7 @@ - (nullable MTRCASESessionResumptionInfo *)_findResumptionInfoWithKey:(nullable
279318
__block id resumptionInfo;
280319
dispatch_sync(_storageDelegateQueue, ^{
281320
@autoreleasepool {
282-
resumptionInfo = [_storageDelegate controller:_controller
321+
resumptionInfo = [_storageDelegate controller:controller
283322
valueForKey:key
284323
securityLevel:MTRStorageSecurityLevelSecure
285324
sharingType:MTRStorageSharingTypeNotShared];
@@ -318,9 +357,15 @@ - (nullable MTRCASESessionResumptionInfo *)_findResumptionInfoWithKey:(nullable
318357

319358
- (id)_fetchAttributeCacheValueForKey:(NSString *)key expectedClass:(Class)expectedClass;
320359
{
360+
MTRDeviceController * controller = _controller;
361+
if (controller == nil) {
362+
// Not expected; no way to call delegate without controller.
363+
return nil;
364+
}
365+
321366
id data;
322367
@autoreleasepool {
323-
data = [_storageDelegate controller:_controller
368+
data = [_storageDelegate controller:controller
324369
valueForKey:key
325370
securityLevel:MTRStorageSecurityLevelSecure
326371
sharingType:MTRStorageSharingTypeNotShared];
@@ -338,7 +383,13 @@ - (id)_fetchAttributeCacheValueForKey:(NSString *)key expectedClass:(Class)expec
338383

339384
- (BOOL)_storeAttributeCacheValue:(id)value forKey:(NSString *)key
340385
{
341-
return [_storageDelegate controller:_controller
386+
MTRDeviceController * controller = _controller;
387+
if (controller == nil) {
388+
// Not expected; no way to call delegate without controller.
389+
return NO;
390+
}
391+
392+
return [_storageDelegate controller:controller
342393
storeValue:value
343394
forKey:key
344395
securityLevel:MTRStorageSecurityLevelSecure
@@ -347,15 +398,27 @@ - (BOOL)_storeAttributeCacheValue:(id)value forKey:(NSString *)key
347398

348399
- (BOOL)_bulkStoreAttributeCacheValues:(NSDictionary<NSString *, id<NSSecureCoding>> *)values
349400
{
350-
return [_storageDelegate controller:_controller
401+
MTRDeviceController * controller = _controller;
402+
if (controller == nil) {
403+
// Not expected; no way to call delegate without controller.
404+
return NO;
405+
}
406+
407+
return [_storageDelegate controller:controller
351408
storeValues:values
352409
securityLevel:MTRStorageSecurityLevelSecure
353410
sharingType:MTRStorageSharingTypeNotShared];
354411
}
355412

356413
- (BOOL)_removeAttributeCacheValueForKey:(NSString *)key
357414
{
358-
return [_storageDelegate controller:_controller
415+
MTRDeviceController * controller = _controller;
416+
if (controller == nil) {
417+
// Not expected; no way to call delegate without controller.
418+
return NO;
419+
}
420+
421+
return [_storageDelegate controller:controller
359422
removeValueForKey:key
360423
securityLevel:MTRStorageSecurityLevelSecure
361424
sharingType:MTRStorageSharingTypeNotShared];

‎src/darwin/Framework/CHIPTests/MTRAsyncCallbackQueueTests.m

+2-3
Original file line numberDiff line numberDiff line change
@@ -16,10 +16,9 @@
1616

1717
#import <Matter/Matter.h>
1818

19-
// system dependencies
20-
#import <XCTest/XCTest.h>
19+
#import "MTRTestCase.h"
2120

22-
@interface MTRAsyncCallbackQueueTests : XCTestCase
21+
@interface MTRAsyncCallbackQueueTests : MTRTestCase
2322

2423
@end
2524

‎src/darwin/Framework/CHIPTests/MTRAsyncWorkQueueTests.m

+3-2
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,12 @@
1515
*/
1616

1717
#import <Matter/Matter.h>
18-
#import <XCTest/XCTest.h>
18+
19+
#import "MTRTestCase.h"
1920

2021
#import "MTRAsyncWorkQueue.h"
2122

22-
@interface MTRAsyncWorkQueueTests : XCTestCase
23+
@interface MTRAsyncWorkQueueTests : MTRTestCase
2324
@end
2425

2526
@implementation MTRAsyncWorkQueueTests {

‎src/darwin/Framework/CHIPTests/MTRBackwardsCompatTests.m

+2-4
Original file line numberDiff line numberDiff line change
@@ -19,15 +19,13 @@
1919
#import <Matter/Matter.h>
2020

2121
#import "MTRErrorTestUtils.h"
22+
#import "MTRTestCase.h"
2223
#import "MTRTestKeys.h"
2324
#import "MTRTestResetCommissioneeHelper.h"
2425
#import "MTRTestStorage.h"
2526

2627
#import <math.h> // For INFINITY
2728

28-
// system dependencies
29-
#import <XCTest/XCTest.h>
30-
3129
// Fixture: chip-all-clusters-app --KVS "$(mktemp -t chip-test-kvs)" --interface-id -1
3230

3331
static const uint16_t kPairingTimeoutInSeconds = 10;
@@ -88,7 +86,7 @@ - (void)onCommissioningComplete:(NSError *)error
8886

8987
@end
9088

91-
@interface MTRBackwardsCompatTests : XCTestCase
89+
@interface MTRBackwardsCompatTests : MTRTestCase
9290
@end
9391

9492
@implementation MTRBackwardsCompatTests

‎src/darwin/Framework/CHIPTests/MTRCertificateInfoTests.m

+3-2
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,10 @@
1616
*/
1717

1818
#import <Matter/Matter.h>
19-
#import <XCTest/XCTest.h>
2019

21-
@interface MTRCertificateInfoTests : XCTestCase
20+
#import "MTRTestCase.h"
21+
22+
@interface MTRCertificateInfoTests : MTRTestCase
2223

2324
@end
2425

‎src/darwin/Framework/CHIPTests/MTRCertificateTests.m

+2-4
Original file line numberDiff line numberDiff line change
@@ -16,12 +16,10 @@
1616

1717
#import <Matter/Matter.h>
1818

19-
// system dependencies
20-
#import <XCTest/XCTest.h>
21-
19+
#import "MTRTestCase.h"
2220
#import "MTRTestKeys.h"
2321

24-
@interface MTRCertificateTests : XCTestCase
22+
@interface MTRCertificateTests : MTRTestCase
2523

2624
@end
2725

‎src/darwin/Framework/CHIPTests/MTRCertificateValidityTests.m

+4-4
Original file line numberDiff line numberDiff line change
@@ -19,13 +19,11 @@
1919
#import <Matter/Matter.h>
2020

2121
#import "MTRErrorTestUtils.h"
22+
#import "MTRTestCase.h"
2223
#import "MTRTestKeys.h"
2324
#import "MTRTestResetCommissioneeHelper.h"
2425
#import "MTRTestStorage.h"
2526

26-
// system dependencies
27-
#import <XCTest/XCTest.h>
28-
2927
static const uint16_t kPairingTimeoutInSeconds = 10;
3028
static const uint16_t kTimeoutInSeconds = 3;
3129
static const uint64_t kDeviceId = 0x12341234;
@@ -193,7 +191,7 @@ - (nullable instancetype)initWithRootKey:(MTRTestKeys *)key fabricID:(NSNumber *
193191

194192
@end
195193

196-
@interface MTRCertificateValidityTests : XCTestCase
194+
@interface MTRCertificateValidityTests : MTRTestCase
197195
@end
198196

199197
static BOOL sNeedsStackShutdown = YES;
@@ -210,6 +208,8 @@ + (void)tearDown
210208
// we did not run test999_TearDown.
211209
[self shutdownStack];
212210
}
211+
212+
[super tearDown];
213213
}
214214

215215
- (void)setUp

‎src/darwin/Framework/CHIPTests/MTRCommissionableBrowserTests.m

+2-4
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,7 @@
1717

1818
#import <Matter/Matter.h>
1919

20-
// system dependencies
21-
#import <XCTest/XCTest.h>
22-
20+
#import "MTRTestCase.h"
2321
#import "MTRTestKeys.h"
2422
#import "MTRTestStorage.h"
2523

@@ -115,7 +113,7 @@ - (void)controller:(MTRDeviceController *)controller didRemoveCommissionableDevi
115113
}
116114
@end
117115

118-
@interface MTRCommissionableBrowserTests : XCTestCase
116+
@interface MTRCommissionableBrowserTests : MTRTestCase
119117
@end
120118

121119
@implementation MTRCommissionableBrowserTests

0 commit comments

Comments
 (0)