Make FindOrEstablishSession inside CASE callbacks work. (#27942)

The following sequence of events:

1. Someone calls FindOrEstablishSession.
2. When the session establishment fails, OperationalSessionSetup dispatches the
   relevant notifications.
3. One of those notification handlers tries to FindOrEstablishSession to the
   same peer.

failed, because FindOrEstablishSession would pick up the existing
OperationalSessionSetup and add the new callbacks to it... but the existing
OperationalSessionSetup was already in callback notification, so would just
ignore the new callbacks.  So the new FindOrEstablishSession call would never
complete and would never try any actual session establishment.

The fix is to destroy the OperationalSessionSetup before notifying its
callbacks, so a FindOrEstablishSession from one of the callbacks ends up
creating a new OperationalSessionSetup as needed.

src/app/OperationalSessionSetup.cpp

@@ -276,7 +276,7 @@ void OperationalSessionSetup::EnqueueConnectionCallbacks(Callback::Callback<OnDe
     }
 }
 
-void OperationalSessionSetup::DequeueConnectionCallbacksWithoutReleasing(CHIP_ERROR error)
+void OperationalSessionSetup::DequeueConnectionCallbacks(CHIP_ERROR error, ReleaseBehavior releaseBehavior)
 {
     Cancelable failureReady, successReady;
 
@@ -297,50 +297,64 @@ void OperationalSessionSetup::DequeueConnectionCallbacksWithoutReleasing(CHIP_ER
     }
 #endif // CHIP_DEVICE_CONFIG_ENABLE_AUTOMATIC_CASE_RETRIES
 
+    // Gather up state we will need for our notifications.
+    bool performingAddressUpdate                  = mPerformingAddressUpdate;
+    auto * exchangeMgr                            = mInitParams.exchangeMgr;
+    Optional<SessionHandle> optionalSessionHandle = mSecureSession.Get();
+    ScopedNodeId peerId                           = mPeerId;
+
+    if (releaseBehavior == ReleaseBehavior::Release)
+    {
+        VerifyOrDie(mReleaseDelegate != nullptr);
+        mReleaseDelegate->ReleaseSession(this);
+    }
+
+    // DO NOT touch any members of this object after this point.  It's dead.
+
+    NotifyConnectionCallbacks(failureReady, successReady, error, peerId, performingAddressUpdate, exchangeMgr,
+                              optionalSessionHandle);
+}
+
+void OperationalSessionSetup::NotifyConnectionCallbacks(Cancelable & failureReady, Cancelable & successReady, CHIP_ERROR error,
+                                                        const ScopedNodeId & peerId, bool performingAddressUpdate,
+                                                        Messaging::ExchangeManager * exchangeMgr,
+                                                        const Optional<SessionHandle> & optionalSessionHandle)
+{
     //
     // If we encountered no error, go ahead and call all success callbacks. Otherwise,
     // call the failure callbacks.
     //
     while (failureReady.mNext != &failureReady)
     {
         // We expect that we only have callbacks if we are not performing just address update.
-        VerifyOrDie(!mPerformingAddressUpdate);
+        VerifyOrDie(!performingAddressUpdate);
         Callback::Callback<OnDeviceConnectionFailure> * cb =
             Callback::Callback<OnDeviceConnectionFailure>::FromCancelable(failureReady.mNext);
 
         cb->Cancel();
 
         if (error != CHIP_NO_ERROR)
         {
-            cb->mCall(cb->mContext, mPeerId, error);
+            cb->mCall(cb->mContext, peerId, error);
         }
     }
 
     while (successReady.mNext != &successReady)
     {
         // We expect that we only have callbacks if we are not performing just address update.
-        VerifyOrDie(!mPerformingAddressUpdate);
+        VerifyOrDie(!performingAddressUpdate);
         Callback::Callback<OnDeviceConnected> * cb = Callback::Callback<OnDeviceConnected>::FromCancelable(successReady.mNext);
 
         cb->Cancel();
         if (error == CHIP_NO_ERROR)
         {
-            auto * exchangeMgr = mInitParams.exchangeMgr;
             VerifyOrDie(exchangeMgr);
             // We know that we for sure have the SessionHandle in the successful case.
-            auto optionalSessionHandle = mSecureSession.Get();
             cb->mCall(cb->mContext, *exchangeMgr, optionalSessionHandle.Value());
         }
     }
 }
 
-void OperationalSessionSetup::DequeueConnectionCallbacks(CHIP_ERROR error)
-{
-    DequeueConnectionCallbacksWithoutReleasing(error);
-    VerifyOrDie(mReleaseDelegate != nullptr);
-    mReleaseDelegate->ReleaseSession(this);
-}
-
 void OperationalSessionSetup::OnSessionEstablishmentError(CHIP_ERROR error)
 {
     VerifyOrReturn(mState == State::Connecting,
@@ -447,7 +461,7 @@ OperationalSessionSetup::~OperationalSessionSetup()
     CancelSessionSetupReattempt();
 #endif // CHIP_DEVICE_CONFIG_ENABLE_AUTOMATIC_CASE_RETRIES
 
-    DequeueConnectionCallbacksWithoutReleasing(CHIP_ERROR_CANCELLED);
+    DequeueConnectionCallbacks(CHIP_ERROR_CANCELLED, ReleaseBehavior::DoNotRelease);
 }
 
 CHIP_ERROR OperationalSessionSetup::LookupPeerAddress()

src/app/OperationalSessionSetup.h

@@ -301,24 +301,36 @@ class DLL_EXPORT OperationalSessionSetup : public SessionEstablishmentDelegate,
     void EnqueueConnectionCallbacks(Callback::Callback<OnDeviceConnected> * onConnection,
                                     Callback::Callback<OnDeviceConnectionFailure> * onFailure);
 
+    enum class ReleaseBehavior
+    {
+        Release,
+        DoNotRelease
+    };
+
     /*
      * This dequeues all failure and success callbacks and appropriately
      * invokes either set depending on the value of error.
      *
      * If error == CHIP_NO_ERROR, only success callbacks are invoked.
      * Otherwise, only failure callbacks are invoked.
      *
-     * This uses mReleaseDelegate to release ourselves (aka `this`). As a
-     * result any caller should return right away without touching `this`.
+     * If releaseBehavior is Release, this uses mReleaseDelegate to release
+     * ourselves (aka `this`). As a result any caller should return right away
+     * without touching `this`.
      *
+     * Setting releaseBehavior to DoNotRelease is meant for use from the destructor
      */
-    void DequeueConnectionCallbacks(CHIP_ERROR error);
+    void DequeueConnectionCallbacks(CHIP_ERROR error, ReleaseBehavior releaseBehavior = ReleaseBehavior::Release);
 
-    /*
-     * Like DequeueConnectionCallbacks but does not release ourselves.  For use
-     * from our destructor.
+    /**
+     * Helper for DequeueConnectionCallbacks that handles the actual callback
+     * notifications. This happens after the object has been released, if it's
+     * being released.
      */
-    void DequeueConnectionCallbacksWithoutReleasing(CHIP_ERROR error);
+    static void NotifyConnectionCallbacks(Callback::Cancelable & failureReady, Callback::Cancelable & successReady,
+                                          CHIP_ERROR error, const ScopedNodeId & peerId, bool performingAddressUpdate,
+                                          Messaging::ExchangeManager * exchangeMgr,
+                                          const Optional<SessionHandle> & optionalSessionHandle);
 
     /**
      * Triggers a DNSSD lookup to find a usable peer address.