fix: own native blocks with Block_copy/Block_release (#394)

* fix: own native blocks with Block_copy/Block_release

A block returned from a native method is +0 and may still be a stack
block. Interop::GetResult retained it with CFRetain and
ObjectManager::DisposeValue released it with CFRelease, but CFRetain does
not promote a stack block to the heap. By the time the JS wrapper was
finalized during GC the CFRelease ran against a freed stack frame and
crashed in objc_release (EXC_BAD_ACCESS).

Use Block_copy when taking ownership of the returned block (which moves a
stack block to the heap, or bumps the refcount of a heap/global block) and
Block_release as the matching counterpart on disposal.

* test: add regression test for owned block

---------

Co-authored-by: Eduardo Speroni <edusperoni@gmail.com>

Author: NathanWalker

NativeScript/runtime/Interop.mm

@@ -1055,12 +1055,18 @@ inline bool isBool() {
       return callback;
     }
 
-    BlockWrapper* blockWrapper = new BlockWrapper(block, typeEncoding, true);
+    // Take ownership of the returned block. Block_copy (not CFRetain) is the
+    // correct primitive here: a block returned by a native method is +0 and may
+    // still be a stack block. CFRetain does not promote a stack block to the
+    // heap, so releasing it later (during GC, on a different stack) would touch a
+    // dead frame and crash in objc_release. Block_copy moves it to the heap (or
+    // just bumps the refcount when it is already a heap/global block); the
+    // matching Block_release runs in ObjectManager::DisposeValue.
+    JSBlock* ownedBlock = reinterpret_cast<JSBlock*>(Block_copy(block));
+    BlockWrapper* blockWrapper = new BlockWrapper(ownedBlock, typeEncoding, true);
     Local<External> ext = External::New(isolate, blockWrapper);
     Local<v8::Function> callback;
 
-    CFRetain(block);
-
     bool success =
         v8::Function::New(
             context,

NativeScript/runtime/ObjectManager.mm

@@ -1,4 +1,5 @@
 #include "ObjectManager.h"
+#include <Block.h>
 #include <CoreFoundation/CoreFoundation.h>
 #include <sstream>
 #include "Caches.h"
@@ -108,10 +109,12 @@
     case WrapperType::Block: {
       BlockWrapper* blockWrapper = static_cast<BlockWrapper*>(wrapper);
       if (blockWrapper->OwnsBlock()) {
-        // Balance the CFRetain taken when a native block was wrapped for JS
-        // (see Interop::GetResult). This runs the block's dispose helper if
-        // we held the last reference.
-        CFRelease(blockWrapper->Block());
+        // Balance the Block_copy taken when a native block was wrapped for JS
+        // (see Interop::GetResult). Block_release is the correct counterpart to
+        // Block_copy and runs the block's dispose helper once we drop the last
+        // reference. (Using CFRelease here over-released stack blocks that were
+        // never promoted to the heap, crashing in objc_release during GC.)
+        Block_release(blockWrapper->Block());
       }
       // Blocks created from JS callbacks (OwnsBlock() == false) are owned by
       // the native code they were handed to (e.g. NSNotificationCenter);

TestFixtures/Api/TNSReturnsRetained.h

@@ -7,8 +7,14 @@ CF_IMPLICIT_BRIDGING_DISABLED
 
 id functionExplicitCreateNSObject();
 
+typedef int (^TNSIntBlock)(void);
+
 @interface TNSReturnsRetained : NSObject
 + (id)methodReturnsNSRetained NS_RETURNS_RETAINED;
 + (id)methodReturnsCFRetained CF_RETURNS_RETAINED;
 + (id)newNSObjectMethod;
+// Returns a +0 __NSStackBlock__ capturing `value` (see implementation). Used to
+// regression-test native block ownership: the runtime must take it with
+// Block_copy and release it with Block_release, not CFRetain/CFRelease.
++ (TNSIntBlock)blockCapturing:(int)value;
 @end

TestFixtures/Api/TNSReturnsRetained.m

@@ -1,26 +1,36 @@
 #import "TNSReturnsRetained.h"
 
-id functionReturnsNSRetained() {
-    return [[NSObject alloc] init];
-}
-id functionReturnsCFRetained() {
-    return [[NSObject alloc] init];
-}
-CFTypeRef functionImplicitCreate() {
-    return [[NSObject alloc] init];
-}
-id functionExplicitCreateNSObject() {
-    return [[NSObject alloc] init];
-}
+// Identity passthrough: hides the stack-block-ness from the compiler's
+// return-stack-address diagnostic without copying the block to the heap.
+static TNSIntBlock TNSPassthroughIntBlock(TNSIntBlock block) { return block; }
+
+id functionReturnsNSRetained() { return [[NSObject alloc] init]; }
+id functionReturnsCFRetained() { return [[NSObject alloc] init]; }
+CFTypeRef functionImplicitCreate() { return [[NSObject alloc] init]; }
+id functionExplicitCreateNSObject() { return [[NSObject alloc] init]; }
 
 @implementation TNSReturnsRetained
 + (id)methodReturnsNSRetained {
-    return [[NSObject alloc] init];
+  return [[NSObject alloc] init];
 }
 + (id)methodReturnsCFRetained {
-    return [[NSObject alloc] init];
+  return [[NSObject alloc] init];
 }
 + (id)newNSObjectMethod {
-    return [[TNSReturnsRetained alloc] init];
+  return [[TNSReturnsRetained alloc] init];
+}
++ (TNSIntBlock)blockCapturing:(int)value {
+  // This file is compiled with -fno-objc-arc. Capturing a non-constant value
+  // (the parameter) forces a __NSStackBlock__ - capturing only a compile-time
+  // constant would let clang promote it to a global block, which CFRetain
+  // handles fine and would not reproduce the bug. The block is routed through
+  // TNSPassthroughIntBlock so the compiler's "returning a stack block" check
+  // can't see through the call boundary; it is still a +0 stack block living in
+  // this frame at runtime. A correct runtime Block_copy's it to take ownership
+  // (heap-promoting it). CFRetain does not promote a stack block, so the wrapper
+  // is left pointing at this (about to be dead) stack frame.
+  return TNSPassthroughIntBlock(^{
+    return value;
+  });
 }
 @end

TestRunner/app/tests/ApiTests.js

@@ -661,6 +661,35 @@ describe(module.id, function () {
         expect(TNSReturnsRetained.newNSObjectMethod().retainCount()).toBe(1);
     });
 
+    it("takes ownership of +0 stack blocks returned from native", function () {
+        // Regression for the native-block ownership bug. blockCapturing: returns a
+        // +0 __NSStackBlock__ capturing its argument. A correct runtime Block_copy's
+        // each one into its own distinct heap block. The buggy CFRetain path does
+        // NOT promote a stack block to the heap, so all three wrappers are left
+        // pointing at the same (reused) stack slot and alias each other - reading
+        // whichever value was written last (and a later CFRelease would fault on
+        // the dead frame). See Interop::GetResult and ObjectManager::DisposeValue.
+        var b1 = TNSReturnsRetained.blockCapturing(11);
+        var b2 = TNSReturnsRetained.blockCapturing(22);
+        var b3 = TNSReturnsRetained.blockCapturing(33);
+
+        // On the fixed runtime each block is an independent heap copy.
+        expect(b1()).toBe(11);
+        expect(b2()).toBe(22);
+        expect(b3()).toBe(33);
+
+        // The wrappers must also be safely releasable on GC (Block_release of a
+        // heap copy vs. CFRelease of a dead stack frame).
+        b1 = b2 = b3 = null;
+        __collect();
+        var sink = 0;
+        for (var i = 0; i < 100000; i++) {
+            sink += i % 7;
+        }
+        __collect();
+        expect(sink).toBeGreaterThan(0);
+    });
+
     it("unmanaged", function () {
         var unmanaged = functionReturnsUnmanaged();
         expect('takeRetainedValue' in unmanaged).toBe(true);