[ffi] Make nativeFunction throw if already closed

This is a *breaking change*. See
https://github.com/dart-lang/sdk/issues/53311

Make nativeFunction throw a StateError if the NativeCallable is already
closed. Also make close and keepIsolateAlive not throw.

I'm not adding an isClosed getter, as discussed on the bug.

Bug: https://github.com/dart-lang/sdk/issues/53311
Fixes: https://github.com/dart-lang/sdk/issues/53311
Change-Id: Ib0066352d3cfc01d31df8ae8fd61be426fcdf6e1
CoreLibraryReviewExempt: The FFI package is VM-only
TEST=async_void_function_callbacks_test and isolate_local_function_callbacks_test
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/328280
Reviewed-by: Daco Harkes <dacoharkes@google.com>
Commit-Queue: Liam Appelbe <liama@google.com>

CHANGELOG.md

@@ -105,6 +105,9 @@ constraint][language version] lower bound to 3.2 or greater (`sdk: '^3.2.0'`).
 - All `NativeCallable` constructors can now accept closures. Previously
   `NativeCallable`s had the same restrictions as `Pointer.fromFunction`, and
   could only create callbacks for static functions.
+- **Breaking change** [#53311][]: `NativeCallable.nativeFunction` now throws an
+  error if is called after the `NativeCallable` has already been `close`d. Calls
+  to `close` after the first are now ignored.
 
 #### `dart:io`
 

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

@@ -227,16 +227,34 @@ abstract final class _NativeCallableBase<T extends Function>
   _NativeCallableBase(this._pointer);
 
   @override
-  Pointer<NativeFunction<T>> get nativeFunction => _pointer;
+  Pointer<NativeFunction<T>> get nativeFunction {
+    if (_isClosed) {
+      throw StateError("NativeCallable is already closed.");
+    }
+    return _pointer;
+  }
 
   @override
   void close() {
-    if (_pointer == nullptr) {
-      throw StateError("NativeCallable is already closed.");
+    if (!_isClosed) {
+      _deleteNativeCallable(_pointer);
+      _pointer = nullptr;
     }
-    _deleteNativeCallable(_pointer);
-    _pointer = nullptr;
   }
+
+  @override
+  void set keepIsolateAlive(bool value) {
+    if (!_isClosed) {
+      _setKeepIsolateAlive(value);
+    }
+  }
+
+  @override
+  bool get keepIsolateAlive => _isClosed ? false : _getKeepIsolateAlive();
+
+  void _setKeepIsolateAlive(bool value);
+  bool _getKeepIsolateAlive();
+  bool get _isClosed => _pointer == nullptr;
 }
 
 final class _NativeCallableIsolateLocal<T extends Function>
@@ -252,13 +270,6 @@ final class _NativeCallableIsolateLocal<T extends Function>
   }
 
   @override
-  void set keepIsolateAlive(bool value) {
-    if (_pointer == nullptr) {
-      throw StateError("NativeCallable is already closed.");
-    }
-    _setKeepIsolateAlive(value);
-  }
-
   void _setKeepIsolateAlive(bool value) {
     if (_keepIsolateAlive != value) {
       _keepIsolateAlive = value;
@@ -267,7 +278,7 @@ final class _NativeCallableIsolateLocal<T extends Function>
   }
 
   @override
-  bool get keepIsolateAlive => _keepIsolateAlive;
+  bool _getKeepIsolateAlive() => _keepIsolateAlive;
 }
 
 final class _NativeCallableListener<T extends Function>
@@ -286,12 +297,12 @@ final class _NativeCallableListener<T extends Function>
   }
 
   @override
-  void set keepIsolateAlive(bool value) {
+  void _setKeepIsolateAlive(bool value) {
     _port.keepIsolateAlive = value;
   }
 
   @override
-  bool get keepIsolateAlive => _port.keepIsolateAlive;
+  bool _getKeepIsolateAlive() => _port.keepIsolateAlive;
 }
 
 @patch

sdk/lib/ffi/ffi.dart

@@ -270,15 +270,15 @@ abstract final class NativeCallable<T extends Function> {
   /// The native function pointer which can be used to invoke the `callback`
   /// passed to the constructor.
   ///
-  /// If this receiver has been [close]d, the pointer is a [nullptr].
+  /// This pointer must not be read after the callable has been [close]d.
   Pointer<NativeFunction<T>> get nativeFunction;
 
   /// Closes this callback and releases its resources.
   ///
   /// Further calls to existing [nativeFunction]s will result in undefined
-  /// behavior. New accesses to [nativeFunction] will give a [nullptr].
+  /// behavior.
   ///
-  /// This method must not be called more than once on each native callback.
+  /// Subsequent calls to [close] will be ignored.
   ///
   /// It is safe to call [close] inside the [callback].
   void close();

tests/ffi/async_void_function_callbacks_test.dart

@@ -89,10 +89,15 @@ testNativeCallableDoubleCloseError() {
   final callback = NativeCallable<CallbackNativeType>.listener(simpleFunction);
   Expect.notEquals(nullptr, callback.nativeFunction);
   callback.close();
-  Expect.equals(nullptr, callback.nativeFunction);
+
   Expect.throwsStateError(() {
-    callback.close();
+    final _ = callback.nativeFunction;
   });
+
+  // Expect that these do not throw.
+  callback.close();
+  callback.keepIsolateAlive = true;
+  Expect.isFalse(callback.keepIsolateAlive);
 }
 
 Future<void> testNativeCallableUseAfterFree() async {
@@ -129,7 +134,9 @@ Future<void> testNativeCallableNestedCloseCall() async {
   Expect.equals(1123, await simpleFunctionResult.future);
 
   // The callback is already closed.
-  Expect.equals(nullptr, simpleFunctionAndCloseSelf_callable!.nativeFunction);
+  Expect.throwsStateError(() {
+    final _ = simpleFunctionAndCloseSelf_callable!.nativeFunction;
+  });
 }
 
 void simpleFunctionThrows(int a, int b) {

tests/ffi/isolate_local_function_callbacks_test.dart

@@ -84,10 +84,15 @@ testNativeCallableDoubleCloseError() {
       exceptionalReturn: 0);
   Expect.notEquals(nullptr, callback.nativeFunction);
   callback.close();
-  Expect.equals(nullptr, callback.nativeFunction);
+
   Expect.throwsStateError(() {
-    callback.close();
+    final _ = callback.nativeFunction;
   });
+
+  // Expect that these do not throw.
+  callback.close();
+  callback.keepIsolateAlive = true;
+  Expect.isFalse(callback.keepIsolateAlive);
 }
 
 late NativeCallable selfClosingStaticCallback;
@@ -105,7 +110,9 @@ testNativeCallableNestedCloseCallStatic() {
       callTwoIntFunction(selfClosingStaticCallback.nativeFunction, 1000, 234));
 
   // The callback is already closed.
-  Expect.equals(nullptr, selfClosingStaticCallback.nativeFunction);
+  Expect.throwsStateError(() {
+    final _ = selfClosingStaticCallback.nativeFunction;
+  });
 }
 
 testNativeCallableNestedCloseCallClosure() {
@@ -122,7 +129,9 @@ testNativeCallableNestedCloseCallClosure() {
   Expect.equals(1234, callTwoIntFunction(callback.nativeFunction, 1000, 234));
 
   // The callback is already closed.
-  Expect.equals(nullptr, callback.nativeFunction);
+  Expect.throwsStateError(() {
+    final _ = callback.nativeFunction;
+  });
 }
 
 int throwerCallback(int a, int b) {