[ffigen] Use isolate ownership API to fix blocking block deadlocks (#2068)

Author: liamappelbe

.github/workflows/ffigen.yml

@@ -80,7 +80,7 @@ jobs:
       - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683
       - uses: subosito/flutter-action@f2c4f6686ca8e8d6e6d0f28410eeef506ed66aff
         with:
-          channel: 'stable'
+          channel: stable
       - name: Install dependencies
         run: flutter pub get && flutter pub get --directory="../objective_c" && flutter pub get --directory="../jni"
       - name: Install clang-format
@@ -108,6 +108,28 @@ jobs:
           github-token: ${{ secrets.GITHUB_TOKEN }}
           parallel-finished: true
 
+  test-mac-latest:
+    needs: analyze
+    runs-on: 'macos-latest'
+    defaults:
+      run:
+        working-directory: pkgs/ffigen/
+    steps:
+      - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683
+      - uses: subosito/flutter-action@f2c4f6686ca8e8d6e6d0f28410eeef506ed66aff
+        with:
+          channel: master
+      - name: Install dependencies
+        run: flutter pub get && flutter pub get --directory="../objective_c" && flutter pub get --directory="../jni"
+      - name: Install clang-format
+        uses: ConorMacBride/install-package@3e7ad059e07782ee54fa35f827df52aae0626f30
+        with:
+          brew: clang-format
+      - name: Build test dylib and bindings
+        run: dart test/setup.dart
+      - name: Run VM tests and collect coverage
+        run: dart test
+
   test-mac-flutter:
     needs: analyze
     runs-on: 'macos-latest'

pkgs/ffigen/CHANGELOG.md

@@ -17,6 +17,10 @@
   jnigen. The main change is that `$` is used as a delimiter now, to avoid
   renamed identifiers from colliding with other identifiers. For example, `foo`
   is renamed to `foo$1` if there's already a `foo` in the namespace.
+- Fix [a bug](https://github.com/dart-lang/native/issues/1967) where blocking
+  blocks could deadlock if invoked from the Flutter UI thread. Note that this
+  relies on changes in Flutter that are currently only available in the main
+  channel. These changes will likely be released in Flutter 3.31.0.
 
 ## 17.0.0
 

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

@@ -169,3 +169,5 @@ final objCBlockType =
     ImportedType(objcPkgImport, 'ObjCBlockImpl', 'ObjCBlockImpl', 'id');
 final objCProtocolType =
     ImportedType(objcPkgImport, 'ObjCProtocol', 'ObjCProtocol', 'void');
+final objCContextType = ImportedType(
+    objcPkgImport, 'DOBJC_Context', 'DOBJC_Context', 'DOBJC_Context');

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

@@ -148,7 +148,7 @@ class ObjCBlock extends BindingType {
     final newClosureBlock = ObjCBuiltInFunctions.newClosureBlock.gen(w);
     final getBlockClosure = ObjCBuiltInFunctions.getBlockClosure.gen(w);
     final releaseFn = ObjCBuiltInFunctions.objectRelease.gen(w);
-    final wrapBlockingBlockFn = ObjCBuiltInFunctions.wrapBlockingBlock.gen(w);
+    final objCContext = ObjCBuiltInFunctions.objCContext.gen(w);
     final signalWaiterFn = ObjCBuiltInFunctions.signalWaiter.gen(w);
     final returnFfiDartType = returnType.getFfiDartType(w);
     final voidPtrCType = voidPtr.getCType(w);
@@ -312,7 +312,7 @@ abstract final class $name {
     final rawListener = $newClosureBlock(
         $blockingListenerCallable.nativeFunction.cast(),
         $listenerConvFn, keepIsolateAlive);
-    final wrapper = $wrapBlockingBlockFn($wrapBlockingFn, raw, rawListener);
+    final wrapper = $wrapBlockingFn(raw, rawListener, $objCContext);
     $releaseFn(raw.cast());
     $releaseFn(rawListener.cast());
     return $blockType(wrapper, retain: false, release: true);
@@ -406,19 +406,14 @@ typedef ${returnType.getNativeType()} (^$blockingName)($blockingArgStr);
 __attribute__((visibility("default"))) __attribute__((used))
 $listenerName $blockingWrapper(
     $blockingName block, $blockingName listenerBlock,
-    void* (*newWaiter)(), void (*awaitWaiter)(void*)) NS_RETURNS_RETAINED {
-  NSThread *targetThread = [NSThread currentThread];
-  return ^void($argStr) {
-    if ([NSThread currentThread] == targetThread) {
-      ${generateRetain('block')};
-      block(${blockingRetains.join(', ')});
-    } else {
-      void* waiter = newWaiter();
-      ${generateRetain('listenerBlock')};
-      listenerBlock(${blockingListenerRetains.join(', ')});
-      awaitWaiter(waiter);
-    }
-  };
+    DOBJC_Context* ctx) NS_RETURNS_RETAINED {
+  BLOCKING_BLOCK_IMPL(ctx, ^void($argStr), {
+    ${generateRetain('block')};
+    block(${blockingRetains.join(', ')});
+  }, {
+    ${generateRetain('listenerBlock')};
+    listenerBlock(${blockingListenerRetains.join(', ')});
+  });
 }
 ''';
   }

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

@@ -34,7 +34,7 @@ class ObjCBuiltInFunctions {
   static const getProtocol = ObjCImport('getProtocol');
   static const objectRelease = ObjCImport('objectRelease');
   static const signalWaiter = ObjCImport('signalWaiter');
-  static const wrapBlockingBlock = ObjCImport('wrapBlockingBlock');
+  static const objCContext = ObjCImport('objCContext');
   static const objectBase = ObjCImport('ObjCObjectBase');
   static const protocolBase = ObjCImport('ObjCProtocolBase');
   static const blockType = ObjCImport('ObjCBlock');
@@ -261,16 +261,8 @@ class ObjCBuiltInFunctions {
                 type: PointerType(objCBlockType),
                 objCConsumed: false),
             Parameter(
-                name: 'newWaiter',
-                type: PointerType(NativeFunc(FunctionType(
-                    returnType: PointerType(voidType), parameters: []))),
-                objCConsumed: false),
-            Parameter(
-                name: 'awaitWaiter',
-                type: PointerType(
-                    NativeFunc(FunctionType(returnType: voidType, parameters: [
-                  Parameter(type: PointerType(voidType), objCConsumed: false),
-                ]))),
+                name: 'context',
+                type: PointerType(objCContextType),
                 objCConsumed: false),
           ],
         ],

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

@@ -416,13 +416,50 @@ class Writer {
     for (final entryPoint in nativeEntryPoints) {
       s.write(_objcImport(entryPoint, outDir));
     }
-    s.write('''
+    s.write(r'''
 
 #if !__has_feature(objc_arc)
 #error "This file must be compiled with ARC enabled"
 #endif
 
+typedef struct {
+  int64_t version;
+  void* (*newWaiter)(void);
+  void (*awaitWaiter)(void*);
+  void* (*currentIsolate)(void);
+  void (*enterIsolate)(void*);
+  void (*exitIsolate)(void);
+  int64_t (*getMainPortId)(void);
+  bool (*getCurrentThreadOwnsIsolate)(int64_t);
+} DOBJC_Context;
+
 id objc_retainBlock(id);
+
+#define BLOCKING_BLOCK_IMPL(ctx, BLOCK_SIG, INVOKE_DIRECT, INVOKE_LISTENER)    \
+  assert(ctx->version >= 1);                                                   \
+  void* targetIsolate = ctx->currentIsolate();                                 \
+  int64_t targetPort = ctx->getMainPortId == NULL ? 0 : ctx->getMainPortId();  \
+  return BLOCK_SIG {                                                           \
+    void* currentIsolate = ctx->currentIsolate();                              \
+    bool mayEnterIsolate =                                                     \
+        currentIsolate == NULL &&                                              \
+        ctx->getCurrentThreadOwnsIsolate != NULL &&                            \
+        ctx->getCurrentThreadOwnsIsolate(targetPort);                          \
+    if (currentIsolate == targetIsolate || mayEnterIsolate) {                  \
+      if (mayEnterIsolate) {                                                   \
+        ctx->enterIsolate(targetIsolate);                                      \
+      }                                                                        \
+      INVOKE_DIRECT;                                                           \
+      if (mayEnterIsolate) {                                                   \
+        ctx->exitIsolate();                                                    \
+      }                                                                        \
+    } else {                                                                   \
+      void* waiter = ctx->newWaiter();                                         \
+      INVOKE_LISTENER;                                                         \
+      ctx->awaitWaiter(waiter);                                                \
+    }                                                                          \
+  };
+
 ''');
 
     var empty = true;
@@ -433,6 +470,11 @@ id objc_retainBlock(id);
         s.write(bindingString.string);
       }
     }
+
+    s.write('''
+#undef BLOCKING_BLOCK_IMPL
+''');
+
     return empty ? null : s.toString();
   }
 }

pkgs/ffigen/test/native_objc_test/block_test.dart

@@ -41,6 +41,12 @@ typedef BlockBlock = ObjCBlock_IntBlock_IntBlock;
 typedef IntPtrBlock = ObjCBlock_ffiVoid_Int32;
 typedef ResultBlock = ObjCBlock_ffiVoid_Int32$1;
 
+bool get hasIsolateOwnershipApi =>
+    DynamicLibrary.process().providesSymbol('Dart_SetCurrentThreadOwnsIsolate');
+void setCurrentThreadOwnsIsolate() =>
+    DynamicLibrary.process().lookupFunction<Void Function(), void Function()>(
+        'Dart_SetCurrentThreadOwnsIsolate')();
+
 void main() {
   late final BlockTestObjCLibrary lib;
 
@@ -52,7 +58,9 @@ void main() {
       verifySetupFile(dylib);
       lib = BlockTestObjCLibrary(DynamicLibrary.open(dylib.absolute.path));
 
-      generateBindingsForCoverage('block');
+      // generateBindingsForCoverage('block');
+
+      BlockTester.setup_(NativeApi.initializeApiDLData);
     });
 
     test('BlockTester is working', () {
@@ -1051,6 +1059,22 @@ void main() {
 
       receivePort.close();
     }, skip: !canDoGC);
+
+    test('Blocking block deadlock', () async {
+      // Regression test for https://github.com/dart-lang/native/issues/1967
+      // Run the test on a new isolate so we can safely claim ownership of it.
+      final value = await Isolate.run(() {
+        setCurrentThreadOwnsIsolate();
+
+        var innerValue = 0;
+        final block = VoidBlock.blocking(() {
+          innerValue = 123;
+        });
+        BlockTester.callOnSameThreadOutsideIsolate_(block);
+        return innerValue;
+      });
+      expect(value, 123);
+    }, skip: !hasIsolateOwnershipApi);
   });
 }
 

pkgs/ffigen/test/native_objc_test/block_test.h

@@ -55,13 +55,15 @@ typedef void (^ResultBlock)(int32_t);
   __strong IntBlock myBlock;
   __strong ObjectListenerBlock myListener;
 }
++ (void)setup:(void*)apiData;
 + (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;
 + (void)callOnSameThread:(VoidBlock)block;
++ (void)callOnSameThreadOutsideIsolate:(VoidBlock)block;
 + (NSThread*)callOnNewThread:(VoidBlock)block NS_RETURNS_RETAINED;
 + (NSThread*)callWithBlockOnNewThread:(ListenerBlock)block NS_RETURNS_RETAINED;
 + (float)callFloatBlock:(FloatBlock)block;

pkgs/ffigen/test/native_objc_test/block_test.m

@@ -7,6 +7,7 @@
 #import <Foundation/NSThread.h>
 
 #include "block_test.h"
+#include "../../../objective_c/src/include/dart_api_dl.h"
 
 @implementation DummyObject
 
@@ -33,6 +34,10 @@ - (void)dealloc {
 
 @implementation BlockTester
 
++ (void)setup:(void*)apiData {
+  Dart_InitializeApiDL(apiData);
+}
+
 + (BlockTester*)newFromBlock:(IntBlock)block {
   BlockTester* bt = [BlockTester new];
   bt->myBlock = block;
@@ -72,6 +77,13 @@ + (void)callOnSameThread:(VoidBlock)block {
   block();
 }
 
++ (void)callOnSameThreadOutsideIsolate:(VoidBlock)block {
+  Dart_Isolate isolate = Dart_CurrentIsolate_DL();
+  Dart_ExitIsolate_DL();
+  block();
+  Dart_EnterIsolate_DL(isolate);
+}
+
 + (NSThread*)callOnNewThread:(VoidBlock)block NS_RETURNS_RETAINED {
   return [[NSThread alloc] initWithBlock:block];
 }

pkgs/ffigen/test/native_objc_test/setup.dart

@@ -24,12 +24,14 @@ Future<void> _runClang(List<String> flags, String output) async {
   print('Generated file: $output');
 }
 
-Future<String> _buildObject(String input) async {
+Future<String> _buildObject(String input, {bool objc = false}) async {
   final output = '$input.o';
   await _runClang([
-    '-x',
-    'objective-c',
-    if (!arcDisabledFiles.contains(input)) '-fobjc-arc',
+    if (objc) ...[
+      '-x',
+      'objective-c',
+    ],
+    if (objc && !arcDisabledFiles.contains(input)) '-fobjc-arc',
     '-Wno-nullability-completeness',
     '-c',
     input,
@@ -48,8 +50,10 @@ Future<void> _linkLib(List<String> inputs, String output) => _runClang([
 Future<void> _buildLib(List<String> inputs, String output) async {
   final objFiles = <String>[];
   for (final input in inputs) {
-    objFiles.add(await _buildObject(input));
+    objFiles.add(await _buildObject(input, objc: true));
   }
+  objFiles.add(
+      await _buildObject('../../../objective_c/src/include/dart_api_dl.c'));
   await _linkLib(objFiles, output);
 }