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,161 @@
 
 #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)
+    /**
+     * Run the given MTRLocalActionBlock on the Matter thread, then handle
+     * converting the value produced by the success callback to the right type
+     * so it can be passed to a callback of the type we're templated over.
+     *
+     * Does not attempt to establish any sessions to devices.  Must not be used
+     * with any action blocks that need a session.
+     */
+    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);
+            }
+        });
+    }
+
+    /**
+     * Run the given MTRActionBlock on the Matter thread, after getting a CASE
+     * session (possibly pre-existing) to the given node ID on the fabric
+     * represented by the given MTRDeviceController.  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, 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);
+    }
+
+    /**
+     * 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.
+     */
+    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 +214,7 @@ template <class T> class MTRCallbackBridge {
     }
 
     ResponseHandler mHandler;
+    MTRActionBlock mAction;
     bool mKeepAlive;
 
     chip::Callback::Callback<T> mSuccess;

src/darwin/Framework/CHIP/MTRCluster.h

@@ -36,7 +36,7 @@ NS_ASSUME_NONNULL_BEGIN
  *    If not provided (i.e. nil passed for the CHIPWriteParams argument), will be
  *    treated as if a default-initialized object was passed in.
  */
-@interface MTRWriteParams : NSObject
+@interface MTRWriteParams : NSObject <NSCopying>
 
 /**
  * Controls whether the write is a timed write.
@@ -65,6 +65,7 @@ NS_ASSUME_NONNULL_BEGIN
 @property (nonatomic, copy, nullable) NSNumber * dataVersion;
 
 - (instancetype)init;
+- (id)copyWithZone:(nullable NSZone *)zone;
 
 @end
 
@@ -74,7 +75,7 @@ NS_ASSUME_NONNULL_BEGIN
  *    If not provided (i.e. nil passed for the MTRReadParams argument), will be
  *    treated as if a default-initialized object was passed in.
  */
-@interface MTRReadParams : NSObject
+@interface MTRReadParams : NSObject <NSCopying>
 
 /**
  * Whether the read/subscribe is fabric-filtered. nil (the default value) is
@@ -89,6 +90,7 @@ NS_ASSUME_NONNULL_BEGIN
 @property (nonatomic, copy, nullable) NSNumber * fabricFiltered;
 
 - (instancetype)init;
+- (id)copyWithZone:(nullable NSZone *)zone;
 
 @end
 
@@ -126,6 +128,7 @@ NS_ASSUME_NONNULL_BEGIN
 @property (nonatomic, copy, nullable) NSNumber * autoResubscribe;
 
 - (instancetype)init;
+- (id)copyWithZone:(nullable NSZone *)zone;
 
 @end
 

src/darwin/Framework/CHIP/MTRCluster.mm

@@ -51,6 +51,14 @@ - (instancetype)init
     return self;
 }
 
+- (id)copyWithZone:(nullable NSZone *)zone
+{
+    auto other = [[MTRWriteParams alloc] init];
+    other.timedWriteTimeout = self.timedWriteTimeout;
+    other.dataVersion = self.dataVersion;
+    return other;
+}
+
 @end
 
 @implementation MTRReadParams
@@ -62,6 +70,13 @@ - (instancetype)init
     return self;
 }
 
+- (id)copyWithZone:(nullable NSZone *)zone
+{
+    auto other = [[MTRReadParams alloc] init];
+    other.fabricFiltered = self.fabricFiltered;
+    return other;
+}
+
 @end
 
 @implementation MTRSubscribeParams
@@ -74,4 +89,13 @@ - (instancetype)init
     return self;
 }
 
+- (id)copyWithZone:(nullable NSZone *)zone
+{
+    auto other = [[MTRSubscribeParams alloc] init];
+    other.fabricFiltered = self.fabricFiltered;
+    other.keepPreviousSubscriptions = self.keepPreviousSubscriptions;
+    other.autoResubscribe = self.autoResubscribe;
+    return other;
+}
+
 @end

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();
 }