Delete the Dart functions associated with closure blocks (#1100)

* ObjC example, and infra for shipping native code with plugin

* Fix the block leak

* Mostly working

* Fix analysis

* Fix analysis

* Fix example linker error

* fmt

* Daco's comments

* Fix NSData

Author: liamappelbe

.github/labeler.yml

@@ -27,3 +27,7 @@
 'package:native_toolchain_c':
   - changed-files:
       - any-glob-to-any-file: 'pkgs/native_toolchain_c/**'
+
+'package:objective_c':
+  - changed-files:
+      - any-glob-to-any-file: 'pkgs/objective_c/**'

.github/workflows/objective_c.yaml

@@ -57,6 +57,9 @@ jobs:
           channel: 'stable'
       - name: Install dependencies
         run: flutter pub get
+      - name: Build test dylib
+        # TODO(https://github.com/dart-lang/native/issues/1068): Remove this.
+        run: dart test/setup.dart
       - name: Run VM tests
         run: dart test
       - name: Collect coverage

pkgs/ffigen/.gitignore

@@ -4,6 +4,8 @@
 .dart_tool/
 .packages
 pubspec.lock
+.flutter-plugins
+.flutter-plugins-dependencies
 
 # IDE and debugger files.
 .clangd

pkgs/ffigen/CHANGELOG.md

@@ -27,6 +27,8 @@
       generated bindings.
 - Add --[no-]format option to ffigen command line, which controls whether the
   formatting step happens. Defaults to true.
+- Delete Dart functions associated with ObjC closure blocks when the block is
+  destroyed. Fixes https://github.com/dart-lang/native/issues/204
 
 ## 11.0.0
 

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

@@ -58,12 +58,9 @@ class ObjCBlock extends BindingType {
         w.topLevelUniqueNamer.makeUnique('_${name}_fnPtrTrampoline');
     final closureTrampoline =
         w.topLevelUniqueNamer.makeUnique('_${name}_closureTrampoline');
-    final registerClosure =
-        w.topLevelUniqueNamer.makeUnique('_${name}_registerClosure');
-    final closureRegistry =
-        w.topLevelUniqueNamer.makeUnique('_${name}_closureRegistry');
-    final closureRegistryIndex =
-        w.topLevelUniqueNamer.makeUnique('_${name}_closureRegistryIndex');
+    final newPointerBlock = ObjCBuiltInFunctions.newPointerBlock.gen(w);
+    final newClosureBlock = ObjCBuiltInFunctions.newClosureBlock.gen(w);
+    final getBlockClosure = ObjCBuiltInFunctions.getBlockClosure.gen(w);
     final trampFuncType = FunctionType(
         returnType: returnType,
         parameters: [Parameter(type: blockPtr, name: 'block'), ...params]);
@@ -92,21 +89,10 @@ $returnFfiDartType $funcPtrTrampoline($blockCType block, $paramsFfiDartType) =>
         .asFunction<$funcFfiDartType>()($paramsNameOnly);
 ''');
 
-    // Write the closure registry function.
-    s.write('''
-final $closureRegistry = <int, $funcFfiDartType>{};
-int $closureRegistryIndex = 0;
-$voidPtr $registerClosure($funcFfiDartType fn) {
-  final id = ++$closureRegistryIndex;
-  $closureRegistry[id] = fn;
-  return $voidPtr.fromAddress(id);
-}
-''');
-
     // Write the closure based trampoline function.
     s.write('''
 $returnFfiDartType $closureTrampoline($blockCType block, $paramsFfiDartType) =>
-    $closureRegistry[block.ref.target.address]!($paramsNameOnly);
+    ($getBlockClosure(block) as $funcFfiDartType)($paramsNameOnly);
 ''');
 
     // Snippet that converts a Dart typed closure to FfiDart type. This snippet
@@ -141,7 +127,7 @@ class $name extends ${ObjCBuiltInFunctions.blockBase.gen(w)} {
   /// the isolate that registered it. Invoking the block on the wrong thread
   /// will result in a crash.
   $name.fromFunctionPointer($natFnPtr ptr) :
-      this._(${ObjCBuiltInFunctions.newBlock.gen(w)}(
+      this._($newPointerBlock(
           _cFuncTrampoline ??= ${w.ffiLibraryPrefix}.Pointer.fromFunction<
               $trampFuncCType>($funcPtrTrampoline
                   $exceptionalReturn).cast(), ptr.cast()));
@@ -153,10 +139,10 @@ class $name extends ${ObjCBuiltInFunctions.blockBase.gen(w)} {
   /// the isolate that registered it. Invoking the block on the wrong thread
   /// will result in a crash.
   $name.fromFunction($funcDartType fn) :
-      this._(${ObjCBuiltInFunctions.newBlock.gen(w)}(
+      this._($newClosureBlock(
           _dartFuncTrampoline ??= ${w.ffiLibraryPrefix}.Pointer.fromFunction<
-              $trampFuncCType>($closureTrampoline
-                  $exceptionalReturn).cast(), $registerClosure($convFn)));
+              $trampFuncCType>($closureTrampoline $exceptionalReturn).cast(),
+          $convFn));
   static $voidPtr? _dartFuncTrampoline;
 
 ''');
@@ -174,11 +160,10 @@ class $name extends ${ObjCBuiltInFunctions.blockBase.gen(w)} {
   /// Note that unlike the default behavior of NativeCallable.listener, listener
   /// blocks do not keep the isolate alive.
   $name.listener($funcDartType fn) :
-      this._(${ObjCBuiltInFunctions.newBlock.gen(w)}(
+      this._($newClosureBlock(
           (_dartFuncListenerTrampoline ??= $nativeCallableType.listener(
               $closureTrampoline $exceptionalReturn)..keepIsolateAlive =
-                  false).nativeFunction.cast(),
-          $registerClosure($convFn)));
+                  false).nativeFunction.cast(), $convFn));
   static $nativeCallableType? _dartFuncListenerTrampoline;
 
 ''');

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

@@ -19,7 +19,9 @@ class ObjCBuiltInFunctions {
   static const msgSendFpretPointer = ObjCImport('msgSendFpretPointer');
   static const msgSendStretPointer = ObjCImport('msgSendStretPointer');
   static const useMsgSendVariants = ObjCImport('useMsgSendVariants');
-  static const newBlock = ObjCImport('newBlock');
+  static const newPointerBlock = ObjCImport('newPointerBlock');
+  static const newClosureBlock = ObjCImport('newClosureBlock');
+  static const getBlockClosure = ObjCImport('getBlockClosure');
   static const objectBase = ObjCImport('ObjCObjectBase');
   static const blockBase = ObjCImport('ObjCBlockBase');
 

pkgs/ffigen/test/native_objc_test/automated_ref_count_test.dart

@@ -23,6 +23,8 @@ void main() {
   group('Automatic reference counting', () {
     setUpAll(() {
       logWarnings();
+      // TODO(https://github.com/dart-lang/native/issues/1068): Remove this.
+      DynamicLibrary.open('../objective_c/test/objective_c.dylib');
       final dylib =
           File('test/native_objc_test/automated_ref_count_test.dylib');
       verifySetupFile(dylib);

pkgs/ffigen/test/native_objc_test/bad_method_test.dart

@@ -17,6 +17,8 @@ void main() {
   group('bad_method_test', () {
     setUpAll(() {
       logWarnings();
+      // TODO(https://github.com/dart-lang/native/issues/1068): Remove this.
+      DynamicLibrary.open('../objective_c/test/objective_c.dylib');
       final dylib = File('test/native_objc_test/bad_method_test.dylib');
       verifySetupFile(dylib);
       DynamicLibrary.open(dylib.absolute.path);

pkgs/ffigen/test/native_objc_test/block_test.dart

@@ -12,6 +12,8 @@ import 'dart:ffi';
 import 'dart:io';
 
 import 'package:ffi/ffi.dart';
+import 'package:objective_c/src/internal.dart' as internal_for_testing
+    show blockHasRegisteredClosure;
 import 'package:test/test.dart';
 
 import '../test_utils.dart';
@@ -34,6 +36,8 @@ void main() {
   group('Blocks', () {
     setUpAll(() {
       logWarnings();
+      // TODO(https://github.com/dart-lang/native/issues/1068): Remove this.
+      DynamicLibrary.open('../objective_c/test/objective_c.dylib');
       final dylib = File('test/native_objc_test/block_test.dylib');
       verifySetupFile(dylib);
       lib = BlockTestObjCLibrary(DynamicLibrary.open(dylib.absolute.path));
@@ -229,6 +233,8 @@ void main() {
     Pointer<Void> funcPointerBlockRefCountTest() {
       final block =
           IntBlock.fromFunctionPointer(Pointer.fromFunction(_add100, 999));
+      expect(
+          internal_for_testing.blockHasRegisteredClosure(block.pointer), false);
       expect(lib.getBlockRetainCount(block.pointer.cast()), 1);
       return block.pointer.cast();
     }
@@ -241,18 +247,25 @@ void main() {
 
     Pointer<Void> funcBlockRefCountTest() {
       final block = IntBlock.fromFunction(makeAdder(4000));
+      expect(
+          internal_for_testing.blockHasRegisteredClosure(block.pointer), true);
       expect(lib.getBlockRetainCount(block.pointer.cast()), 1);
       return block.pointer.cast();
     }
 
-    test('Function block ref counting', () {
+    test('Function block ref counting', () async {
       final rawBlock = funcBlockRefCountTest();
       doGC();
+      await Future<void>.delayed(Duration.zero); // Let dispose message arrive.
       expect(lib.getBlockRetainCount(rawBlock), 0);
+      expect(internal_for_testing.blockHasRegisteredClosure(rawBlock.cast()),
+          false);
     });
 
     Pointer<Void> blockManualRetainRefCountTest() {
       final block = IntBlock.fromFunction(makeAdder(4000));
+      expect(
+          internal_for_testing.blockHasRegisteredClosure(block.pointer), true);
       expect(lib.getBlockRetainCount(block.pointer.cast()), 1);
       final rawBlock = block.retainAndReturnPointer().cast<Void>();
       expect(lib.getBlockRetainCount(rawBlock.cast()), 2);
@@ -265,13 +278,16 @@ void main() {
       return lib.getBlockRetainCount(block.pointer.cast());
     }
 
-    test('Block ref counting with manual retain and release', () {
+    test('Block ref counting with manual retain and release', () async {
       final rawBlock = blockManualRetainRefCountTest();
       doGC();
       expect(lib.getBlockRetainCount(rawBlock), 1);
       expect(blockManualRetainRefCountTest2(rawBlock), 1);
       doGC();
+      await Future<void>.delayed(Duration.zero); // Let dispose message arrive.
       expect(lib.getBlockRetainCount(rawBlock), 0);
+      expect(internal_for_testing.blockHasRegisteredClosure(rawBlock.cast()),
+          false);
     });
 
     (Pointer<Void>, Pointer<Void>, Pointer<Void>)
@@ -474,7 +490,7 @@ void main() {
       expect(descPtr.ref.reserved, 0);
       expect(descPtr.ref.size, isNot(0));
       expect(descPtr.ref.copy_helper, nullptr);
-      expect(descPtr.ref.dispose_helper, nullptr);
+      expect(descPtr.ref.dispose_helper, isNot(nullptr));
       expect(descPtr.ref.signature, nullptr);
     });
   });

pkgs/ffigen/test/native_objc_test/cast_test.dart

@@ -21,6 +21,8 @@ void main() {
   group('cast', () {
     setUpAll(() {
       logWarnings();
+      // TODO(https://github.com/dart-lang/native/issues/1068): Remove this.
+      DynamicLibrary.open('../objective_c/test/objective_c.dylib');
       final dylib = File('test/native_objc_test/cast_test.dylib');
       verifySetupFile(dylib);
       DynamicLibrary.open(dylib.absolute.path);

pkgs/ffigen/test/native_objc_test/category_test.dart

@@ -19,6 +19,8 @@ void main() {
   group('categories', () {
     setUpAll(() {
       logWarnings();
+      // TODO(https://github.com/dart-lang/native/issues/1068): Remove this.
+      DynamicLibrary.open('../objective_c/test/objective_c.dylib');
       final dylib = File('test/native_objc_test/category_test.dylib');
       verifySetupFile(dylib);
       DynamicLibrary.open(dylib.absolute.path);

pkgs/ffigen/test/native_objc_test/forward_decl_test.dart

@@ -17,6 +17,8 @@ void main() {
   group('forward decl', () {
     setUpAll(() {
       logWarnings();
+      // TODO(https://github.com/dart-lang/native/issues/1068): Remove this.
+      DynamicLibrary.open('../objective_c/test/objective_c.dylib');
       final dylib = File('test/native_objc_test/forward_decl_test.dylib');
       verifySetupFile(dylib);
       DynamicLibrary.open(dylib.absolute.path);

pkgs/ffigen/test/native_objc_test/inherited_instancetype_test.dart

@@ -19,6 +19,8 @@ void main() {
   group('inheritedInstancetype', () {
     setUpAll(() {
       logWarnings();
+      // TODO(https://github.com/dart-lang/native/issues/1068): Remove this.
+      DynamicLibrary.open('../objective_c/test/objective_c.dylib');
       final dylib =
           File('test/native_objc_test/inherited_instancetype_test.dylib');
       verifySetupFile(dylib);

pkgs/ffigen/test/native_objc_test/is_instance_test.dart

@@ -20,6 +20,8 @@ void main() {
   group('isInstance', () {
     setUpAll(() {
       logWarnings();
+      // TODO(https://github.com/dart-lang/native/issues/1068): Remove this.
+      DynamicLibrary.open('../objective_c/test/objective_c.dylib');
       final dylib = File('test/native_objc_test/is_instance_test.dylib');
       verifySetupFile(dylib);
       DynamicLibrary.open(dylib.absolute.path);

pkgs/ffigen/test/native_objc_test/method_test.dart

@@ -20,6 +20,8 @@ void main() {
   group('method calls', () {
     setUpAll(() {
       logWarnings();
+      // TODO(https://github.com/dart-lang/native/issues/1068): Remove this.
+      DynamicLibrary.open('../objective_c/test/objective_c.dylib');
       final dylib = File('test/native_objc_test/method_test.dylib');
       verifySetupFile(dylib);
       DynamicLibrary.open(dylib.absolute.path);

pkgs/ffigen/test/native_objc_test/native_objc_test.dart

@@ -18,6 +18,8 @@ void main() {
   group('native_objc_test', () {
     setUpAll(() {
       logWarnings();
+      // TODO(https://github.com/dart-lang/native/issues/1068): Remove this.
+      DynamicLibrary.open('../objective_c/test/objective_c.dylib');
       final dylib = File('test/native_objc_test/native_objc_test.dylib');
       verifySetupFile(dylib);
       DynamicLibrary.open(dylib.absolute.path);

pkgs/ffigen/test/native_objc_test/nullable_inheritance_test.dart

@@ -22,6 +22,8 @@ void main() {
   group('Nullable inheritance', () {
     setUpAll(() {
       logWarnings();
+      // TODO(https://github.com/dart-lang/native/issues/1068): Remove this.
+      DynamicLibrary.open('../objective_c/test/objective_c.dylib');
       final dylib =
           File('test/native_objc_test/nullable_inheritance_test.dylib');
       verifySetupFile(dylib);

pkgs/ffigen/test/native_objc_test/nullable_test.dart

@@ -21,6 +21,8 @@ void main() {
   group('Nullability', () {
     setUpAll(() {
       logWarnings();
+      // TODO(https://github.com/dart-lang/native/issues/1068): Remove this.
+      DynamicLibrary.open('../objective_c/test/objective_c.dylib');
       final dylib = File('test/native_objc_test/nullable_test.dylib');
       verifySetupFile(dylib);
       DynamicLibrary.open(dylib.absolute.path);

pkgs/ffigen/test/native_objc_test/property_test.dart

@@ -20,6 +20,8 @@ void main() {
   group('properties', () {
     setUpAll(() {
       logWarnings();
+      // TODO(https://github.com/dart-lang/native/issues/1068): Remove this.
+      DynamicLibrary.open('../objective_c/test/objective_c.dylib');
       final dylib = File('test/native_objc_test/property_test.dylib');
       verifySetupFile(dylib);
       DynamicLibrary.open(dylib.absolute.path);

pkgs/ffigen/test/native_objc_test/rename_test.dart

@@ -17,6 +17,8 @@ void main() {
   group('rename_test', () {
     setUpAll(() {
       logWarnings();
+      // TODO(https://github.com/dart-lang/native/issues/1068): Remove this.
+      DynamicLibrary.open('../objective_c/test/objective_c.dylib');
       final dylib = File('test/native_objc_test/rename_test.dylib');
       verifySetupFile(dylib);
       DynamicLibrary.open(dylib.absolute.path);

pkgs/ffigen/test/native_objc_test/setup.dart

@@ -48,21 +48,24 @@ Future<void> _buildSwift(
   print('Generated files: $outputHeader and $outputLib');
 }
 
-Future<void> _generateBindings(String config) async {
-  final args = [
-    'run',
-    'ffigen',
-    '--config',
-    'test/native_objc_test/$config',
-  ];
+Future<void> _runDart(List<String> args) async {
   final process =
       await Process.start(Platform.executable, args, workingDirectory: '../..');
   unawaited(stdout.addStream(process.stdout));
   unawaited(stderr.addStream(process.stderr));
   final result = await process.exitCode;
   if (result != 0) {
-    throw ProcessException('dart', args, 'Generating bindings', result);
+    throw ProcessException('dart', args, 'Running Dart command', result);
   }
+}
+
+Future<void> _generateBindings(String config) async {
+  await _runDart([
+    'run',
+    'ffigen',
+    '--config',
+    'test/native_objc_test/$config',
+  ]);
   print('Generated bindings for: $config');
 }
 
@@ -129,5 +132,6 @@ Future<void> main(List<String> arguments) async {
     return await clean(_getTestNames());
   }
 
+  await _runDart(['../objective_c/test/setup.dart']);
   return await build(arguments.isNotEmpty ? arguments : _getTestNames());
 }

pkgs/ffigen/test/native_objc_test/static_func_native_test.dart

@@ -26,6 +26,8 @@ void main() {
   group('static functions', () {
     setUpAll(() {
       logWarnings();
+      // TODO(https://github.com/dart-lang/native/issues/1068): Remove this.
+      DynamicLibrary.open('../objective_c/test/objective_c.dylib');
       final dylib = File('test/native_objc_test/static_func_test.dylib');
       verifySetupFile(dylib);
       DynamicLibrary.open(dylib.absolute.path);

pkgs/ffigen/test/native_objc_test/static_func_test.dart

@@ -28,6 +28,8 @@ void main() {
   group('static functions', () {
     setUpAll(() {
       logWarnings();
+      // TODO(https://github.com/dart-lang/native/issues/1068): Remove this.
+      DynamicLibrary.open('../objective_c/test/objective_c.dylib');
       final dylib = File('test/native_objc_test/static_func_test.dylib');
       verifySetupFile(dylib);
       lib = StaticFuncTestObjCLibrary(DynamicLibrary.open(dylib.absolute.path));

pkgs/ffigen/test/native_objc_test/string_test.dart

@@ -19,6 +19,8 @@ void main() {
   group('string', () {
     setUpAll(() {
       logWarnings();
+      // TODO(https://github.com/dart-lang/native/issues/1068): Remove this.
+      DynamicLibrary.open('../objective_c/test/objective_c.dylib');
       final dylib = File('test/native_objc_test/string_test.dylib');
       verifySetupFile(dylib);
       DynamicLibrary.open(dylib.absolute.path);

pkgs/ffigen/test/native_objc_test/swift_class_test.dart

@@ -17,6 +17,8 @@ void main() {
   group('swift_class_test', () {
     setUpAll(() {
       logWarnings();
+      // TODO(https://github.com/dart-lang/native/issues/1068): Remove this.
+      DynamicLibrary.open('../objective_c/test/objective_c.dylib');
       final dylib = File('test/native_objc_test/swift_class_test.dylib');
       verifySetupFile(dylib);
       DynamicLibrary.open(dylib.absolute.path);