[vm/ffi] Introduce pragma's 'vm:shareable' and 'vm:shared' and make NativeFinalizers live in an isolate group instead of isolate.

[dcharkes]

For NativeFinalizers it would be really useful if they could live in an isolate group rather than in an isolate. #55050 (comment) (TL;DR: Migrating NativeFinalizers to the longest living isolate, or doing refcounting can both not be encapsulated. Artificially keeping all NativeFinalizer objects alive until the isolate group dies leads to a memory leak.)

In order to achieve NativeFinalizers living in an isolate group instead of an isolated, we'd like to be able to mark static fields as shared across an isolate group.

We introduce two pragmas:

• pragma('vm:sharable') Marks a type as being sharable, cannot be used by users, only by Dart SDK code. Dart SDK developers would need to make sure that all code reachable from such type is concurrency safe.
• pragma('vm:shared') Marks a static field as shared across isolates in an isolate groups. Can be used by users. The type of the field must be marked pragma('vm:sharable'), all its subtypes must be shareable too, and the class must be final or sealed.

Users would then be able to write

@pragma('vm:deeply-immutable')
final class MyFinalizable implements Finalizable {
  @pragma('vm:shared')
  static final NativeFinalizer _finalizer = ...;

  MyFinalizable(...) {
    _finalizer.attach(this, ...);
  }
}

void foo(SendPort sendPort) {
  sendPort.send(MyFinalizable(...));
}

Whichever isolate lives the longest would keep the NativeFinalizer alive. No need to migrate the NativeFinalizer to the longest living isolate with Isolate.exit #55050 (comment).

We might need to introduce a NativeFinalizer.shared(...) constructor to signify that this finalizer is shared across isolates. Or NativeFinalizer.concurrent(...).

@mraleph This is implementing a subset of dart-lang/language#3531. But not as a general language feature but for a single type in the VM for which this enables some use cases.

cc @mkustermann @HosseinYousefi

[mkustermann]

My suggestion to use deeply immutable objects and finalizers was about a specific use case.

So let's look at this use case first:

class JObject {
  final _JGlobalRef ref;
}
class JFoo extends JObject {
  void foo() {}
  void baz() {}
}

void foo(JFoo foo) async {
  await runOnPlatformThread(() {
    foo.bar();
    foo.baz();
  });
}

The thought is: If _JGlobalRef ref is a deeply immutable object, shared across isolates, we can attach a finalizer and it will be safely sharable, the finalizer will free the global reference once it's no longer used.

If we go this route we have to first think carefully about what this means:

• we cannot allow _JLocalRef to be sent across isolates (if we have local refs?)
• users eagerly freeing _JGlobalRef must guarantee that the helper isolates no longer hold on to those references (do we think this may be a problem?)
• do we see any use case of one isolate creating a _JGlobalRef and another trying to eagerly delete it (maybe not - users could make new handles from existing handles and attach/detach the new handles in their isolate)

@HosseinYousefi wdyt?

If we do it like this, the next question is how to implement it.

• The deeply immutable object can be done via pragma or marker interface + static validation
• Then we have to decide how to handle the finalizers
  • Using Dart_NewFinalizableHandle & Dart_DeleteFinalizable: Those are isolate-group global and would provide already today the semantics we'd need for this to work
  • Using NativeFinalizer: We'd need to think whether we could provide a global native finalizer in dart:ffi that will be alive in the isolate group or whether we need something like NativeFinalizer.shared() - e.g. the sketch above (sharable static fields would be also usable for other use cases)

[HosseinYousefi]

The thought is: If _JGlobalRef ref is a deeply immutable object, shared across isolates, we can attach a finalizer and it will be safely sharable, the finalizer will free the global reference once it's no longer used.

Yes, I think this is the way forward.

The concern is if we create this object in a helper isolate, send it to the main isolate, and then exit the helper isolate, the static native finalizer in the helper isolate eagerly gets called for all of the attached objects.

• Using Dart_NewFinalizableHandle & Dart_DeleteFinalizable: Those are isolate-group global and would provide already today the semantics we'd need for this to work

I didn't know about these. But they could work since they're isolate-group global.

[dcharkes]

• users eagerly freeing _JGlobalRef must guarantee that the helper isolates no longer hold on to those references (do we think this may be a problem?)

If _JGlobalRef is deeply immutable, we cannot keep an internal bool to track that we have called delete on it. (Which would cause every subsequent method invocation to throw a state error.)
If we add such field to the wrapper object, it would be indeed useless across helper isolates.
(The only way to track whether native resources have been eagerly freed across multiple isolates is sharing a mutable object across isolates.)

• Using Dart_NewFinalizableHandle & Dart_DeleteFinalizable: Those are isolate-group global and would provide already today the semantics we'd need for this to work

That's a great alternative. 👍

[mkustermann]

If _JGlobalRef is deeply immutable, we cannot keep an internal bool to track that we have called delete on it. (Which would cause every subsequent method invocation to throw a state error.)

This _JGlobalRef could have a final Pointer<Bool> bool field pointing to malloc()ed data containing a value we update using atomics :) (But whether that's a good idea is another question)

[dcharkes]

If _JGlobalRef is deeply immutable, we cannot keep an internal bool to track that we have called delete on it. (Which would cause every subsequent method invocation to throw a state error.)

This _JGlobalRef could have a final Pointer<Bool> bool field pointing to malloc()ed data containing a value we update using atomics :) (But whether that's a good idea is another question)

When would we free that though?

[mkustermann]

If _JGlobalRef is deeply immutable, we cannot keep an internal bool to track that we have called delete on it. (Which would cause every subsequent method invocation to throw a state error.)

This _JGlobalRef could have a final Pointer bool field pointing to malloc()ed data containing a value we update using atomics :) (But whether that's a good idea is another question)

When would we free that though?

With another finalizable handle 😆 which would always be attached and always be managed by GC.
This one would only occur a cost of a few bytes (so ok to delay freeing) as opposed to the java global ref (which may hold on to lots of memory, so good to eagerly free - and can do so only once). A cleaner way would be to introduce e.g. an final Atomic<IntPtr> state. Then the logic could be a bit like this

@immutabale
class _JavaGlobalRefValue implements Finalizable {
  final Pointer<Void> javaHandlePointer;
  final Atomic<IntPtr> state = Atomic<IntPtr>(); // -1 if freed, 0 if unused, positive: number of users
}

@immutable
class _JGlobalRef {
  static const kFreed = -1;

  final _JGlobalRefValue javaHandle;
  final Pointer<Void> dartFinalizableHandle;
  
  _JGlobalRef(localHandle)
        : javaHandle = _JavaGlobalRefValue(javaNewGlobalHandle(localHandle)),
          dartFinalizableHandle = createFinalizableHandle(javaHandle, ...);  // global handle freed by default by GC

  void startUse() {
    int currentValue = 0;
    while (true) {
      (currentValue, success) = javaHandle.state.compareAndSwap(currentValue, currentValue + 1);
      if (success) return;
      if (currentValue == kFreed) throw 'Global ref is no longer valid.';
    }
  }

  void endUse() {
    final oldValue = javaHandle.state.atomicDecrement();
    assert(oldValue > 0);
  }

  void eagerFree() {
    if (!javaHandle.state.compareAndSwap(0, kFreed)) throw 'Ref is currently in use (maybe by another thread?)';
    deleteFinalizableHandle(dartFinalizableHandle, javaHandle);
    javaDeleteGlobalRef(javaHandle.javaHandlePointer);
  }
}

JObject foo(JFoo obj) {
 obj.ref.startUse();  // if it doesn't throw, safe to use
 try {
   <perform jni call>
 } finally {
   obj.ref.endUse();
 }
}

Doing this would shift incorrectly eager freeing (user eagerly frees java global ref even though the same isolate or other isolates will use the global ref afterwards)

• from undefined behavior (e.g. double-free of java global ref, use-after-free of java global ref)
• to dart exceptions (which shouldn't happen in practice)

In the end for an app to work properly the code that does eager freeing must know what it's doing.

[dcharkes]

👍 Yes, and then we don't allow eager free on _JavaGlobalRefValue.

As long as we haven't implemented Atomic<IntPtr> yet in Dart, we can achieve the same thing by just making C functions for it in package:jni's C code.

@HosseinYousefi WDYT? I believe you should then be unblocked after https://dart-review.googlesource.com/c/sdk/+/354902 (still WIP at this point).

[HosseinYousefi]

JObject foo(JFoo obj) {
 obj.ref.startUse();  // if it doesn't throw, safe to use
 try {
   <perform jni call>
 } finally {
   obj.ref.endUse();
 }
}

Maybe the implementation is easier to be done in C. All the JNI APIs would get a "jobject_ext" instead of jobject which is a struct of atomic intptr_t and jobject. This way all the existing code still works and users can use package:jni directly without knowing about startUse and endUse.

[dcharkes]

My understanding of using Dart code to prevent use after free is the following:

1. We allocate some native memory for the atomic counter that helps us know if we're trying to use after free.
2. That memory backing the atomic counter is freed by a native finalizer that is attached to the object _JGlobalRef implements Finalizable wrapping the deeply immutable finalizable (_JavaGlobalRefValue implements Finalizable).

@HosseinYousefi Are you suggesting we keep that structure, but we do the atomic counter increase/decrease all in the JNI calls? And still the Dart wrapper object (_JGlobalRef) has a native finalizer to free the native memory containing the counter? I believe that should work as long as our eager delete is just a release that doesn't take into account counting.

Maybe what @mkustermann was trying to suggest here was to prevent the eager delete() to actually release the java object in between a startUse() and endUse(). I'm not sure if we want to support that use case. It feels like maybe that should be built on top of package:jni instead of built in? WDYT?