From fa8b62d831fbd6ed9ac821cd361518677b8da036 Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Fri, 9 Oct 2026 16:20:26 -0300 Subject: [PATCH 1/2] test(runtime): confirm the stranded JS block dispose actually happened The specs for a JS block whose dispose waits on the isolate relied on a 1 ms release landing inside a 30 ms sleep. When the release slipped past the turn, the block disposed normally and the specs passed without exercising the race. The fixture now keeps blocks until the test releases them. The stranding helper drops the last reference on a background queue and waits for libclosure's deallocating flag on the block, so the spec fails instead of passing vacuously when the dispose never started. The handleof-while-held spec releases its block before it ends, and the worker spec releases the block only after the worker's nsworkerended event, once its runtime has been deleted. --- TestFixtures/TNSTestNativeCallbacks.h | 19 +++-- TestFixtures/TNSTestNativeCallbacks.m | 77 +++++++++++++++++-- TestRunner/app/tests/BlockCacheRaceTests.js | 13 +++- .../app/tests/blockFunctionReferenceWorker.js | 4 +- 4 files changed, 94 insertions(+), 19 deletions(-) diff --git a/TestFixtures/TNSTestNativeCallbacks.h b/TestFixtures/TNSTestNativeCallbacks.h index 23b83beb..e56af01f 100644 --- a/TestFixtures/TNSTestNativeCallbacks.h +++ b/TestFixtures/TNSTestNativeCallbacks.h @@ -73,11 +73,20 @@ // the block is also enqueued on the main operation queue first. + (void)keepBlock:(void (^)(void))block releaseMode:(int)mode; -// Keeps `block` and drops that reference from a global queue after `ms`. -+ (void)keepBlock:(void (^)(void))block forMilliseconds:(int)ms; - -// Blocks the calling thread, and with it the current JS turn, for `ms`. -+ (void)sleepMilliseconds:(int)ms; +// Keeps a reference to `block` until one of the releaseKeptBlocks methods +// drops every reference kept so far. Shared by all isolates. ++ (void)keepBlockUntilReleased:(void (^)(void))block; + +// Drops the kept references on the calling thread and returns how many there +// were. ++ (int)releaseKeptBlocks; + +// Drops the kept references on a global queue and waits up to `ms` until +// every one of those blocks has started its dispose (libclosure's +// deallocating flag is set). Returns NO on a timeout or when nothing was kept. +// The blocks' memory is read until their flags show the dispose, so the +// caller must hold the lock that dispose waits for: the blocks' isolate. ++ (BOOL)releaseKeptBlocksAwaitingDispose:(int)ms; // Calls `step` `count` times on the calling thread, each call inside its own // autorelease pool, so the runtime's autoreleased copy of a block marshalled diff --git a/TestFixtures/TNSTestNativeCallbacks.m b/TestFixtures/TNSTestNativeCallbacks.m index 7438d9f4..17909b6e 100644 --- a/TestFixtures/TNSTestNativeCallbacks.m +++ b/TestFixtures/TNSTestNativeCallbacks.m @@ -391,16 +391,77 @@ + (void)keepBlock:(void (^)(void))block releaseMode:(int)mode { }); } -+ (void)keepBlock:(void (^)(void))block forMilliseconds:(int)ms { - __block void (^kept)(void) = block; - dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)ms * NSEC_PER_MSEC), - dispatch_get_global_queue(QOS_CLASS_DEFAULT, 0), ^{ - kept = nil; - }); +static NSMutableArray* keptBlocks; + ++ (void)keepBlockUntilReleased:(void (^)(void))block { + @synchronized(self) { + if (keptBlocks == nil) { + keptBlocks = [NSMutableArray array]; + } + [keptBlocks addObject:[block copy]]; + } } -+ (void)sleepMilliseconds:(int)ms { - usleep((useconds_t)ms * 1000); ++ (int)releaseKeptBlocks { + int count; + // The pool keeps autoreleased temporaries from outliving the release. + @autoreleasepool { + NSArray* blocks; + @synchronized(self) { + blocks = keptBlocks; + keptBlocks = nil; + } + count = (int)blocks.count; + } + return count; +} + +// The head of every block (the Block ABI's Block_layout). +struct TNSBlockHeader { + void* isa; + volatile int32_t flags; +}; + +// libclosure sets this bit when the last release starts disposing a block. +static const int32_t TNSBlockDeallocating = 0x0001; + ++ (BOOL)releaseKeptBlocksAwaitingDispose:(int)ms { + NSUInteger count; + volatile int32_t** flags; + // The pool keeps autoreleased temporaries from outliving the release. + @autoreleasepool { + __block NSArray* blocks; + @synchronized(self) { + blocks = keptBlocks; + keptBlocks = nil; + } + count = blocks.count; + if (count == 0) { + return NO; + } + flags = calloc(count, sizeof(*flags)); + for (NSUInteger i = 0; i < count; i++) { + flags[i] = &((__bridge struct TNSBlockHeader*)blocks[i])->flags; + } + dispatch_async(dispatch_get_global_queue(QOS_CLASS_DEFAULT, 0), ^{ + blocks = nil; + }); + } + + BOOL disposing = NO; + CFAbsoluteTime deadline = CFAbsoluteTimeGetCurrent() + ms / 1000.0; + while (!disposing && CFAbsoluteTimeGetCurrent() < deadline) { + disposing = YES; + for (NSUInteger i = 0; i < count; i++) { + if ((__atomic_load_n(flags[i], __ATOMIC_ACQUIRE) & TNSBlockDeallocating) == 0) { + disposing = NO; + usleep(100); + break; + } + } + } + free(flags); + return disposing; } + (void)repeat:(int)count pausingAfter:(void (^)(int))step { diff --git a/TestRunner/app/tests/BlockCacheRaceTests.js b/TestRunner/app/tests/BlockCacheRaceTests.js index 05fbb512..f0b69ce1 100644 --- a/TestRunner/app/tests/BlockCacheRaceTests.js +++ b/TestRunner/app/tests/BlockCacheRaceTests.js @@ -74,15 +74,16 @@ describe("JS block whose dispose is waiting for the isolate", function () { // stays parked on the Locker for the rest of the turn. function strandDispose(fn) { TNSTestNativeCallbacks.repeatPausingAfter(1, function () { - TNSTestNativeCallbacks.keepBlockForMilliseconds(fn, 1); + TNSTestNativeCallbacks.keepBlockUntilReleased(fn); }); - TNSTestNativeCallbacks.sleepMilliseconds(30); + expect(TNSTestNativeCallbacks.releaseKeptBlocksAwaitingDispose(5000)).toBe(true); } it("is reported by interop.handleof while native code holds it", function () { var fn = function () {}; - TNSTestNativeCallbacks.keepBlockForMilliseconds(fn, 1000); + TNSTestNativeCallbacks.keepBlockUntilReleased(fn); expect(interop.handleof(fn) instanceof interop.Pointer).toBe(true); + expect(TNSTestNativeCallbacks.releaseKeptBlocks()).toBe(1); }); it("is not handed out by interop.handleof", function () { @@ -129,10 +130,14 @@ describe("JS block outliving its worker", function () { // gone. it("is released after a teardown that disposed its function", function (done) { var worker = new Worker("./blockFunctionReferenceWorker.js"); + // Dispatched once the worker's runtime has been deleted. + worker.addEventListener("nsworkerended", function () { + expect(TNSTestNativeCallbacks.releaseKeptBlocks()).toBe(1); + done(); + }); worker.onmessage = function (msg) { expect(msg.data).toBe("kept"); worker.terminate(); - setTimeout(done, 600); }; worker.onerror = function (e) { expect(String(e && e.message ? e.message : e)).toBe(""); diff --git a/TestRunner/app/tests/blockFunctionReferenceWorker.js b/TestRunner/app/tests/blockFunctionReferenceWorker.js index 0ddf1a34..676a48f7 100644 --- a/TestRunner/app/tests/blockFunctionReferenceWorker.js +++ b/TestRunner/app/tests/blockFunctionReferenceWorker.js @@ -1,8 +1,8 @@ // Hands native code a block built from a function that interop.FunctionReference -// also registered, and keeps it past this worker's teardown. +// also registered. The parent releases it after this worker's teardown. onmessage = function () { var fn = function () {}; new interop.FunctionReference(fn); - TNSTestNativeCallbacks.keepBlockForMilliseconds(fn, 300); + TNSTestNativeCallbacks.keepBlockUntilReleased(fn); postMessage("kept"); }; From 1a6d059b64e9389521d061fe2ad9ecf4f3527cdb Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Fri, 9 Oct 2026 16:20:27 -0300 Subject: [PATCH 2/2] docs(runtime): tighten the JS block lifetime comments State the JSBlock dispose's ownership rule, keep the Block_copy rationale next to TryRetainJSBlock's definition only, describe the handleof reference as lasting until the autorelease pool drains, and drop the bug narration from DisposeValue's Block case. --- NativeScript/runtime/Interop.h | 4 ++-- NativeScript/runtime/Interop.mm | 5 +++-- NativeScript/runtime/InteropTypes.mm | 5 +++-- NativeScript/runtime/ObjectManager.mm | 8 +++----- 4 files changed, 11 insertions(+), 11 deletions(-) diff --git a/NativeScript/runtime/Interop.h b/NativeScript/runtime/Interop.h index b35cb5a0..7f135b93 100644 --- a/NativeScript/runtime/Interop.h +++ b/NativeScript/runtime/Interop.h @@ -224,8 +224,8 @@ class Interop { static JSBlockDescriptor kJSBlockDescriptor; } JSBlock; - // Takes a reference to a cached JSBlock only while it is live. Block_copy - // would also revive a block whose last release already started its dispose. + // Takes a reference to a cached JSBlock and returns true, unless its last + // release has already started its dispose. static bool TryRetainJSBlock(JSBlock* block); }; diff --git a/NativeScript/runtime/Interop.mm b/NativeScript/runtime/Interop.mm index 998742e9..f8e74557 100644 --- a/NativeScript/runtime/Interop.mm +++ b/NativeScript/runtime/Interop.mm @@ -72,8 +72,9 @@ } } } - // Outside the gate: once the teardown is done the cache slot is - // unreachable and nothing else can free the wrapper. + // Outside the gate: the JSBlock is the wrapper's only owner + // (ObjectManager::DisposeValue leaves it alone), whether or not + // the isolate is alive. delete blockWrapper; delete wrapper; ffi_closure_free(block->ffiClosure); diff --git a/NativeScript/runtime/InteropTypes.mm b/NativeScript/runtime/InteropTypes.mm index a149d055..cb76eb02 100644 --- a/NativeScript/runtime/InteropTypes.mm +++ b/NativeScript/runtime/InteropTypes.mm @@ -697,8 +697,9 @@ new PrimitiveDataWrapper(sizeof(void*), return Pointer::NewInstance(context, blockWrapper->Block()); } // A JS function does not keep its block alive: only native - // references do. A live block is kept for the rest of the turn, - // like one passed to a native call; a dying one is no handle. + // references do. A live block is kept until the current + // autorelease pool drains, like one passed to a native call; a + // dying one is no handle. JSBlock* block = static_cast(blockWrapper->Block()); if (TryRetainJSBlock(block)) { CFAutorelease(block); diff --git a/NativeScript/runtime/ObjectManager.mm b/NativeScript/runtime/ObjectManager.mm index 18a2c6cd..90a7aae2 100644 --- a/NativeScript/runtime/ObjectManager.mm +++ b/NativeScript/runtime/ObjectManager.mm @@ -253,11 +253,9 @@ void DisposeHandle(v8::Isolate* isolate, // native reference goes, possibly after this isolate is gone. return true; } - // 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.) + // Balances the Block_copy taken when a native block was wrapped for JS + // (see Interop::GetResult). Block_release, not CFRelease: the block may + // have been a stack block promoted by Block_copy. Block_release(blockWrapper->Block()); break; }