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.

Author: ksperling-apple

src/controller/CHIPDeviceControllerFactory.cpp

@@ -68,6 +68,8 @@ CHIP_ERROR DeviceControllerFactory::Init(FactoryInitParams params)
     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::Init(FactoryInitParams params)
 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 @@ void DeviceControllerFactory::Shutdown()
 {
     if (mSystemState != nullptr)
     {
+        // ~DeviceControllerSystemState will call Shutdown(),
+        // which in turn ensures that the reference count is 0.
         Platform::Delete(mSystemState);
         mSystemState = nullptr;
     }

src/controller/CHIPDeviceControllerFactory.h

@@ -172,7 +172,9 @@ class DeviceControllerFactory
 
     // 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 @@ class DeviceControllerFactory
     // 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 @@ class DeviceControllerFactory
     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();
 
     //

src/controller/CHIPDeviceControllerSystemState.h

@@ -169,7 +169,7 @@ class DeviceControllerSystemState
     {
         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 @@ class DeviceControllerSystemState
             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; };