[ffi/isolate_group] Move IsolateGroupBound callbacks from isolate to isolate group.

This allows creation of isolategroup-bound callbacks when isolate is not available. For example, to support proposed Isolate::onEvent/Isolate::handleEvent api https://github.com/dart-lang/language/blob/main/working/333%20-%20shared%20memory%20multithreading/shared_native_memory.md#additional-isolate-apis.

BUG=https://github.com/dart-lang/sdk/issues/63291
TEST=isolate_group_bound_callback_test

CoreLibraryReviewExempt: vm ffi-specific comments changes
Change-Id: I9a757a39ba8e152db64f59a66d337ca672103277
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/499661
Reviewed-by: Ryan Macnak <rmacnak@google.com>
Commit-Queue: Alexander Aprelev <aam@google.com>

runtime/lib/ffi.cc

@@ -52,7 +52,8 @@ DEFINE_NATIVE_ENTRY(Ffi_createNativeCallableIsolateGroupBound, 1, 2) {
       Function::CheckedHandle(zone, arguments->NativeArg0());
   const auto& target = Closure::CheckedHandle(zone, arguments->NativeArgAt(1));
   return Pointer::New(
-      isolate->CreateIsolateGroupBoundFfiCallback(zone, trampoline, target));
+      thread->isolate_group()->CreateIsolateGroupBoundFfiCallback(
+          zone, trampoline, target));
 }
 
 DEFINE_NATIVE_ENTRY(Ffi_deleteNativeCallable, 1, 1) {
@@ -61,6 +62,12 @@ DEFINE_NATIVE_ENTRY(Ffi_deleteNativeCallable, 1, 1) {
   return Object::null();
 }
 
+DEFINE_NATIVE_ENTRY(Ffi_deleteIsolateGroupNativeCallable, 1, 1) {
+  const auto& pointer = Pointer::CheckedHandle(zone, arguments->NativeArg0());
+  thread->isolate_group()->DeleteFfiCallback(pointer.NativeAddress());
+  return Object::null();
+}
+
 DEFINE_NATIVE_ENTRY(Ffi_updateNativeCallableKeepIsolateAliveCounter, 1, 1) {
   const int64_t delta =
       Integer::CheckedHandle(zone, arguments->NativeArg0()).Value();

runtime/vm/bootstrap_natives.h

@@ -302,6 +302,7 @@ namespace dart {
   V(Ffi_createNativeCallableIsolateLocal, 3)                                   \
   V(Ffi_createNativeCallableIsolateGroupBound, 2)                              \
   V(Ffi_deleteNativeCallable, 1)                                               \
+  V(Ffi_deleteIsolateGroupNativeCallable, 1)                                   \
   V(Ffi_updateNativeCallableKeepIsolateAliveCounter, 1)                        \
   V(Ffi_dl_open, 1)                                                            \
   V(Ffi_dl_close, 1)                                                           \

runtime/vm/isolate.cc

@@ -2705,6 +2705,13 @@ void Isolate::LowLevelCleanup(Isolate* isolate) {
       FinalizeWeakPersistentHandlesVisitor visitor(isolate_group);
       isolate_group->api_state()->VisitWeakHandlesUnlocked(&visitor);
 
+      // Clean up any synchronous FFI callbacks registered with this
+      // isolate group. Skip if this isolate group never registered any.
+      if (isolate_group->ffi_callback_list_head_ != nullptr) {
+        FfiCallbackMetadata::Instance()->DeleteAllCallbacks(
+            &isolate_group->ffi_callback_list_head_);
+      }
+
       Thread::ExitIsolateGroupAsHelper(/*bypass_safepoint=*/false);
     }
 
@@ -3817,16 +3824,20 @@ FfiCallbackMetadata::Trampoline Isolate::CreateIsolateLocalFfiCallback(
       &ffi_callback_list_head_);
 }
 
-// TODO(aam): Should this be in IsolateGroup?
-FfiCallbackMetadata::Trampoline Isolate::CreateIsolateGroupBoundFfiCallback(
-    Zone* zone,
-    const Function& trampoline,
-    const Closure& target) {
+FfiCallbackMetadata::Trampoline
+IsolateGroup::CreateIsolateGroupBoundFfiCallback(Zone* zone,
+                                                 const Function& trampoline,
+                                                 const Closure& target) {
   return FfiCallbackMetadata::Instance()->CreateLocalFfiCallback(
-      /*isolate=*/nullptr, group(), zone, trampoline, target,
+      /*isolate=*/nullptr, this, zone, trampoline, target,
       &ffi_callback_list_head_);
 }
 
+void IsolateGroup::DeleteFfiCallback(FfiCallbackMetadata::Trampoline callback) {
+  FfiCallbackMetadata::Instance()->DeleteCallback(callback,
+                                                  &ffi_callback_list_head_);
+}
+
 bool Isolate::HasLivePorts() {
   ASSERT(0 <= open_ports_ && 0 <= open_ports_keepalive_ &&
          open_ports_keepalive_ <= open_ports_);

runtime/vm/isolate.h

@@ -846,6 +846,12 @@ class IsolateGroup : public IntrusiveDListEntry<IsolateGroup> {
     return thread_locals_count_.fetch_add(1u, std::memory_order_relaxed);
   }
 
+  FfiCallbackMetadata::Trampoline CreateIsolateGroupBoundFfiCallback(
+      Zone* zone,
+      const Function& trampoline,
+      const Closure& target);
+  void DeleteFfiCallback(FfiCallbackMetadata::Trampoline callback);
+
  private:
   friend class Dart;  // For `object_store_ = ` in Dart::Init
   friend class Heap;
@@ -1008,6 +1014,8 @@ class IsolateGroup : public IntrusiveDListEntry<IsolateGroup> {
   std::atomic<intptr_t> thread_locals_count_ = 0;
 
   std::unique_ptr<Roots> roots_;
+
+  FfiCallbackMetadata::MetadataEntry* ffi_callback_list_head_ = nullptr;
 };
 
 // When an isolate sends-and-exits this class represent things that it passed
@@ -1338,10 +1346,6 @@ class Isolate : public IntrusiveDListEntry<Isolate> {
       const Function& trampoline,
       const Closure& target,
       bool keep_isolate_alive);
-  FfiCallbackMetadata::Trampoline CreateIsolateGroupBoundFfiCallback(
-      Zone* zone,
-      const Function& trampoline,
-      const Closure& target);
   void DeleteFfiCallback(FfiCallbackMetadata::Trampoline callback);
   void UpdateNativeCallableKeepIsolateAliveCounter(intptr_t delta);
   bool HasOpenNativeCallables();

sdk/lib/_internal/vm/lib/ffi_patch.dart

@@ -203,6 +203,11 @@ external void _deleteNativeCallable<NS extends NativeFunction>(
   Pointer<NS> pointer,
 );
 
+@pragma("vm:external-name", "Ffi_deleteIsolateGroupNativeCallable")
+external void _deleteIsolateGroupNativeCallable<NS extends NativeFunction>(
+  Pointer<NS> pointer,
+);
+
 @pragma("vm:external-name", "Ffi_updateNativeCallableKeepIsolateAliveCounter")
 external void _updateNativeCallableKeepIsolateAliveCounter<
   NS extends NativeFunction
@@ -348,28 +353,40 @@ final class _NativeCallableListener<T extends Function>
 }
 
 final class _NativeCallableIsolateGroupBound<T extends Function>
-    extends _NativeCallableBase<T> {
-  bool _isKeepingIsolateAlive = true;
+    implements NativeCallable<T> {
+  Pointer<NativeFunction<T>> _pointer;
 
-  _NativeCallableIsolateGroupBound(super._pointer) {
-    _updateNativeCallableKeepIsolateAliveCounter(1);
+  _NativeCallableIsolateGroupBound(this._pointer);
+
+  @override
+  Pointer<NativeFunction<T>> get nativeFunction {
+    if (_isClosed) {
+      throw StateError("NativeCallable is already closed.");
+    }
+    return _pointer;
   }
 
   @override
-  void _close() {
-    _keepIsolateAlive = false;
-  }
-
-  @override
-  void set _keepIsolateAlive(bool value) {
-    if (_isKeepingIsolateAlive != value) {
-      _isKeepingIsolateAlive = value;
-      _updateNativeCallableKeepIsolateAliveCounter(value ? 1 : -1);
+  void close() {
+    if (!_isClosed) {
+      _deleteIsolateGroupNativeCallable(_pointer);
+      _pointer = nullptr;
     }
   }
 
   @override
-  bool get _keepIsolateAlive => _isKeepingIsolateAlive;
+  void set keepIsolateAlive(bool value) {
+    if (value) {
+      throw ArgumentError(
+        "IsolateGroupBound callables can not keep isolate alive",
+      );
+    }
+  }
+
+  @override
+  bool get keepIsolateAlive => false;
+
+  bool get _isClosed => _pointer == nullptr;
 }
 
 @patch

sdk/lib/ffi/ffi.dart

@@ -414,8 +414,7 @@ abstract final class NativeCallable<T extends Function> {
   /// _trivially shareable_.
   ///
   /// This callback must be [close]d when it is no longer
-  /// needed. An [Isolate] that created the callback will
-  /// be kept alive until [close] is called.
+  /// needed.
   ///
   /// After [NativeCallable.close] is called, invoking
   /// the [nativeFunction] from native code will cause
@@ -522,6 +521,10 @@ abstract final class NativeCallable<T extends Function> {
   /// By default, [NativeCallable]s keep the [Isolate] that created them alive
   /// until [close] is called. If [keepIsolateAlive] is set to `false`, the
   /// isolate may exit even if the [NativeCallable] isn't closed.
+  ///
+  /// Note: IsolateGroupBound-callbacks are not associated with an [Isolate],
+  /// so won't keep an isolate alive. Attempts to set [keepIsolateAlive] to
+  /// true, will throw.
   external bool get keepIsolateAlive;
   external set keepIsolateAlive(bool value);
 }

tests/ffi/isolate_group_bound_callback_test.dart

@@ -249,31 +249,18 @@ void testNativeCallableAccessNonSharedVar() {
   callback.close();
 }
 
-Future<void> testKeepIsolateAliveTrue() async {
-  mutexCondvar = Mutex();
-  conditionVariable = ConditionVariable();
-  ReceivePort rpOnExit = ReceivePort("onExit");
-  unawaited(
-    Isolate.spawn(
-      (_) async {
-        final callback = NativeCallable<CallbackNativeType>.isolateGroupBound(
-          simpleFunction,
-        );
-        callback.keepIsolateAlive = true;
-      },
-      /*message=*/ null,
-      onExit: rpOnExit.sendPort,
-    ),
+Future<void> testKeepIsolateAliveTrueThrows() async {
+  final callback = NativeCallable<CallbackNativeType>.isolateGroupBound(
+    simpleFunction,
   );
-  try {
-    await rpOnExit.first.timeout(Duration(seconds: 5));
-    // should not fall through, should throw TimeoutException
-    Expect.isTrue(false);
-  } catch (e) {
-    print('testKeepIsolateAliveTrue caught $e');
-    Expect.isTrue(e is TimeoutException);
-  }
-  rpOnExit.close();
+  Expect.throwsArgumentError(() {
+    callback.keepIsolateAlive = true;
+  });
+  print(callback.nativeFunction);
+  callback.close();
+  Expect.throwsStateError(() {
+    print(callback.nativeFunction);
+  });
 }
 
 Future<void> testKeepIsolateAliveFalse() async {
@@ -312,7 +299,7 @@ main(args, message) async {
   testNativeCallableSync();
   testNativeCallableSyncThrows();
   testNativeCallableAccessNonSharedVar();
-  await testKeepIsolateAliveTrue();
+  await testKeepIsolateAliveTrueThrows();
   await testKeepIsolateAliveFalse();
   asyncEnd();
   print("All tests completed :)");