Prevent unclean dealloc of SystemState (#33128)
* Prevent unclean dealloc of SystemState
Per current documentation calling Shutdown while any controller is alive is
already prohibited. Also rename the recently-introduced isShutdown method to
isShutDown.
* Add comments and remove the VerifyOrDie()
Upon closer reading Shutdown() was already guaranteed to be called and ensures
that the reference count is 0 at that time, but the documentation was wrong.
diff --git a/src/controller/CHIPDeviceControllerFactory.cpp b/src/controller/CHIPDeviceControllerFactory.cpp
index 248e96c..198d0c4 100644
--- a/src/controller/CHIPDeviceControllerFactory.cpp
+++ b/src/controller/CHIPDeviceControllerFactory.cpp
@@ -68,6 +68,8 @@
mSessionResumptionStorage = params.sessionResumptionStorage;
mEnableServerInteractions = params.enableServerInteractions;
+ // Initialize the system state. Note that it is left in a somewhat
+ // special state where it is initialized, but has a ref count of 0.
CHIP_ERROR err = InitSystemState(params);
return err;
@@ -76,7 +78,7 @@
CHIP_ERROR DeviceControllerFactory::ReinitSystemStateIfNecessary()
{
VerifyOrReturnError(mSystemState != nullptr, CHIP_ERROR_INCORRECT_STATE);
- VerifyOrReturnError(mSystemState->IsShutdown(), CHIP_NO_ERROR);
+ VerifyOrReturnError(mSystemState->IsShutDown(), CHIP_NO_ERROR);
FactoryInitParams params;
params.systemLayer = mSystemState->SystemLayer();
@@ -404,6 +406,8 @@
{
if (mSystemState != nullptr)
{
+ // ~DeviceControllerSystemState will call Shutdown(),
+ // which in turn ensures that the reference count is 0.
Platform::Delete(mSystemState);
mSystemState = nullptr;
}
diff --git a/src/controller/CHIPDeviceControllerFactory.h b/src/controller/CHIPDeviceControllerFactory.h
index d4e50e9..474357e 100644
--- a/src/controller/CHIPDeviceControllerFactory.h
+++ b/src/controller/CHIPDeviceControllerFactory.h
@@ -172,7 +172,9 @@
// Shuts down matter and frees the system state.
//
- // Must not be called while any controllers are alive.
+ // Must not be called while any controllers are alive, or while any calls
+ // to RetainSystemState or EnsureAndRetainSystemState have not been balanced
+ // by a call to ReleaseSystemState.
void Shutdown();
CHIP_ERROR SetupController(SetupParams params, DeviceController & controller);
@@ -195,7 +197,8 @@
// all device controllers have ceased to exist. To avoid that, this method has been
// created to permit retention of the underlying system state.
//
- // NB: The system state will still be freed in Shutdown() regardless of this call.
+ // Calls to this method must be balanced by calling ReleaseSystemState before Shutdown.
+ //
void RetainSystemState();
//
@@ -210,6 +213,7 @@
bool ReleaseSystemState();
// Like RetainSystemState(), but will re-initialize the system state first if necessary.
+ // Calls to this method must be balanced by calling ReleaseSystemState before Shutdown.
CHIP_ERROR EnsureAndRetainSystemState();
//
diff --git a/src/controller/CHIPDeviceControllerSystemState.h b/src/controller/CHIPDeviceControllerSystemState.h
index 8a3be25..389bb55 100644
--- a/src/controller/CHIPDeviceControllerSystemState.h
+++ b/src/controller/CHIPDeviceControllerSystemState.h
@@ -169,7 +169,7 @@
{
auto count = mRefCount++;
VerifyOrDie(count < std::numeric_limits<decltype(count)>::max()); // overflow
- VerifyOrDie(!IsShutdown()); // avoid zombie
+ VerifyOrDie(!IsShutDown()); // avoid zombie
return this;
};
@@ -197,7 +197,7 @@
mGroupDataProvider != nullptr && mReportScheduler != nullptr && mTimerDelegate != nullptr &&
mSessionKeystore != nullptr && mSessionResumptionStorage != nullptr && mBDXTransferServer != nullptr;
};
- bool IsShutdown() { return mHaveShutDown; }
+ bool IsShutDown() { return mHaveShutDown; }
System::Layer * SystemLayer() const { return mSystemLayer; };
Inet::EndPointManager<Inet::TCPEndPoint> * TCPEndPointManager() const { return mTCPEndPointManager; };
