Review feedback

examples/chip-tool/commands/tests/TestCommand.cpp

@@ -32,7 +32,17 @@ CHIP_ERROR TestCommand::RunCommand()
 
 CHIP_ERROR TestCommand::WaitForCommissionee(chip::NodeId nodeId)
 {
-    CurrentCommissioner().ReleaseOperationalDevice(nodeId);
+    chip::FabricIndex fabricIndex;
+
+    ReturnErrorOnFailure(CurrentCommissioner().GetFabricIndex(&fabricIndex));
+
+    //
+    // There's a chance the commissionee may have rebooted before this call here as part of a test flow
+    // or is just starting out fresh outright. Let's make sure we're not re-using any cached CASE sessions
+    // that will now be stale and mismatched with the peer, causing subsequent interactions to fail.
+    //
+    CurrentCommissioner().SessionMgr()->ExpireAllPairings(nodeId, fabricIndex);
+
     return CurrentCommissioner().GetConnectedDevice(nodeId, &mOnDeviceConnectedCallback, &mOnDeviceConnectionFailureCallback);
 }
 

src/app/OperationalDeviceProxy.cpp

@@ -58,18 +58,17 @@ void OperationalDeviceProxy::MoveToState(State aTargetState)
     }
 }
 
-bool OperationalDeviceProxy::CheckAndLoadExistingSession()
+bool OperationalDeviceProxy::AttachToExistingSecureSession()
 {
     VerifyOrReturnError(mState == State::NeedsAddress || mState == State::Initialized, false);
 
-    SessionHolder existingSession;
     ScopedNodeId peerNodeId(mPeerId.GetNodeId(), mFabricInfo->GetFabricIndex());
-
-    mInitParams.sessionManager->FindSecureSessionForNode(mSecureSession, peerNodeId, Transport::SecureSession::Type::kCASE);
-    if (mSecureSession)
+    auto sessionHandle = mInitParams.sessionManager->FindSecureSessionForNode(peerNodeId, Transport::SecureSession::Type::kCASE);
+    if (sessionHandle.HasValue())
     {
-        ChipLogProgress(Controller, "Found an existing secure session to [" ChipLogFormatX64 ":" ChipLogFormatX64 "]!",
+        ChipLogProgress(Controller, "Found an existing secure session to [" ChipLogFormatX64 "-" ChipLogFormatX64 "]!",
                         ChipLogValueX64(mPeerId.GetCompressedFabricId()), ChipLogValueX64(mPeerId.GetNodeId()));
+        mSecureSession.Grab(sessionHandle.Value());
         return true;
     }
 
@@ -96,7 +95,7 @@ CHIP_ERROR OperationalDeviceProxy::Connect(Callback::Callback<OnDeviceConnected>
         break;
 
     case State::NeedsAddress:
-        isConnected = CheckAndLoadExistingSession();
+        isConnected = AttachToExistingSecureSession();
         if (!isConnected)
         {
             err = LookupPeerAddress();
@@ -105,7 +104,7 @@ CHIP_ERROR OperationalDeviceProxy::Connect(Callback::Callback<OnDeviceConnected>
         break;
 
     case State::Initialized:
-        isConnected = CheckAndLoadExistingSession();
+        isConnected = AttachToExistingSecureSession();
         if (!isConnected)
         {
             err = EstablishConnection();
@@ -235,17 +234,24 @@ void OperationalDeviceProxy::EnqueueConnectionCallbacks(Callback::Callback<OnDev
 
 void OperationalDeviceProxy::DequeueConnectionCallbacks(CHIP_ERROR error)
 {
-    Cancelable ready;
-    mConnectionFailure.DequeueAll(ready);
+    Cancelable failureReady, successReady;
+
+    //
+    // Dequeue both failure and success callback lists into temporary stack args before invoking either of them.
+    // We do this since we may not have a valid 'this' pointer anymore upon invoking any of those callbacks
+    // since the callee may destroy this object as part of that callback.
+    //
+    mConnectionFailure.DequeueAll(failureReady);
+    mConnectionSuccess.DequeueAll(successReady);
 
     //
     // If we encountered no error, go ahead and call all success callbacks. Otherwise,
     // call the failure callbacks.
     //
-    while (ready.mNext != &ready)
+    while (failureReady.mNext != &failureReady)
     {
         Callback::Callback<OnDeviceConnectionFailure> * cb =
-            Callback::Callback<OnDeviceConnectionFailure>::FromCancelable(ready.mNext);
+            Callback::Callback<OnDeviceConnectionFailure>::FromCancelable(failureReady.mNext);
 
         cb->Cancel();
 
@@ -255,10 +261,9 @@ void OperationalDeviceProxy::DequeueConnectionCallbacks(CHIP_ERROR error)
         }
     }
 
-    mConnectionSuccess.DequeueAll(ready);
-    while (ready.mNext != &ready)
+    while (successReady.mNext != &successReady)
     {
-        Callback::Callback<OnDeviceConnected> * cb = Callback::Callback<OnDeviceConnected>::FromCancelable(ready.mNext);
+        Callback::Callback<OnDeviceConnected> * cb = Callback::Callback<OnDeviceConnected>::FromCancelable(successReady.mNext);
 
         cb->Cancel();
         if (error == CHIP_NO_ERROR)
@@ -286,7 +291,7 @@ void OperationalDeviceProxy::HandleCASEConnectionFailure(void * context, CASECli
     device->DequeueConnectionCallbacks(error);
 
     //
-    // Do not touch this instance anymore; it might have been destroyed by a failure
+    // Do not touch device instance anymore; it might have been destroyed by a failure
     // callback.
     //
 }
@@ -311,7 +316,7 @@ void OperationalDeviceProxy::HandleCASEConnected(void * context, CASEClient * cl
     }
 
     //
-    // Do not touch this instance anymore; it might have been destroyed by a failure
+    // Do not touch this instance anymore; it might have been destroyed by a
     // callback.
     //
 }

src/app/OperationalDeviceProxy.h

@@ -270,7 +270,7 @@ class DLL_EXPORT OperationalDeviceProxy : public DeviceProxy,
      * Returns true if a valid session was found, false otherwise.
      *
      */
-    bool CheckAndLoadExistingSession();
+    bool AttachToExistingSecureSession();
 
     bool IsSecureConnected() const override { return mState == State::SecureConnected; }
 

src/controller/CHIPDeviceController.h

@@ -146,6 +146,16 @@ class DLL_EXPORT DeviceController : public SessionRecoveryDelegate, public Abstr
      */
     virtual CHIP_ERROR Shutdown();
 
+    SessionManager * SessionMgr()
+    {
+        if (mSystemState)
+        {
+            return mSystemState->SessionMgr();
+        }
+
+        return nullptr;
+    }
+
     CHIP_ERROR GetPeerAddressAndPort(PeerId peerId, Inet::IPAddress & addr, uint16_t & port);
 
     /**

src/transport/SessionManager.cpp

@@ -814,8 +814,7 @@ void SessionManager::ExpiryTimerCallback(System::Layer * layer, void * param)
     mgr->ScheduleExpiryTimer(); // re-schedule the oneshot timer
 }
 
-void SessionManager::FindSecureSessionForNode(SessionHolder & sessionHolder, ScopedNodeId peerNodeId,
-                                              Transport::SecureSession::Type type)
+Optional<SessionHandle> SessionManager::FindSecureSessionForNode(ScopedNodeId peerNodeId, Transport::SecureSession::Type type)
 {
     SecureSession * found = nullptr;
     mSecureSessions.ForEachSession([&peerNodeId, type, &found](auto session) {
@@ -828,12 +827,7 @@ void SessionManager::FindSecureSessionForNode(SessionHolder & sessionHolder, Sco
         return Loop::Continue;
     });
 
-    sessionHolder.Release();
-
-    if (found)
-    {
-        sessionHolder.Grab(SessionHandle(*found));
-    }
+    return found != nullptr ? MakeOptional<SessionHandle>(*found) : Optional<SessionHandle>::Missing();
 }
 
 /**

src/transport/SessionManager.h

@@ -251,10 +251,15 @@ class DLL_EXPORT SessionManager : public TransportMgrDelegate
 
     //
     // Find an existing secure session given a peer's scoped NodeId and a type of session to match against.
-    // If matching against all types of sessions is desired, kUnDefined should be passed into type.
+    // If matching against all types of sessions is desired, kUndefined should be passed into type.
     //
-    void FindSecureSessionForNode(SessionHolder & sessionHolder, ScopedNodeId peerNodeId,
-                                  Transport::SecureSession::Type type = Transport::SecureSession::Type::kUndefined);
+    // If a valid session is found, an Optional<SessionHandle> with the value set to the SessionHandle of the session
+    // is returned. Otherwise, an Optional<SessionHandle> with no value set is returned.
+    //
+    //
+    Optional<SessionHandle>
+    FindSecureSessionForNode(ScopedNodeId peerNodeId,
+                             Transport::SecureSession::Type type = Transport::SecureSession::Type::kUndefined);
 
     using SessionHandleCallback = bool (*)(void * context, SessionHandle & sessionHandle);
     CHIP_ERROR ForEachSessionHandle(void * context, SessionHandleCallback callback);