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.

Sign up for GitHub

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]Fix Jni local and global reference leaks for generic IM #29236

Merged
merged 1 commit into from
Oct 5, 2023
Merged

[Android]Fix Jni local and global reference leaks for generic IM #29236

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
39 changes: 29 additions & 10 deletions ...troller/java/src/com/matter/controller/commands/pairing/PairOnNetworkLongImReadCommand.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,13 @@ class PairOnNetworkLongImReadCommand(
eventPath: ChipEventPath?,
e: Exception
) {
if (attributePath != null && attributePath.clusterId.getId() == UNIT_TEST_CLUSTER) {
logger.log(
Level.INFO,
"TODO: skip the error check for unit test cluster that covers most error result"
)
return
}
logger.log(Level.INFO, "Read receive onError")
setFailure("read failure")
}
Expand All @@ -55,23 +62,31 @@ class PairOnNetworkLongImReadCommand(
fun checkStartUpEventJson(event: EventState): Boolean =
event.getJson().toString() == """{"0:STRUCT":{"0:UINT":1}}"""

fun checkAllAttributesJsonForBasicCluster(cluster: String): Boolean {
fun checkAllAttributesJsonForFixedLabel(cluster: String): Boolean {
val expected =
"""{"16:BOOL":false,""" +
""""65531:ARRAY-UINT":[""" +
"""0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,18,19,20,65528,65529,65531,65532,65533]}"""
"""{"65528:ARRAY-?":[],"0:ARRAY-STRUCT":[{"0:STRING":"room","1:STRING":"bedroom 2"},""" +
"""{"0:STRING":"orientation","1:STRING":"North"},{"0:STRING":"floor","1:STRING":"2"},""" +
"""{"0:STRING":"direction","1:STRING":"up"}],"65531:ARRAY-UINT":[0,65528,65529,65531,65532,65533],""" +
""""65533:UINT":1,"65529:ARRAY-?":[],"65532:UINT":0}"""
return cluster.equals(expected)
}

private fun validateResponse(nodeState: NodeState) {
val endpointZero =
requireNotNull(nodeState.getEndpointState(0)) { "Endpoint zero not found." }

val endpointOne = requireNotNull(nodeState.getEndpointState(0)) { "Endpoint one not found." }

val basicCluster =
requireNotNull(endpointZero.getClusterState(CLUSTER_ID_BASIC)) {
"Basic cluster not found."
}

val fixedLabelCluster =
requireNotNull(endpointOne.getClusterState(FIXED_LABEL_CLUSTER)) {
"fixed label cluster not found."
}

val localConfigDisabledAttribute =
requireNotNull(basicCluster.getAttributeState(ATTR_ID_LOCAL_CONFIG_DISABLED)) {
"No local config disabled attribute found."
Expand All @@ -81,7 +96,9 @@ class PairOnNetworkLongImReadCommand(
requireNotNull(basicCluster.getEventState(EVENT_ID_START_UP)) { "No start up event found." }

val clusterAttributes =
requireNotNull(basicCluster.getAttributesJson()) { "No basicCluster attribute found." }
requireNotNull(fixedLabelCluster.getAttributesJson()) {
"No fixed label cluster attribute found."
}

require(checkLocalConfigDisableAttributeTlv(localConfigDisabledAttribute)) {
"Invalid local config disabled attribute TLV ${localConfigDisabledAttribute.getTlv().contentToString()}"
Expand All @@ -101,8 +118,8 @@ class PairOnNetworkLongImReadCommand(
"Invalid start up event Json ${startUpEvents[0].getJson().toString()}"
}

require(checkAllAttributesJsonForBasicCluster(clusterAttributes)) {
"Invalid basic cluster attributes Json ${clusterAttributes}"
require(checkAllAttributesJsonForFixedLabel(clusterAttributes)) {
"Invalid fixed label cluster attributes Json ${clusterAttributes}"
}
}

Expand Down Expand Up @@ -132,9 +149,9 @@ class PairOnNetworkLongImReadCommand(
val attributePathList =
listOf(
ChipAttributePath.newInstance(
ChipPathId.forId(/* endpointId= */ 0),
ChipPathId.forId(CLUSTER_ID_BASIC),
ChipPathId.forId(ATTR_ID_LOCAL_CONFIG_DISABLED),
ChipPathId.forWildcard(),
ChipPathId.forWildcard(),
ChipPathId.forWildcard()
),
ChipAttributePath.newInstance(
ChipPathId.forId(/* endpointId= */ 0),
Expand Down Expand Up @@ -176,6 +193,8 @@ class PairOnNetworkLongImReadCommand(

private const val MATTER_PORT = 5540
private const val CLUSTER_ID_BASIC = 0x0028L
private const val FIXED_LABEL_CLUSTER = 0x0040L
private const val UNIT_TEST_CLUSTER = 0xfff1fc05
private const val ATTR_ID_LOCAL_CONFIG_DISABLED = 16L
private const val EVENT_ID_START_UP = 0L
private const val GLOBAL_ATTRIBUTE_LIST = 65531L
Expand Down
211 changes: 145 additions & 66 deletions src/controller/java/AndroidCallbacks.cpp
View file
Open in desktop

Large diffs are not rendered by default.

3 changes: 1 addition & 2 deletions src/controller/java/AndroidCallbacks.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,6 @@ struct ReportCallback : public app::ClusterStateCache::Callback
jobject mReportCallbackRef = nullptr;
// NodeState Java object that will be returned to the application.
jobject mNodeStateObj = nullptr;
jclass mNodeStateCls = nullptr;
};

struct WriteAttributesCallback : public app::WriteClient::Callback
Expand Down Expand Up @@ -127,7 +126,7 @@ struct InvokeCallback : public app::CommandSender::Callback

void OnDone(app::CommandSender * apCommandSender) override;

CHIP_ERROR CreateInvokeElement(const app::ConcreteCommandPath & aPath, TLV::TLVReader * apData, jobject & outObj);
CHIP_ERROR CreateInvokeElement(JNIEnv * env, const app::ConcreteCommandPath & aPath, TLV::TLVReader * apData, jobject & outObj);
void ReportError(CHIP_ERROR err);
void ReportError(Protocols::InteractionModel::Status status);
void ReportError(const char * message, ChipError::StorageType errorCode);
Expand Down
15 changes: 13 additions & 2 deletions src/controller/java/AndroidCommissioningWindowOpener.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,11 @@ AndroidCommissioningWindowOpener::AndroidCommissioningWindowOpener(DeviceControl
{
JNIEnv * env = JniReferences::GetInstance().GetEnvForCurrentThread();
mJavaCallback = env->NewGlobalRef(jCallbackObject);
if (mJavaCallback == nullptr)
{
ChipLogError(Controller, "Failed to create global reference for mJavaCallback");
return;
}

jclass callbackClass = env->GetObjectClass(jCallbackObject);

Expand All @@ -60,7 +65,10 @@ AndroidCommissioningWindowOpener::~AndroidCommissioningWindowOpener()
{
ChipLogError(Controller, "Delete AndroidCommissioningWindowOpener");
JNIEnv * env = JniReferences::GetInstance().GetEnvForCurrentThread();
env->DeleteGlobalRef(mJavaCallback);
if (mJavaCallback != nullptr)
{
env->DeleteGlobalRef(mJavaCallback);
}
}

CHIP_ERROR AndroidCommissioningWindowOpener::OpenBasicCommissioningWindow(DeviceController * controller, NodeId deviceId,
Expand Down Expand Up @@ -112,6 +120,8 @@ void AndroidCommissioningWindowOpener::OnOpenCommissioningWindowResponse(void *
{
auto * self = static_cast<AndroidCommissioningWindowOpener *>(context);
JNIEnv * env = JniReferences::GetInstance().GetEnvForCurrentThread();
VerifyOrReturn(env != nullptr, ChipLogError(Controller, "Could not get JNIEnv for current thread"));
JniLocalReferenceManager manager(env);

VerifyOrExit(self->mJavaCallback != nullptr, ChipLogError(Controller, "mJavaCallback is not allocated."));

Expand Down Expand Up @@ -146,10 +156,11 @@ void AndroidCommissioningWindowOpener::OnOpenCommissioningWindowResponse(void *
void AndroidCommissioningWindowOpener::OnOpenBasicCommissioningWindowResponse(void * context, NodeId deviceId, CHIP_ERROR status)
{
auto * self = static_cast<AndroidCommissioningWindowOpener *>(context);

if (self->mJavaCallback != nullptr)
{
JNIEnv * env = JniReferences::GetInstance().GetEnvForCurrentThread();
VerifyOrReturn(env != nullptr, ChipLogError(Controller, "Could not get JNIEnv for current thread"));
JniLocalReferenceManager manager(env);
if (status == CHIP_NO_ERROR)
{
if (self->mOnSuccessMethod != nullptr)
Expand Down
11 changes: 7 additions & 4 deletions src/controller/java/AndroidControllerExceptions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,15 @@ CHIP_ERROR AndroidControllerExceptions::CreateAndroidControllerException(JNIEnv
CHIP_ERROR err = JniReferences::GetInstance().GetClassRef(env, "chip/devicecontroller/ChipDeviceControllerException",
controllerExceptionCls);
VerifyOrReturnError(err == CHIP_NO_ERROR, CHIP_JNI_ERROR_TYPE_NOT_FOUND);
JniClass controllerExceptionJniCls(controllerExceptionCls);

JniObject controllerExceptionJniCls(env, static_cast<jobject>(controllerExceptionCls));
yunhanw-google marked this conversation as resolved.
Show resolved Hide resolved

jmethodID exceptionConstructor = env->GetMethodID(controllerExceptionCls, "<init>", "(JLjava/lang/String;)V");
outEx = (jthrowable) env->NewObject(controllerExceptionCls, exceptionConstructor, static_cast<jlong>(errorCode),
env->NewStringUTF(message));
VerifyOrReturnError(outEx != nullptr, CHIP_JNI_ERROR_TYPE_NOT_FOUND);
jobject localRef =
env->NewObject(controllerExceptionCls, exceptionConstructor, static_cast<jlong>(errorCode), env->NewStringUTF(message));
VerifyOrReturnError(localRef != nullptr, CHIP_JNI_ERROR_NULL_OBJECT);
outEx = (jthrowable) (env->NewGlobalRef(localRef));
VerifyOrReturnError(outEx != nullptr, CHIP_JNI_ERROR_NULL_OBJECT);
return CHIP_NO_ERROR;
}

Expand Down
2 changes: 2 additions & 0 deletions src/controller/java/AndroidDeviceControllerWrapper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -557,6 +557,8 @@ void AndroidDeviceControllerWrapper::OnCommissioningStatusUpdate(PeerId peerId,
{
chip::DeviceLayer::StackUnlock unlock;
JNIEnv * env = JniReferences::GetInstance().GetEnvForCurrentThread();
VerifyOrReturn(env != nullptr, ChipLogError(Controller, "Could not get JNIEnv for current thread"));
JniLocalReferenceManager manager(env);
jmethodID onCommissioningStatusUpdateMethod;
CHIP_ERROR err = JniReferences::GetInstance().FindMethod(env, mJavaObjectRef, "onCommissioningStatusUpdate",
"(JLjava/lang/String;I)V", &onCommissioningStatusUpdateMethod);
Expand Down
4 changes: 2 additions & 2 deletions src/controller/java/src/chip/devicecontroller/model/NodeState.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@
public final class NodeState {
private Map<Integer, EndpointState> endpoints;

public NodeState(Map<Integer, EndpointState> endpoints) {
this.endpoints = endpoints;
public NodeState() {
this.endpoints = new HashMap<>();
}

public Map<Integer, EndpointState> getEndpointStates() {
Expand Down
107 changes: 93 additions & 14 deletions src/lib/support/JniTypeWrappers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include <lib/support/Span.h>
#include <string>

#define JNI_LOCAL_REF_COUNT 256
namespace chip {
/// Exposes the underlying UTF string from a jni string
class JniUtfString
Expand Down Expand Up @@ -87,19 +88,41 @@ class JniByteArray
class UtfString
{
public:
UtfString(JNIEnv * env, const char * data) : mEnv(env) { mData = data != nullptr ? mEnv->NewStringUTF(data) : nullptr; }
UtfString(JNIEnv * env, const char * data) : mEnv(env)
{
jstring localRef = data != nullptr ? mEnv->NewStringUTF(data) : nullptr;
if (localRef == nullptr)
{
return;
}
mData = static_cast<jstring>(env->NewGlobalRef(localRef));
}

UtfString(JNIEnv * env, chip::CharSpan data) : mEnv(env)
{
std::string str(data.data(), data.size());
mData = env->NewStringUTF(str.c_str());
jstring localRef = env->NewStringUTF(str.c_str());
if (localRef == nullptr)
{
return;
}
mData = static_cast<jstring>(env->NewGlobalRef(localRef));
}

~UtfString()
{
if (mEnv != nullptr && mData != nullptr)
{
mEnv->DeleteGlobalRef(mData);
mData = nullptr;
}
}
~UtfString() { mEnv->DeleteLocalRef(mData); }

jstring jniValue() { return mData; }

private:
JNIEnv * mEnv;
jstring mData;
JNIEnv * mEnv = nullptr;
jstring mData = nullptr;
};

/// wrap a byte array as a JNI byte array
Expand All @@ -108,27 +131,39 @@ class ByteArray
public:
ByteArray(JNIEnv * env, const jbyte * data, jsize dataLen) : mEnv(env)
{
mArray = data != nullptr ? mEnv->NewByteArray(dataLen) : nullptr;
if (mArray != nullptr)
jbyteArray localRef = data != nullptr ? mEnv->NewByteArray(dataLen) : nullptr;
if (localRef == nullptr)
{
env->SetByteArrayRegion(mArray, 0, dataLen, data);
return;
}
env->SetByteArrayRegion(localRef, 0, dataLen, data);
mArray = static_cast<jbyteArray>(env->NewGlobalRef(localRef));
}

ByteArray(JNIEnv * env, chip::ByteSpan data) : mEnv(env)
{
mArray = mEnv->NewByteArray(static_cast<jsize>(data.size()));
if (mArray != nullptr)
jbyteArray localRef = mEnv->NewByteArray(static_cast<jsize>(data.size()));
if (localRef == nullptr)
{
env->SetByteArrayRegion(mArray, 0, static_cast<jsize>(data.size()), reinterpret_cast<const jbyte *>(data.data()));
return;
}
env->SetByteArrayRegion(localRef, 0, static_cast<jsize>(data.size()), reinterpret_cast<const jbyte *>(data.data()));
mArray = static_cast<jbyteArray>(env->NewGlobalRef(localRef));
}

~ByteArray()
{
if (mEnv != nullptr && mArray != nullptr)
{
mEnv->DeleteGlobalRef(mArray);
}
}
~ByteArray() { mEnv->DeleteLocalRef(mArray); }

jbyteArray jniValue() { return mArray; }

private:
JNIEnv * mEnv;
jbyteArray mArray;
JNIEnv * mEnv = nullptr;
jbyteArray mArray = nullptr;
};

/// Manages an pre-existing global reference to a jclass.
Expand All @@ -143,4 +178,48 @@ class JniClass
private:
jclass mClassRef;
};

class JniLocalReferenceManager
{
public:
JniLocalReferenceManager(JNIEnv * env) : mEnv(env)
{
if (mEnv->PushLocalFrame(JNI_LOCAL_REF_COUNT) == 0)
{
mlocalFramePushed = true;
}
}
~JniLocalReferenceManager()
{
if (mlocalFramePushed)
{
mEnv->PopLocalFrame(nullptr);
mlocalFramePushed = false;
}
}

private:
JNIEnv * mEnv = nullptr;
bool mlocalFramePushed = false;
};

class JniObject
{
public:
JniObject(JNIEnv * aEnv, jobject aObjectRef) : mEnv(aEnv), mObjectRef(aObjectRef) {}
~JniObject()
{
if (mEnv != nullptr && mObjectRef != nullptr)
{
mEnv->DeleteGlobalRef(mObjectRef);
}
}

jobject objectRef() { return mObjectRef; }

private:
JNIEnv * mEnv = nullptr;
jobject mObjectRef = nullptr;
};

} // namespace chip