Fix handling of OperationalDeviceProxy and CHIPCluster in Darwin fram…

…ework.

Darwin framework objects were holding on to OperationalDeviceProxy and CHIPCluster instances, but OperationalDeviceProxy can be destroyed randomly by anyone who cares to do it, and CHIPCluster is really designed as a stack-only object.

This PR makes the following changes:

* MTRBaseDevice now either stores a DeviceProxy (for PASE only) or a node id
  (for CASE).

* MTRBaseCluster instances now just store a DeviceProxy.

* MTRCallbackBridge will handle getting an exchange manager and session before
  invoking the action block, unless the action block is runnable without any
  remote interaction (the "read from cache" case).

* Most of the MTRDevice and MTRClusters APIs that used to sync-run things on the
  Matter queue now do it async when doing CASE.  They were changed to copy their
  incoming arguments to avoid the caller mutating them after the call.

I believe this should fix #21430

src/darwin/Framework/CHIP/MTRBaseDevice_Internal.h

@@ -22,12 +22,20 @@
 #include <app/ConcreteCommandPath.h>
 #include <app/DeviceProxy.h>
 
+@class MTRDeviceController;
+
 NS_ASSUME_NONNULL_BEGIN
 
 @interface MTRBaseDevice ()
 
-- (instancetype)initWithDevice:(chip::DeviceProxy *)device;
-- (chip::DeviceProxy *)internalDevice;
+- (instancetype)initWithPASEDevice:(chip::DeviceProxy *)device controller:(MTRDeviceController *)controller;
+- (instancetype)initWithNodeID:(chip::NodeId)nodeID 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
@@ -36,6 +44,23 @@ NS_ASSUME_NONNULL_BEGIN
  */
 - (void)invalidateCASESession;
 
+/**
+ * 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.
+ */
+@property (nonatomic, assign, readonly) chip::NodeId nodeID;
+
+/**
+ * Controllers are created via the MTRControllerFactory object.
+ */
+- (instancetype)init NS_UNAVAILABLE;
++ (instancetype)new NS_UNAVAILABLE;
+
 @end
 
 @interface MTRAttributePath ()

src/darwin/Framework/CHIP/MTRCallbackBridgeBase_internal.h

@@ -17,42 +17,139 @@
 
 #import <Foundation/Foundation.h>
 
+#import "MTRBaseDevice_Internal.h"
+#import "MTRDeviceController_Internal.h"
 #import "MTRError_Internal.h"
 #import "zap-generated/MTRBaseClusters.h"
 
 #include <app/data-model/NullObject.h>
+#include <messaging/ExchangeMgr.h>
 #include <platform/CHIPDeviceLayer.h>
+#include <transport/SessionHandle.h>
 
-typedef CHIP_ERROR (^MTRActionBlock)(chip::Callback::Cancelable * success, chip::Callback::Cancelable * failure);
+/**
+ * Bridge that allows invoking a given MTRActionBlock on the Matter queue, after
+ * communication with the device in question has been established, as far as we
+ * know.
+ */
+
+// TODO: ADD NS_ASSUME_NONNULL_BEGIN to this header.  When that happens, note
+// that in MTRActionBlock the two callback pointers are nonnull.
+
+typedef CHIP_ERROR (^MTRActionBlock)(chip::Messaging::ExchangeManager & exchangeManager, const chip::SessionHandle & session,
+    chip::Callback::Cancelable * success, chip::Callback::Cancelable * failure);
+typedef CHIP_ERROR (^MTRLocalActionBlock)(chip::Callback::Cancelable * success, chip::Callback::Cancelable * failure);
 typedef void (*DefaultFailureCallbackType)(void *, CHIP_ERROR);
 
 template <class T> class MTRCallbackBridge {
 public:
-    MTRCallbackBridge(dispatch_queue_t queue, ResponseHandler handler, MTRActionBlock action, T OnSuccessFn, bool keepAlive)
+    MTRCallbackBridge(dispatch_queue_t queue, ResponseHandler handler, MTRLocalActionBlock action, T OnSuccessFn, bool keepAlive)
+        : mQueue(queue)
+        , mHandler(handler)
+        , mKeepAlive(keepAlive)
+        , mSuccess(OnSuccessFn, this)
+        , mFailure(OnFailureFn, this)
+    {
+        LogRequestStart();
+
+        // For now keep sync dispatch here.
+        dispatch_sync(chip::DeviceLayer::PlatformMgrImpl().GetWorkQueue(), ^{
+            CHIP_ERROR err = action(mSuccess.Cancel(), mFailure.Cancel());
+            if (err != CHIP_NO_ERROR) {
+                NSLog(@"Failure performing action. C++-mangled success callback type: '%s', error: %s", typeid(T).name(),
+                    chip::ErrorStr(err));
+
+                // Take the normal async error-reporting codepath.  This will also
+                // handle cleaning us up properly.
+                OnFailureFn(this, err);
+            }
+        });
+    }
+
+    MTRCallbackBridge(dispatch_queue_t queue, chip::NodeId nodeID, MTRDeviceController * controller, ResponseHandler handler,
+        MTRActionBlock action, T OnSuccessFn, bool keepAlive)
+        : mQueue(queue)
+        , mHandler(handler)
+        , mAction(action)
+        , mKeepAlive(keepAlive)
+        , mSuccess(OnSuccessFn, this)
+        , mFailure(OnFailureFn, this)
+    {
+        ActionWithNodeID(nodeID, controller);
+    }
+
+    MTRCallbackBridge(dispatch_queue_t queue, MTRBaseDevice * device, ResponseHandler handler, MTRActionBlock action, T OnSuccessFn,
+        bool keepAlive)
         : mQueue(queue)
         , mHandler(handler)
+        , mAction(action)
         , mKeepAlive(keepAlive)
         , mSuccess(OnSuccessFn, this)
         , mFailure(OnFailureFn, this)
+    {
+        auto * paseDevice = [device paseDevice];
+        if (paseDevice != nullptr) {
+            ActionWithDevice(paseDevice);
+        } else {
+            ActionWithNodeID(device.nodeID, device.deviceController);
+        }
+    };
+
+    void ActionWithDevice(chip::DeviceProxy * 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);
+        });
+    }
+
+    void ActionWithNodeID(chip::NodeId nodeID, MTRDeviceController * controller)
+    {
+        LogRequestStart();
+
+        BOOL ok = [controller getSessionForNode:nodeID
+                              completionHandler:^(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 LogRequestStart()
     {
         mRequestTime = [NSDate date];
         // Generate a unique cookie to track this operation
         mCookie = [NSString stringWithFormat:@"Response Time: %s+%u", typeid(T).name(), arc4random()];
         ChipLogDetail(Controller, "%s", mCookie.UTF8String);
-        __block CHIP_ERROR err = CHIP_NO_ERROR;
-        dispatch_sync(chip::DeviceLayer::PlatformMgrImpl().GetWorkQueue(), ^{
-            err = action(mSuccess.Cancel(), mFailure.Cancel());
-        });
+    }
 
-        if (CHIP_NO_ERROR != err) {
+    void MaybeDoAction(
+        chip::Messaging::ExchangeManager * exchangeManager, const chip::Optional<chip::SessionHandle> & session, NSError * error)
+    {
+        // Make sure we don't hold on to our action longer than we have to.
+        auto action = mAction;
+        mAction = nil;
+        if (error != nil) {
+            DispatchFailure(this, error);
+            return;
+        }
+
+        CHIP_ERROR err = action(*exchangeManager, session.Value(), mSuccess.Cancel(), mFailure.Cancel());
+        if (err != CHIP_NO_ERROR) {
             NSLog(@"Failure performing action. C++-mangled success callback type: '%s', error: %s", typeid(T).name(),
                 chip::ErrorStr(err));
 
             // Take the normal async error-reporting codepath.  This will also
             // handle cleaning us up properly.
-            DispatchFailure(this, [MTRError errorForCHIPErrorCode:err]);
+            OnFailureFn(this, err);
         }
-    };
+    }
 
     virtual ~MTRCallbackBridge() {};
 
@@ -95,6 +192,7 @@ template <class T> class MTRCallbackBridge {
     }
 
     ResponseHandler mHandler;
+    MTRActionBlock mAction;
     bool mKeepAlive;
 
     chip::Callback::Callback<T> mSuccess;

src/darwin/Framework/CHIP/MTRDeviceConnectionBridge.h

@@ -21,31 +21,46 @@
 
 #include <controller/CHIPDeviceController.h>
 #include <lib/core/ReferenceCounted.h>
+#include <messaging/ExchangeMgr.h>
+#include <transport/SessionHandle.h>
 
 NS_ASSUME_NONNULL_BEGIN
 
-class MTRDeviceConnectionBridge : public chip::ReferenceCounted<MTRDeviceConnectionBridge>
-{
+// Either exchangeManager will be non-nil and session will have a value, or
+// error will be non-nil.
+typedef void (^MTRInternalDeviceConnectionCallback)(chip::Messaging::ExchangeManager * _Nullable exchangeManager,
+    const chip::Optional<chip::SessionHandle> & session, NSError * _Nullable error);
+
+/**
+ * Helper to establish or look up a CASE session and hand its session
+ * information back to the consumer.
+ */
+class MTRDeviceConnectionBridge : public chip::ReferenceCounted<MTRDeviceConnectionBridge> {
 public:
-    MTRDeviceConnectionBridge(MTRDeviceConnectionCallback completionHandler, dispatch_queue_t queue) :
-        mCompletionHandler(completionHandler), mQueue(queue), mOnConnected(OnConnected, this),
-        mOnConnectFailed(OnConnectionFailure, this)
-    {}
+    MTRDeviceConnectionBridge(MTRInternalDeviceConnectionCallback completionHandler)
+        : mCompletionHandler(completionHandler)
+        , mOnConnected(OnConnected, this)
+        , mOnConnectFailed(OnConnectionFailure, this)
+    {
+    }
 
     ~MTRDeviceConnectionBridge()
     {
         mOnConnected.Cancel();
         mOnConnectFailed.Cancel();
     }
 
+    /**
+     * connect must be called on the Matter queue, and will invoke the
+     * completionHandler on the Matter queue as well.
+     */
     CHIP_ERROR connect(chip::Controller::DeviceController * controller, chip::NodeId deviceID)
     {
         return controller->GetConnectedDevice(deviceID, &mOnConnected, &mOnConnectFailed);
     }
 
 private:
-    MTRDeviceConnectionCallback mCompletionHandler;
-    dispatch_queue_t mQueue;
+    MTRInternalDeviceConnectionCallback mCompletionHandler;
     chip::Callback::Callback<chip::OnDeviceConnected> mOnConnected;
     chip::Callback::Callback<chip::OnDeviceConnectionFailure> mOnConnectFailed;
 

src/darwin/Framework/CHIP/MTRDeviceConnectionBridge.mm

@@ -22,18 +22,13 @@
 void MTRDeviceConnectionBridge::OnConnected(void * context, chip::OperationalDeviceProxy * device)
 {
     auto * object = static_cast<MTRDeviceConnectionBridge *>(context);
-    MTRBaseDevice * chipDevice = [[MTRBaseDevice alloc] initWithDevice:device];
-    dispatch_async(object->mQueue, ^{
-        object->mCompletionHandler(chipDevice, nil);
-        object->Release();
-    });
+    object->mCompletionHandler(device->GetExchangeManager(), device->GetSecureSession(), nil);
+    object->Release();
 }
 
 void MTRDeviceConnectionBridge::OnConnectionFailure(void * context, const chip::ScopedNodeId & peerId, CHIP_ERROR error)
 {
     auto * object = static_cast<MTRDeviceConnectionBridge *>(context);
-    dispatch_async(object->mQueue, ^{
-        object->mCompletionHandler(nil, [MTRError errorForCHIPErrorCode:error]);
-        object->Release();
-    });
+    object->mCompletionHandler(nil, chip::NullOptional, [MTRError errorForCHIPErrorCode:error]);
+    object->Release();
 }

src/darwin/Framework/CHIP/MTRDeviceController.mm

@@ -496,7 +496,7 @@ - (MTRBaseDevice *)getDeviceBeingCommissioned:(uint64_t)deviceId error:(NSError
     });
     VerifyOrReturnValue(success, nil);
 
-    return [[MTRBaseDevice alloc] initWithDevice:deviceProxy];
+    return [[MTRBaseDevice alloc] initWithPASEDevice:deviceProxy controller:self];
 }
 
 - (BOOL)getBaseDevice:(uint64_t)deviceID
@@ -511,19 +511,24 @@ - (BOOL)getBaseDevice:(uint64_t)deviceID
         return NO;
     }
 
-    dispatch_async(_chipWorkQueue, ^{
-        VerifyOrReturn([self checkIsRunning]);
-
-        auto connectionBridge = new MTRDeviceConnectionBridge(completionHandler, queue);
-        auto errorCode = connectionBridge->connect(self->_cppCommissioner, deviceID);
-        if ([self checkForError:errorCode logMsg:kErrorGetPairedDevice error:nil]) {
-            // Errors are propagated to the caller through completionHandler.
-            // No extra error handling is needed here.
-            return;
-        }
-    });
-
-    return YES;
+    // We know getSessionForNode will return YES here, since we already checked
+    // that we are running.
+    return [self getSessionForNode:deviceID
+                 completionHandler:^(chip::Messaging::ExchangeManager * _Nullable exchangeManager,
+                     const chip::Optional<chip::SessionHandle> & session, NSError * _Nullable error) {
+                     // Create an MTRBaseDevice for the node id involved, now that our
+                     // CASE session is primed.  We don't actually care about the session
+                     // information here.
+                     dispatch_async(queue, ^{
+                         MTRBaseDevice * device;
+                         if (error == nil) {
+                             device = [[MTRBaseDevice alloc] initWithNodeID:deviceID controller:self];
+                         } else {
+                             device = nil;
+                         }
+                         completionHandler(device, error);
+                     });
+                 }];
 }
 
 - (BOOL)openPairingWindow:(uint64_t)deviceID duration:(NSUInteger)duration error:(NSError * __autoreleasing *)error
@@ -693,6 +698,29 @@ - (BOOL)_deviceBeingCommissionedOverBLE:(uint64_t)deviceId
     return deviceProxy->GetDeviceTransportType() == chip::Transport::Type::kBle;
 }
 
+- (BOOL)getSessionForNode:(chip::NodeId)nodeID completionHandler:(MTRInternalDeviceConnectionCallback)completionHandler
+{
+    if (![self checkIsRunning]) {
+        return NO;
+    }
+
+    dispatch_async(_chipWorkQueue, ^{
+        NSError * error;
+        if (![self checkIsRunning:&error]) {
+            completionHandler(nullptr, chip::NullOptional, error);
+            return;
+        }
+
+        auto connectionBridge = new MTRDeviceConnectionBridge(completionHandler);
+
+        // MTRDeviceConnectionBridge always delivers errors async via
+        // completionHandler.
+        connectionBridge->connect(self->_cppCommissioner, nodeID);
+    });
+
+    return YES;
+}
+
 @end
 
 @implementation MTRDeviceController (InternalMethods)
@@ -734,4 +762,19 @@ - (CHIP_ERROR)isRunningOnFabric:(chip::FabricTable *)fabricTable
     return CHIP_NO_ERROR;
 }
 
+- (void)invalidateCASESessionForNode:(chip::NodeId)nodeID;
+{
+    if (![self checkIsRunning]) {
+        return;
+    }
+
+    dispatch_sync(_chipWorkQueue, ^{
+        if (![self checkIsRunning]) {
+            return;
+        }
+
+        // TODO: This is a hack and needs to go away or use some sane API.
+        self->_cppCommissioner->DisconnectDevice(nodeID);
+    });
+}
 @end