Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Android] Keep GetConnectedDeviceCallbackJni from being deleted early #14772

Merged
merged 1 commit into from
Feb 5, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/controller/java/AndroidCallbacks-JNI.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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>

Expand All @@ -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);
}

Expand Down
13 changes: 12 additions & 1 deletion src/controller/java/AndroidCallbacks.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand All @@ -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);
Expand All @@ -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);
Expand Down
5 changes: 3 additions & 2 deletions src/controller/java/AndroidCallbacks.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 4 additions & 2 deletions src/controller/java/CHIPDeviceController-JNI.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/
Expand Down