[ffigen] Stop using NSProxy (#2029)

Author: liamappelbe

pkgs/ffigen/CHANGELOG.md

@@ -10,6 +10,9 @@
   allows a block or protocol to keep its owner isolate alive.
 - __Breaking change__: `keepIsolateAlive` defaults to true, so all existing ObjC
   blocks and protocols now keep their isolates alive by default.
+- Change how protocols are implemented to fix
+  [a bug](https://github.com/dart-lang/http/issues/1702), by removing all uses
+  of `NSProxy`.
 
 ## 17.0.0
 

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

@@ -15,6 +15,7 @@ class ObjCBlock extends BindingType {
   final List<Parameter> params;
   final bool returnsRetained;
   ObjCBlockWrapperFuncs? _blockWrappers;
+  ObjCProtocolMethodTrampoline? protocolTrampoline;
 
   factory ObjCBlock({
     required Type returnType,
@@ -64,6 +65,10 @@ class ObjCBlock extends BindingType {
     }
   }
 
+  void fillProtocolTrampoline() {
+    protocolTrampoline ??= builtInFunctions.getProtocolMethodTrampoline(this);
+  }
+
   // Generates a human readable name for the block based on the args and return
   // type. These names will be pretty verbose and unweildy, but they're at least
   // sensible and stable. Users can always add their own typedef with a simpler
@@ -346,6 +351,16 @@ ref.pointer.ref.invoke.cast<${func.trampNatFnCType}>()
 
   @override
   BindingString? toObjCBindingString(Writer w) {
+    final chunks = [
+      _blockWrappersBindingString(w),
+      _protocolTrampolineBindingString(w),
+    ].nonNulls;
+    if (chunks.isEmpty) return null;
+    return BindingString(
+        type: BindingStringType.objcBlock, string: chunks.join(''));
+  }
+
+  String? _blockWrappersBindingString(Writer w) {
     if (_blockWrappers?.objCBindingsGenerated ?? true) return null;
     _blockWrappers!.objCBindingsGenerated = true;
 
@@ -375,8 +390,7 @@ ref.pointer.ref.invoke.cast<${func.trampNatFnCType}>()
     final blockingName =
         w.objCLevelUniqueNamer.makeUnique('_BlockingTrampoline');
 
-    final s = StringBuffer();
-    s.write('''
+    return '''
 
 typedef ${returnType.getNativeType()} (^$listenerName)($argStr);
 __attribute__((visibility("default"))) __attribute__((used))
@@ -405,9 +419,39 @@ $listenerName $blockingWrapper(
     }
   };
 }
-''');
-    return BindingString(
-        type: BindingStringType.objcBlock, string: s.toString());
+''';
+  }
+
+  String? _protocolTrampolineBindingString(Writer w) {
+    if (protocolTrampoline?.objCBindingsGenerated ?? true) return null;
+    protocolTrampoline!.objCBindingsGenerated = true;
+
+    final argsReceived = <String>[];
+    final argsPassed = <String>[];
+    for (var i = 0; i < params.length; ++i) {
+      final param = params[i];
+      final argName = i == 0 ? 'sel' : 'arg$i';
+      argsReceived.add(param.getNativeType(varName: argName));
+      argsPassed.add(argName);
+    }
+
+    final ret = returnType.getNativeType();
+    final argRecv = argsReceived.join(', ');
+    final argPass = argsPassed.join(', ');
+    final fnName = protocolTrampoline!.func.name;
+    final block = w.objCLevelUniqueNamer.makeUnique('_ProtocolTrampoline');
+    final msgSend = '((id (*)(id, SEL, SEL))objc_msgSend)';
+    final getterSel = '@selector(getDOBJCDartProtocolMethodForSelector:)';
+    final blkGetter = '(($block)$msgSend(target, $getterSel, sel))';
+
+    return '''
+
+typedef $ret (^$block)($argRecv);
+__attribute__((visibility("default"))) __attribute__((used))
+$ret $fnName(id target, $argRecv) {
+  return $blkGetter($argPass);
+}
+''';
   }
 
   @override
@@ -465,6 +509,7 @@ $listenerName $blockingWrapper(
     visitor.visit(returnType);
     visitor.visitAll(params);
     visitor.visit(_blockWrappers);
+    visitor.visit(protocolTrampoline);
   }
 
   @override

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

@@ -44,7 +44,6 @@ class ObjCBuiltInFunctions {
   static const protocolListenableMethod =
       ObjCImport('ObjCProtocolListenableMethod');
   static const protocolBuilder = ObjCImport('ObjCProtocolBuilder');
-  static const dartProxy = ObjCImport('DartProxy');
   static const unimplementedOptionalMethodException =
       ObjCImport('UnimplementedOptionalMethodException');
   static const checkOsVersion = ObjCImport('checkOsVersion');
@@ -54,8 +53,8 @@ class ObjCBuiltInFunctions {
   @visibleForTesting
   static const builtInInterfaces = {
     'DartInputStreamAdapter',
-    'DartProxy',
-    'DartProxyBuilder',
+    'DartProtocol',
+    'DartProtocolBuilder',
     'NSArray',
     'NSCharacterSet',
     'NSCoder',
@@ -83,7 +82,6 @@ class ObjCBuiltInFunctions {
     'NSOrderedCollectionDifference',
     'NSOrderedSet',
     'NSOutputStream',
-    'NSProxy',
     'NSRunLoop',
     'NSSet',
     'NSStream',
@@ -178,12 +176,9 @@ class ObjCBuiltInFunctions {
   ObjCMsgSendFunc getMsgSendFunc(Type returnType, List<Parameter> params) {
     params = _methodSigParams(params);
     returnType = _methodSigType(returnType);
-    final id = _methodSigId(returnType, params);
+    final (id, idHash) = _methodSigId(returnType, params);
     return _msgSendFuncs[id] ??= ObjCMsgSendFunc(
-        '_objc_msgSend_${fnvHash32(id).toRadixString(36)}',
-        returnType,
-        params,
-        useMsgSendVariants);
+        '_objc_msgSend_$idHash', returnType, params, useMsgSendVariants);
   }
 
   final _selObjects = <String, ObjCInternalGlobal>{};
@@ -194,7 +189,7 @@ class ObjCBuiltInFunctions {
     );
   }
 
-  String _methodSigId(Type returnType, List<Parameter> params) {
+  (String, String) _methodSigId(Type returnType, List<Parameter> params) {
     final paramIds = <String>[];
     for (final p in params) {
       // The trampoline ID is based on the getNativeType of the param. Objects
@@ -205,7 +200,8 @@ class ObjCBuiltInFunctions {
     }
     final rt =
         returnType.getNativeType(varName: returnType.generateRetain('') ?? '');
-    return '$rt,${paramIds.join(',')}';
+    final id = '$rt,${paramIds.join(',')}';
+    return (id, fnvHash32(id).toRadixString(36));
   }
 
   Type _methodSigType(Type t) {
@@ -242,8 +238,7 @@ class ObjCBuiltInFunctions {
 
   final _blockTrampolines = <String, ObjCBlockWrapperFuncs>{};
   ObjCBlockWrapperFuncs? getBlockTrampolines(ObjCBlock block) {
-    final id = _methodSigId(block.returnType, block.params);
-    final idHash = fnvHash32(id).toRadixString(36);
+    final (id, idHash) = _methodSigId(block.returnType, block.params);
     return _blockTrampolines[id] ??= ObjCBlockWrapperFuncs(
       _blockTrampolineFunc('_${wrapperName}_wrapListenerBlock_$idHash'),
       _blockTrampolineFunc('_${wrapperName}_wrapBlockingBlock_$idHash',
@@ -285,6 +280,27 @@ class ObjCBuiltInFunctions {
         ffiNativeConfig: const FfiNativeConfig(enabled: true),
       );
 
+  final _protocolTrampolines = <String, ObjCProtocolMethodTrampoline>{};
+  ObjCProtocolMethodTrampoline? getProtocolMethodTrampoline(ObjCBlock block) {
+    final (id, idHash) = _methodSigId(block.returnType, block.params);
+    return _protocolTrampolines[id] ??= ObjCProtocolMethodTrampoline(Func(
+      name: '_${wrapperName}_protocolTrampoline_$idHash',
+      returnType: block.returnType,
+      parameters: [
+        Parameter(
+            name: 'target',
+            type: PointerType(objCObjectType),
+            objCConsumed: false),
+        ...block.params,
+      ],
+      objCReturnsRetained: false,
+      isLeaf: false,
+      isInternal: true,
+      useNameForLookup: true,
+      ffiNativeConfig: const FfiNativeConfig(enabled: true),
+    ));
+  }
+
   static bool isInstanceType(Type type) {
     if (type is ObjCInstanceType) return true;
     final baseType = type.typealiasType;
@@ -308,6 +324,24 @@ class ObjCBlockWrapperFuncs extends AstNode {
   }
 }
 
+/// A native trampoline function for a protocol method.
+class ObjCProtocolMethodTrampoline extends AstNode {
+  final Func func;
+  bool objCBindingsGenerated = false;
+
+  ObjCProtocolMethodTrampoline(this.func);
+
+  @override
+  void visitChildren(Visitor visitor) {
+    super.visitChildren(visitor);
+    visitor.visit(func);
+  }
+
+  @override
+  void visit(Visitation visitation) =>
+      visitation.visitObjCProtocolMethodTrampoline(this);
+}
+
 /// A function, global variable, or helper type defined in package:objective_c.
 class ObjCImport {
   final String name;

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

@@ -260,7 +260,7 @@ class ObjCMethod extends AstNode {
       ],
       returnsRetained: returnsRetained,
       builtInFunctions: builtInFunctions,
-    );
+    )..fillProtocolTrampoline();
   }
 
   String getDartMethodName(UniqueNamer uniqueNamer,

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

@@ -164,6 +164,7 @@ interface class $name extends $protocolBase $impls{
         methodFields.write('''static final $fieldName = $methodClass<$funcType>(
       ${_protocolPointer.name},
       ${method.selObject.name},
+      ${_trampolineAddress(w, block)},
       $getSignature(
           ${_protocolPointer.name},
           ${method.selObject.name},
@@ -185,13 +186,15 @@ interface class $name extends $protocolBase $impls{
   /// If `\$keepIsolateAlive` is true, this protocol will keep this isolate
   /// alive until it is garbage collected by both Dart and ObjC.
   static $name implement($args) {
-    final builder = $protocolBuilder();
+    final builder = $protocolBuilder(debugName: '$originalName');
     $buildImplementations
     return $name.castFrom(builder.build(keepIsolateAlive: \$keepIsolateAlive));
   }
 
   /// Adds the implementation of the $originalName protocol to an existing
   /// [$protocolBuilder].
+  ///
+  /// Note: You cannot call this method after you have called `builder.build`.
   static void addToBuilder($protocolBuilder builder, $args) {
     $buildImplementations
   }
@@ -207,14 +210,16 @@ interface class $name extends $protocolBase $impls{
   /// If `\$keepIsolateAlive` is true, this protocol will keep this isolate
   /// alive until it is garbage collected by both Dart and ObjC.
   static $name implementAsListener($args) {
-    final builder = $protocolBuilder();
+    final builder = $protocolBuilder(debugName: '$originalName');
     $buildListenerImplementations
     return $name.castFrom(builder.build(keepIsolateAlive: \$keepIsolateAlive));
   }
 
   /// Adds the implementation of the $originalName protocol to an existing
   /// [$protocolBuilder]. All methods that can be implemented as listeners will
   /// be.
+  ///
+  /// Note: You cannot call this method after you have called `builder.build`.
   static void addToBuilderAsListener($protocolBuilder builder, $args) {
     $buildListenerImplementations
   }
@@ -226,14 +231,16 @@ interface class $name extends $protocolBase $impls{
   /// If `\$keepIsolateAlive` is true, this protocol will keep this isolate
   /// alive until it is garbage collected by both Dart and ObjC.
   static $name implementAsBlocking($args) {
-    final builder = $protocolBuilder();
+    final builder = $protocolBuilder(debugName: '$originalName');
     $buildBlockingImplementations
     return $name.castFrom(builder.build(keepIsolateAlive: \$keepIsolateAlive));
   }
 
   /// Adds the implementation of the $originalName protocol to an existing
   /// [$protocolBuilder]. All methods that can be implemented as blocking
   /// listeners will be.
+  ///
+  /// Note: You cannot call this method after you have called `builder.build`.
   static void addToBuilderAsBlocking($protocolBuilder builder, $args) {
     $buildBlockingImplementations
   }
@@ -264,6 +271,13 @@ interface class $name extends $protocolBase $impls{
         type: BindingStringType.objcProtocol, string: s.toString());
   }
 
+  static String _trampolineAddress(Writer w, ObjCBlock block) {
+    final func = block.protocolTrampoline!.func;
+    final type =
+        NativeFunc(func.functionType).getCType(w, writeArgumentNames: false);
+    return '${w.ffiLibraryPrefix}.Native.addressOf<$type>(${func.name}).cast()';
+  }
+
   @override
   BindingString? toObjCBindingString(Writer w) {
     final wrapName = builtInFunctions.wrapperName;

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

@@ -429,6 +429,7 @@ class Writer {
     s.write('''
 #include <stdint.h>
 #import <Foundation/Foundation.h>
+#import <objc/message.h>
 ''');
 
     for (final entryPoint in nativeEntryPoints) {

pkgs/ffigen/lib/src/visitor/find_transitive_deps.dart

@@ -70,6 +70,12 @@ class FindDirectTransitiveDepsVisitation extends Visitation {
     visitor.visitAll(node.superProtocols);
   }
 
+  @override
+  void visitObjCProtocolMethodTrampoline(ObjCProtocolMethodTrampoline node) {
+    // Don't visit transitive deps of ObjCProtocolMethodTrampoline, as it can
+    // force include protocols that would otherwise be omitted.
+  }
+
   @override
   void visitBinding(Binding node) => _visitImpl(node, true);
 }

pkgs/ffigen/lib/src/visitor/visitor.dart

@@ -83,6 +83,8 @@ abstract class Visitation {
   void visitGlobal(Global node) => visitLookUpBinding(node);
   void visitTypealias(Typealias node) => visitBindingType(node);
   void visitPointerType(PointerType node) => visitType(node);
+  void visitObjCProtocolMethodTrampoline(ObjCProtocolMethodTrampoline node) =>
+      visitAstNode(node);
 
   /// Default behavior for all visit methods.
   void visitAstNode(AstNode node) => node..visitChildren(visitor);