Fix the ReadClient destructor to call OnDeallocatePaths. (#21660)

This is actually somewhat dangerous, because there is a good chance ~ReadClient
is running from under the destructor of the ReadClient::Callback and we are
invoking a virtual method on the callback.  But this is the only way to ensure
we don't leak.  ReadClient::Callback implementations need to be able to handle
this.

Fixes #21658

src/app/ReadClient.cpp

@@ -68,20 +68,29 @@ void ReadClient::ClearActiveSubscriptionState()
 
 void ReadClient::StopResubscription()
 {
-
     CancelLivenessCheckTimer();
     CancelResubscribeTimer();
-    mpCallback.OnDeallocatePaths(std::move(mReadPrepareParams));
+
+    // Only deallocate the paths if they are not already deallocated.
+    if (mReadPrepareParams.mpAttributePathParamsList != nullptr || mReadPrepareParams.mpEventPathParamsList != nullptr ||
+        mReadPrepareParams.mpDataVersionFilterList != nullptr)
+    {
+        mpCallback.OnDeallocatePaths(std::move(mReadPrepareParams));
+        // Make sure we will never try to free those pointers again.
+        mReadPrepareParams.mpAttributePathParamsList    = nullptr;
+        mReadPrepareParams.mAttributePathParamsListSize = 0;
+        mReadPrepareParams.mpEventPathParamsList        = nullptr;
+        mReadPrepareParams.mEventPathParamsListSize     = 0;
+        mReadPrepareParams.mpDataVersionFilterList      = nullptr;
+        mReadPrepareParams.mDataVersionFilterListSize   = 0;
+    }
 }
 
 ReadClient::~ReadClient()
 {
+    Close(CHIP_NO_ERROR, /* allowResubscription = */ false, /* allowOnDone = */ false);
     if (IsSubscriptionType())
     {
-        CancelLivenessCheckTimer();
-        CancelResubscribeTimer();
-
-        //
         // Only remove ourselves from the engine's tracker list if we still continue to have a valid pointer to it.
         // This won't be the case if the engine shut down before this destructor was called (in which case, mpImEngine
         // will point to null)
@@ -141,7 +150,7 @@ CHIP_ERROR ReadClient::ScheduleResubscription(uint32_t aTimeTillNextResubscripti
     return CHIP_NO_ERROR;
 }
 
-void ReadClient::Close(CHIP_ERROR aError, bool allowResubscription)
+void ReadClient::Close(CHIP_ERROR aError, bool allowResubscription, bool allowOnDone)
 {
     if (IsReadType())
     {
@@ -181,7 +190,10 @@ void ReadClient::Close(CHIP_ERROR aError, bool allowResubscription)
         StopResubscription();
     }
 
-    mpCallback.OnDone(this);
+    if (allowOnDone)
+    {
+        mpCallback.OnDone(this);
+    }
 }
 
 const char * ReadClient::GetStateStr() const

src/app/ReadClient.h

@@ -200,8 +200,9 @@ class ReadClient : public Messaging::ExchangeDelegate
          * This function is invoked when using SendAutoResubscribeRequest, where the ReadClient was configured to auto re-subscribe
          * and the ReadPrepareParams was moved into this client for management. This will have to be free'ed appropriately by the
          * application. If SendAutoResubscribeRequest fails, this function will be called before it returns the failure. If
-         * SendAutoResubscribeRequest succeeds, this function will be called immediately before calling OnDone. If
-         * SendAutoResubscribeRequest is not called, this function will not be called.
+         * SendAutoResubscribeRequest succeeds, this function will be called immediately before calling OnDone, or
+         * when the ReadClient is destroyed, if that happens before OnDone. If  SendAutoResubscribeRequest is not called,
+         * this function will not be called.
          */
         virtual void OnDeallocatePaths(ReadPrepareParams && aReadPrepareParams) {}
 
@@ -474,8 +475,9 @@ class ReadClient : public Messaging::ExchangeDelegate
      * AND if this ReadClient instance is tracking a subscription AND the applications decides to do so
      * in their implementation of Callback::OnResubscriptionNeeded().
      *
+     * If allowOnDone is false, will not call OnDone.
      */
-    void Close(CHIP_ERROR aError, bool allowResubscription = true);
+    void Close(CHIP_ERROR aError, bool allowResubscription = true, bool allowOnDone = true);
 
     void StopResubscription();
     void ClearActiveSubscriptionState();

src/app/tests/TestReadInteraction.cpp

@@ -1763,6 +1763,7 @@ void TestReadInteraction::TestSubscribeWildcard(nlTestSuite * apSuite, void * ap
 
         delegate.mGotReport = false;
 
+        attributePathParams.release();
         err = readClient.SendAutoResubscribeRequest(std::move(readPrepareParams));
         NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
 
@@ -1868,6 +1869,7 @@ void TestReadInteraction::TestSubscribePartialOverlap(nlTestSuite * apSuite, voi
 
         delegate.mGotReport = false;
 
+        attributePathParams.release();
         err = readClient.SendAutoResubscribeRequest(std::move(readPrepareParams));
         NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
 
@@ -1944,6 +1946,7 @@ void TestReadInteraction::TestSubscribeSetDirtyFullyOverlap(nlTestSuite * apSuit
 
         delegate.mGotReport = false;
 
+        attributePathParams.release();
         err = readClient.SendAutoResubscribeRequest(std::move(readPrepareParams));
         NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
 

src/controller/TypedReadCallback.h

@@ -65,6 +65,13 @@ class TypedReadAttributeCallback final : public app::ReadClient::Callback
         mBufferedReadAdapter(*this)
     {}
 
+    ~TypedReadAttributeCallback()
+    {
+        // Ensure we release the ReadClient before we tear down anything else,
+        // so it can call our OnDeallocatePaths properly.
+        mReadClient = nullptr;
+    }
+
     app::BufferedReadCallback & GetBufferedCallback() { return mBufferedReadAdapter; }
 
     void AdoptReadClient(Platform::UniquePtr<app::ReadClient> aReadClient) { mReadClient = std::move(aReadClient); }
@@ -180,6 +187,13 @@ class TypedReadEventCallback final : public app::ReadClient::Callback
         mOnResubscriptionAttempt(aOnResubscriptionAttempt)
     {}
 
+    ~TypedReadEventCallback()
+    {
+        // Ensure we release the ReadClient before we tear down anything else,
+        // so it can call our OnDeallocatePaths properly.
+        mReadClient = nullptr;
+    }
+
     void AdoptReadClient(Platform::UniquePtr<app::ReadClient> aReadClient) { mReadClient = std::move(aReadClient); }
 
 private:

src/darwin/Framework/CHIP/MTRBaseDevice.mm

@@ -300,6 +300,13 @@ - (void)invalidateCASESession
     {
     }
 
+    ~SubscriptionCallback()
+    {
+        // Ensure we release the ReadClient before we tear down anything else,
+        // so it can call our OnDeallocatePaths properly.
+        mReadClient = nullptr;
+    }
+
     BufferedReadCallback & GetBufferedCallback() { return mBufferedReadAdapter; }
 
     // We need to exist to get a ReadClient, so can't take this as a constructor argument.
@@ -768,6 +775,13 @@ CHIP_ERROR Encode(chip::TLV::TLVWriter & writer, chip::TLV::Tag tag) const
     {
     }
 
+    ~BufferedReadAttributeCallback()
+    {
+        // Ensure we release the ReadClient before we tear down anything else,
+        // so it can call our OnDeallocatePaths properly.
+        mReadClient = nullptr;
+    }
+
     app::BufferedReadCallback & GetBufferedCallback() { return mBufferedReadAdapter; }
 
     void AdoptReadClient(Platform::UniquePtr<app::ReadClient> aReadClient) { mReadClient = std::move(aReadClient); }

src/darwin/Framework/CHIP/MTRDevice.mm

@@ -118,6 +118,13 @@ - (id)strongObject
     {
     }
 
+    ~SubscriptionCallback()
+    {
+        // Ensure we release the ReadClient before we tear down anything else,
+        // so it can call our OnDeallocatePaths properly.
+        mReadClient = nullptr;
+    }
+
     BufferedReadCallback & GetBufferedCallback() { return mBufferedReadAdapter; }
 
     // We need to exist to get a ReadClient, so can't take this as a constructor argument.