Avoid double-delete when a Darwin subscribeAttribute subscription err…

…ors out. (project-chip#23557)

* Avoid double-delete when a Darwin subscribeAttribute subscription errors out.

Right now Darwin subscribeAttribute subscriptions do not support turning off auto-resubscribe, so never error out.  But if we were to add that, we would end up with double delete: once when the error happens, and once from OnDone.

This fix:

1) Ensures that we don't flag callback bridges as mKeepAlive until we have
   successfully sent a subscribe request.
2) Stops deleting on error if mKeepAlive is true.
3) Ensures the delete of the bridge from OnDone does not happen until after
   we have finished using the bridge object (e.g. to handle already-queued data
   or error notifications).  We do this by queueing the delete to the same
   dispatch queue as the data/error notifications.

* Address review comment.

src/darwin/Framework/CHIP/MTRBaseDevice.mm

@@ -688,9 +688,8 @@ 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, MTRDeviceResponseHandler handler, MTRActionBlock action, bool keepAlive = false)
-        : MTRCallbackBridge<MTRDataValueDictionaryCallback>(queue, handler, action, OnSuccessFn, keepAlive) {};
+    MTRDataValueDictionaryCallbackBridge(dispatch_queue_t queue, MTRDeviceResponseHandler handler, MTRActionBlock action)
+        : MTRCallbackBridge<MTRDataValueDictionaryCallback>(queue, handler, action, OnSuccessFn) {};
 
     static void OnSuccessFn(void * context, id value) { DispatchSuccess(context, value); }
 };

src/darwin/Framework/CHIP/MTRCallbackBridgeBase_internal.h

@@ -72,10 +72,9 @@ template <class T> class MTRCallbackBridge : public MTRCallbackBridgeBase {
      * Construct a callback bridge, which can then have DispatcLocalAction() called
      * on it.
      */
-    MTRCallbackBridge(dispatch_queue_t queue, MTRResponseHandler handler, T OnSuccessFn, bool keepAlive)
+    MTRCallbackBridge(dispatch_queue_t queue, MTRResponseHandler handler, T OnSuccessFn)
         : mQueue(queue)
         , mHandler(handler)
-        , mKeepAlive(keepAlive)
         , mSuccess(OnSuccessFn)
         , mFailure(OnFailureFn)
     {
@@ -85,11 +84,10 @@ template <class T> class MTRCallbackBridge : public MTRCallbackBridgeBase {
      * Construct a callback bridge, which can then have DispatchAction() called
      * on it.
      */
-    MTRCallbackBridge(dispatch_queue_t queue, MTRResponseHandler handler, MTRActionBlock action, T OnSuccessFn, bool keepAlive)
+    MTRCallbackBridge(dispatch_queue_t queue, MTRResponseHandler handler, MTRActionBlock action, T OnSuccessFn)
         : mQueue(queue)
         , mHandler(handler)
         , mAction(action)
-        , mKeepAlive(keepAlive)
         , mSuccess(OnSuccessFn)
         , mFailure(OnFailureFn)
     {
@@ -219,6 +217,26 @@ template <class T> class MTRCallbackBridge : public MTRCallbackBridgeBase {
     static void DispatchFailure(void * context, NSError * error) { DispatchCallbackResult(context, error, nil); }
 
 protected:
+    // OnDone and KeepAliveOnCallback really only make sense for subscription
+    // bridges, but we put them here to avoid many copies of this code in
+    // generated bits.
+    void OnDone()
+    {
+        if (!mQueue) {
+            delete this;
+            return;
+        }
+
+        // Delete ourselves async, so that any error/data reports we
+        // queued up before getting OnDone have a chance to run.
+        auto * self = this;
+        dispatch_async(mQueue, ^{
+            delete self;
+        });
+    }
+
+    void KeepAliveOnCallback() { mKeepAlive = true; }
+
     dispatch_queue_t mQueue;
 
 private:
@@ -230,15 +248,12 @@ template <class T> class MTRCallbackBridge : public MTRCallbackBridgeBase {
         }
 
         if (!callbackBridge->mQueue) {
-            delete callbackBridge;
+            if (!callbackBridge->mKeepAlive) {
+                delete callbackBridge;
+            }
             return;
         }
 
-        if (error) {
-            // We should delete ourselves; there will be no more callbacks.
-            callbackBridge->mKeepAlive = false;
-        }
-
         dispatch_async(callbackBridge->mQueue, ^{
             ChipLogDetail(Controller, "%s %f seconds", callbackBridge->mCookie.UTF8String,
                 -[callbackBridge->mRequestTime timeIntervalSinceNow]);
@@ -252,7 +267,7 @@ template <class T> class MTRCallbackBridge : public MTRCallbackBridgeBase {
 
     MTRResponseHandler mHandler;
     MTRActionBlock mAction;
-    bool mKeepAlive;
+    bool mKeepAlive = false;
 
     T mSuccess;
     MTRErrorCallback mFailure;

src/darwin/Framework/CHIP/templates/MTRBaseClusters-src.zapt

@@ -195,12 +195,18 @@ reportHandler:(void (^)({{asObjectiveCClass type parent.name}} * _Nullable value
           using TypeInfo = {{asUpperCamelCase parent.name}}::Attributes::{{asUpperCamelCase name}}::TypeInfo;
 
           chip::Controller::{{asUpperCamelCase parent.name}}Cluster cppCluster(exchangeManager, session, self->_endpoint);
-          return cppCluster.SubscribeAttribute<TypeInfo>(bridge, successCb, failureCb, [params.minInterval unsignedShortValue], [params.maxInterval unsignedShortValue], MTR{{>attribute_data_callback_name}}CallbackSubscriptionBridge::OnSubscriptionEstablished, nil,
+          CHIP_ERROR err = cppCluster.SubscribeAttribute<TypeInfo>(bridge, successCb, failureCb, [params.minInterval unsignedShortValue], [params.maxInterval unsignedShortValue], MTR{{>attribute_data_callback_name}}CallbackSubscriptionBridge::OnSubscriptionEstablished, nil,
               params.filterByFabric,
               !params.replaceExistingSubscriptions,
               chip::NullOptional,
-              [typedBridge](void) { delete typedBridge; }
+              [typedBridge](void) { typedBridge->OnDone(); }
           );
+          if (err == CHIP_NO_ERROR) {
+              // Now that we kicked off the subscribe, flag our callback bridge
+              // to stay alive until we get an OnDone.
+              typedBridge->KeepAliveOnCallback();
+          }
+          return err;
       }, subscriptionEstablished);
     std::move(*bridge).DispatchAction(self.device);
 }

src/darwin/Framework/CHIP/templates/partials/MTRCallbackBridge.zapt

@@ -13,12 +13,12 @@
 class MTR{{> @partial-block}}Bridge : public MTRCallbackBridge<{{>callbackType}}>
 {
 public:
-    MTR{{> @partial-block}}Bridge(dispatch_queue_t queue, ResponseHandler handler, bool keepAlive = false)
-      : MTRCallbackBridge<{{>callbackType}}>(queue, handler, OnSuccessFn, keepAlive)
+    MTR{{> @partial-block}}Bridge(dispatch_queue_t queue, ResponseHandler handler)
+      : MTRCallbackBridge<{{>callbackType}}>(queue, handler, OnSuccessFn)
       {};
 
-    MTR{{> @partial-block}}Bridge(dispatch_queue_t queue, ResponseHandler handler, MTRActionBlock action, bool keepAlive = false)
-      : MTRCallbackBridge<{{>callbackType}}>(queue, handler, action, OnSuccessFn, keepAlive)
+    MTR{{> @partial-block}}Bridge(dispatch_queue_t queue, ResponseHandler handler, MTRActionBlock action)
+      : MTRCallbackBridge<{{>callbackType}}>(queue, handler, action, OnSuccessFn)
       {};
 
     static void OnSuccessFn(void * context
@@ -43,11 +43,13 @@ class MTR{{> @partial-block}}SubscriptionBridge : public MTR{{> @partial-block}}
 {
 public:
     MTR{{> @partial-block}}SubscriptionBridge(dispatch_queue_t queue, ResponseHandler handler, MTRActionBlock action, MTRSubscriptionEstablishedHandler establishedHandler)
-      : MTR{{> @partial-block}}Bridge(queue, handler, action, true),
+      : MTR{{> @partial-block}}Bridge(queue, handler, action),
         mEstablishedHandler(establishedHandler)
       {}
 
     static void OnSubscriptionEstablished(void * context);
+    using MTR{{> @partial-block}}Bridge::OnDone;
+    using MTR{{> @partial-block}}Bridge::KeepAliveOnCallback;
 
 private:
     MTRSubscriptionEstablishedHandler mEstablishedHandler;
@@ -110,5 +112,6 @@ void MTR{{> @partial-block}}SubscriptionBridge::OnSubscriptionEstablished(void *
          self->mEstablishedHandler = nil;
      }
 }
+
 {{/unless}}
 {{/if}}