Stop holding on to a PASE DeviceProxy pointer in MTRBaseDevice. (#22848)

Fixes #22846

src/darwin/Framework/CHIP/MTRBaseDevice.mm

@@ -74,15 +74,6 @@
 
 class MTRDataValueDictionaryCallbackBridge;
 
-@interface MTRBaseDevice ()
-
-@property (nonatomic, readonly, assign, nullable) chip::DeviceProxy * cppPASEDevice;
-@property (nonatomic, readwrite) NSMutableDictionary * reportHandlerBridges;
-
-- (chip::NodeId)deviceID;
-- (BOOL)isPASEDevice;
-@end
-
 @interface MTRReadClientContainer : NSObject
 @property (nonatomic, readwrite) app::ReadClient * readClientPtr;
 @property (nonatomic, readwrite) app::AttributePathParams * pathParams;
@@ -223,19 +214,8 @@ @implementation MTRBaseDevice
 - (instancetype)initWithPASEDevice:(chip::DeviceProxy *)device controller:(MTRDeviceController *)controller
 {
     if (self = [super init]) {
-        chip::Optional<SessionHandle> session = device->GetSecureSession();
-        if (!session.HasValue()) {
-            MTR_LOG_ERROR("Failing to initialize MTRBaseDevice: no secure session");
-            return nil;
-        }
-
-        if (!session.Value()->AsSecureSession()->IsPASESession()) {
-            MTR_LOG_ERROR("Failing to initialize MTRBaseDevice: not a PASE session");
-            return nil;
-        }
-
-        _cppPASEDevice = device;
-        _nodeID = kUndefinedNodeId;
+        _isPASEDevice = YES;
+        _nodeID = device->GetDeviceId();
         _deviceController = controller;
     }
     return self;
@@ -244,7 +224,7 @@ - (instancetype)initWithPASEDevice:(chip::DeviceProxy *)device controller:(MTRDe
 - (instancetype)initWithNodeID:(NSNumber *)nodeID controller:(MTRDeviceController *)controller
 {
     if (self = [super init]) {
-        _cppPASEDevice = nil;
+        _isPASEDevice = NO;
         _nodeID = [nodeID unsignedLongLongValue];
         _deviceController = controller;
     }
@@ -258,32 +238,13 @@ + (instancetype)deviceWithNodeID:(NSNumber *)nodeID controller:(MTRDeviceControl
     return [controller baseDeviceForNodeID:nodeID];
 }
 
-- (chip::DeviceProxy * _Nullable)paseDevice
-{
-    return _cppPASEDevice;
-}
-
-- (chip::NodeId)deviceID
-{
-    if (_cppPASEDevice != nullptr) {
-        return _cppPASEDevice->GetDeviceId();
-    }
-
-    return self.nodeID;
-}
-
-- (BOOL)isPASEDevice
-{
-    return _cppPASEDevice != nullptr;
-}
-
 - (void)invalidateCASESession
 {
     if (self.isPASEDevice) {
         return;
     }
 
-    [self.deviceController invalidateCASESessionForNode:self.deviceID];
+    [self.deviceController invalidateCASESessionForNode:self.nodeID];
 }
 
 namespace {
@@ -1195,7 +1156,7 @@ - (void)subscribeAttributePathWithEndpointID:(NSNumber * _Nullable)endpointID
                    };
 
                    MTRReadClientContainer * container = [[MTRReadClientContainer alloc] init];
-                   container.deviceID = [self deviceID];
+                   container.deviceID = self.nodeID;
                    container.pathParams = Platform::New<app::AttributePathParams>();
                    if (endpointID) {
                        container.pathParams->mEndpointId = static_cast<chip::EndpointId>([endpointID unsignedShortValue]);
@@ -1261,7 +1222,7 @@ - (void)deregisterReportHandlersWithQueue:(dispatch_queue_t)queue completion:(di
 {
     // This method must only be used for MTRDeviceOverXPC. However, for unit testing purpose, the method purges all read clients.
     MTR_LOG_DEBUG("Unexpected call to deregister report handlers");
-    PurgeReadClientContainers([self deviceID], queue, completion);
+    PurgeReadClientContainers(self.nodeID, queue, completion);
 }
 
 namespace {
@@ -1407,7 +1368,7 @@ - (void)openCommissioningWindowWithSetupPasscode:(NSNumber *)setupPasscode
 - (void)failSubscribers:(dispatch_queue_t)queue completion:(void (^)(void))completion
 {
     MTR_LOG_DEBUG("Causing failure in subscribers on purpose");
-    CauseReadClientFailure([self deviceID], queue, completion);
+    CauseReadClientFailure(self.nodeID, queue, completion);
 }
 #endif
 

src/darwin/Framework/CHIP/MTRBaseDevice_Internal.h

@@ -29,27 +29,27 @@ NS_ASSUME_NONNULL_BEGIN
 
 - (instancetype)initWithPASEDevice:(chip::DeviceProxy *)device controller:(MTRDeviceController *)controller;
 
-/**
- * Only returns non-nil if the device is using a PASE session.  Otherwise, the
- * deviceController + nodeId should be used to get a CASE session.
- */
-- (chip::DeviceProxy * _Nullable)paseDevice;
-
 /**
  * Invalidate the CASE session, so an attempt to getConnectedDevice for this
  * device id will have to create a new CASE session.  Ideally this API will go
  * away.
  */
 - (void)invalidateCASESession;
 
+/**
+ * Whether this device represents a PASE session or not.
+ */
+@property (nonatomic, assign, readonly) BOOL isPASEDevice;
+
 /**
  * Controller that that this MTRDevice was gotten from.
  */
 @property (nonatomic, strong, readonly) MTRDeviceController * deviceController;
 
 /**
- * Node id for this MTRDevice.  Only set to a usable value if this device
- * represents a CASE session.
+ * Node id for this MTRDevice.  If this device represents a CASE session, this
+ * is set to the node ID of the target node.  If this device represents a PASE
+ * session, this is set to the device id of the PASE device.
  */
 @property (nonatomic, assign, readonly) chip::NodeId nodeID;
 
@@ -60,10 +60,10 @@ NS_ASSUME_NONNULL_BEGIN
 + (instancetype)new NS_UNAVAILABLE;
 
 /**
- * Initialize the device object with the given node id and controller.  This
- * will always succeed, even if there is no such node id on the controller's
- * fabric, but attempts to actually use the MTRBaseDevice will fail
- * (asynchronously) in that case.
+ * Initialize the device object as a CASE device with the given node id and
+ * controller.  This will always succeed, even if there is no such node id on
+ * the controller's fabric, but attempts to actually use the MTRBaseDevice will
+ * fail (asynchronously) in that case.
  */
 - (instancetype)initWithNodeID:(NSNumber *)nodeID controller:(MTRDeviceController *)controller;
 

src/darwin/Framework/CHIP/MTRCallbackBridgeBase_internal.h

@@ -96,11 +96,10 @@ template <class T> class MTRCallbackBridge {
     }
 
     /**
-     * Run the given MTRActionBlock on the Matter thread (possibly
-     * synchronously, if the MTRBaseDevice represents a PASE connection), after
-     * getting a secure session corresponding to the given MTRBaseDevice.  On
-     * success, convert the success value to whatever type it needs to be to
-     * call the callback type we're templated over.
+     * Run the given MTRActionBlock on the Matter thread after getting a secure
+     * session corresponding to the given MTRBaseDevice.  On success, convert
+     * the success value to whatever type it needs to be to call the callback
+     * type we're templated over.
      */
     MTRCallbackBridge(dispatch_queue_t queue, MTRBaseDevice * device, ResponseHandler handler, MTRActionBlock action, T OnSuccessFn,
         bool keepAlive)
@@ -111,23 +110,27 @@ template <class T> class MTRCallbackBridge {
         , mSuccess(OnSuccessFn, this)
         , mFailure(OnFailureFn, this)
     {
-        auto * paseDevice = [device paseDevice];
-        if (paseDevice != nullptr) {
-            ActionWithDevice(paseDevice);
+        if (device.isPASEDevice) {
+            ActionWithPASEDevice(device);
         } else {
             ActionWithNodeID(device.nodeID, device.deviceController);
         }
     };
 
-    void ActionWithDevice(chip::DeviceProxy * device)
+    void ActionWithPASEDevice(MTRBaseDevice * device)
     {
         LogRequestStart();
 
-        // Keep doing dispatch_sync here for now, because we don't want device
-        // to become stale.
-        dispatch_sync(chip::DeviceLayer::PlatformMgrImpl().GetWorkQueue(), ^{
-            MaybeDoAction(device->GetExchangeManager(), device->GetSecureSession(), nil);
-        });
+        BOOL ok = [device.deviceController
+            getSessionForCommissioneeDevice:device.nodeID
+                                 completion:^(chip::Messaging::ExchangeManager * exchangeManager,
+                                     const chip::Optional<chip::SessionHandle> & session, NSError * error) {
+                                     MaybeDoAction(exchangeManager, session, error);
+                                 }];
+
+        if (ok == NO) {
+            OnFailureFn(this, CHIP_ERROR_INCORRECT_STATE);
+        }
     }
 
     void ActionWithNodeID(chip::NodeId nodeID, MTRDeviceController * controller)

src/darwin/Framework/CHIP/MTRDeviceController.mm

@@ -489,18 +489,19 @@ - (MTRBaseDevice *)deviceBeingCommissionedWithNodeID:(NSNumber *)nodeID error:(N
 {
     VerifyOrReturnValue([self checkIsRunning:error], nil);
 
-    __block chip::CommissioneeDeviceProxy * deviceProxy;
-
+    __block MTRBaseDevice * device;
     __block BOOL success = NO;
     dispatch_sync(_chipWorkQueue, ^{
         VerifyOrReturn([self checkIsRunning:error]);
 
+        chip::CommissioneeDeviceProxy * deviceProxy;
         auto errorCode = self->_cppCommissioner->GetDeviceBeingCommissioned([nodeID unsignedLongLongValue], &deviceProxy);
         success = ![MTRDeviceController checkForError:errorCode logMsg:kErrorStopPairing error:error];
+        device = [[MTRBaseDevice alloc] initWithPASEDevice:deviceProxy controller:self];
     });
     VerifyOrReturnValue(success, nil);
 
-    return [[MTRBaseDevice alloc] initWithPASEDevice:deviceProxy controller:self];
+    return device;
 }
 
 - (MTRBaseDevice *)baseDeviceForNodeID:(NSNumber *)nodeID
@@ -708,6 +709,38 @@ - (BOOL)getSessionForNode:(chip::NodeId)nodeID completion:(MTRInternalDeviceConn
     return YES;
 }
 
+- (BOOL)getSessionForCommissioneeDevice:(chip::NodeId)deviceID completion:(MTRInternalDeviceConnectionCallback)completion
+{
+    if (![self checkIsRunning]) {
+        return NO;
+    }
+
+    dispatch_async(_chipWorkQueue, ^{
+        NSError * error;
+        if (![self checkIsRunning:&error]) {
+            completion(nullptr, chip::NullOptional, error);
+            return;
+        }
+
+        chip::CommissioneeDeviceProxy * deviceProxy;
+        CHIP_ERROR err = self->_cppCommissioner->GetDeviceBeingCommissioned(deviceID, &deviceProxy);
+        if (err != CHIP_NO_ERROR) {
+            completion(nullptr, chip::NullOptional, [MTRError errorForCHIPErrorCode:err]);
+            return;
+        }
+
+        chip::Optional<chip::SessionHandle> session = deviceProxy->GetSecureSession();
+        if (!session.HasValue() || !session.Value()->AsSecureSession()->IsPASESession()) {
+            completion(nullptr, chip::NullOptional, [MTRError errorForCHIPErrorCode:CHIP_ERROR_INCORRECT_STATE]);
+            return;
+        }
+
+        completion(deviceProxy->GetExchangeManager(), session, nil);
+    });
+
+    return YES;
+}
+
 - (void)asyncDispatchToMatterQueue:(void (^)(chip::Controller::DeviceCommissioner *))block
                       errorHandler:(void (^)(NSError *))errorHandler
 {

src/darwin/Framework/CHIP/MTRDeviceController_Internal.h

@@ -120,6 +120,20 @@ NS_ASSUME_NONNULL_BEGIN
  */
 - (BOOL)getSessionForNode:(chip::NodeId)nodeID completion:(MTRInternalDeviceConnectionCallback)completion;
 
+/**
+ * Get a session for the commissionee device with the given device id.  This may
+ * be called on any queue (including the Matter event queue) and will always
+ * call the provided connection callback on the Matter queue, asynchronously.
+ * Consumers must be prepared to run on the Matter queue (an in particular must
+ * not use any APIs that will try to do sync dispatch to the Matter queue).
+ *
+ * If the controller is not running when this function is called, will return NO
+ * and never invoke the completion.  If the controller is not running when the
+ * async dispatch on the Matter queue would happen, an error will be dispatched
+ * to the completion handler.
+ */
+- (BOOL)getSessionForCommissioneeDevice:(chip::NodeId)deviceID completion:(MTRInternalDeviceConnectionCallback)completion;
+
 /**
  * Invalidate the CASE session for the given node ID.  This is a temporary thing
  * just to support MTRBaseDevice's invalidateCASESession.  Must not be called on