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; } 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"); };