Fix race when trying to use Darwin callback bridge from action block. (#23553)

We have some callback bridge action blocks that need access to the bridge
itself.  Since the action block was queued to run on a different thread from
inside the bridge constructor, it could race with the bridge constructor
finishing execution, leading to unexpected behavior (including possibly
destruction of the bridge before its constructor finishes executing.

Switch to running the action block after the bridge is constructed.

Author: bzbarsky-apple

src/darwin/Framework/CHIP/MTRBaseDevice.mm

@@ -648,7 +648,6 @@ static CHIP_ERROR MTREncodeTLVFromDataValueDictionary(id object, chip::TLV::TLVW
 
 // Callback type to pass data value as an NSObject
 typedef void (*MTRDataValueDictionaryCallback)(void * context, id value);
-typedef void (*MTRErrorCallback)(void * context, CHIP_ERROR error);
 
 // Rename to be generic for decode and encode
 class MTRDataValueDictionaryDecodableType {
@@ -689,9 +688,9 @@ CHIP_ERROR Encode(chip::TLV::TLVWriter & writer, chip::TLV::Tag tag) const
 // Callback bridge for MTRDataValueDictionaryCallback
 class MTRDataValueDictionaryCallbackBridge : public MTRCallbackBridge<MTRDataValueDictionaryCallback> {
 public:
-    MTRDataValueDictionaryCallbackBridge(dispatch_queue_t queue, MTRBaseDevice * device, MTRDeviceResponseHandler handler,
-        MTRActionBlock action, bool keepAlive = false)
-        : MTRCallbackBridge<MTRDataValueDictionaryCallback>(queue, device, handler, action, OnSuccessFn, keepAlive) {};
+    MTRDataValueDictionaryCallbackBridge(
+        dispatch_queue_t queue, MTRDeviceResponseHandler handler, MTRActionBlock action, bool keepAlive = false)
+        : MTRCallbackBridge<MTRDataValueDictionaryCallback>(queue, handler, action, OnSuccessFn, keepAlive) {};
 
     static void OnSuccessFn(void * context, id value) { DispatchSuccess(context, value); }
 };
@@ -791,14 +790,9 @@ - (void)readAttributesWithEndpointID:(NSNumber * _Nullable)endpointID
     clusterID = (clusterID == nil) ? nil : [clusterID copy];
     attributeID = (attributeID == nil) ? nil : [attributeID copy];
     params = (params == nil) ? nil : [params copy];
-    new MTRDataValueDictionaryCallbackBridge(queue, self, completion,
-        ^(ExchangeManager & exchangeManager, const SessionHandle & session, chip::Callback::Cancelable * success,
-            chip::Callback::Cancelable * failure) {
-            auto successFn = chip::Callback::Callback<MTRDataValueDictionaryCallback>::FromCancelable(success);
-            auto failureFn = chip::Callback::Callback<MTRErrorCallback>::FromCancelable(failure);
-            auto context = successFn->mContext;
-            auto successCb = successFn->mCall;
-            auto failureCb = failureFn->mCall;
+    auto * bridge = new MTRDataValueDictionaryCallbackBridge(queue, completion,
+        ^(ExchangeManager & exchangeManager, const SessionHandle & session, MTRDataValueDictionaryCallback successCb,
+            MTRErrorCallback failureCb, MTRCallbackBridgeBase * bridge) {
             auto resultArray = [[NSMutableArray alloc] init];
             auto resultSuccess = [[NSMutableArray alloc] init];
             auto resultFailure = [[NSMutableArray alloc] init];
@@ -842,23 +836,23 @@ new MTRDataValueDictionaryCallbackBridge(queue, self, completion,
             readParams.mAttributePathParamsListSize = 1;
             readParams.mIsFabricFiltered = params.filterByFabric;
 
-            auto onDone = [resultArray, resultSuccess, resultFailure, context, successCb, failureCb](
+            auto onDone = [resultArray, resultSuccess, resultFailure, bridge, successCb, failureCb](
                               BufferedReadAttributeCallback<MTRDataValueDictionaryDecodableType> * callback) {
                 if ([resultFailure count] > 0 || [resultSuccess count] == 0) {
                     // Failure
                     if (failureCb) {
                         if ([resultFailure count] > 0) {
-                            failureCb(context, [MTRError errorToCHIPErrorCode:resultFailure[0]]);
+                            failureCb(bridge, [MTRError errorToCHIPErrorCode:resultFailure[0]]);
                         } else if ([resultArray count] > 0) {
-                            failureCb(context, [MTRError errorToCHIPErrorCode:resultArray[0][MTRErrorKey]]);
+                            failureCb(bridge, [MTRError errorToCHIPErrorCode:resultArray[0][MTRErrorKey]]);
                         } else {
-                            failureCb(context, CHIP_ERROR_READ_FAILED);
+                            failureCb(bridge, CHIP_ERROR_READ_FAILED);
                         }
                     }
                 } else {
                     // Success
                     if (successCb) {
-                        successCb(context, resultArray);
+                        successCb(bridge, resultArray);
                     }
                 }
                 chip::Platform::Delete(callback);
@@ -887,6 +881,7 @@ new MTRDataValueDictionaryCallbackBridge(queue, self, completion,
             callback.release();
             return err;
         });
+    std::move(*bridge).DispatchAction(self);
 }
 
 - (void)writeAttributeWithEndpointID:(NSNumber *)endpointID
@@ -897,14 +892,9 @@ - (void)writeAttributeWithEndpointID:(NSNumber *)endpointID
                                queue:(dispatch_queue_t)queue
                           completion:(MTRDeviceResponseHandler)completion
 {
-    new MTRDataValueDictionaryCallbackBridge(queue, self, completion,
-        ^(ExchangeManager & exchangeManager, const SessionHandle & session, chip::Callback::Cancelable * success,
-            chip::Callback::Cancelable * failure) {
-            auto successFn = chip::Callback::Callback<MTRDataValueDictionaryCallback>::FromCancelable(success);
-            auto failureFn = chip::Callback::Callback<MTRErrorCallback>::FromCancelable(failure);
-            auto context = successFn->mContext;
-            auto successCb = successFn->mCall;
-            auto failureCb = failureFn->mCall;
+    auto * bridge = new MTRDataValueDictionaryCallbackBridge(queue, completion,
+        ^(ExchangeManager & exchangeManager, const SessionHandle & session, MTRDataValueDictionaryCallback successCb,
+            MTRErrorCallback failureCb, MTRCallbackBridgeBase * bridge) {
             auto resultArray = [[NSMutableArray alloc] init];
             auto resultSuccess = [[NSMutableArray alloc] init];
             auto resultFailure = [[NSMutableArray alloc] init];
@@ -929,22 +919,22 @@ new MTRDataValueDictionaryCallbackBridge(queue, self, completion,
             };
 
             auto onDoneCb
-                = [context, successCb, failureCb, resultArray, resultSuccess, resultFailure](app::WriteClient * pWriteClient) {
+                = [bridge, successCb, failureCb, resultArray, resultSuccess, resultFailure](app::WriteClient * pWriteClient) {
                       if ([resultFailure count] > 0 || [resultSuccess count] == 0) {
                           // Failure
                           if (failureCb) {
                               if ([resultFailure count] > 0) {
-                                  failureCb(context, [MTRError errorToCHIPErrorCode:resultFailure[0]]);
+                                  failureCb(bridge, [MTRError errorToCHIPErrorCode:resultFailure[0]]);
                               } else if ([resultArray count] > 0) {
-                                  failureCb(context, [MTRError errorToCHIPErrorCode:resultArray[0][MTRErrorKey]]);
+                                  failureCb(bridge, [MTRError errorToCHIPErrorCode:resultArray[0][MTRErrorKey]]);
                               } else {
-                                  failureCb(context, CHIP_ERROR_WRITE_FAILED);
+                                  failureCb(bridge, CHIP_ERROR_WRITE_FAILED);
                               }
                           }
                       } else {
                           // Success
                           if (successCb) {
-                              successCb(context, resultArray);
+                              successCb(bridge, resultArray);
                           }
                       }
                   };
@@ -956,6 +946,7 @@ new MTRDataValueDictionaryCallbackBridge(queue, self, completion,
                 onSuccessCb, onFailureCb, (timeoutMs == nil) ? NullOptional : Optional<uint16_t>([timeoutMs unsignedShortValue]),
                 onDoneCb, NullOptional);
         });
+    std::move(*bridge).DispatchAction(self);
 }
 
 class NSObjectCommandCallback final : public app::CommandSender::Callback {
@@ -1045,14 +1036,9 @@ - (void)invokeCommandWithEndpointID:(NSNumber *)endpointID
     commandFields = (commandFields == nil) ? nil : [commandFields copy];
     timeoutMs = (timeoutMs == nil) ? nil : [timeoutMs copy];
 
-    new MTRDataValueDictionaryCallbackBridge(queue, self, completion,
-        ^(ExchangeManager & exchangeManager, const SessionHandle & session, chip::Callback::Cancelable * success,
-            chip::Callback::Cancelable * failure) {
-            auto successFn = chip::Callback::Callback<MTRDataValueDictionaryCallback>::FromCancelable(success);
-            auto failureFn = chip::Callback::Callback<MTRErrorCallback>::FromCancelable(failure);
-            auto context = successFn->mContext;
-            auto successCb = successFn->mCall;
-            auto failureCb = failureFn->mCall;
+    auto * bridge = new MTRDataValueDictionaryCallbackBridge(queue, completion,
+        ^(ExchangeManager & exchangeManager, const SessionHandle & session, MTRDataValueDictionaryCallback successCb,
+            MTRErrorCallback failureCb, MTRCallbackBridgeBase * bridge) {
             auto resultArray = [[NSMutableArray alloc] init];
             auto resultSuccess = [[NSMutableArray alloc] init];
             auto resultFailure = [[NSMutableArray alloc] init];
@@ -1086,21 +1072,21 @@ new MTRDataValueDictionaryCallbackBridge(queue, self, completion,
             VerifyOrReturnError(decoder != nullptr, CHIP_ERROR_NO_MEMORY);
 
             auto rawDecoderPtr = decoder.get();
-            auto onDoneCb = [rawDecoderPtr, context, successCb, failureCb, resultArray, resultSuccess, resultFailure](
+            auto onDoneCb = [rawDecoderPtr, bridge, successCb, failureCb, resultArray, resultSuccess, resultFailure](
                                 app::CommandSender * commandSender) {
                 if ([resultFailure count] > 0 || [resultSuccess count] == 0) {
                     // Failure
                     if (failureCb) {
                         if ([resultFailure count] > 0) {
-                            failureCb(context, [MTRError errorToCHIPErrorCode:resultFailure[0]]);
+                            failureCb(bridge, [MTRError errorToCHIPErrorCode:resultFailure[0]]);
                         } else {
-                            failureCb(context, CHIP_ERROR_WRITE_FAILED);
+                            failureCb(bridge, CHIP_ERROR_WRITE_FAILED);
                         }
                     }
                 } else {
                     // Success
                     if (successCb) {
-                        successCb(context, resultArray);
+                        successCb(bridge, resultArray);
                     }
                 }
                 chip::Platform::Delete(commandSender);
@@ -1121,6 +1107,7 @@ new MTRDataValueDictionaryCallbackBridge(queue, self, completion,
             commandSender.release();
             return CHIP_NO_ERROR;
         });
+    std::move(*bridge).DispatchAction(self);
 }
 
 - (void)subscribeToAttributesWithEndpointID:(NSNumber * _Nullable)endpointID

src/darwin/Framework/CHIP/MTRCallbackBridgeBase_internal.h

@@ -32,90 +32,122 @@
  * communication with the device in question has been established, as far as we
  * know.
  */
+class MTRCallbackBridgeBase {
+};
 
 // TODO: ADD NS_ASSUME_NONNULL_BEGIN to this header.  When that happens, note
 // that in MTRActionBlock the two callback pointers are nonnull and the two
-// arguments of ResponseHandler are both nullable.
+// arguments of MTRResponseHandler are both nullable.
 
-typedef void (^ResponseHandler)(id value, NSError * error);
-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);
+typedef void (^MTRResponseHandler)(id value, NSError * error);
+typedef void (*MTRErrorCallback)(void * context, CHIP_ERROR error);
 
-template <class T> class MTRCallbackBridge {
+/**
+ * The bridge will pass itself as the last argument to the action block.
+ *
+ * The action block must do one of three things:
+ *
+ * 1) Return an error.
+ * 2) Call the "successCb" callback, with the bridge passed to the block as the
+ *    context, possibly asynchronously.
+ * 3) Call the "failureCb" callback, with the bridge passed to the block as the
+ *    context, possibly asynchronously.
+ *
+ * For an MTRCallbackBridge that has keepAlive set to true, the success/failure
+ * callbacks may be called multiple times.  If keepAlive is false, there must be
+ * no calls after the first one.
+ */
+template <typename SuccessCallback>
+using MTRActionBlockT = CHIP_ERROR (^)(chip::Messaging::ExchangeManager & exchangeManager, const chip::SessionHandle & session,
+    SuccessCallback successCb, MTRErrorCallback failureCb, MTRCallbackBridgeBase * bridge);
+template <typename SuccessCallback>
+using MTRLocalActionBlockT = CHIP_ERROR (^)(SuccessCallback successCb, MTRErrorCallback failureCb);
+
+template <class T> class MTRCallbackBridge : public MTRCallbackBridgeBase {
 public:
+    using MTRActionBlock = MTRActionBlockT<T>;
+    using MTRLocalActionBlock = MTRLocalActionBlockT<T>;
+
     /**
-     * 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.
+     * Construct a callback bridge, which can then have DispatcLocalAction() called
+     * on it.
      */
-    MTRCallbackBridge(dispatch_queue_t queue, ResponseHandler handler, MTRLocalActionBlock action, T OnSuccessFn, bool keepAlive)
+    MTRCallbackBridge(dispatch_queue_t queue, MTRResponseHandler handler, T OnSuccessFn, bool keepAlive)
         : mQueue(queue)
         , mHandler(handler)
         , mKeepAlive(keepAlive)
-        , mSuccess(OnSuccessFn, this)
-        , mFailure(OnFailureFn, this)
+        , mSuccess(OnSuccessFn)
+        , mFailure(OnFailureFn)
     {
-        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.
+     * Construct a callback bridge, which can then have DispatchAction() called
+     * on it.
      */
-    MTRCallbackBridge(dispatch_queue_t queue, chip::NodeId nodeID, MTRDeviceController * controller, ResponseHandler handler,
-        MTRActionBlock action, T OnSuccessFn, bool keepAlive)
+    MTRCallbackBridge(dispatch_queue_t queue, MTRResponseHandler handler, MTRActionBlock action, T OnSuccessFn, bool keepAlive)
         : mQueue(queue)
         , mHandler(handler)
         , mAction(action)
         , mKeepAlive(keepAlive)
-        , mSuccess(OnSuccessFn, this)
-        , mFailure(OnFailureFn, this)
+        , mSuccess(OnSuccessFn)
+        , mFailure(OnFailureFn)
     {
-        ActionWithNodeID(nodeID, controller);
     }
 
+    /**
+     * 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.  Once this function has been called, on a callback
+     * bridge allocated with `new`, the bridge object must not be accessed by
+     * the caller.  The action block will handle deleting the bridge.
+     */
+    void DispatchAction(chip::NodeId nodeID, MTRDeviceController * controller) && { ActionWithNodeID(nodeID, controller); }
+
     /**
      * 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.
+     * type we're templated over.  Once this function has been called, on a callback
+     * bridge allocated with `new`, the bridge object must not be accessed by
+     * the caller.  The action block will handle deleting the bridge.
      */
-    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)
+    void DispatchAction(MTRBaseDevice * device) &&
     {
         if (device.isPASEDevice) {
             ActionWithPASEDevice(device);
         } else {
             ActionWithNodeID(device.nodeID, device.deviceController);
         }
-    };
+    }
+
+    /**
+     * 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.
+     */
+    void DispatchLocalAction(MTRLocalActionBlock action)
+    {
+        LogRequestStart();
+
+        // For now keep sync dispatch here.
+        dispatch_sync(chip::DeviceLayer::PlatformMgrImpl().GetWorkQueue(), ^{
+            CHIP_ERROR err = action(mSuccess, mFailure);
+            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);
+            }
+        });
+    }
 
     void ActionWithPASEDevice(MTRBaseDevice * device)
     {
@@ -167,7 +199,7 @@ template <class T> class MTRCallbackBridge {
             return;
         }
 
-        CHIP_ERROR err = action(*exchangeManager, session.Value(), mSuccess.Cancel(), mFailure.Cancel());
+        CHIP_ERROR err = action(*exchangeManager, session.Value(), mSuccess, mFailure, this);
         if (err != CHIP_NO_ERROR) {
             NSLog(@"Failure performing action. C++-mangled success callback type: '%s', error: %s", typeid(T).name(),
                 chip::ErrorStr(err));
@@ -218,12 +250,12 @@ template <class T> class MTRCallbackBridge {
         });
     }
 
-    ResponseHandler mHandler;
+    MTRResponseHandler mHandler;
     MTRActionBlock mAction;
     bool mKeepAlive;
 
-    chip::Callback::Callback<T> mSuccess;
-    chip::Callback::Callback<DefaultFailureCallbackType> mFailure;
+    T mSuccess;
+    MTRErrorCallback mFailure;
 
     // Measure the time it took for the callback to trigger
     NSDate * mRequestTime;