Fix java im sub crash (#25505)

* Fix java im sub crash

* address comments

When onDone is triggered from ShutdownSubscription API in android ui
thread, RAII unlock in OnDone could cause context switch, then eventloop in matter would
acquire the lock in matter thread, complete the work, switch back with
the lock acquired from matter thread, then it delete readClient, and
session, where it crash when it find the lock's thread id is different
from ui thread.

The fix is to move the stackUnlock after readClient delete operation.

enable lock error detection

examples/android/CHIPTool/app/src/main/java/com/google/chip/chiptool/clusterclient/OnOffClientFragment.kt

@@ -129,8 +129,8 @@ class OnOffClientFragment : Fragment() {
         showReportMessage(message)
       }
 
-      override fun onSubscriptionEstablished() {
-        val message = "Subscription for on/off established"
+      override fun onSubscriptionEstablished(subscriptionId: Long) {
+        val message = "Subscription for on/off established with subscriptionId: $subscriptionId"
         Log.v(TAG, message)
         showMessage(message)
       }

examples/android/CHIPTool/app/src/main/java/com/google/chip/chiptool/clusterclient/SensorClientFragment.kt

@@ -181,7 +181,7 @@ class SensorClientFragment : Fragment() {
       return
 
     try {
-      ChipClient.getDeviceController(requireContext()).shutdownSubscriptions(subscribedDevicePtr)
+      ChipClient.getDeviceController(requireContext()).shutdownSubscriptions()
       subscribedDevicePtr = 0
     } catch (ex: Exception) {
       showMessage(R.string.sensor_client_unsubscribe_error_text, ex.toString())

examples/chip-tool/commands/clusters/SubscriptionsCommands.h

@@ -30,7 +30,7 @@ class ShutdownSubscription : public CHIPCommand
         CHIPCommand("shutdown-one", credsIssuerConfig,
                     "Shut down a single subscription, identified by its subscription id and target node id.")
     {
-        AddArgument("subscription-id", 0, UINT64_MAX, &mSubscriptionId);
+        AddArgument("subscription-id", 0, UINT32_MAX, &mSubscriptionId);
         AddArgument("node-id", 0, UINT64_MAX, &mNodeId,
                     "The node id, scoped to the commissioner name the command is running under.");
     }

examples/java-matter-controller/BUILD.gn

@@ -69,6 +69,7 @@ kotlin_binary("java-matter-controller") {
     "java/src/com/matter/controller/commands/pairing/PairOnNetworkInstanceNameCommand.kt",
     "java/src/com/matter/controller/commands/pairing/PairOnNetworkLongCommand.kt",
     "java/src/com/matter/controller/commands/pairing/PairOnNetworkLongImInvokeCommand.kt",
+    "java/src/com/matter/controller/commands/pairing/PairOnNetworkLongImSubscribeCommand.kt",
     "java/src/com/matter/controller/commands/pairing/PairOnNetworkLongImWriteCommand.kt",
     "java/src/com/matter/controller/commands/pairing/PairOnNetworkShortCommand.kt",
     "java/src/com/matter/controller/commands/pairing/PairOnNetworkVendorCommand.kt",

examples/java-matter-controller/args.gni

@@ -23,4 +23,4 @@ chip_system_project_config_include = "<SystemProjectConfig.h>"
 chip_project_config_include_dirs =
     [ "${chip_root}/examples/java-matter-controller/include" ]
 chip_project_config_include_dirs += [ "${chip_root}/config/standalone" ]
-chip_stack_lock_tracking = "none"
+chip_stack_lock_tracking = "fatal"

examples/java-matter-controller/java/src/com/matter/controller/Main.kt

@@ -62,6 +62,7 @@ private fun getImCommands(
   credentialsIssuer: CredentialsIssuer
 ): List<Command> {
   return listOf(
+    PairOnNetworkLongImSubscribeCommand(controller, credentialsIssuer),
     PairOnNetworkLongImWriteCommand(controller, credentialsIssuer),
     PairOnNetworkLongImInvokeCommand(controller, credentialsIssuer),
   )

examples/java-matter-controller/java/src/com/matter/controller/commands/pairing/PairOnNetworkLongImSubscribeCommand.kt

@@ -0,0 +1,98 @@
+package com.matter.controller.commands.pairing
+
+import chip.devicecontroller.ChipDeviceController
+import chip.devicecontroller.GetConnectedDeviceCallbackJni.GetConnectedDeviceCallback
+import chip.devicecontroller.ReportCallback
+import chip.devicecontroller.SubscriptionEstablishedCallback
+import chip.devicecontroller.ResubscriptionAttemptCallback
+import chip.devicecontroller.model.NodeState
+import chip.devicecontroller.model.ChipAttributePath
+import chip.devicecontroller.model.ChipEventPath
+import com.matter.controller.commands.common.CredentialsIssuer
+import java.util.Collections
+import java.util.logging.Level
+import java.util.logging.Logger
+
+class PairOnNetworkLongImSubscribeCommand(
+  controller: ChipDeviceController, credsIssue: CredentialsIssuer?
+) : PairingCommand(
+  controller,
+  "onnetwork-long-im-subscribe",
+  credsIssue,
+  PairingModeType.ON_NETWORK,
+  PairingNetworkType.NONE,
+  DiscoveryFilterType.LONG_DISCRIMINATOR
+) {
+  private var devicePointer: Long = 0
+  private var subscriptionId: Long = 0
+
+  private inner class InternalReportCallback : ReportCallback {
+    override fun onError(attributePath: ChipAttributePath?, eventPath: ChipEventPath?, e: Exception) {
+      logger.log(Level.INFO, "Subscribe receive onError")
+      setFailure("write failure")
+    }
+
+    override fun onReport(nodeState: NodeState) {
+      logger.log(Level.INFO, "Subscribe receve onReport")
+    }
+  }
+
+  private inner class InternalGetConnectedDeviceCallback : GetConnectedDeviceCallback {
+    override fun onDeviceConnected(devicePointer: Long) {
+      this@PairOnNetworkLongImSubscribeCommand.devicePointer = devicePointer
+      logger.log(Level.INFO, "onDeviceConnected")
+    }
+
+    override fun onConnectionFailure(nodeId: Long, error: Exception?) {
+      logger.log(Level.INFO, "onConnectionFailure")
+    }
+  }
+
+  private inner class InternalSubscriptionEstablishedCallback : SubscriptionEstablishedCallback {
+    override fun onSubscriptionEstablished(subscriptionId: Long) {
+      this@PairOnNetworkLongImSubscribeCommand.subscriptionId = subscriptionId
+      logger.log(Level.INFO, "onSubscriptionEstablished with Id" + subscriptionId)
+      setSuccess()
+    }
+  }
+
+  private inner class InternalResubscriptionAttemptCallback : ResubscriptionAttemptCallback {
+    override fun onResubscriptionAttempt(terminationCause: Int, nextResubscribeIntervalMsec: Int) {
+      logger.log(Level.INFO, "ResubscriptionAttemptCallback");
+    }
+  }
+
+  override fun runCommand() {
+    val attributePathList = listOf(ChipAttributePath.newInstance(
+        /* endpointId= */ 0, CLUSTER_ID_BASIC, ATTR_ID_LOCAL_CONFIG_DISABLED))
+
+    currentCommissioner()
+      .pairDeviceWithAddress(
+        getNodeId(),
+        getRemoteAddr().getHostAddress(),
+        MATTER_PORT,
+        getDiscriminator(),
+        getSetupPINCode(),
+        null
+      )
+    currentCommissioner().setCompletionListener(this)
+    waitCompleteMs(getTimeoutMillis())
+    currentCommissioner()
+      .getConnectedDevicePointer(getNodeId(), InternalGetConnectedDeviceCallback())
+    clear()
+    currentCommissioner()
+      .subscribeToPath(InternalSubscriptionEstablishedCallback(), InternalResubscriptionAttemptCallback(), InternalReportCallback(), devicePointer, attributePathList, Collections.emptyList(), 0, 5, false, false)
+    waitCompleteMs(getTimeoutMillis())
+    currentCommissioner().shutdownSubscriptions(currentCommissioner().getFabricIndex(), getNodeId(), subscriptionId);
+  }
+
+  companion object {
+    private val logger = Logger.getLogger(
+      PairOnNetworkLongImSubscribeCommand::class.java.name
+    )
+
+    private const val MATTER_PORT = 5540
+    private const val CLUSTER_ID_BASIC = 0x0028L
+    private const val ATTR_ID_LOCAL_CONFIG_DISABLED = 16L
+  }
+}

examples/tv-casting-app/linux/CastingUtils.cpp

@@ -160,7 +160,7 @@ void OnCurrentStateReadResponseFailure(void * context, CHIP_ERROR err)
     ChipLogProgress(AppServer, "OnCurrentStateReadResponseFailure called with %" CHIP_ERROR_FORMAT, err.Format());
 }
 
-void OnCurrentStateSubscriptionEstablished(void * context)
+void OnCurrentStateSubscriptionEstablished(void * context, SubscriptionId aSubscriptionId)
 {
     ChipLogProgress(AppServer, "OnCurrentStateSubscriptionEstablished called");
     if (context != nullptr)

src/app/DeviceProxy.h

@@ -46,8 +46,6 @@ class DLL_EXPORT DeviceProxy
 
     virtual NodeId GetDeviceId() const = 0;
 
-    virtual void ShutdownSubscriptions() = 0;
-
     virtual CHIP_ERROR SendCommands(app::CommandSender * commandObj, chip::Optional<System::Clock::Timeout> timeout = NullOptional);
 
     virtual Messaging::ExchangeManager * GetExchangeManager() const = 0;

src/app/InteractionModelEngine.cpp

@@ -253,30 +253,37 @@ uint32_t InteractionModelEngine::GetNumActiveWriteHandlers() const
 
 CHIP_ERROR InteractionModelEngine::ShutdownSubscription(const ScopedNodeId & aPeerNodeId, SubscriptionId aSubscriptionId)
 {
-    for (auto * readClient = mpActiveReadClientList; readClient != nullptr; readClient = readClient->GetNextClient())
+    assertChipStackLockedByCurrentThread();
+    for (auto * readClient = mpActiveReadClientList; readClient != nullptr;)
     {
+        // Grab the next client now, because we might be about to delete readClient.
+        auto * nextClient = readClient->GetNextClient();
         if (readClient->IsSubscriptionType() && readClient->IsMatchingSubscriptionId(aSubscriptionId) &&
             readClient->GetFabricIndex() == aPeerNodeId.GetFabricIndex() && readClient->GetPeerNodeId() == aPeerNodeId.GetNodeId())
         {
             readClient->Close(CHIP_NO_ERROR);
             return CHIP_NO_ERROR;
         }
+        readClient = nextClient;
     }
 
     return CHIP_ERROR_KEY_NOT_FOUND;
 }
 
 void InteractionModelEngine::ShutdownSubscriptions(FabricIndex aFabricIndex, NodeId aPeerNodeId)
 {
+    assertChipStackLockedByCurrentThread();
     ShutdownMatchingSubscriptions(MakeOptional(aFabricIndex), MakeOptional(aPeerNodeId));
 }
 void InteractionModelEngine::ShutdownSubscriptions(FabricIndex aFabricIndex)
 {
+    assertChipStackLockedByCurrentThread();
     ShutdownMatchingSubscriptions(MakeOptional(aFabricIndex));
 }
 
 void InteractionModelEngine::ShutdownAllSubscriptions()
 {
+    assertChipStackLockedByCurrentThread();
     ShutdownMatchingSubscriptions();
 }
 

src/app/OperationalSessionSetup.h

@@ -84,8 +84,6 @@ class OperationalDeviceProxy : public DeviceProxy
     {}
     OperationalDeviceProxy() {}
 
-    // Recommended to use InteractionModelEngine::ShutdownSubscriptions directly.
-    void ShutdownSubscriptions() override { VerifyOrDie(false); } // Currently not implemented.
     void Disconnect() override
     {
         if (IsSecureConnected())

src/controller/CHIPCluster.h

@@ -50,7 +50,7 @@ template <typename T>
 using ReadResponseSuccessCallback     = void (*)(void * context, T responseData);
 using ReadResponseFailureCallback     = void (*)(void * context, CHIP_ERROR err);
 using ReadDoneCallback                = void (*)(void * context);
-using SubscriptionEstablishedCallback = void (*)(void * context);
+using SubscriptionEstablishedCallback = void (*)(void * context, SubscriptionId subscriptionId);
 using ResubscriptionAttemptCallback   = void (*)(void * context, CHIP_ERROR aError, uint32_t aNextResubscribeIntervalMsec);
 using SubscriptionOnDoneCallback      = std::function<void(void)>;
 
@@ -298,10 +298,11 @@ class DLL_EXPORT ClusterBase
             }
         };
 
-        auto onSubscriptionEstablishedCb = [context, subscriptionEstablishedCb](const app::ReadClient & readClient) {
+        auto onSubscriptionEstablishedCb = [context, subscriptionEstablishedCb](const app::ReadClient & readClient,
+                                                                                SubscriptionId subscriptionId) {
             if (subscriptionEstablishedCb != nullptr)
             {
-                subscriptionEstablishedCb(context);
+                subscriptionEstablishedCb(context, subscriptionId);
             }
         };
 
@@ -377,10 +378,11 @@ class DLL_EXPORT ClusterBase
             }
         };
 
-        auto onSubscriptionEstablishedCb = [context, subscriptionEstablishedCb](const app::ReadClient & readClient) {
+        auto onSubscriptionEstablishedCb = [context, subscriptionEstablishedCb](const app::ReadClient & readClient,
+                                                                                SubscriptionId subscriptionId) {
             if (subscriptionEstablishedCb != nullptr)
             {
-                subscriptionEstablishedCb(context);
+                subscriptionEstablishedCb(context, subscriptionId);
             }
         };
 

src/controller/CommissioneeDeviceProxy.h

@@ -133,7 +133,6 @@ class CommissioneeDeviceProxy : public DeviceProxy, public SessionDelegate
     bool IsSessionSetupInProgress() const { return mState == ConnectionState::Connecting; }
 
     NodeId GetDeviceId() const override { return mPeerId.GetNodeId(); }
-    void ShutdownSubscriptions() override {}
     PeerId GetPeerId() const { return mPeerId; }
     CHIP_ERROR SetPeerId(ByteSpan rcac, ByteSpan noc) override;
     const Transport::PeerAddress & GetPeerAddress() const { return mDeviceAddress; }

src/controller/TypedReadCallback.h

@@ -52,7 +52,8 @@ class TypedReadAttributeCallback final : public app::ReadClient::Callback
         std::function<void(const app::ConcreteDataAttributePath & aPath, const DecodableAttributeType & aData)>;
     using OnErrorCallbackType = std::function<void(const app::ConcreteDataAttributePath * aPath, CHIP_ERROR aError)>;
     using OnDoneCallbackType  = std::function<void(TypedReadAttributeCallback * callback)>;
-    using OnSubscriptionEstablishedCallbackType = std::function<void(const app::ReadClient & readClient)>;
+    using OnSubscriptionEstablishedCallbackType =
+        std::function<void(const app::ReadClient & readClient, SubscriptionId aSubscriptionId)>;
     using OnResubscriptionAttemptCallbackType =
         std::function<void(const app::ReadClient & readClient, CHIP_ERROR aError, uint32_t aNextResubscribeIntervalMsec)>;
     TypedReadAttributeCallback(ClusterId aClusterId, AttributeId aAttributeId, OnSuccessCallbackType aOnSuccess,
@@ -127,7 +128,7 @@ class TypedReadAttributeCallback final : public app::ReadClient::Callback
     {
         if (mOnSubscriptionEstablished)
         {
-            mOnSubscriptionEstablished(*mReadClient.get());
+            mOnSubscriptionEstablished(*mReadClient.get(), aSubscriptionId);
         }
     }
 
@@ -175,7 +176,8 @@ class TypedReadEventCallback final : public app::ReadClient::Callback
     using OnSuccessCallbackType = std::function<void(const app::EventHeader & aEventHeader, const DecodableEventType & aData)>;
     using OnErrorCallbackType   = std::function<void(const app::EventHeader * apEventHeader, CHIP_ERROR aError)>;
     using OnDoneCallbackType    = std::function<void(app::ReadClient * apReadClient)>;
-    using OnSubscriptionEstablishedCallbackType = std::function<void(const app::ReadClient & aReadClient)>;
+    using OnSubscriptionEstablishedCallbackType =
+        std::function<void(const app::ReadClient & aReadClient, SubscriptionId aSubscriptionId)>;
     using OnResubscriptionAttemptCallbackType =
         std::function<void(const app::ReadClient & aReadClient, CHIP_ERROR aError, uint32_t aNextResubscribeIntervalMsec)>;
 
@@ -260,7 +262,7 @@ class TypedReadEventCallback final : public app::ReadClient::Callback
     {
         if (mOnSubscriptionEstablished)
         {
-            mOnSubscriptionEstablished(*mReadClient.get());
+            mOnSubscriptionEstablished(*mReadClient.get(), aSubscriptionId);
         }
     }
 

src/controller/java/AndroidCallbacks.cpp

@@ -15,7 +15,6 @@
  *    limitations under the License.
  */
 #include "AndroidCallbacks.h"
-
 #include <controller/java/AndroidClusterExceptions.h>
 #include <controller/java/CHIPAttributeTLVValueDecoder.h>
 #include <controller/java/CHIPEventTLVValueDecoder.h>
@@ -511,6 +510,12 @@ void ReportCallback::OnDone(app::ReadClient *)
     err = JniReferences::GetInstance().FindMethod(env, mReportCallbackRef, "onDone", "()V", &onDoneMethod);
     VerifyOrReturn(err == CHIP_NO_ERROR, ChipLogError(Controller, "Could not find onDone method"));
 
+    if (mReadClient != nullptr)
+    {
+        Platform::Delete(mReadClient);
+    }
+    mReadClient = nullptr;
+
     DeviceLayer::StackUnlock unlock;
     env->CallVoidMethod(mReportCallbackRef, onDoneMethod);
     VerifyOrReturn(!env->ExceptionCheck(), env->ExceptionDescribe());
@@ -519,7 +524,8 @@ void ReportCallback::OnDone(app::ReadClient *)
 
 void ReportCallback::OnSubscriptionEstablished(SubscriptionId aSubscriptionId)
 {
-    JniReferences::GetInstance().CallSubscriptionEstablished(mSubscriptionEstablishedCallbackRef);
+    DeviceLayer::StackUnlock unlock;
+    JniReferences::GetInstance().CallSubscriptionEstablished(mSubscriptionEstablishedCallbackRef, aSubscriptionId);
 }
 
 CHIP_ERROR ReportCallback::OnResubscriptionNeeded(app::ReadClient * apReadClient, CHIP_ERROR aTerminationCause)
@@ -771,7 +777,8 @@ void ReportEventCallback::OnDone(app::ReadClient *)
 
 void ReportEventCallback::OnSubscriptionEstablished(SubscriptionId aSubscriptionId)
 {
-    JniReferences::GetInstance().CallSubscriptionEstablished(mSubscriptionEstablishedCallbackRef);
+    chip::DeviceLayer::StackUnlock unlock;
+    JniReferences::GetInstance().CallSubscriptionEstablished(mSubscriptionEstablishedCallbackRef, aSubscriptionId);
 }
 
 CHIP_ERROR ReportEventCallback::OnResubscriptionNeeded(app::ReadClient * apReadClient, CHIP_ERROR aTerminationCause)