diff --git a/NativeScript/runtime/Interop.mm b/NativeScript/runtime/Interop.mm index 23bc4647..0097ef5b 100644 --- a/NativeScript/runtime/Interop.mm +++ b/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(Block_copy(block)); + BlockWrapper* blockWrapper = new BlockWrapper(ownedBlock, typeEncoding, true); Local ext = External::New(isolate, blockWrapper); Local callback; - CFRetain(block); - bool success = v8::Function::New( context, diff --git a/NativeScript/runtime/ObjectManager.mm b/NativeScript/runtime/ObjectManager.mm index 9b07e8ff..b109ae18 100644 --- a/NativeScript/runtime/ObjectManager.mm +++ b/NativeScript/runtime/ObjectManager.mm @@ -1,4 +1,5 @@ #include "ObjectManager.h" +#include #include #include #include "Caches.h" @@ -108,10 +109,12 @@ case WrapperType::Block: { BlockWrapper* blockWrapper = static_cast(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); diff --git a/TestFixtures/Api/TNSReturnsRetained.h b/TestFixtures/Api/TNSReturnsRetained.h index 1bff9232..4fb06bbc 100644 --- a/TestFixtures/Api/TNSReturnsRetained.h +++ b/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 diff --git a/TestFixtures/Api/TNSReturnsRetained.m b/TestFixtures/Api/TNSReturnsRetained.m index b9283e90..869f6c8b 100644 --- a/TestFixtures/Api/TNSReturnsRetained.m +++ b/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 diff --git a/TestRunner/app/tests/ApiTests.js b/TestRunner/app/tests/ApiTests.js index 84834d17..16ff1436 100644 --- a/TestRunner/app/tests/ApiTests.js +++ b/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);