Use finalizable handles and vm:deeply-immutable pragma instead of NativeFinalizers for JReference

Author: HosseinYousefi

pkgs/jni/java/src/main/java/com/github/dart_lang/jni/JniUtils.java

@@ -0,0 +1,5 @@
+package com.github.dart_lang.jni;
+
+public class JniUtils {
+  public static native Object fromReferenceAddress(long id);
+}

pkgs/jni/lib/src/jni.dart

@@ -62,12 +62,17 @@ abstract final class Jni {
     _dylibDir = dylibDir;
   }
 
+  static bool _initialized = false;
+
   /// Initializes DartApiDL used for Continuations and interface implementation.
   static void initDLApi() {
-    assert(NativeApi.majorVersion == 2);
-    assert(NativeApi.minorVersion >= 3);
-    final result = _bindings.InitDartApiDL(NativeApi.initializeApiDLData);
-    assert(result == 0);
+    if (!_initialized) {
+      assert(NativeApi.majorVersion == 2);
+      assert(NativeApi.minorVersion >= 3);
+      final result = _bindings.InitDartApiDL(NativeApi.initializeApiDLData);
+      _initialized = result == 0;
+      assert(_initialized);
+    }
   }
 
   /// Spawn an instance of JVM using JNI. This method should be called at the
@@ -267,6 +272,21 @@ extension ProtectedJniExtensions on Jni {
       Pointer<CallbackResult> result, JObjectPtr object) async {
     Jni._bindings.resultFor(result, object);
   }
+
+  static Dart_FinalizableHandle newFinalizableHandle(
+    Object object,
+    Pointer<Void> reference,
+    int refType,
+  ) {
+    Jni.initDLApi();
+    return Jni._bindings.newFinalizableHandle(object, reference, refType);
+  }
+
+  static void deleteFinalizableHandle(
+      Dart_FinalizableHandle finalizableHandle, Object object) {
+    Jni.initDLApi();
+    Jni._bindings.deleteFinalizableHandle(finalizableHandle, object);
+  }
 }
 
 extension AdditionalEnvMethods on GlobalJniEnv {

pkgs/jni/lib/src/jreference.dart

@@ -11,11 +11,9 @@ import 'jni.dart';
 
 extension ProtectedJReference on JReference {
   void setAsReleased() {
-    if (_released) {
-      throw DoubleReleaseError();
-    }
-    _released = true;
-    JGlobalReference._finalizer.detach(this);
+    ProtectedJniExtensions.deleteFinalizableHandle(
+        _finalizableHandle, _finalizable);
+    // FIXME: No [DoubleReleaseError] will be thrown.
   }
 
   void ensureNotNull() {
@@ -29,15 +27,25 @@ extension ProtectedJReference on JReference {
   /// Detaches the finalizer so the underlying pointer will not be deleted.
   JObjectPtr toPointer() {
     setAsReleased();
-    return _pointer;
+    return _finalizable._pointer;
   }
 }
 
+@pragma('vm:deeply-immutable')
+final class _JFinalizable implements Finalizable {
+  final Pointer<Void> _pointer;
+
+  _JFinalizable(this._pointer);
+}
+
+@pragma('vm:deeply-immutable')
 abstract final class JReference {
-  final JObjectPtr _pointer;
-  bool _released = false;
+  final _JFinalizable _finalizable;
+  final Dart_FinalizableHandle _finalizableHandle;
 
-  JReference(this._pointer);
+  JReference(this._finalizable, int kind)
+      : _finalizableHandle = ProtectedJniExtensions.newFinalizableHandle(
+            _finalizable, _finalizable._pointer, kind);
 
   /// The underlying JNI reference.
   ///
@@ -46,12 +54,13 @@ abstract final class JReference {
   /// Be careful when storing this in a variable since it might have gotten
   /// released upon use.
   JObjectPtr get pointer {
-    if (_released) throw UseAfterReleaseError();
-    return _pointer;
+    // FIXME: No [UseAfterReleaseError] will be thrown.
+    return _finalizable._pointer;
   }
 
   /// Whether the underlying JNI reference is deleted or not.
-  bool get isReleased => _released;
+  // FIXME: releasing does not work.
+  bool get isReleased => false;
 
   /// Whether the underlying JNI reference is `null` or not.
   bool get isNull;
@@ -72,27 +81,26 @@ abstract final class JReference {
 /// A managed JNI global reference.
 ///
 /// Uses a [NativeFinalizer] to delete the JNI global reference when finalized.
-final class JGlobalReference extends JReference implements Finalizable {
-  static final _finalizer =
-      NativeFinalizer(Jni.env.ptr.ref.DeleteGlobalRef.cast());
-
-  JGlobalReference(super._reference) {
-    _finalizer.attach(this, _pointer, detach: this);
-  }
+@pragma('vm:deeply-immutable')
+final class JGlobalReference extends JReference {
+  JGlobalReference(Pointer<Void> pointer)
+      : super(_JFinalizable(pointer), JObjectRefType.JNIGlobalRefType);
 
   @override
   bool get isNull => pointer == nullptr;
 
   @override
   void _deleteReference() {
-    Jni.env.DeleteGlobalRef(_pointer);
+    Jni.env.DeleteGlobalRef(_finalizable._pointer);
   }
 }
 
-final jNullReference = _JNullReference();
+final JReference jNullReference = _JNullReference();
 
+@pragma('vm:deeply-immutable')
 final class _JNullReference extends JReference {
-  _JNullReference() : super(nullptr);
+  _JNullReference()
+      : super(_JFinalizable(nullptr), JObjectRefType.JNIInvalidRefType);
 
   @override
   void _deleteReference() {

pkgs/jni/lib/src/third_party/jni_bindings_generated.dart

@@ -242,6 +242,42 @@ class JniBindings {
   late final _resultFor = _resultForPtr
       .asFunction<void Function(ffi.Pointer<CallbackResult>, JObjectPtr)>();
 
+  Dart_FinalizableHandle newFinalizableHandle(
+    Object object,
+    JObjectPtr reference,
+    int refType,
+  ) {
+    return _newFinalizableHandle(
+      object,
+      reference,
+      refType,
+    );
+  }
+
+  late final _newFinalizableHandlePtr = _lookup<
+      ffi.NativeFunction<
+          Dart_FinalizableHandle Function(
+              ffi.Handle, JObjectPtr, ffi.Int32)>>('newFinalizableHandle');
+  late final _newFinalizableHandle = _newFinalizableHandlePtr
+      .asFunction<Dart_FinalizableHandle Function(Object, JObjectPtr, int)>();
+
+  void deleteFinalizableHandle(
+    Dart_FinalizableHandle finalizableHandle,
+    Object object,
+  ) {
+    return _deleteFinalizableHandle(
+      finalizableHandle,
+      object,
+    );
+  }
+
+  late final _deleteFinalizableHandlePtr = _lookup<
+      ffi.NativeFunction<
+          ffi.Void Function(
+              Dart_FinalizableHandle, ffi.Handle)>>('deleteFinalizableHandle');
+  late final _deleteFinalizableHandle = _deleteFinalizableHandlePtr
+      .asFunction<void Function(Dart_FinalizableHandle, Object)>();
+
   ffi.Pointer<GlobalJniEnvStruct> GetGlobalEnv() {
     return _GetGlobalEnv();
   }
@@ -2021,6 +2057,10 @@ final class _opaque_pthread_cond_t extends ffi.Struct {
   external ffi.Array<ffi.Char> __opaque;
 }
 
+typedef Dart_FinalizableHandle = ffi.Pointer<_Dart_FinalizableHandle>;
+
+final class _Dart_FinalizableHandle extends ffi.Opaque {}
+
 final class GlobalJniEnvStruct extends ffi.Struct {
   external ffi.Pointer<ffi.Void> reserved0;
 

pkgs/jni/src/dartjni.c

@@ -8,8 +8,6 @@
 
 #include "dartjni.h"
 
-#include "include/dart_api_dl.h"
-
 void initAllLocks(JniLocks* locks) {
   init_lock(&locks->classLoadingLock);
   init_lock(&locks->fieldLoadingLock);
@@ -659,6 +657,46 @@ void resultFor(CallbackResult* result, jobject object) {
   release_lock(&result->lock);
 }
 
+void doNotFinalize(void* isolate_callback_data, void* peer) {}
+
+void finalizeLocal(void* isolate_callback_data, void* peer) {
+  attach_thread();
+  (*jniEnv)->DeleteLocalRef(jniEnv, peer);
+}
+
+void finalizeGlobal(void* isolate_callback_data, void* peer) {
+  attach_thread();
+  (*jniEnv)->DeleteGlobalRef(jniEnv, peer);
+}
+
+void finalizeWeakGlobal(void* isolate_callback_data, void* peer) {
+  attach_thread();
+  (*jniEnv)->DeleteWeakGlobalRef(jniEnv, peer);
+}
+
+FFI_PLUGIN_EXPORT
+Dart_FinalizableHandle newFinalizableHandle(Dart_Handle object,
+                                            jobject reference,
+                                            jobjectRefType refType) {
+  switch (refType) {
+    case JNIInvalidRefType:
+      return Dart_NewFinalizableHandle_DL(object, reference, 0, doNotFinalize);
+    case JNILocalRefType:
+      return Dart_NewFinalizableHandle_DL(object, reference, 0, finalizeLocal);
+    case JNIGlobalRefType:
+      return Dart_NewFinalizableHandle_DL(object, reference, 0, finalizeGlobal);
+    case JNIWeakGlobalRefType:
+      return Dart_NewFinalizableHandle_DL(object, reference, 0,
+                                          finalizeWeakGlobal);
+  }
+}
+
+FFI_PLUGIN_EXPORT
+void deleteFinalizableHandle(Dart_FinalizableHandle finalizableHandle,
+                             Dart_Handle object) {
+  return Dart_DeleteFinalizableHandle_DL(finalizableHandle, object);
+}
+
 jclass _c_Object = NULL;
 jclass _c_Long = NULL;
 
@@ -747,3 +785,11 @@ Java_com_github_dart_1lang_jni_PortCleaner_clean(JNIEnv* env,
   close_signal.type = Dart_CObject_kNull;
   Dart_PostCObject_DL(port, &close_signal);
 }
+
+JNIEXPORT jobject JNICALL
+Java_com_github_dart_1lang_jni_JniUtils_fromReferenceAddress(JNIEnv* env,
+                                                             jclass clazz,
+                                                             jlong id) {
+  attach_thread();
+  return (jobject)(id);
+}

pkgs/jni/src/dartjni.h

@@ -5,6 +5,8 @@
 #pragma once
 
 // Note: include appropriate system jni.h as found by CMake, not third_party/jni.h.
+#include "include/dart_api_dl.h"
+
 #include <jni.h>
 #include <stdint.h>
 #include <stdio.h>
@@ -422,3 +424,12 @@ JniResult PortProxy__newInstance(jobject binaryName,
                                  int64_t functionPtr);
 
 FFI_PLUGIN_EXPORT void resultFor(CallbackResult* result, jobject object);
+
+FFI_PLUGIN_EXPORT
+Dart_FinalizableHandle newFinalizableHandle(Dart_Handle object,
+                                            jobject reference,
+                                            jobjectRefType refType);
+
+FFI_PLUGIN_EXPORT
+void deleteFinalizableHandle(Dart_FinalizableHandle finalizableHandle,
+                             Dart_Handle object);

pkgs/jni/test/load_test.dart

@@ -67,7 +67,7 @@ void run({required TestRunnerCallback testRunner}) {
         // The actual expect here does not matter. I am just being paranoid
         // against assigning to `_` because compiler may optimize it. (It has
         // side effect of calling FFI but still.)
-        expect(random.reference, isNot(nullptr));
+        expect(random.reference.pointer, isNot(nullptr));
       });
     }
   });
@@ -76,7 +76,7 @@ void run({required TestRunnerCallback testRunner}) {
       () {
     for (int i = 0; i < k256; i++) {
       final random = newRandom();
-      expect(random.reference, isNot(nullptr));
+      expect(random.reference.pointer, isNot(nullptr));
       random.release();
     }
   });
@@ -86,7 +86,7 @@ void run({required TestRunnerCallback testRunner}) {
       using((arena) {
         for (int i = 0; i < 256; i++) {
           final r = newRandom()..releasedBy(arena);
-          expect(r.reference, isNot(nullptr));
+          expect(r.reference.pointer, isNot(nullptr));
         }
       });
     }
@@ -103,6 +103,20 @@ void run({required TestRunnerCallback testRunner}) {
     }
   });
 
+  void testFinalizer() {
+    testRunner('Finalizer correctly removes the references', () {
+      // We are checking if we can run this for large number of times.
+      // More than the number of available global references.
+      for (var i = 0; i < k256; ++i) {
+        final random = newRandom();
+        expect(random.reference.pointer, isNot(nullptr));
+        if (i % k4 == 0) {
+          doGC();
+        }
+      }
+    });
+  }
+
   void testRefValidityAfterGC(int delayInSeconds) {
     testRunner('Validate reference after GC & ${delayInSeconds}s sleep', () {
       final random = newRandom();
@@ -122,6 +136,7 @@ void run({required TestRunnerCallback testRunner}) {
 
   // Dart_ExecuteInternalCommand doesn't exist in Android.
   if (!Platform.isAndroid) {
+    testFinalizer();
     testRefValidityAfterGC(1);
     testRefValidityAfterGC(10);
   }