More block ref counting fixes

dart-archive · Oct 26, 2023 · 7f1dd06 · 7f1dd06
1 parent 2f41968
commit 7f1dd06
Show file tree

Hide file tree

Showing 3 changed files with 231 additions and 14 deletions.
diff --git a/lib/src/code_generator/objc_block.dart b/lib/src/code_generator/objc_block.dart
@@ -119,7 +119,7 @@ $returnFfiDartType $closureTrampoline($blockCType block, $paramsFfiDartType) =>
     // is used below. Note that the closure being converted is called `fn`.
     final convertedFnArgs = params
         .map((p) => p.type.convertFfiDartTypeToDartType(w, p.name, 'lib',
-            objCShouldRetain: false))
+            objCShouldRetain: true))
         .join(', ');
     final convFnInvocation = returnType.convertDartTypeToFfiDartType(
         w, 'fn($convertedFnArgs)',
@@ -175,8 +175,9 @@ class $name extends _ObjCBlockBase {
   /// blocks do not keep the isolate alive.
   $name.listener(${w.className} lib, $funcDartType fn) :
       this._(lib.${builtInFunctions.newBlock.name}(
-          (_dartFuncListenerTrampoline ??= $nativeCallableType.listener($closureTrampoline
-                  $exceptionalReturn)..keepIsolateAlive = false).nativeFunction.cast(),
+          (_dartFuncListenerTrampoline ??= $nativeCallableType.listener(
+              $closureTrampoline $exceptionalReturn)..keepIsolateAlive =
+                  false).nativeFunction.cast(),
           $registerClosure($convFn)), lib);
   static $nativeCallableType? _dartFuncListenerTrampoline;
 

diff --git a/test/native_objc_test/block_test.dart b/test/native_objc_test/block_test.dart
@@ -215,10 +215,23 @@ void main() {
       final result1 = blockBlock(intBlock);
       expect(result1(1), 15);
 
-      final result2 = BlockTester.callBlockBlock_(lib, blockBlock);
+      final result2 = BlockTester.newBlock_withMult_(lib, blockBlock, 2);
       expect(result2(1), 6);
     });
 
+    test('Native block block', () {
+      final blockBlock = BlockTester.newBlockBlock_(lib, 7);
+
+      final intBlock = IntBlock.fromFunction(lib, (int x) {
+        return 5 * x;
+      });
+      final result1 = blockBlock(intBlock);
+      expect(result1(1), 35);
+
+      final result2 = BlockTester.newBlock_withMult_(lib, blockBlock, 2);
+      expect(result2(1), 14);
+    });
+
     Pointer<Void> funcPointerBlockRefCountTest() {
       final block =
           IntBlock.fromFunctionPointer(lib, Pointer.fromFunction(_add100, 999));
@@ -229,7 +242,7 @@ void main() {
     test('Function pointer block ref counting', () {
       final rawBlock = funcPointerBlockRefCountTest();
       doGC();
-      expect(BlockTester.getBlockRetainCount_(lib, rawBlock.cast()), 0);
+      expect(BlockTester.getBlockRetainCount_(lib, rawBlock), 0);
     });
 
     Pointer<Void> funcBlockRefCountTest() {
@@ -241,7 +254,173 @@ void main() {
     test('Function block ref counting', () {
       final rawBlock = funcBlockRefCountTest();
       doGC();
-      expect(BlockTester.getBlockRetainCount_(lib, rawBlock.cast()), 0);
+      expect(BlockTester.getBlockRetainCount_(lib, rawBlock), 0);
+    });
+
+    (Pointer<Void>, Pointer<Void>, Pointer<Void>) blockBlockDartCallRefCountTest() {
+      final inputBlock = IntBlock.fromFunction(lib, (int x) {
+        return 5 * x;
+      });
+      final blockBlock = BlockBlock.fromFunction(lib, (IntBlock intBlock) {
+        return IntBlock.fromFunction(lib, (int x) {
+          return 3 * intBlock(x);
+        });
+      });
+      final outputBlock = blockBlock(inputBlock);
+      expect(outputBlock(1), 15);
+      doGC();
+
+      // One reference held by inputBlock object, another bound to the
+      // outputBlock lambda.
+      expect(BlockTester.getBlockRetainCount_(lib, inputBlock.pointer.cast()), 2);
+
+      expect(BlockTester.getBlockRetainCount_(lib, blockBlock.pointer.cast()), 1);
+      expect(BlockTester.getBlockRetainCount_(lib, outputBlock.pointer.cast()), 1);
+      return (inputBlock.pointer.cast(), blockBlock.pointer.cast(), outputBlock.pointer.cast());
+    }
+
+    test('Calling a block block from Dart has correct ref counting', () {
+      final (inputBlock, blockBlock, outputBlock) = blockBlockDartCallRefCountTest();
+      doGC();
+
+      // This leaks because block functions aren't cleaned up at the moment.
+      // TODO(https://github.com/dart-lang/ffigen/issues/428): Fix this leak.
+      expect(BlockTester.getBlockRetainCount_(lib, inputBlock), 1);
+
+      expect(BlockTester.getBlockRetainCount_(lib, blockBlock), 0);
+      expect(BlockTester.getBlockRetainCount_(lib, outputBlock), 0);
+    });
+
+    (Pointer<Void>, Pointer<Void>, Pointer<Void>) blockBlockObjCCallRefCountTest() {
+      late Pointer<Void> inputBlock;
+      final blockBlock = BlockBlock.fromFunction(lib, (IntBlock intBlock) {
+        inputBlock = intBlock.pointer.cast();
+        return IntBlock.fromFunction(lib, (int x) {
+          return 3 * intBlock(x);
+        });
+      });
+      final outputBlock = BlockTester.newBlock_withMult_(lib, blockBlock, 2);
+      expect(outputBlock(1), 6);
+      doGC();
+
+      expect(BlockTester.getBlockRetainCount_(lib, inputBlock), 2);
+      expect(BlockTester.getBlockRetainCount_(lib, blockBlock.pointer.cast()), 1);
+      expect(BlockTester.getBlockRetainCount_(lib, outputBlock.pointer.cast()), 1);
+      return (inputBlock, blockBlock.pointer.cast(), outputBlock.pointer.cast());
+    }
+
+    test('Calling a block block from ObjC has correct ref counting', () {
+      final (inputBlock, blockBlock, outputBlock) = blockBlockObjCCallRefCountTest();
+      doGC();
+
+      // This leaks because block functions aren't cleaned up at the moment.
+      // TODO(https://github.com/dart-lang/ffigen/issues/428): Fix this leak.
+      expect(BlockTester.getBlockRetainCount_(lib, inputBlock), 2);
+
+      expect(BlockTester.getBlockRetainCount_(lib, blockBlock), 0);
+      expect(BlockTester.getBlockRetainCount_(lib, outputBlock), 0);
+    });
+
+    (Pointer<Void>, Pointer<Void>, Pointer<Void>) nativeBlockBlockDartCallRefCountTest() {
+      final inputBlock = IntBlock.fromFunction(lib, (int x) {
+        return 5 * x;
+      });
+      final blockBlock = BlockTester.newBlockBlock_(lib, 7);
+      final outputBlock = blockBlock(inputBlock);
+      expect(outputBlock(1), 35);
+      doGC();
+
+      // One reference held by inputBlock object, another held internally by the
+      // ObjC implementation of the blockBlock.
+      expect(BlockTester.getBlockRetainCount_(lib, inputBlock.pointer.cast()), 2);
+
+      expect(BlockTester.getBlockRetainCount_(lib, blockBlock.pointer.cast()), 1);
+      expect(BlockTester.getBlockRetainCount_(lib, outputBlock.pointer.cast()), 1);
+      return (inputBlock.pointer.cast(), blockBlock.pointer.cast(), outputBlock.pointer.cast());
+    }
+
+    test('Calling a native block block from Dart has correct ref counting', () {
+      final (inputBlock, blockBlock, outputBlock) = nativeBlockBlockDartCallRefCountTest();
+      doGC();
+      expect(BlockTester.getBlockRetainCount_(lib, inputBlock), 0);
+      expect(BlockTester.getBlockRetainCount_(lib, blockBlock), 0);
+      expect(BlockTester.getBlockRetainCount_(lib, outputBlock), 0);
+    });
+
+    (Pointer<Void>, Pointer<Void>) nativeBlockBlockObjCCallRefCountTest() {
+      final blockBlock = BlockTester.newBlockBlock_(lib, 7);
+      final outputBlock = BlockTester.newBlock_withMult_(lib, blockBlock, 2);
+      expect(outputBlock(1), 14);
+      doGC();
+
+      expect(BlockTester.getBlockRetainCount_(lib, blockBlock.pointer.cast()), 1);
+      expect(BlockTester.getBlockRetainCount_(lib, outputBlock.pointer.cast()), 1);
+      return (blockBlock.pointer.cast(), outputBlock.pointer.cast());
+    }
+
+    test('Calling a native block block from ObjC has correct ref counting', () {
+      final (blockBlock, outputBlock) = nativeBlockBlockObjCCallRefCountTest();
+      doGC();
+      expect(BlockTester.getBlockRetainCount_(lib, blockBlock), 0);
+      expect(BlockTester.getBlockRetainCount_(lib, outputBlock), 0);
+    });
+
+    (Pointer<Int32>, Pointer<Int32>) objectBlockRefCountTest() {
+      final inputCounter = calloc<Int32>();
+      final outputCounter = calloc<Int32>();
+      inputCounter.value = 0;
+      outputCounter.value = 0;
+
+      final block = ObjectBlock.fromFunction(lib, (DummyObject x) {
+        return DummyObject.newWithCounter_(lib, outputCounter);
+      });
+
+      final inputObj = DummyObject.newWithCounter_(lib, inputCounter);
+      final outputObj = block(inputObj);
+      expect(inputCounter.value, 1);
+      expect(outputCounter.value, 1);
+
+      return (inputCounter, outputCounter);
+    }
+
+    test('Objects received and returned by blocks have correct ref counts', () {
+      final (inputCounter, outputCounter) = objectBlockRefCountTest();
+      doGC();
+      expect(inputCounter.value, 0);
+      expect(outputCounter.value, 0);
+      calloc.free(inputCounter);
+      calloc.free(outputCounter);
+    });
+
+    (Pointer<Int32>, Pointer<Int32>) objectNativeBlockRefCountTest() {
+      final inputCounter = calloc<Int32>();
+      final outputCounter = calloc<Int32>();
+      inputCounter.value = 0;
+      outputCounter.value = 0;
+
+      final block = ObjectBlock.fromFunction(lib, (DummyObject x) {
+        x.setCounter_(inputCounter);
+        return DummyObject.newWithCounter_(lib, outputCounter);
+      });
+
+      final outputObj = BlockTester.callObjectBlock_(lib, block);
+      expect(inputCounter.value, 1);
+      expect(outputCounter.value, 1);
+
+      return (inputCounter, outputCounter);
+    }
+
+    test('Objects received and returned by native blocks have correct ref counts', () {
+      final (inputCounter, outputCounter) = objectNativeBlockRefCountTest();
+      doGC();
+
+      // This leaks because block functions aren't cleaned up at the moment.
+      // TODO(https://github.com/dart-lang/ffigen/issues/428): Fix this leak.
+      expect(inputCounter.value, 1);
+
+      expect(outputCounter.value, 0);
+      calloc.free(inputCounter);
+      calloc.free(outputCounter);
     });
 
     test('Block fields have sensible values', () {

diff --git a/test/native_objc_test/block_test.m b/test/native_objc_test/block_test.m
@@ -12,9 +12,36 @@
   double w;
 } Vec4;
 
-@interface DummyObject : NSObject {}
+@interface DummyObject : NSObject {
+  int32_t* counter;
+}
++ (instancetype)newWithCounter:(int32_t*) _counter;
+- (instancetype)initWithCounter:(int32_t*) _counter;
+- (void)setCounter:(int32_t*) _counter;
+- (void)dealloc;
 @end
 @implementation DummyObject
+
++ (instancetype)newWithCounter:(int32_t*) _counter {
+  return [[DummyObject alloc] initWithCounter: _counter];
+}
+
+- (instancetype)initWithCounter:(int32_t*) _counter {
+  counter = _counter;
+  ++*counter;
+  return [super init];
+}
+
+- (void)setCounter:(int32_t*) _counter {
+  counter = _counter;
+  ++*counter;
+}
+
+- (void)dealloc {
+  if (counter != nil) --*counter;
+  [super dealloc];
+}
+
 @end
 
 
@@ -43,9 +70,10 @@ + (NSThread*)callOnNewThread:(VoidBlock)block;
 + (float)callFloatBlock:(FloatBlock)block;
 + (double)callDoubleBlock:(DoubleBlock)block;
 + (Vec4)callVec4Block:(Vec4Block)block;
-+ (DummyObject*)callObjectBlock:(ObjectBlock)block;
++ (DummyObject*)callObjectBlock:(ObjectBlock)block NS_RETURNS_RETAINED;
 + (DummyObject* _Nullable)callNullableBlock:(NullableBlock)block;
-+ (IntBlock)callBlockBlock:(BlockBlock)block;
++ (IntBlock)newBlock:(BlockBlock)block withMult:(int)mult;
++ (BlockBlock)newBlockBlock:(int)mult;
 @end
 
 @implementation BlockTester
@@ -128,18 +156,27 @@ + (Vec4)callVec4Block:(Vec4Block)block {
   return block(vec4);
 }
 
-+ (DummyObject*)callObjectBlock:(ObjectBlock)block {
++ (DummyObject*)callObjectBlock:(ObjectBlock)block NS_RETURNS_RETAINED {
   return block([DummyObject new]);
 }
 
 + (DummyObject* _Nullable)callNullableBlock:(NullableBlock)block {
   return block(nil);
 }
 
-+ (IntBlock)callBlockBlock:(BlockBlock)block {
-  return block(^int(int x) {
-    return 2 * x;
-  });
++ (IntBlock)newBlock:(BlockBlock)block withMult:(int)mult {
+  return block([^int(int x) {
+    return mult * x;
+  } copy]);
+  // ^ copy this stack allocated block to the heap.
+}
+
++ (BlockBlock)newBlockBlock:(int)mult {
+  return [^IntBlock(IntBlock block) {
+    return [^int(int x) {
+      return mult * block(x);
+    } copy];
+  } copy];
 }
 
 @end