Fix windows specifics and global reference ownership in interface imp…

…lementation (#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.
dart-archive · Jul 31, 2023 · b6f97bf · b6f97bf
1 parent f4fb338
commit b6f97bf
Show file tree

Hide file tree

Showing 13 changed files with 132 additions and 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
@@ -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
@@ -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));
   }
-}
+  // 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, "<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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -1534,8 +1534,9 @@ class _InterfaceMethodIf extends Visitor<Method, void> {
 
   @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,21 +1589,33 @@ class _InterfaceParamCast extends Visitor<Param, void> {
 /// 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<String> {
   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
   String visitPrimitiveType(PrimitiveType node) {
     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
@@ -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
@@ -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
@@ -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
@@ -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;
   }