Address API review issues in MTRControllerFactory. (#22606)

* Rename to MTRDeviceControllerFactory.
* Change the startup params startServer to shouldStartServer.
* Change the startup params init signatures to be more aligned.
* Change isRunning to running.
* Rename startup to startControllerFactory and add NSError outparam.
* Rename shutdown to stopControllerFactory
* Rename startControllerOnExistingFabric to
  createControllerOnExistingFabric and add NSError outparam.
* Rename startControllerOnNewFabric to createControllerOnNewFabric and add
  NSError outparam.
* Fix a leak when we failed to start a controller because it wanted a fabric
  that does not exist, or wanted a new fabric and a matching one existed.  This
  used to not show up in LSan before, maybe because we did not have an
  autoreleasepool in place.

Fixes #22594

Addresses part of #22420

examples/darwin-framework-tool/commands/common/CHIPCommandBridge.mm

@@ -111,25 +111,26 @@
 
     mOTADelegate = [[OTAProviderDelegate alloc] init];
 
-    auto factory = [MTRControllerFactory sharedInstance];
+    auto factory = [MTRDeviceControllerFactory sharedInstance];
     if (factory == nil) {
         ChipLogError(chipTool, "Controller factory is nil");
         return CHIP_ERROR_INTERNAL;
     }
 
-    auto params = [[MTRControllerFactoryParams alloc] initWithStorage:storage];
+    auto params = [[MTRDeviceControllerFactoryParams alloc] initWithStorage:storage];
     params.port = @(kListenPort);
-    params.startServer = YES;
+    params.shouldStartServer = YES;
     params.otaProviderDelegate = mOTADelegate;
     NSArray<NSData *> * paaCertResults;
     ReturnLogErrorOnFailure(GetPAACertsFromFolder(&paaCertResults));
     if ([paaCertResults count] > 0) {
         params.paaCerts = paaCertResults;
     }
 
-    if ([factory startup:params] == NO) {
+    NSError * error;
+    if ([factory startControllerFactory:params error:&error] == NO) {
         ChipLogError(chipTool, "Controller factory startup failed");
-        return CHIP_ERROR_INTERNAL;
+        return MTRErrorToCHIPErrorCode(error);
     }
 
     ReturnLogErrorOnFailure([gNocSigner createOrLoadKeys:storage]);
@@ -138,21 +139,19 @@
 
     constexpr const char * identities[] = { kIdentityAlpha, kIdentityBeta, kIdentityGamma };
     for (size_t i = 0; i < ArraySize(identities); ++i) {
-        auto controllerParams = [[MTRDeviceControllerStartupParams alloc] initWithSigningKeypair:gNocSigner
-                                                                                        fabricID:@(i + 1)
-                                                                                             ipk:ipk];
+        auto controllerParams = [[MTRDeviceControllerStartupParams alloc] initWithIPK:ipk fabricID:@(i + 1) nocSigner:gNocSigner];
 
         // We're not sure whether we're creating a new fabric or using an
         // existing one, so just try both.
-        auto controller = [factory startControllerOnExistingFabric:controllerParams];
+        auto controller = [factory createControllerOnExistingFabric:controllerParams error:&error];
         if (controller == nil) {
             // Maybe we didn't have this fabric yet.
             controllerParams.vendorID = @(chip::VendorId::TestVendor1);
-            controller = [factory startControllerOnNewFabric:controllerParams];
+            controller = [factory createControllerOnNewFabric:controllerParams error:&error];
         }
         if (controller == nil) {
             ChipLogError(chipTool, "Controller startup failure.");
-            return CHIP_ERROR_INTERNAL;
+            return MTRErrorToCHIPErrorCode(error);
         }
 
         mControllers[identities[i]] = controller;
@@ -195,16 +194,14 @@
 {
     StopCommissioners();
 
-    auto factory = [MTRControllerFactory sharedInstance];
+    auto factory = [MTRDeviceControllerFactory sharedInstance];
     NSData * ipk = [gNocSigner getIPK];
 
     constexpr const char * identities[] = { kIdentityAlpha, kIdentityBeta, kIdentityGamma };
     for (size_t i = 0; i < ArraySize(identities); ++i) {
-        auto controllerParams = [[MTRDeviceControllerStartupParams alloc] initWithSigningKeypair:gNocSigner
-                                                                                        fabricID:@(i + 1)
-                                                                                             ipk:ipk];
+        auto controllerParams = [[MTRDeviceControllerStartupParams alloc] initWithIPK:ipk fabricID:@(i + 1) nocSigner:gNocSigner];
 
-        auto controller = [factory startControllerOnExistingFabric:controllerParams];
+        auto controller = [factory createControllerOnExistingFabric:controllerParams error:nil];
         mControllers[identities[i]] = controller;
     }
 }
@@ -216,7 +213,7 @@
     mControllers.clear();
     mCurrentController = nil;
 
-    [[MTRControllerFactory sharedInstance] shutdown];
+    [[MTRDeviceControllerFactory sharedInstance] stopControllerFactory];
 }
 
 CHIP_ERROR CHIPCommandBridge::StartWaiting(chip::System::Clock::Timeout duration)

examples/darwin-framework-tool/main.mm

@@ -30,13 +30,15 @@
 
 int main(int argc, const char * argv[])
 {
-    Commands commands;
-    registerCommandsPairing(commands);
-    registerCommandsInteractive(commands);
-    registerCommandsPayload(commands);
-    registerClusterOtaSoftwareUpdateProviderInteractive(commands);
-    registerCommandsStorage(commands);
-    registerCommandsTests(commands);
-    registerClusters(commands);
-    return commands.Run(argc, (char **) argv);
+    @autoreleasepool {
+        Commands commands;
+        registerCommandsPairing(commands);
+        registerCommandsInteractive(commands);
+        registerCommandsPayload(commands);
+        registerClusterOtaSoftwareUpdateProviderInteractive(commands);
+        registerCommandsStorage(commands);
+        registerCommandsTests(commands);
+        registerClusters(commands);
+        return commands.Run(argc, (char **) argv);
+    }
 }

src/darwin/CHIPTool/CHIPTool/Framework Helpers/DefaultsUtils.m

@@ -76,9 +76,9 @@ void MTRSetNextAvailableDeviceID(uint64_t id)
     static dispatch_once_t onceToken;
     dispatch_once(&onceToken, ^{
         CHIPToolPersistentStorageDelegate * storage = [[CHIPToolPersistentStorageDelegate alloc] init];
-        __auto_type * factory = [MTRControllerFactory sharedInstance];
-        __auto_type * factoryParams = [[MTRControllerFactoryParams alloc] initWithStorage:storage];
-        if (![factory startup:factoryParams]) {
+        __auto_type * factory = [MTRDeviceControllerFactory sharedInstance];
+        __auto_type * factoryParams = [[MTRDeviceControllerFactoryParams alloc] initWithStorage:storage];
+        if (![factory startControllerFactory:factoryParams error:nil]) {
             return;
         }
 
@@ -87,14 +87,14 @@ void MTRSetNextAvailableDeviceID(uint64_t id)
             return;
         }
 
-        __auto_type * params = [[MTRDeviceControllerStartupParams alloc] initWithSigningKeypair:keys fabricID:@(1) ipk:keys.ipk];
+        __auto_type * params = [[MTRDeviceControllerStartupParams alloc] initWithIPK:keys.ipk fabricID:@(1) nocSigner:keys];
         params.vendorID = @(kTestVendorId);
 
         // We're not sure whether we have a fabric configured already; try as if
         // we did, and if not fall back to creating a new one.
-        sController = [factory startControllerOnExistingFabric:params];
+        sController = [factory createControllerOnExistingFabric:params error:nil];
         if (sController == nil) {
-            sController = [factory startControllerOnNewFabric:params];
+            sController = [factory createControllerOnNewFabric:params error:nil];
         }
     });
 
@@ -113,9 +113,9 @@ void MTRSetNextAvailableDeviceID(uint64_t id)
     [controller shutdown];
 
     NSLog(@"Starting up the stack");
-    __auto_type * params = [[MTRDeviceControllerStartupParams alloc] initWithSigningKeypair:keys fabricID:@(1) ipk:keys.ipk];
+    __auto_type * params = [[MTRDeviceControllerStartupParams alloc] initWithIPK:keys.ipk fabricID:@(1) nocSigner:keys];
 
-    sController = [[MTRControllerFactory sharedInstance] startControllerOnExistingFabric:params];
+    sController = [[MTRDeviceControllerFactory sharedInstance] createControllerOnExistingFabric:params error:nil];
 
     return sController;
 }

src/darwin/Framework/CHIP/MTRBaseDevice_Internal.h

@@ -54,7 +54,7 @@ NS_ASSUME_NONNULL_BEGIN
 @property (nonatomic, assign, readonly) chip::NodeId nodeID;
 
 /**
- * Controllers are created via the MTRControllerFactory object.
+ * Controllers are created via the MTRDeviceControllerFactory object.
  */
 - (instancetype)init NS_UNAVAILABLE;
 + (instancetype)new NS_UNAVAILABLE;

src/darwin/Framework/CHIP/MTRDeviceController.h

@@ -30,7 +30,7 @@ typedef void (^MTRDeviceConnectionCallback)(MTRBaseDevice * _Nullable device, NS
 
 @interface MTRDeviceController : NSObject
 
-@property (readonly, nonatomic) BOOL isRunning;
+@property (readonly, nonatomic, getter=isRunning) BOOL running;
 
 /**
  * Return the Node ID assigned to the controller.  Will return nil if the
@@ -96,7 +96,7 @@ typedef void (^MTRDeviceConnectionCallback)(MTRBaseDevice * _Nullable device, NS
 - (BOOL)getBaseDevice:(uint64_t)deviceID queue:(dispatch_queue_t)queue completion:(MTRDeviceConnectionCallback)completion;
 
 /**
- * Controllers are created via the MTRControllerFactory object.
+ * Controllers are created via the MTRDeviceControllerFactory object.
  */
 - (instancetype)init NS_UNAVAILABLE;
 + (instancetype)new NS_UNAVAILABLE;

src/darwin/Framework/CHIP/MTRDeviceController.mm

@@ -20,7 +20,7 @@
 
 #import "MTRBaseDevice_Internal.h"
 #import "MTRCommissioningParameters.h"
-#import "MTRControllerFactory_Internal.h"
+#import "MTRDeviceControllerFactory_Internal.h"
 #import "MTRDeviceControllerStartupParams.h"
 #import "MTRDeviceControllerStartupParams_Internal.h"
 #import "MTRDevicePairingDelegateBridge.h"
@@ -86,14 +86,14 @@ @interface MTRDeviceController ()
 @property (readonly) MTRP256KeypairBridge signingKeypairBridge;
 @property (readonly) MTRP256KeypairBridge operationalKeypairBridge;
 @property (readonly) MTRDeviceAttestationDelegateBridge * deviceAttestationDelegateBridge;
-@property (readonly) MTRControllerFactory * factory;
+@property (readonly) MTRDeviceControllerFactory * factory;
 @property (readonly) NSMutableDictionary * nodeIDToDeviceMap;
 @property (readonly) os_unfair_lock deviceMapLock; // protects nodeIDToDeviceMap
 @end
 
 @implementation MTRDeviceController
 
-- (instancetype)initWithFactory:(MTRControllerFactory *)factory queue:(dispatch_queue_t)queue
+- (instancetype)initWithFactory:(MTRDeviceControllerFactory *)factory queue:(dispatch_queue_t)queue
 {
     if (self = [super init]) {
         _chipWorkQueue = queue;
@@ -139,11 +139,10 @@ - (void)shutdown
 - (void)cleanupAfterStartup
 {
     [_factory controllerShuttingDown:self];
-    [self cleanup];
 }
 
 // Part of cleanupAfterStartup that has to interact with the Matter work queue
-// in a very specific way that only MTRControllerFactory knows about.
+// in a very specific way that only MTRDeviceControllerFactory knows about.
 - (void)shutDownCppController
 {
     if (_cppCommissioner) {
@@ -160,6 +159,11 @@ - (void)shutDownCppController
     }
 }
 
+- (void)deinitFromFactory
+{
+    [self cleanup];
+}
+
 // Clean up any members we might have allocated.
 - (void)cleanup
 {

src/darwin/Framework/CHIP/MTRControllerFactory.h → src/darwin/Framework/CHIP/MTRDeviceControllerFactory.h

@@ -31,7 +31,7 @@ NS_ASSUME_NONNULL_BEGIN
 @class MTRDeviceController;
 @class MTRDeviceControllerStartupParams;
 
-@interface MTRControllerFactoryParams : NSObject
+@interface MTRDeviceControllerFactoryParams : NSObject
 /*
  * Storage delegate must be provided for correct functioning of Matter
  * controllers.  It is used to store persistent information for the fabrics the
@@ -66,44 +66,47 @@ NS_ASSUME_NONNULL_BEGIN
  * Whether to run a server capable of accepting incoming CASE
  * connections.  Defaults to NO.
  */
-@property (nonatomic, assign) BOOL startServer;
+@property (nonatomic, assign) BOOL shouldStartServer;
 
 - (instancetype)init NS_UNAVAILABLE;
 - (instancetype)initWithStorage:(id<MTRStorage>)storage;
 @end
 
-@interface MTRControllerFactory : NSObject
+@interface MTRDeviceControllerFactory : NSObject
 
-@property (readonly, nonatomic) BOOL isRunning;
-
-- (instancetype)init NS_UNAVAILABLE;
-+ (instancetype)new NS_UNAVAILABLE;
+/**
+ * If true, the factory is in a state where it can create controllers:
+ * startControllerFactory has been called, but stopControllerFactory has not been called
+ * since then.
+ */
+@property (readonly, nonatomic, getter=isRunning) BOOL running;
 
 /**
- * Return the single MTRControllerFactory we support existing.  It starts off
+ * Return the single MTRDeviceControllerFactory we support existing.  It starts off
  * in a "not started" state.
  */
 + (instancetype)sharedInstance;
 
 /**
- * Start the controller factory. Repeated calls to startup without calls to
- * shutdown in between are NO-OPs. Use the isRunning property to check whether
- * the controller factory needs to be started up.
+ * Start the controller factory. Repeated calls to startControllerFactory
+ * without calls to stopControllerFactory in between are NO-OPs. Use the
+ * isRunning property to check whether the controller factory needs to be
+ * started up.
  *
  * @param[in] startupParams data needed to start up the controller factory.
  *
  * @return Whether startup succeded.
  */
-- (BOOL)startup:(MTRControllerFactoryParams *)startupParams;
+- (BOOL)startControllerFactory:(MTRDeviceControllerFactoryParams *)startupParams error:(NSError * __autoreleasing *)error;
 
 /**
- * Shut down the controller factory. This will shut down any outstanding
- * controllers as part of the factory shutdown.
+ * Stop the controller factory. This will shut down any outstanding
+ * controllers as part of the factory stopping.
  *
- * Repeated calls to shutdown without calls to startup in between are
- * NO-OPs.
+ * Repeated calls to stopControllerFactory without calls to
+ * startControllerFactory in between are NO-OPs.
  */
-- (void)shutdown;
+- (void)stopControllerFactory;
 
 /**
  * Create a MTRDeviceController on an existing fabric.  Returns nil on failure.
@@ -114,7 +117,8 @@ NS_ASSUME_NONNULL_BEGIN
  * The fabric is identified by the root public key and fabric id in
  * the startupParams.
  */
-- (MTRDeviceController * _Nullable)startControllerOnExistingFabric:(MTRDeviceControllerStartupParams *)startupParams;
+- (MTRDeviceController * _Nullable)createControllerOnExistingFabric:(MTRDeviceControllerStartupParams *)startupParams
+                                                              error:(NSError * __autoreleasing *)error;
 
 /**
  * Create a MTRDeviceController on a new fabric.  Returns nil on failure.
@@ -124,7 +128,11 @@ NS_ASSUME_NONNULL_BEGIN
  * The fabric is identified by the root public key and fabric id in
  * the startupParams.
  */
-- (MTRDeviceController * _Nullable)startControllerOnNewFabric:(MTRDeviceControllerStartupParams *)startupParams;
+- (MTRDeviceController * _Nullable)createControllerOnNewFabric:(MTRDeviceControllerStartupParams *)startupParams
+                                                         error:(NSError * __autoreleasing *)error;
+
+- (instancetype)init NS_UNAVAILABLE;
++ (instancetype)new NS_UNAVAILABLE;
 
 @end