Clean up GetConnectedDevice (#14772)

project-chip · Dec 13, 2023 · 1089582 · 1089582
1 parent eea5962
commit 1089582
Show file tree

Hide file tree

Showing 5 changed files with 24 additions and 6 deletions.
diff --git a/src/controller/java/AndroidCallbacks-JNI.cpp b/src/controller/java/AndroidCallbacks-JNI.cpp
@@ -17,6 +17,7 @@
 #include "AndroidCallbacks.h"
 
 #include <jni.h>
+#include <lib/support/CHIPMem.h>
 #include <lib/support/CodeUtils.h>
 #include <lib/support/logging/CHIPLogging.h>
 
@@ -27,7 +28,7 @@ using namespace chip::Controller;
 
 JNI_METHOD(jlong, GetConnectedDeviceCallbackJni, newCallback)(JNIEnv * env, jobject self, jobject callback)
 {
-    GetConnectedDeviceCallback * connectedDeviceCallback = new GetConnectedDeviceCallback(callback);
+    GetConnectedDeviceCallback * connectedDeviceCallback = chip::Platform::New<GetConnectedDeviceCallback>(self, callback);
     return reinterpret_cast<jlong>(connectedDeviceCallback);
 }
 

diff --git a/src/controller/java/AndroidCallbacks.cpp b/src/controller/java/AndroidCallbacks.cpp
@@ -25,11 +25,16 @@
 
 using namespace chip::Controller;
 
-GetConnectedDeviceCallback::GetConnectedDeviceCallback(jobject javaCallback) :
+GetConnectedDeviceCallback::GetConnectedDeviceCallback(jobject wrapperCallback, jobject javaCallback) :
     mOnSuccess(OnDeviceConnectedFn, this), mOnFailure(OnDeviceConnectionFailureFn, this)
 {
     JNIEnv * env = JniReferences::GetInstance().GetEnvForCurrentThread();
     VerifyOrReturn(env != nullptr, ChipLogError(Controller, "Could not get JNIEnv for current thread"));
+    mWrapperCallbackRef = env->NewGlobalRef(wrapperCallback);
+    if (mWrapperCallbackRef == nullptr)
+    {
+        ChipLogError(Controller, "Could not create global reference for Java callback");
+    }
     mJavaCallbackRef = env->NewGlobalRef(javaCallback);
     if (mJavaCallbackRef == nullptr)
     {
@@ -50,6 +55,9 @@ void GetConnectedDeviceCallback::OnDeviceConnectedFn(void * context, Operational
     auto * self          = static_cast<GetConnectedDeviceCallback *>(context);
     jobject javaCallback = self->mJavaCallbackRef;
 
+    // Release global ref so application can clean up.
+    env->DeleteGlobalRef(self->mWrapperCallbackRef);
+
     jclass getConnectedDeviceCallbackCls = nullptr;
     JniReferences::GetInstance().GetClassRef(env, "chip/devicecontroller/GetConnectedDeviceCallbackJni$GetConnectedDeviceCallback",
                                              getConnectedDeviceCallbackCls);
@@ -71,6 +79,9 @@ void GetConnectedDeviceCallback::OnDeviceConnectionFailureFn(void * context, Pee
     auto * self          = static_cast<GetConnectedDeviceCallback *>(context);
     jobject javaCallback = self->mJavaCallbackRef;
 
+    // Release global ref so application can clean up.
+    env->DeleteGlobalRef(self->mWrapperCallbackRef);
+
     jclass getConnectedDeviceCallbackCls = nullptr;
     JniReferences::GetInstance().GetClassRef(env, "chip/devicecontroller/GetConnectedDeviceCallbackJni$GetConnectedDeviceCallback",
                                              getConnectedDeviceCallbackCls);

diff --git a/src/controller/java/AndroidCallbacks.h b/src/controller/java/AndroidCallbacks.h
@@ -25,15 +25,16 @@ namespace Controller {
 // Callback for success and failure cases of GetConnectedDevice().
 struct GetConnectedDeviceCallback
 {
-    GetConnectedDeviceCallback(jobject javaCallback);
+    GetConnectedDeviceCallback(jobject wrapperCallback, jobject javaCallback);
     ~GetConnectedDeviceCallback();
 
     static void OnDeviceConnectedFn(void * context, OperationalDeviceProxy * device);
     static void OnDeviceConnectionFailureFn(void * context, PeerId peerId, CHIP_ERROR error);
 
     Callback::Callback<OnDeviceConnected> mOnSuccess;
     Callback::Callback<OnDeviceConnectionFailure> mOnFailure;
-    jobject mJavaCallbackRef;
+    jobject mWrapperCallbackRef = nullptr;
+    jobject mJavaCallbackRef    = nullptr;
 };
 
 } // namespace Controller

diff --git a/src/controller/java/CHIPDeviceController-JNI.cpp b/src/controller/java/CHIPDeviceController-JNI.cpp
@@ -385,12 +385,14 @@ JNI_METHOD(jlong, getDeviceBeingCommissionedPointer)(JNIEnv * env, jobject self,
 JNI_METHOD(void, getConnectedDevicePointer)(JNIEnv * env, jobject self, jlong handle, jlong nodeId, jlong callbackHandle)
 {
     chip::DeviceLayer::StackLock lock;
+    CHIP_ERROR err                           = CHIP_NO_ERROR;
     AndroidDeviceControllerWrapper * wrapper = AndroidDeviceControllerWrapper::FromJNIHandle(handle);
 
     GetConnectedDeviceCallback * connectedDeviceCallback = reinterpret_cast<GetConnectedDeviceCallback *>(callbackHandle);
     VerifyOrReturn(connectedDeviceCallback != nullptr, ChipLogError(Controller, "GetConnectedDeviceCallback handle is nullptr"));
-    wrapper->Controller()->GetCompressedFabricId();
-    wrapper->Controller()->GetConnectedDevice(nodeId, &connectedDeviceCallback->mOnSuccess, &connectedDeviceCallback->mOnFailure);
+    err = wrapper->Controller()->GetConnectedDevice(nodeId, &connectedDeviceCallback->mOnSuccess,
+                                                    &connectedDeviceCallback->mOnFailure);
+    VerifyOrReturn(err == CHIP_NO_ERROR, ChipLogError(Controller, "Error invoking GetConnectedDevice"));
 }
 
 JNI_METHOD(void, disconnectDevice)(JNIEnv * env, jobject self, jlong handle, jlong deviceId)

diff --git a/src/controller/java/src/chip/devicecontroller/ChipDeviceController.java b/src/controller/java/src/chip/devicecontroller/ChipDeviceController.java
@@ -174,6 +174,9 @@ public long getDeviceBeingCommissionedPointer(long nodeId) {
   /**
    * Through GetConnectedDeviceCallback, returns a pointer to a connected device or an error.
    *
+   * <p>The native code invoked by this method creates a strong reference to the provided callback,
+   * which is released only when GetConnectedDeviceCallback has returned success or failure.
+   *
    * <p>TODO(#8443): This method could benefit from a ChipDevice abstraction to hide the pointer
    * passing.
    */