[Android] Fix memory leakage in AndroidLogDownloadFromNode Class (project-chip#36742)

* Fix memory leakage in android diag Class

* Add terminate sleep time

* Update VerifyOrExit

* Update AndroidLogDownloadFromNode.cpp

---------

Co-authored-by: yunhanw-google <yunhanw@google.com>

Author: 2 people

.github/workflows/java-tests.yaml

@@ -260,9 +260,6 @@ jobs:
                      --factoryreset \
                   '
             - name: Run Pairing Onnetwork and get diagnostic log Test
-              # TODO: test below is disabled because it seems flaky (crashes on pool not being empty on app exit)
-              #       See: https://github.com/project-chip/connectedhomeip/issues/36734
-              if: false
               run: |
                   scripts/run_in_python_env.sh out/venv \
                   './scripts/tests/run_java_test.py \

examples/java-matter-controller/java/src/com/matter/controller/commands/bdx/PairOnNetworkLongDownloadLogCommand.kt

@@ -9,6 +9,7 @@ import com.matter.controller.commands.pairing.PairingCommand
 import com.matter.controller.commands.pairing.PairingModeType
 import com.matter.controller.commands.pairing.PairingNetworkType
 import java.io.File
+import java.util.concurrent.TimeUnit
 import java.util.logging.Level
 import java.util.logging.Logger
 
@@ -85,12 +86,19 @@ class PairOnNetworkLongDownloadLogCommand(
       )
     logger.log(Level.INFO, "Waiting response : ${getTimeoutMillis()}")
     waitCompleteMs(getTimeoutMillis())
+    // For waiting both side terminating.
+    try {
+      TimeUnit.SECONDS.sleep(WAIT_FOR_TERMINATE)
+    } catch (e: InterruptedException) {
+      throw RuntimeException(e)
+    }
   }
 
   companion object {
     private val logger = Logger.getLogger(PairOnNetworkLongDownloadLogCommand::class.java.name)
 
     private const val MATTER_PORT = 5540
     private const val MS_TO_SEC = 1000
+    private const val WAIT_FOR_TERMINATE = 1L
   }
 }

kotlin-detect-config.yaml

@@ -232,6 +232,7 @@ exceptions:
     TooGenericExceptionThrown:
         excludes:
             - "**/examples/java-matter-controller/java/src/com/matter/controller/commands/bdx/DownloadLogCommand.kt"
+            - "**/examples/java-matter-controller/java/src/com/matter/controller/commands/bdx/PairOnNetworkLongDownloadLogCommand.kt"
             - "**/examples/java-matter-controller/java/src/com/matter/controller/commands/discover/DiscoverCommissionablesCommand.kt"
             - "**/src/controller/java/generated/java/**/*"
     ThrowingExceptionsWithoutMessageOrCause:

src/controller/java/AndroidLogDownloadFromNode.cpp

@@ -99,7 +99,7 @@ CHIP_ERROR AndroidLogDownloadFromNode::SendRetrieveLogsRequest(Messaging::Exchan
     if (err != CHIP_NO_ERROR)
     {
         ChipLogError(Controller, "Make BDX URI failure : %" CHIP_ERROR_FORMAT, err.Format());
-        FinishLogDownloadFromNode(err);
+        FinishLogDownloadFromNode(static_cast<void *>(this), err);
     }
 
     mBdxReceiver =
@@ -132,18 +132,14 @@ void AndroidLogDownloadFromNode::OnDeviceConnectedFn(void * context, Messaging::
     if (err != CHIP_NO_ERROR)
     {
         ChipLogError(Controller, "Log Download failure : %" CHIP_ERROR_FORMAT, err.Format());
-        self->FinishLogDownloadFromNode(err);
+        FinishLogDownloadFromNode(context, err);
     }
 }
 
 void AndroidLogDownloadFromNode::OnDeviceConnectionFailureFn(void * context, const ScopedNodeId & peerId, CHIP_ERROR err)
 {
     ChipLogProgress(Controller, "OnDeviceConnectionFailureFn: %" CHIP_ERROR_FORMAT, err.Format());
-
-    auto * self = static_cast<AndroidLogDownloadFromNode *>(context);
-    VerifyOrReturn(self != nullptr, ChipLogProgress(Controller, "Device connected failure callback with null context. Ignoring"));
-
-    self->FinishLogDownloadFromNode(err);
+    FinishLogDownloadFromNode(context, err);
 }
 
 void AndroidLogDownloadFromNode::OnResponseRetrieveLogs(void * context,
@@ -162,25 +158,25 @@ void AndroidLogDownloadFromNode::OnResponseRetrieveLogs(void * context,
     {
         CHIP_ERROR err = CHIP_NO_ERROR;
         self->OnTransferCallback(self->mController->GetFabricIndex(), self->mRemoteNodeId, data.logContent, &err);
-        self->FinishLogDownloadFromNode(err);
+        FinishLogDownloadFromNode(context, err);
     }
     else if (data.status == StatusEnum::kNoLogs)
     {
         CHIP_ERROR err = CHIP_NO_ERROR;
         self->OnTransferCallback(self->mController->GetFabricIndex(), self->mRemoteNodeId, ByteSpan(), &err);
-        self->FinishLogDownloadFromNode(err);
+        FinishLogDownloadFromNode(context, err);
     }
     else if (data.status == StatusEnum::kBusy)
     {
-        self->FinishLogDownloadFromNode(CHIP_ERROR_BUSY);
+        FinishLogDownloadFromNode(context, CHIP_ERROR_BUSY);
     }
     else if (data.status == StatusEnum::kDenied)
     {
-        self->FinishLogDownloadFromNode(CHIP_ERROR_ACCESS_DENIED);
+        FinishLogDownloadFromNode(context, CHIP_ERROR_ACCESS_DENIED);
     }
     else
     {
-        self->FinishLogDownloadFromNode(CHIP_ERROR_INVALID_DATA_LIST);
+        FinishLogDownloadFromNode(context, CHIP_ERROR_INVALID_DATA_LIST);
     }
 }
 
@@ -191,48 +187,60 @@ void AndroidLogDownloadFromNode::OnCommandFailure(void * context, CHIP_ERROR err
     auto * self = static_cast<AndroidLogDownloadFromNode *>(context);
     VerifyOrReturn(self != nullptr, ChipLogProgress(Controller, "Send command failure callback with null context. Ignoring"));
 
-    self->FinishLogDownloadFromNode(err);
+    FinishLogDownloadFromNode(context, err);
 }
 
-void AndroidLogDownloadFromNode::FinishLogDownloadFromNode(CHIP_ERROR err)
+void AndroidLogDownloadFromNode::FinishLogDownloadFromNode(void * context, CHIP_ERROR err)
 {
-    CHIP_ERROR jniErr = CHIP_NO_ERROR;
-    if (mBdxReceiver != nullptr)
+    auto * self = static_cast<AndroidLogDownloadFromNode *>(context);
+    VerifyOrReturn(self != nullptr, ChipLogProgress(Controller, "Finish Log Download with null context. Ignoring"));
+
+    if (self->mBdxReceiver != nullptr)
     {
-        if (mTimeout > 0)
+        if (self->mTimeout > 0 && err != CHIP_ERROR_TIMEOUT)
         {
-            mBdxReceiver->CancelBDXTransferTimeout();
+            self->mBdxReceiver->CancelBDXTransferTimeout();
         }
-        delete mBdxReceiver;
-        mBdxReceiver = nullptr;
+        delete self->mBdxReceiver;
+        self->mBdxReceiver = nullptr;
     }
 
-    JNIEnv * env = JniReferences::GetInstance().GetEnvForCurrentThread();
+    CHIP_ERROR jniErr = CHIP_NO_ERROR;
+    JNIEnv * env      = JniReferences::GetInstance().GetEnvForCurrentThread();
     JniLocalReferenceScope scope(env);
 
+    jobject jCallback   = self->mJavaCallback.ObjectRef();
+    jint jFabricIndex   = self->mController->GetFabricIndex();
+    jlong jremoteNodeId = self->mRemoteNodeId;
+
+    VerifyOrExit(env != nullptr, ChipLogError(Controller, "Could not get JNIEnv for current thread"));
+
     if (err == CHIP_NO_ERROR)
     {
         ChipLogProgress(Controller, "Log Download succeeded.");
         jmethodID onSuccessMethod;
         // Java method signature : boolean onSuccess(int fabricIndex, long nodeId)
-        jniErr = JniReferences::GetInstance().FindMethod(env, mJavaCallback.ObjectRef(), "onSuccess", "(IJ)V", &onSuccessMethod);
+        jniErr = JniReferences::GetInstance().FindMethod(env, jCallback, "onSuccess", "(IJ)V", &onSuccessMethod);
 
-        VerifyOrReturn(jniErr == CHIP_NO_ERROR, ChipLogError(Controller, "Could not find onSuccess method"));
+        VerifyOrExit(jniErr == CHIP_NO_ERROR, ChipLogError(Controller, "Could not find onSuccess method"));
 
-        env->CallVoidMethod(mJavaCallback.ObjectRef(), onSuccessMethod, static_cast<jint>(mController->GetFabricIndex()),
-                            static_cast<jlong>(mRemoteNodeId));
-        return;
+        env->CallVoidMethod(jCallback, onSuccessMethod, jFabricIndex, jremoteNodeId);
     }
+    else
+    {
+        ChipLogError(Controller, "Log Download Failed : %" CHIP_ERROR_FORMAT, err.Format());
 
-    ChipLogError(Controller, "Log Download Failed : %" CHIP_ERROR_FORMAT, err.Format());
+        jmethodID onErrorMethod;
+        // Java method signature : void onError(int fabricIndex, long nodeId, long errorCode)
+        jniErr = JniReferences::GetInstance().FindMethod(env, jCallback, "onError", "(IJJ)V", &onErrorMethod);
+        VerifyOrExit(jniErr == CHIP_NO_ERROR, ChipLogError(Controller, "Could not find onError method"));
 
-    jmethodID onErrorMethod;
-    // Java method signature : void onError(int fabricIndex, long nodeId, long errorCode)
-    jniErr = JniReferences::GetInstance().FindMethod(env, mJavaCallback.ObjectRef(), "onError", "(IJJ)V", &onErrorMethod);
-    VerifyOrReturn(jniErr == CHIP_NO_ERROR, ChipLogError(Controller, "Could not find onError method"));
+        env->CallVoidMethod(jCallback, onErrorMethod, jFabricIndex, jremoteNodeId, static_cast<jlong>(err.AsInteger()));
+    }
 
-    env->CallVoidMethod(mJavaCallback.ObjectRef(), onErrorMethod, static_cast<jint>(mController->GetFabricIndex()),
-                        static_cast<jlong>(mRemoteNodeId), static_cast<jlong>(err.AsInteger()));
+exit:
+    // Finish this function, this object will be deleted.
+    delete self;
 }
 
 void AndroidLogDownloadFromNode::OnBdxTransferCallback(void * context, FabricIndex fabricIndex, NodeId remoteNodeId,
@@ -277,7 +285,7 @@ void AndroidLogDownloadFromNode::OnBdxTransferSuccessCallback(void * context, Fa
     auto * self = static_cast<AndroidLogDownloadFromNode *>(context);
     VerifyOrReturn(self != nullptr, ChipLogProgress(Controller, "Send command failure callback with null context. Ignoring"));
 
-    self->FinishLogDownloadFromNode(CHIP_NO_ERROR);
+    FinishLogDownloadFromNode(context, CHIP_NO_ERROR);
 }
 
 void AndroidLogDownloadFromNode::OnBdxTransferFailureCallback(void * context, FabricIndex fabricIndex, NodeId remoteNodeId,
@@ -288,7 +296,7 @@ void AndroidLogDownloadFromNode::OnBdxTransferFailureCallback(void * context, Fa
     auto * self = static_cast<AndroidLogDownloadFromNode *>(context);
     VerifyOrReturn(self != nullptr, ChipLogProgress(Controller, "Send command failure callback with null context. Ignoring"));
 
-    self->FinishLogDownloadFromNode(status);
+    FinishLogDownloadFromNode(context, status);
 }
 } // namespace Controller
 } // namespace chip

src/controller/java/AndroidLogDownloadFromNode.h

@@ -54,6 +54,8 @@ class AndroidLogDownloadFromNode
     AndroidLogDownloadFromNode(DeviceController * controller, NodeId remoteNodeId, app::Clusters::DiagnosticLogs::IntentEnum intent,
                                uint16_t timeout, jobject javaCallback);
 
+    ~AndroidLogDownloadFromNode() {}
+
     DeviceController * mController = nullptr;
 
     chip::Callback::Callback<OnDeviceConnected> mOnDeviceConnectedCallback;
@@ -76,7 +78,7 @@ class AndroidLogDownloadFromNode
     CHIP_ERROR SendRetrieveLogsRequest(Messaging::ExchangeManager & exchangeMgr, const SessionHandle & sessionHandle);
     void OnTransferCallback(FabricIndex fabricIndex, NodeId remoteNodeId, const chip::ByteSpan & data,
                             CHIP_ERROR * errInfoOnFailure);
-    void FinishLogDownloadFromNode(CHIP_ERROR err);
+    static void FinishLogDownloadFromNode(void * context, CHIP_ERROR err);
 
     static void OnDeviceConnectedFn(void * context, Messaging::ExchangeManager & exchangeMgr, const SessionHandle & sessionHandle);
     static void OnDeviceConnectionFailureFn(void * context, const ScopedNodeId & peerId, CHIP_ERROR error);