From 7922b203737a3b8a86dfdd0dbc3b4bb1aede6f14 Mon Sep 17 00:00:00 2001 From: Liam Appelbe Date: Fri, 20 Sep 2024 12:23:56 +1200 Subject: [PATCH] [ffigen] Retain listener blocks before invocation (#1574) --- pkgs/ffigen/CHANGELOG.md | 3 + .../lib/src/code_generator/objc_block.dart | 57 ++++++++++--------- .../objc_built_in_functions.dart | 5 -- pkgs/ffigen/pubspec.yaml | 1 + .../test/native_objc_test/block_test.dart | 39 +++++++++++++ .../ffigen/test/native_objc_test/block_test.h | 4 ++ .../ffigen/test/native_objc_test/block_test.m | 17 ++++++ .../test/native_objc_test/isolate_config.yaml | 6 +- .../test/native_objc_test/isolate_test.h | 12 ++++ .../test/native_objc_test/isolate_test.m | 10 +--- .../native_objc_test/protocol_config.yaml | 4 +- .../test/native_objc_test/protocol_test.h | 2 - pkgs/ffigen/test/native_objc_test/util.dart | 11 +++- 13 files changed, 125 insertions(+), 46 deletions(-) create mode 100644 pkgs/ffigen/test/native_objc_test/isolate_test.h diff --git a/pkgs/ffigen/CHANGELOG.md b/pkgs/ffigen/CHANGELOG.md index e16b02ed4..e39edcbdf 100644 --- a/pkgs/ffigen/CHANGELOG.md +++ b/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 diff --git a/pkgs/ffigen/lib/src/code_generator/objc_block.dart b/pkgs/ffigen/lib/src/code_generator/objc_block.dart index aa9c0ae53..4677ae850 100644 --- a/pkgs/ffigen/lib/src/code_generator/objc_block.dart +++ b/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,12 +201,8 @@ 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. /// @@ -192,11 +210,8 @@ abstract final class $name { /// 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(', ')}); }; } diff --git a/pkgs/ffigen/lib/src/code_generator/objc_built_in_functions.dart b/pkgs/ffigen/lib/src/code_generator/objc_built_in_functions.dart index 8bba53030..6046685bd 100644 --- a/pkgs/ffigen/lib/src/code_generator/objc_built_in_functions.dart +++ b/pkgs/ffigen/lib/src/code_generator/objc_built_in_functions.dart @@ -146,13 +146,9 @@ class ObjCBuiltInFunctions { ObjCListenerBlockTrampoline? getListenerBlockTrampoline(ObjCBlock block) { assert(!_depsAdded); - var needsTrampoline = false; final paramIds = []; 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 @@ -160,7 +156,6 @@ class ObjCBuiltInFunctions { // 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( diff --git a/pkgs/ffigen/pubspec.yaml b/pkgs/ffigen/pubspec.yaml index 09b0c2dbd..b818f9462 100644 --- a/pkgs/ffigen/pubspec.yaml +++ b/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 diff --git a/pkgs/ffigen/test/native_objc_test/block_test.dart b/pkgs/ffigen/test/native_objc_test/block_test.dart index 22bbfe1e7..4d4a78eec 100644 --- a/pkgs/ffigen/test/native_objc_test/block_test.dart +++ b/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, Pointer) regress1571Inner( + Completer 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(); + 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); + } + }); }); } diff --git a/pkgs/ffigen/test/native_objc_test/block_test.h b/pkgs/ffigen/test/native_objc_test/block_test.h index a6880548f..d13e2ac80 100644 --- a/pkgs/ffigen/test/native_objc_test/block_test.h +++ b/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 diff --git a/pkgs/ffigen/test/native_objc_test/block_test.m b/pkgs/ffigen/test/native_objc_test/block_test.m index 4995b3b4f..7dbf86de3 100644 --- a/pkgs/ffigen/test/native_objc_test/block_test.m +++ b/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 diff --git a/pkgs/ffigen/test/native_objc_test/isolate_config.yaml b/pkgs/ffigen/test/native_objc_test/isolate_config.yaml index 9d53d174a..97336c5df 100644 --- a/pkgs/ffigen/test/native_objc_test/isolate_config.yaml +++ b/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 diff --git a/pkgs/ffigen/test/native_objc_test/isolate_test.h b/pkgs/ffigen/test/native_objc_test/isolate_test.h new file mode 100644 index 000000000..221f1fe81 --- /dev/null +++ b/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 + +typedef void (^Listener)(int32_t); + +@interface Sendable : NSObject {} +@property int32_t value; ++ (Listener)dummyMethodToForceGenerationOfListener; +@end diff --git a/pkgs/ffigen/test/native_objc_test/isolate_test.m b/pkgs/ffigen/test/native_objc_test/isolate_test.m index fe96647a1..2b8750751 100644 --- a/pkgs/ffigen/test/native_objc_test/isolate_test.m +++ b/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 - +#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 diff --git a/pkgs/ffigen/test/native_objc_test/protocol_config.yaml b/pkgs/ffigen/test/native_objc_test/protocol_config.yaml index b75a882b9..cf09e13f6 100644 --- a/pkgs/ffigen/test/native_objc_test/protocol_config.yaml +++ b/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: diff --git a/pkgs/ffigen/test/native_objc_test/protocol_test.h b/pkgs/ffigen/test/native_objc_test/protocol_test.h index 0405b82f9..cc394b06f 100644 --- a/pkgs/ffigen/test/native_objc_test/protocol_test.h +++ b/pkgs/ffigen/test/native_objc_test/protocol_test.h @@ -5,8 +5,6 @@ #import #import -#include "util.h" - typedef struct { int32_t x; int32_t y; diff --git a/pkgs/ffigen/test/native_objc_test/util.dart b/pkgs/ffigen/test/native_objc_test/util.dart index 5e7c13493..a40358dea 100644 --- a/pkgs/ffigen/test/native_objc_test/util.dart +++ b/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 flutterDoGC() async { + await leak_tracker.forceGC(); + await Future.delayed(Duration(milliseconds: 500)); +} + @Native)>(isLeaf: true, symbol: 'isReadableMemory') external bool _isReadableMemory(Pointer ptr);