From b6f97bf14e40ffd8dee83db45112d1cd5a608c9a Mon Sep 17 00:00:00 2001 From: Hossein Yousefi Date: Mon, 31 Jul 2023 12:55:57 +0200 Subject: [PATCH] Fix windows specifics and global reference ownership in interface implementation (#335) Since Dart doesn't know that this global reference is still used, it might garbage collect it via NativeFinalizer thus making it invalid. We're letting Java handle the clean up and use setAsDeleted to detach the native finalizer from the return value in Dart. --- .../com/github/dart_lang/jni/PortProxy.java | 15 +++++-- jni/src/dartjni.c | 42 +++++++++++++++---- jni/src/dartjni.h | 9 +++- .../in_app_java/src/android_utils/dartjni.h | 9 +++- jnigen/example/kotlin_plugin/src/dartjni.h | 9 +++- .../example/notification_plugin/src/dartjni.h | 9 +++- .../pdfbox_plugin/src/third_party/dartjni.h | 9 +++- jnigen/lib/src/bindings/dart_generator.dart | 21 ++++++++-- .../third_party/c_based/c_bindings/dartjni.h | 9 +++- .../kotlin_test/c_based/c_bindings/dartjni.h | 9 +++- .../c_based/c_bindings/dartjni.h | 9 +++- .../c_based/dart_bindings/simple_package.dart | 10 +++-- .../dart_bindings/simple_package.dart | 10 +++-- 13 files changed, 132 insertions(+), 38 deletions(-) diff --git a/jni/java/src/main/java/com/github/dart_lang/jni/PortProxy.java b/jni/java/src/main/java/com/github/dart_lang/jni/PortProxy.java index a75a914f..bae1c325 100644 --- a/jni/java/src/main/java/com/github/dart_lang/jni/PortProxy.java +++ b/jni/java/src/main/java/com/github/dart_lang/jni/PortProxy.java @@ -62,21 +62,28 @@ private static void appendType(StringBuilder descriptor, Class type) { public static Object newInstance(String binaryName, long port, long threadId, long functionPtr) throws ClassNotFoundException { - Class clazz = Class.forName(binaryName); + Class clazz = Class.forName(binaryName); return Proxy.newProxyInstance( clazz.getClassLoader(), new Class[] {clazz}, new PortProxy(port, threadId, functionPtr)); } @Override - public Object invoke(Object proxy, Method method, Object[] args) throws Throwable { - return _invoke(port, threadId, functionPtr, proxy, getDescriptor(method), args); + public Object invoke(Object proxy, Method method, Object[] args) { + Object[] result = _invoke(port, threadId, functionPtr, proxy, getDescriptor(method), args); + _cleanUp((Long) result[0]); + return result[1]; } - private native Object _invoke( + /// Returns an array with two objects: + /// [0]: The address of the result pointer used for the clean-up. + /// [1]: The result of the invocation. + private native Object[] _invoke( long port, long threadId, long functionPtr, Object proxy, String methodDescriptor, Object[] args); + + private native void _cleanUp(long resultPtr); } diff --git a/jni/src/dartjni.c b/jni/src/dartjni.c index 6f0fa086..df8feb78 100644 --- a/jni/src/dartjni.c +++ b/jni/src/dartjni.c @@ -636,7 +636,12 @@ void resultFor(CallbackResult* result, jobject object) { release_lock(&result->lock); } -JNIEXPORT jobject JNICALL +jclass _c_Object = NULL; +jclass _c_Long = NULL; + +jmethodID _m_Long_init = NULL; + +JNIEXPORT jobjectArray JNICALL Java_com_github_dart_1lang_jni_PortProxy__1invoke(JNIEnv* env, jobject thiz, jlong port, @@ -645,9 +650,8 @@ Java_com_github_dart_1lang_jni_PortProxy__1invoke(JNIEnv* env, jobject proxy, jstring methodDescriptor, jobjectArray args) { - attach_thread(); + CallbackResult* result = (CallbackResult*)malloc(sizeof(CallbackResult)); if (threadId != thread_id()) { - CallbackResult* result = (CallbackResult*)malloc(sizeof(CallbackResult)); init_lock(&result->lock); init_cond(&result->cond); acquire_lock(&result->lock); @@ -674,18 +678,40 @@ Java_com_github_dart_1lang_jni_PortProxy__1invoke(JNIEnv* env, c_post.value.as_array.length = sizeof(c_post_arr) / sizeof(c_post_arr[0]); Dart_PostCObject_DL(port, &c_post); + while (!result->ready) { wait_for(&result->cond, &result->lock); } + release_lock(&result->lock); destroy_lock(&result->lock); destroy_cond(&result->cond); - jobject object = result->object; - free(result); - return object; } else { - return ((jobject(*)(uint64_t, jobject, jobject))functionPtr)( + result->object = ((jobject(*)(uint64_t, jobject, jobject))functionPtr)( port, (*env)->NewGlobalRef(env, methodDescriptor), (*env)->NewGlobalRef(env, args)); } -} \ No newline at end of file + // Returning an array of length 2. + // [0]: The result pointer, used for cleaning up the global reference, and + // freeing the memory since we passed the ownership to Java. + // [1]: The returned object. + attach_thread(); + load_class_global_ref(&_c_Object, "java/lang/Object"); + load_class_global_ref(&_c_Long, "java/lang/Long"); + load_method(_c_Long, &_m_Long_init, "", "(J)V"); + jobject first = (*env)->NewObject(env, _c_Long, _m_Long_init, (jlong)result); + jobject second = result->object; + jobjectArray arr = (*env)->NewObjectArray(env, 2, _c_Object, NULL); + (*env)->SetObjectArrayElement(env, arr, 0, first); + (*env)->SetObjectArrayElement(env, arr, 1, second); + return arr; +} + +JNIEXPORT void JNICALL +Java_com_github_dart_1lang_jni_PortProxy__1cleanUp(JNIEnv* env, + jobject thiz, + jlong resultPtr) { + CallbackResult* result = (CallbackResult*)resultPtr; + (*env)->DeleteGlobalRef(env, result->object); + free(result); +} diff --git a/jni/src/dartjni.h b/jni/src/dartjni.h index cbb1f7f5..8a07d091 100644 --- a/jni/src/dartjni.h +++ b/jni/src/dartjni.h @@ -76,7 +76,7 @@ static inline void wait_for(ConditionVariable* cond, MutexLock* lock) { } static inline void destroy_cond(ConditionVariable* cond) { - DeleteCriticalSection(cond); + // Not available. } #elif defined __APPLE__ || defined __LINUX__ || defined __ANDROID__ || \ @@ -435,7 +435,7 @@ JniResult PortProxy__newInstance(jobject binaryName, int64_t port, int64_t functionPtr); -JNIEXPORT jobject JNICALL +JNIEXPORT jobjectArray JNICALL Java_com_github_dart_1lang_jni_PortProxy__1invoke(JNIEnv* env, jobject thiz, jlong port, @@ -444,3 +444,8 @@ Java_com_github_dart_1lang_jni_PortProxy__1invoke(JNIEnv* env, jobject proxy, jstring methodDescriptor, jobjectArray args); + +JNIEXPORT void JNICALL +Java_com_github_dart_1lang_jni_PortProxy__1cleanUp(JNIEnv* env, + jobject thiz, + jlong resultPtr); diff --git a/jnigen/example/in_app_java/src/android_utils/dartjni.h b/jnigen/example/in_app_java/src/android_utils/dartjni.h index cbb1f7f5..8a07d091 100644 --- a/jnigen/example/in_app_java/src/android_utils/dartjni.h +++ b/jnigen/example/in_app_java/src/android_utils/dartjni.h @@ -76,7 +76,7 @@ static inline void wait_for(ConditionVariable* cond, MutexLock* lock) { } static inline void destroy_cond(ConditionVariable* cond) { - DeleteCriticalSection(cond); + // Not available. } #elif defined __APPLE__ || defined __LINUX__ || defined __ANDROID__ || \ @@ -435,7 +435,7 @@ JniResult PortProxy__newInstance(jobject binaryName, int64_t port, int64_t functionPtr); -JNIEXPORT jobject JNICALL +JNIEXPORT jobjectArray JNICALL Java_com_github_dart_1lang_jni_PortProxy__1invoke(JNIEnv* env, jobject thiz, jlong port, @@ -444,3 +444,8 @@ Java_com_github_dart_1lang_jni_PortProxy__1invoke(JNIEnv* env, jobject proxy, jstring methodDescriptor, jobjectArray args); + +JNIEXPORT void JNICALL +Java_com_github_dart_1lang_jni_PortProxy__1cleanUp(JNIEnv* env, + jobject thiz, + jlong resultPtr); diff --git a/jnigen/example/kotlin_plugin/src/dartjni.h b/jnigen/example/kotlin_plugin/src/dartjni.h index cbb1f7f5..8a07d091 100644 --- a/jnigen/example/kotlin_plugin/src/dartjni.h +++ b/jnigen/example/kotlin_plugin/src/dartjni.h @@ -76,7 +76,7 @@ static inline void wait_for(ConditionVariable* cond, MutexLock* lock) { } static inline void destroy_cond(ConditionVariable* cond) { - DeleteCriticalSection(cond); + // Not available. } #elif defined __APPLE__ || defined __LINUX__ || defined __ANDROID__ || \ @@ -435,7 +435,7 @@ JniResult PortProxy__newInstance(jobject binaryName, int64_t port, int64_t functionPtr); -JNIEXPORT jobject JNICALL +JNIEXPORT jobjectArray JNICALL Java_com_github_dart_1lang_jni_PortProxy__1invoke(JNIEnv* env, jobject thiz, jlong port, @@ -444,3 +444,8 @@ Java_com_github_dart_1lang_jni_PortProxy__1invoke(JNIEnv* env, jobject proxy, jstring methodDescriptor, jobjectArray args); + +JNIEXPORT void JNICALL +Java_com_github_dart_1lang_jni_PortProxy__1cleanUp(JNIEnv* env, + jobject thiz, + jlong resultPtr); diff --git a/jnigen/example/notification_plugin/src/dartjni.h b/jnigen/example/notification_plugin/src/dartjni.h index cbb1f7f5..8a07d091 100644 --- a/jnigen/example/notification_plugin/src/dartjni.h +++ b/jnigen/example/notification_plugin/src/dartjni.h @@ -76,7 +76,7 @@ static inline void wait_for(ConditionVariable* cond, MutexLock* lock) { } static inline void destroy_cond(ConditionVariable* cond) { - DeleteCriticalSection(cond); + // Not available. } #elif defined __APPLE__ || defined __LINUX__ || defined __ANDROID__ || \ @@ -435,7 +435,7 @@ JniResult PortProxy__newInstance(jobject binaryName, int64_t port, int64_t functionPtr); -JNIEXPORT jobject JNICALL +JNIEXPORT jobjectArray JNICALL Java_com_github_dart_1lang_jni_PortProxy__1invoke(JNIEnv* env, jobject thiz, jlong port, @@ -444,3 +444,8 @@ Java_com_github_dart_1lang_jni_PortProxy__1invoke(JNIEnv* env, jobject proxy, jstring methodDescriptor, jobjectArray args); + +JNIEXPORT void JNICALL +Java_com_github_dart_1lang_jni_PortProxy__1cleanUp(JNIEnv* env, + jobject thiz, + jlong resultPtr); diff --git a/jnigen/example/pdfbox_plugin/src/third_party/dartjni.h b/jnigen/example/pdfbox_plugin/src/third_party/dartjni.h index cbb1f7f5..8a07d091 100644 --- a/jnigen/example/pdfbox_plugin/src/third_party/dartjni.h +++ b/jnigen/example/pdfbox_plugin/src/third_party/dartjni.h @@ -76,7 +76,7 @@ static inline void wait_for(ConditionVariable* cond, MutexLock* lock) { } static inline void destroy_cond(ConditionVariable* cond) { - DeleteCriticalSection(cond); + // Not available. } #elif defined __APPLE__ || defined __LINUX__ || defined __ANDROID__ || \ @@ -435,7 +435,7 @@ JniResult PortProxy__newInstance(jobject binaryName, int64_t port, int64_t functionPtr); -JNIEXPORT jobject JNICALL +JNIEXPORT jobjectArray JNICALL Java_com_github_dart_1lang_jni_PortProxy__1invoke(JNIEnv* env, jobject thiz, jlong port, @@ -444,3 +444,8 @@ Java_com_github_dart_1lang_jni_PortProxy__1invoke(JNIEnv* env, jobject proxy, jstring methodDescriptor, jobjectArray args); + +JNIEXPORT void JNICALL +Java_com_github_dart_1lang_jni_PortProxy__1cleanUp(JNIEnv* env, + jobject thiz, + jlong resultPtr); diff --git a/jnigen/lib/src/bindings/dart_generator.dart b/jnigen/lib/src/bindings/dart_generator.dart index 3db7c0bc..c78a3b25 100644 --- a/jnigen/lib/src/bindings/dart_generator.dart +++ b/jnigen/lib/src/bindings/dart_generator.dart @@ -1534,8 +1534,9 @@ class _InterfaceMethodIf extends Visitor { @override void visit(Method node) { + final isVoid = node.returnType.name == 'void'; final signature = node.javaSig; - final saveResult = node.returnType.name == 'void' ? '' : 'final \$r = '; + final saveResult = isVoid ? '' : 'final \$r = '; s.write(''' if (\$d == r"$signature") { ${saveResult}_\$methods[\$p]![\$d]!( @@ -1588,13 +1589,25 @@ class _InterfaceParamCast extends Visitor { /// Only returns the reference for non primitive types. /// Returns null for void. /// -/// For example `$r.toJInteger().reference` when the return type is `integer`. +/// Since Dart doesn't know that this global reference is still used, it might +/// garbage collect it via [NativeFinalizer] thus making it invalid. +/// This passes the ownership to Java using [setAsDeleted]. +/// +/// `..setAsDeleted` detaches the object from the [NativeFinalizer] and Java +/// will clean up the global reference afterwards. +/// +/// For example `($r.toJInteger()..setAsDeleted()).reference` when the return +/// type is `integer`. class _InterfaceReturnBox extends TypeVisitor { const _InterfaceReturnBox(); @override String visitNonPrimitiveType(ReferredType node) { - return '\$r.reference'; + // Casting is done to create a new global reference. The user might + // use the original reference elsewhere and so the original object + // should not be [setAsDeleted]. + return '((\$r as $_jObject).castTo(const ${_jObject}Type())' + '..setAsDeleted()).reference'; } @override @@ -1602,7 +1615,7 @@ class _InterfaceReturnBox extends TypeVisitor { if (node.name == 'void') { return '$_jni.nullptr'; } - return '$_jni.J${node.boxedName}(\$r).reference'; + return '($_jni.J${node.boxedName}(\$r)..setAsDeleted()).reference'; } } diff --git a/jnigen/test/jackson_core_test/third_party/c_based/c_bindings/dartjni.h b/jnigen/test/jackson_core_test/third_party/c_based/c_bindings/dartjni.h index cbb1f7f5..8a07d091 100644 --- a/jnigen/test/jackson_core_test/third_party/c_based/c_bindings/dartjni.h +++ b/jnigen/test/jackson_core_test/third_party/c_based/c_bindings/dartjni.h @@ -76,7 +76,7 @@ static inline void wait_for(ConditionVariable* cond, MutexLock* lock) { } static inline void destroy_cond(ConditionVariable* cond) { - DeleteCriticalSection(cond); + // Not available. } #elif defined __APPLE__ || defined __LINUX__ || defined __ANDROID__ || \ @@ -435,7 +435,7 @@ JniResult PortProxy__newInstance(jobject binaryName, int64_t port, int64_t functionPtr); -JNIEXPORT jobject JNICALL +JNIEXPORT jobjectArray JNICALL Java_com_github_dart_1lang_jni_PortProxy__1invoke(JNIEnv* env, jobject thiz, jlong port, @@ -444,3 +444,8 @@ Java_com_github_dart_1lang_jni_PortProxy__1invoke(JNIEnv* env, jobject proxy, jstring methodDescriptor, jobjectArray args); + +JNIEXPORT void JNICALL +Java_com_github_dart_1lang_jni_PortProxy__1cleanUp(JNIEnv* env, + jobject thiz, + jlong resultPtr); diff --git a/jnigen/test/kotlin_test/c_based/c_bindings/dartjni.h b/jnigen/test/kotlin_test/c_based/c_bindings/dartjni.h index cbb1f7f5..8a07d091 100644 --- a/jnigen/test/kotlin_test/c_based/c_bindings/dartjni.h +++ b/jnigen/test/kotlin_test/c_based/c_bindings/dartjni.h @@ -76,7 +76,7 @@ static inline void wait_for(ConditionVariable* cond, MutexLock* lock) { } static inline void destroy_cond(ConditionVariable* cond) { - DeleteCriticalSection(cond); + // Not available. } #elif defined __APPLE__ || defined __LINUX__ || defined __ANDROID__ || \ @@ -435,7 +435,7 @@ JniResult PortProxy__newInstance(jobject binaryName, int64_t port, int64_t functionPtr); -JNIEXPORT jobject JNICALL +JNIEXPORT jobjectArray JNICALL Java_com_github_dart_1lang_jni_PortProxy__1invoke(JNIEnv* env, jobject thiz, jlong port, @@ -444,3 +444,8 @@ Java_com_github_dart_1lang_jni_PortProxy__1invoke(JNIEnv* env, jobject proxy, jstring methodDescriptor, jobjectArray args); + +JNIEXPORT void JNICALL +Java_com_github_dart_1lang_jni_PortProxy__1cleanUp(JNIEnv* env, + jobject thiz, + jlong resultPtr); diff --git a/jnigen/test/simple_package_test/c_based/c_bindings/dartjni.h b/jnigen/test/simple_package_test/c_based/c_bindings/dartjni.h index cbb1f7f5..8a07d091 100644 --- a/jnigen/test/simple_package_test/c_based/c_bindings/dartjni.h +++ b/jnigen/test/simple_package_test/c_based/c_bindings/dartjni.h @@ -76,7 +76,7 @@ static inline void wait_for(ConditionVariable* cond, MutexLock* lock) { } static inline void destroy_cond(ConditionVariable* cond) { - DeleteCriticalSection(cond); + // Not available. } #elif defined __APPLE__ || defined __LINUX__ || defined __ANDROID__ || \ @@ -435,7 +435,7 @@ JniResult PortProxy__newInstance(jobject binaryName, int64_t port, int64_t functionPtr); -JNIEXPORT jobject JNICALL +JNIEXPORT jobjectArray JNICALL Java_com_github_dart_1lang_jni_PortProxy__1invoke(JNIEnv* env, jobject thiz, jlong port, @@ -444,3 +444,8 @@ Java_com_github_dart_1lang_jni_PortProxy__1invoke(JNIEnv* env, jobject proxy, jstring methodDescriptor, jobjectArray args); + +JNIEXPORT void JNICALL +Java_com_github_dart_1lang_jni_PortProxy__1cleanUp(JNIEnv* env, + jobject thiz, + jlong resultPtr); diff --git a/jnigen/test/simple_package_test/c_based/dart_bindings/simple_package.dart b/jnigen/test/simple_package_test/c_based/dart_bindings/simple_package.dart index 7cb01108..11e873e5 100644 --- a/jnigen/test/simple_package_test/c_based/dart_bindings/simple_package.dart +++ b/jnigen/test/simple_package_test/c_based/dart_bindings/simple_package.dart @@ -2979,13 +2979,17 @@ class MyInterface<$T extends jni.JObject> extends jni.JObject { final $r = _$methods[$p]![$d]!( $a[0].castTo(const jni.JStringType(), deleteOriginal: true), ); - return $r.reference; + return (($r as jni.JObject).castTo(const jni.JObjectType()) + ..setAsDeleted()) + .reference; } if ($d == r"varCallback(Ljava/lang/Object;)Ljava/lang/Object;") { final $r = _$methods[$p]![$d]!( $a[0].castTo(_$types[$p]!["T"]!, deleteOriginal: true), ); - return $r.reference; + return (($r as jni.JObject).castTo(const jni.JObjectType()) + ..setAsDeleted()) + .reference; } if ($d == r"manyPrimitives(IZCD)J") { final $r = _$methods[$p]![$d]!( @@ -3002,7 +3006,7 @@ class MyInterface<$T extends jni.JObject> extends jni.JObject { .castTo(const jni.JDoubleType(), deleteOriginal: true) .doubleValue(deleteOriginal: true), ); - return jni.JLong($r).reference; + return (jni.JLong($r)..setAsDeleted()).reference; } return jni.nullptr; } diff --git a/jnigen/test/simple_package_test/dart_only/dart_bindings/simple_package.dart b/jnigen/test/simple_package_test/dart_only/dart_bindings/simple_package.dart index 7befe1b9..ef24ce3e 100644 --- a/jnigen/test/simple_package_test/dart_only/dart_bindings/simple_package.dart +++ b/jnigen/test/simple_package_test/dart_only/dart_bindings/simple_package.dart @@ -2818,13 +2818,17 @@ class MyInterface<$T extends jni.JObject> extends jni.JObject { final $r = _$methods[$p]![$d]!( $a[0].castTo(const jni.JStringType(), deleteOriginal: true), ); - return $r.reference; + return (($r as jni.JObject).castTo(const jni.JObjectType()) + ..setAsDeleted()) + .reference; } if ($d == r"varCallback(Ljava/lang/Object;)Ljava/lang/Object;") { final $r = _$methods[$p]![$d]!( $a[0].castTo(_$types[$p]!["T"]!, deleteOriginal: true), ); - return $r.reference; + return (($r as jni.JObject).castTo(const jni.JObjectType()) + ..setAsDeleted()) + .reference; } if ($d == r"manyPrimitives(IZCD)J") { final $r = _$methods[$p]![$d]!( @@ -2841,7 +2845,7 @@ class MyInterface<$T extends jni.JObject> extends jni.JObject { .castTo(const jni.JDoubleType(), deleteOriginal: true) .doubleValue(deleteOriginal: true), ); - return jni.JLong($r).reference; + return (jni.JLong($r)..setAsDeleted()).reference; } return jni.nullptr; }