Fix handling of OperationalDeviceProxy and CHIPCluster in Darwin fram…

…ework. (#21504)

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