fix: drop poll timer to avoid crash after TransferFacilitator destruc…

…tion (project-chip#34538)
joonhaengHeo · Sep 11, 2024 · ac289ed · ac289ed
1 parent 6c046cb
commit ac289ed
Show file tree

Hide file tree

Showing 4 changed files with 23 additions and 39 deletions.
diff --git a/examples/ota-provider-app/esp32/main/BdxOtaSender.cpp b/examples/ota-provider-app/esp32/main/BdxOtaSender.cpp
@@ -174,7 +174,6 @@ void BdxOtaSender::HandleTransferSessionOutput(TransferSession::OutputEvent & ev
         {
             ChipLogError(BDX, "onTransferComplete Callback not set");
         }
-        mStopPolling = true; // Stop polling the TransferSession only after receiving BlockAckEOF
         Reset();
         break;
     case TransferSession::OutputEventType::kStatusReceived:
@@ -228,7 +227,7 @@ void BdxOtaSender::Reset()
 {
     mFabricIndex.ClearValue();
     mNodeId.ClearValue();
-    mTransfer.Reset();
+    ResetTransfer();
     if (mExchangeCtx != nullptr)
     {
         mExchangeCtx->Close();

diff --git a/examples/ota-provider-app/ota-provider-common/BdxOtaSender.cpp b/examples/ota-provider-app/ota-provider-common/BdxOtaSender.cpp
@@ -180,7 +180,6 @@ void BdxOtaSender::HandleTransferSessionOutput(TransferSession::OutputEvent & ev
         break;
     case TransferSession::OutputEventType::kAckEOFReceived:
         ChipLogDetail(BDX, "Transfer completed, got AckEOF");
-        mStopPolling = true; // Stop polling the TransferSession only after receiving BlockAckEOF
         Reset();
         break;
     case TransferSession::OutputEventType::kStatusReceived:
@@ -212,7 +211,7 @@ void BdxOtaSender::Reset()
 {
     mFabricIndex.ClearValue();
     mNodeId.ClearValue();
-    Responder::ResetTransfer();
+    ResetTransfer();
     if (mExchangeCtx != nullptr)
     {
         mExchangeCtx->Close();

diff --git a/src/protocols/bdx/TransferFacilitator.cpp b/src/protocols/bdx/TransferFacilitator.cpp
@@ -32,6 +32,20 @@ namespace bdx {
 constexpr System::Clock::Timeout TransferFacilitator::kDefaultPollFreq;
 constexpr System::Clock::Timeout TransferFacilitator::kImmediatePollDelay;
 
+TransferFacilitator::~TransferFacilitator()
+{
+    ResetTransfer();
+}
+
+void TransferFacilitator::ResetTransfer()
+{
+    mTransfer.Reset();
+    ChipLogProgress(BDX, "Stop polling for messages");
+
+    VerifyOrReturn(mSystemLayer != nullptr);
+    mSystemLayer->CancelTimer(PollTimerHandler, this);
+}
+
 CHIP_ERROR TransferFacilitator::OnMessageReceived(chip::Messaging::ExchangeContext * ec, const chip::PayloadHeader & payloadHeader,
                                                   chip::System::PacketBufferHandle && payload)
 {
@@ -78,15 +92,7 @@ void TransferFacilitator::PollForOutput()
     HandleTransferSessionOutput(outEvent);
 
     VerifyOrReturn(mSystemLayer != nullptr, ChipLogError(BDX, "%s mSystemLayer is null", __FUNCTION__));
-    if (!mStopPolling)
-    {
-        mSystemLayer->StartTimer(mPollFreq, PollTimerHandler, this);
-    }
-    else
-    {
-        mSystemLayer->CancelTimer(PollTimerHandler, this);
-        mStopPolling = false;
-    }
+    mSystemLayer->StartTimer(mPollFreq, PollTimerHandler, this);
 }
 
 void TransferFacilitator::ScheduleImmediatePoll()
@@ -106,18 +112,10 @@ CHIP_ERROR Responder::PrepareForTransfer(System::Layer * layer, TransferRole rol
     ReturnErrorOnFailure(mTransfer.WaitForTransfer(role, xferControlOpts, maxBlockSize, timeout));
 
     ChipLogProgress(BDX, "Start polling for messages");
-    mStopPolling = false;
     mSystemLayer->StartTimer(mPollFreq, PollTimerHandler, this);
     return CHIP_NO_ERROR;
 }
 
-void Responder::ResetTransfer()
-{
-    mTransfer.Reset();
-    ChipLogProgress(BDX, "Stop polling for messages");
-    mStopPolling = true;
-}
-
 CHIP_ERROR Initiator::InitiateTransfer(System::Layer * layer, TransferRole role, const TransferSession::TransferInitData & initData,
                                        System::Clock::Timeout timeout, System::Clock::Timeout pollFreq)
 {
@@ -132,12 +130,5 @@ CHIP_ERROR Initiator::InitiateTransfer(System::Layer * layer, TransferRole role,
     return CHIP_NO_ERROR;
 }
 
-void Initiator::ResetTransfer()
-{
-    mTransfer.Reset();
-    ChipLogProgress(BDX, "Stop polling for messages");
-    mStopPolling = true;
-}
-
 } // namespace bdx
 } // namespace chip
diff --git a/src/protocols/bdx/TransferFacilitator.h b/src/protocols/bdx/TransferFacilitator.h
@@ -45,7 +45,12 @@ class TransferFacilitator : public Messaging::ExchangeDelegate, public Messaging
 {
 public:
     TransferFacilitator() : mExchangeCtx(nullptr), mSystemLayer(nullptr), mPollFreq(kDefaultPollFreq) {}
-    ~TransferFacilitator() override = default;
+    ~TransferFacilitator() override;
+
+    /**
+     * Calls reset on the TransferSession object and stops the poll timer.
+     */
+    void ResetTransfer();
 
 private:
     //// UnsolicitedMessageHandler Implementation ////
@@ -96,7 +101,6 @@ class TransferFacilitator : public Messaging::ExchangeDelegate, public Messaging
     System::Clock::Timeout mPollFreq;
     static constexpr System::Clock::Timeout kDefaultPollFreq    = System::Clock::Milliseconds32(500);
     static constexpr System::Clock::Timeout kImmediatePollDelay = System::Clock::Milliseconds32(1);
-    bool mStopPolling                                           = false;
 };
 
 /**
@@ -121,11 +125,6 @@ class Responder : public TransferFacilitator
     CHIP_ERROR PrepareForTransfer(System::Layer * layer, TransferRole role, BitFlags<TransferControlFlags> xferControlOpts,
                                   uint16_t maxBlockSize, System::Clock::Timeout timeout,
                                   System::Clock::Timeout pollFreq = TransferFacilitator::kDefaultPollFreq);
-
-    /**
-     * Calls reset on the TransferSession object and stops the poll timer.
-     */
-    void ResetTransfer();
 };
 
 /**
@@ -150,10 +149,6 @@ class Initiator : public TransferFacilitator
     CHIP_ERROR InitiateTransfer(System::Layer * layer, TransferRole role, const TransferSession::TransferInitData & initData,
                                 System::Clock::Timeout timeout,
                                 System::Clock::Timeout pollFreq = TransferFacilitator::kDefaultPollFreq);
-    /**
-     * Calls reset on the TransferSession object and stops the poll timer.
-     */
-    void ResetTransfer();
 };
 
 } // namespace bdx