[ffigen] Retain listener blocks before invocation (#1574)

pkgs/ffigen/CHANGELOG.md

@@ -10,6 +10,9 @@
   rather than just being incorporated into the child protocol. If you want those
   implementation bindings, you may need to add the super protocol to your
   `objc-protocols` filters.
+- Fix a bug where ObjC listener blocks could be deleted after being invoked by
+  ObjC but before the invocation was received by Dart:
+  https://github.com/dart-lang/native/issues/1571
 
 ## 14.0.1
 

pkgs/ffigen/lib/src/code_generator/objc_block.dart

@@ -103,11 +103,18 @@ class ObjCBlock extends BindingType {
         w.topLevelUniqueNamer.makeUnique('_${name}_fnPtrTrampoline');
     final closureTrampoline =
         w.topLevelUniqueNamer.makeUnique('_${name}_closureTrampoline');
+    final funcPtrCallable =
+        w.topLevelUniqueNamer.makeUnique('_${name}_fnPtrCallable');
+    final closureCallable =
+        w.topLevelUniqueNamer.makeUnique('_${name}_closureCallable');
+    final listenerCallable =
+        w.topLevelUniqueNamer.makeUnique('_${name}_listenerCallable');
     final callExtension =
         w.topLevelUniqueNamer.makeUnique('${name}_CallExtension');
     final newPointerBlock = ObjCBuiltInFunctions.newPointerBlock.gen(w);
     final newClosureBlock = ObjCBuiltInFunctions.newClosureBlock.gen(w);
     final getBlockClosure = ObjCBuiltInFunctions.getBlockClosure.gen(w);
+    final releaseFn = ObjCBuiltInFunctions.objectRelease.gen(w);
     final trampFuncType = FunctionType(returnType: returnType, parameters: [
       Parameter(type: blockPtr, name: 'block', objCConsumed: false),
       ...params
@@ -124,6 +131,8 @@ class ObjCBlock extends BindingType {
     final returnFfiDartType = returnType.getFfiDartType(w);
     final blockCType = blockPtr.getCType(w);
     final blockType = _blockType(w);
+    final defaultValue = returnType.getDefaultValue(w);
+    final exceptionalReturn = defaultValue == null ? '' : ', $defaultValue';
 
     final paramsNameOnly = params.map((p) => p.name).join(', ');
     final paramsFfiDartType =
@@ -136,14 +145,29 @@ class ObjCBlock extends BindingType {
 $returnFfiDartType $funcPtrTrampoline($blockCType block, $paramsFfiDartType) =>
     block.ref.target.cast<${natFnType.getFfiDartType(w)}>()
         .asFunction<$funcFfiDartType>()($paramsNameOnly);
+$voidPtr $funcPtrCallable = ${w.ffiLibraryPrefix}.Pointer.fromFunction<
+    $trampFuncCType>($funcPtrTrampoline $exceptionalReturn).cast();
 ''');
 
     // Write the closure based trampoline function.
     s.write('''
 $returnFfiDartType $closureTrampoline($blockCType block, $paramsFfiDartType) =>
     ($getBlockClosure(block) as $funcFfiDartType)($paramsNameOnly);
+$voidPtr $closureCallable = ${w.ffiLibraryPrefix}.Pointer.fromFunction<
+    $trampFuncCType>($closureTrampoline $exceptionalReturn).cast();
 ''');
 
+    if (hasListener) {
+      // Write the listener trampoline function.
+      s.write('''
+$nativeCallableType $listenerCallable = $nativeCallableType.listener(
+    ($blockCType block, $paramsFfiDartType) {
+  ($getBlockClosure(block) as $funcFfiDartType)($paramsNameOnly);
+  $releaseFn(block.cast());
+} $exceptionalReturn)..keepIsolateAlive = false;
+''');
+    }
+
     // Snippet that converts a Dart typed closure to FfiDart type. This snippet
     // is used below. Note that the closure being converted is called `fn`.
     final convertedFnArgs = params
@@ -162,8 +186,6 @@ $returnFfiDartType $closureTrampoline($blockCType block, $paramsFfiDartType) =>
     final convFn = '($paramsFfiDartType) => $convFnInvocation';
 
     // Write the wrapper class.
-    final defaultValue = returnType.getDefaultValue(w);
-    final exceptionalReturn = defaultValue == null ? '' : ', $defaultValue';
     s.write('''
 
 /// Construction methods for `$blockType`.
@@ -179,24 +201,17 @@ abstract final class $name {
   /// the isolate that registered it. Invoking the block on the wrong thread
   /// will result in a crash.
   static $blockType fromFunctionPointer($natFnPtr ptr) =>
-      $blockType($newPointerBlock(
-          _cFuncTrampoline ??= ${w.ffiLibraryPrefix}.Pointer.fromFunction<
-              $trampFuncCType>($funcPtrTrampoline
-                  $exceptionalReturn).cast(), ptr.cast()),
+      $blockType($newPointerBlock($funcPtrCallable, ptr.cast()),
           retain: false, release: true);
-  static $voidPtr? _cFuncTrampoline;
 
   /// Creates a block from a Dart function.
   ///
   /// This block must be invoked by native code running on the same thread as
   /// the isolate that registered it. Invoking the block on the wrong thread
   /// will result in a crash.
   static $blockType fromFunction($funcDartType fn) =>
-      $blockType($newClosureBlock(
-          _dartFuncTrampoline ??= ${w.ffiLibraryPrefix}.Pointer.fromFunction<
-              $trampFuncCType>($closureTrampoline $exceptionalReturn).cast(),
-          $convFn), retain: false, release: true);
-  static $voidPtr? _dartFuncTrampoline;
+      $blockType($newClosureBlock($closureCallable, $convFn),
+          retain: false, release: true);
 ''');
 
     // Listener block constructor is only available for void blocks.
@@ -216,8 +231,7 @@ abstract final class $name {
       );
       final listenerConvFn =
           '($paramsFfiDartType) => $listenerConvFnInvocation';
-      final wrapFn = _wrapListenerBlock?.func.name;
-      final releaseFn = ObjCBuiltInFunctions.objectRelease.gen(w);
+      final wrapFn = _wrapListenerBlock!.func.name;
 
       s.write('''
 
@@ -232,21 +246,11 @@ abstract final class $name {
   /// blocks do not keep the isolate alive.
   static $blockType listener($funcDartType fn) {
     final raw = $newClosureBlock(
-        (_dartFuncListenerTrampoline ??= $nativeCallableType.listener(
-            $closureTrampoline $exceptionalReturn)..keepIsolateAlive =
-                false).nativeFunction.cast(), $listenerConvFn);''');
-      if (wrapFn != null) {
-        s.write('''
+        $listenerCallable.nativeFunction.cast(), $listenerConvFn);
     final wrapper = $wrapFn(raw);
     $releaseFn(raw.cast());
-    return $blockType(wrapper, retain: false, release: true);''');
-      } else {
-        s.write('''
-    return $blockType(raw, retain: false, release: true);''');
-      }
-      s.write('''
+    return $blockType(wrapper, retain: false, release: true);
   }
-  static $nativeCallableType? _dartFuncListenerTrampoline;
 ''');
     }
     s.write('}\n\n');
@@ -303,6 +307,7 @@ ref.pointer.ref.invoke.cast<$natTrampFnType>().asFunction<$trampFuncFfiDartType>
 typedef $blockTypedef;
 $blockName $fnName($blockName block) NS_RETURNS_RETAINED {
   return ^void($argStr) {
+    ${generateRetain('block')};
     block(${retains.join(', ')});
   };
 }

pkgs/ffigen/lib/src/code_generator/objc_built_in_functions.dart

@@ -146,21 +146,16 @@ class ObjCBuiltInFunctions {
   ObjCListenerBlockTrampoline? getListenerBlockTrampoline(ObjCBlock block) {
     assert(!_depsAdded);
 
-    var needsTrampoline = false;
     final paramIds = <String>[];
     for (final param in block.params) {
       final retainFunc = param.type.generateRetain('');
-      if (retainFunc != null) {
-        needsTrampoline = true;
-      }
 
       // The trampoline ID is based on the getNativeType of the param. Objects
       // and blocks both have `id` as their native type, but need separate
       // trampolines since they have different retain functions. So add the
       // retainFunc (if any) to all the param IDs.
       paramIds.add('${param.getNativeType()}-${retainFunc ?? ''}');
     }
-    if (!needsTrampoline) return null;
     final id = paramIds.join(',');
 
     return _blockTrampolines[id] ??= ObjCListenerBlockTrampoline(Func(

pkgs/ffigen/pubspec.yaml

@@ -36,6 +36,7 @@ dev_dependencies:
   coverage: ^1.8.0
   dart_flutter_team_lints: ^2.0.0
   json_schema: ^5.1.1
+  leak_tracker: ^10.0.7
   meta: ^1.11.0
   objective_c: ^2.0.0
   test: ^1.16.2

pkgs/ffigen/test/native_objc_test/block_test.dart

@@ -684,6 +684,45 @@ void main() {
       expect(objCBindings, contains('Vec2'));
       expect(objCBindings, contains('Vec4'));
     });
+
+    (BlockTester, Pointer<ObjCBlockImpl>, Pointer<ObjCObject>) regress1571Inner(
+        Completer<void> completer) {
+      final dummyObject = DummyObject.new1();
+      DartObjectListenerBlock? block =
+          ObjectListenerBlock.listener((DummyObject obj) {
+        expect(objectRetainCount(obj.ref.pointer), 1);
+        completer.complete();
+        expect(dummyObject, isNotNull);
+      });
+      final tester = BlockTester.newFromListener_(block);
+      expect(blockRetainCount(block.ref.pointer), 2);
+      expect(objectRetainCount(dummyObject.ref.pointer), 1);
+      return (tester, block.ref.pointer, dummyObject.ref.pointer);
+    }
+
+    test('Regression test for https://github.com/dart-lang/native/issues/1571',
+        () async {
+      // Pass a listener block to an ObjC API that retains a reference to the
+      // block, and release the Dart-side reference. Then, on a different
+      // thread, invoke the block and immediately release the ObjC-side
+      // reference. Before the fix, the dtor message could arrive before the
+      // invoke message. This was a flaky error, so try a few times.
+      for (int i = 0; i < 10; ++i) {
+        final completer = Completer<void>();
+        final (tester, blockPtr, objectPtr) = regress1571Inner(completer);
+
+        await flutterDoGC();
+        expect(blockRetainCount(blockPtr), 1);
+        expect(objectRetainCount(objectPtr), 1);
+
+        tester.invokeAndReleaseListenerOnNewThread();
+        await completer.future;
+
+        await flutterDoGC();
+        expect(blockRetainCount(blockPtr), 0);
+        expect(objectRetainCount(objectPtr), 0);
+      }
+    });
   });
 }
 

pkgs/ffigen/test/native_objc_test/block_test.h

@@ -50,9 +50,11 @@ typedef void (^NoTrampolineListenerBlock)(int32_t, Vec4, const char*);
 // blocks in Objective C code.
 @interface BlockTester : NSObject {
   __strong IntBlock myBlock;
+  __strong ObjectListenerBlock myListener;
 }
 + (BlockTester*)newFromBlock:(IntBlock)block;
 + (BlockTester*)newFromMultiplier:(int32_t)mult;
++ (BlockTester*)newFromListener:(ObjectListenerBlock)block;
 - (int32_t)call:(int32_t)x;
 - (IntBlock)getBlock NS_RETURNS_RETAINED;
 - (void)pokeBlock;
@@ -75,4 +77,6 @@ typedef void (^NoTrampolineListenerBlock)(int32_t, Vec4, const char*);
 + (void)callNoTrampolineListener:(NoTrampolineListenerBlock)block;
 + (IntBlock)newBlock:(BlockBlock)block withMult:(int)mult NS_RETURNS_RETAINED;
 + (BlockBlock)newBlockBlock:(int)mult NS_RETURNS_RETAINED;
+- (void)invokeAndReleaseListenerOnNewThread;
+- (void)invokeAndReleaseListener:(id)_;
 @end

pkgs/ffigen/test/native_objc_test/block_test.m

@@ -48,6 +48,12 @@ + (BlockTester*)newFromMultiplier:(int32_t)mult {
   return bt;
 }
 
++ newFromListener:(ObjectListenerBlock)block {
+  BlockTester* bt = [BlockTester new];
+  bt->myListener = block;
+  return bt;
+}
+
 - (int32_t)call:(int32_t)x {
   return myBlock(x);
 }
@@ -177,4 +183,15 @@ + (BlockBlock)newBlockBlock:(int)mult NS_RETURNS_RETAINED {
   };
 }
 
+- (void)invokeAndReleaseListenerOnNewThread {
+  [[[NSThread alloc] initWithTarget:self
+                           selector:@selector(invokeAndReleaseListener:)
+                             object:nil] start];
+}
+
+- (void)invokeAndReleaseListener:(id)_ {
+  myListener([DummyObject new]);
+  myListener = nil;
+}
+
 @end

pkgs/ffigen/test/native_objc_test/isolate_config.yaml

@@ -1,13 +1,15 @@
 name: IsolateTestObjCLibrary
 description: 'Tests sending objects between isolates'
 language: objc
-output: 'isolate_bindings.dart'
+output:
+  bindings: 'isolate_bindings.dart'
+  objc-bindings: 'isolate_bindings.m'
 exclude-all-by-default: true
 objc-interfaces:
   include:
     - Sendable
 headers:
   entry-points:
-    - 'isolate_test.m'
+    - 'isolate_test.h'
 preamble: |
   // ignore_for_file: camel_case_types, non_constant_identifier_names, unnecessary_non_null_assertion, unused_element, unused_field

pkgs/ffigen/test/native_objc_test/isolate_test.h

@@ -0,0 +1,12 @@
+// Copyright (c) 2024, the Dart project authors. Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+#import <Foundation/NSObject.h>
+
+typedef void (^Listener)(int32_t);
+
+@interface Sendable : NSObject {}
+@property int32_t value;
++ (Listener)dummyMethodToForceGenerationOfListener;
+@end

pkgs/ffigen/test/native_objc_test/isolate_test.m

@@ -2,16 +2,8 @@
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
-#import <Foundation/NSObject.h>
-
+#include "isolate_test.h"
 #include "util.h"
 
-typedef void (^Listener)(int32_t);
-
-@interface Sendable : NSObject {}
-@property int32_t value;
-+ (Listener)dummyMethodToForceGenerationOfListener;
-@end
-
 @implementation Sendable
 @end

pkgs/ffigen/test/native_objc_test/protocol_config.yaml

@@ -1,7 +1,9 @@
 name: ProtocolTestObjCLibrary
 description: 'Tests implementing protocols'
 language: objc
-output: 'protocol_bindings.dart'
+output:
+  bindings: 'protocol_bindings.dart'
+  objc-bindings: 'protocol_bindings.m'
 exclude-all-by-default: true
 objc-interfaces:
   include:

pkgs/ffigen/test/native_objc_test/protocol_test.h

@@ -5,8 +5,6 @@
 #import <Foundation/NSObject.h>
 #import <Foundation/NSString.h>
 
-#include "util.h"
-
 typedef struct {
   int32_t x;
   int32_t y;

pkgs/ffigen/test/native_objc_test/util.dart

@@ -7,6 +7,7 @@ import 'dart:io';
 
 import 'package:ffi/ffi.dart';
 import 'package:ffigen/ffigen.dart';
+import 'package:leak_tracker/leak_tracker.dart' as leak_tracker;
 import 'package:logging/logging.dart' show Level;
 import 'package:objective_c/objective_c.dart';
 import 'package:objective_c/src/internal.dart' as internal_for_testing
@@ -39,11 +40,19 @@ final _executeInternalCommand = () {
 bool canDoGC = _executeInternalCommand != null;
 
 void doGC() {
-  final gcNow = "gc-now".toNativeUtf8();
+  final gcNow = 'gc-now'.toNativeUtf8();
   _executeInternalCommand!(gcNow.cast(), nullptr);
   calloc.free(gcNow);
 }
 
+// Dart_ExecuteInternalCommand("gc-now") doesn't work on flutter, so we use
+// leak_tracker's forceGC function instead. It's less reliable, and to combat
+// that we need to wait for quite a long time, which breaks autorelease pools.
+Future<void> flutterDoGC() async {
+  await leak_tracker.forceGC();
+  await Future<void>.delayed(Duration(milliseconds: 500));
+}
+
 @Native<Bool Function(Pointer<Void>)>(isLeaf: true, symbol: 'isReadableMemory')
 external bool _isReadableMemory(Pointer<Void> ptr);