diff --git a/NativeScript/runtime/Interop.h b/NativeScript/runtime/Interop.h index 85a556f1..b35cb5a0 100644 --- a/NativeScript/runtime/Interop.h +++ b/NativeScript/runtime/Interop.h @@ -223,6 +223,10 @@ 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. + static bool TryRetainJSBlock(JSBlock* block); }; } // namespace tns diff --git a/NativeScript/runtime/Interop.mm b/NativeScript/runtime/Interop.mm index d35a05b2..9cb484cf 100644 --- a/NativeScript/runtime/Interop.mm +++ b/NativeScript/runtime/Interop.mm @@ -54,12 +54,10 @@ HandleScope handle_scope(isolate); Local callback = wrapper->callback_->Get(isolate); if (!callback.IsEmpty() && callback->IsObject()) { - // The callback's slot is the cache's owner, so only a wrapper - // still sitting in it is ours to free. + // The slot may hold another wrapper by now; only our own is + // cleared from it. if (tns::GetValue(isolate, callback) == blockWrapper) { tns::DeleteValue(isolate, callback); - } else { - blockWrapper = nullptr; } } // Unconditional: an already-detached callback still owns its @@ -77,6 +75,32 @@ } }}; +// libclosure's flags layout (Block_private.h): bit 0 is set once the last +// release has started disposing the block, bits 1-15 hold the refcount, and a +// saturated refcount latches the block alive. +static constexpr int32_t kBlockDeallocating = 0x0001; +static constexpr int32_t kBlockRefcountMask = 0xfffe; + +// libclosure's increment ignores the deallocating bit, so Block_copy would hand +// out a block whose dispose is already waiting for the isolate's Locker and +// which libclosure frees right after. This CASes the same word libclosure does. +bool Interop::TryRetainJSBlock(JSBlock* block) { + volatile int32_t* flags = &block->flags; + int32_t old = __atomic_load_n(flags, __ATOMIC_RELAXED); + while (true) { + if ((old & kBlockDeallocating) || (old & kBlockRefcountMask) == 0) { + return false; + } + if ((old & kBlockRefcountMask) == kBlockRefcountMask) { + return true; + } + if (__atomic_compare_exchange_n(flags, &old, old + 2, true, __ATOMIC_ACQ_REL, + __ATOMIC_RELAXED)) { + return true; + } + } +} + std::pair Interop::CreateMethodInternal(const uint8_t initialParamIndex, const uint8_t argsCount, const TypeEncoding* typeEncoding, @@ -538,11 +562,20 @@ inline bool isBool() { if (baseWrapper != nullptr && baseWrapper->Type() == WrapperType::Block) { BlockWrapper* wrapper = static_cast(baseWrapper); // The callee takes the block at +0 and copies it if it needs to keep it, - // so the copy that keeps it alive across the call must be balanced: the - // JSBlock dispose helper owns the ffi closure and the callback wrapper + // so the reference that keeps it alive across the call must be balanced: + // the JSBlock dispose helper owns the ffi closure and the callback wrapper // and only runs once the last reference goes away. - blockPtr = CFAutorelease(Block_copy(wrapper->Block())); - } else { + if (wrapper->OwnsBlock()) { + // A native block; the wrapper's own Block_copy keeps it alive. + blockPtr = CFAutorelease(Block_copy(wrapper->Block())); + } else if (TryRetainJSBlock(static_cast(wrapper->Block()))) { + // Reading the block is safe even when its last release raced ahead: + // while the isolate is valid, dispose clears this slot under the + // Locker this thread holds, before libclosure frees the block. + blockPtr = CFAutorelease(wrapper->Block()); + } + } + if (blockPtr == nullptr) { std::shared_ptr> poCallback = std::make_shared>(isolate, arg); MethodCallbackWrapper* userData = diff --git a/NativeScript/runtime/InteropTypes.mm b/NativeScript/runtime/InteropTypes.mm index 5f93e39b..a149d055 100644 --- a/NativeScript/runtime/InteropTypes.mm +++ b/NativeScript/runtime/InteropTypes.mm @@ -693,7 +693,18 @@ new PrimitiveDataWrapper(sizeof(void*), } case WrapperType::Block: { BlockWrapper* blockWrapper = static_cast(wrapper); - return Pointer::NewInstance(context, blockWrapper->Block()); + if (blockWrapper->OwnsBlock()) { + 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. + JSBlock* block = static_cast(blockWrapper->Block()); + if (TryRetainJSBlock(block)) { + CFAutorelease(block); + return Pointer::NewInstance(context, block); + } + break; } default: break; diff --git a/NativeScript/runtime/ObjectManager.mm b/NativeScript/runtime/ObjectManager.mm index 5d84c1e2..18a2c6cd 100644 --- a/NativeScript/runtime/ObjectManager.mm +++ b/NativeScript/runtime/ObjectManager.mm @@ -246,18 +246,19 @@ void DisposeHandle(v8::Isolate* isolate, } case WrapperType::Block: { BlockWrapper* blockWrapper = static_cast(wrapper); - if (blockWrapper->OwnsBlock()) { - // 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()); + if (!blockWrapper->OwnsBlock()) { + // A block created from a JS callback is owned by the native code it + // was handed to (e.g. NSNotificationCenter), and it owns this wrapper + // (see Interop::JSBlock): its dispose helper frees both once the last + // native reference goes, possibly after this isolate is gone. + return true; } - // Blocks created from JS callbacks (OwnsBlock() == false) are owned by - // the native code they were handed to (e.g. NSNotificationCenter); - // freeing them here would leave that code with a dangling pointer. The - // JSBlock dispose helper cleans up once the last native reference goes. + // 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()); break; } case WrapperType::Reference: { diff --git a/TestFixtures/TNSTestNativeCallbacks.h b/TestFixtures/TNSTestNativeCallbacks.h index c5fc3272..23b83beb 100644 --- a/TestFixtures/TNSTestNativeCallbacks.h +++ b/TestFixtures/TNSTestNativeCallbacks.h @@ -66,6 +66,30 @@ + (NSString*)callOnThread:(NSString* (^)())block; +// Keeps `block` with its own Block_copy and drops that reference from a +// background queue 0-2 ms later, so the last release of a JS-created block can +// land on another thread while JS holds the isolate. `mode` picks the +// releasing queue: 0 a concurrent global queue, 1 a serial queue, 2 like 0 but +// 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; + +// 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 +// by `step` is gone before the next call. Sleeps 0-3 ms after each call +// without leaving the JS turn, so a release scheduled by `step` can drop a +// block's last reference while this thread still holds the isolate. ++ (void)repeat:(int)count pausingAfter:(void (^)(int))step; + +// Runs `work` on a global background queue, then `completion` on the main +// queue. ++ (void)runOnBackgroundQueue:(void (^)(void))work completion:(void (^)(void))completion; + - (void (^)())getBlock; - (void (^)())getBlockFromNative; diff --git a/TestFixtures/TNSTestNativeCallbacks.m b/TestFixtures/TNSTestNativeCallbacks.m index abd0a28d..7438d9f4 100644 --- a/TestFixtures/TNSTestNativeCallbacks.m +++ b/TestFixtures/TNSTestNativeCallbacks.m @@ -371,6 +371,54 @@ + (NSString*)callOnThread:(NSString* (^)())block { return result; } ++ (void)keepBlock:(void (^)(void))block releaseMode:(int)mode { + static dispatch_queue_t serialQueue; + static dispatch_once_t onceToken; + dispatch_once(&onceToken, ^{ + serialQueue = + dispatch_queue_create("org.nativescript.TestFixtures.blockRelease", DISPATCH_QUEUE_SERIAL); + }); + + __block void (^kept)(void) = block; + if (mode == 2) { + [[NSOperationQueue mainQueue] addOperationWithBlock:kept]; + } + dispatch_queue_t queue = + mode == 1 ? serialQueue : dispatch_get_global_queue(QOS_CLASS_DEFAULT, 0); + int64_t delay = (int64_t)arc4random_uniform(2 * NSEC_PER_MSEC + 1); + dispatch_after(dispatch_time(DISPATCH_TIME_NOW, delay), queue, ^{ + kept = nil; + }); +} + ++ (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; + }); +} + ++ (void)sleepMilliseconds:(int)ms { + usleep((useconds_t)ms * 1000); +} + ++ (void)repeat:(int)count pausingAfter:(void (^)(int))step { + for (int i = 0; i < count; i++) { + @autoreleasepool { + step(i); + } + usleep(arc4random_uniform(3001)); + } +} + ++ (void)runOnBackgroundQueue:(void (^)(void))work completion:(void (^)(void))completion { + dispatch_async(dispatch_get_global_queue(QOS_CLASS_DEFAULT, 0), ^{ + work(); + dispatch_async(dispatch_get_main_queue(), completion); + }); +} + - (void (^)())getBlock { return nil; } diff --git a/TestRunner/app/tests/BlockCacheRaceTests.js b/TestRunner/app/tests/BlockCacheRaceTests.js new file mode 100644 index 00000000..2e0efac5 --- /dev/null +++ b/TestRunner/app/tests/BlockCacheRaceTests.js @@ -0,0 +1,143 @@ +// A JS function marshalled as a block caches that block on itself. These tests +// keep re-marshalling one function while native code drops the cached block's +// last reference on another thread, so the block's dispose helper (which waits +// for the isolate's Locker) overlaps cache hits made by the Locker's holder. +describe("JS block cache under cross-thread release", function () { + var ITERATIONS = 600; + var RELEASE_MODES = [ + { name: "a concurrent queue", mode: 0 }, + { name: "a serial queue", mode: 1 }, + { name: "the main operation queue", mode: 2 }, + ]; + + var originalTimeout; + beforeEach(function () { + originalTimeout = jasmine.DEFAULT_TIMEOUT_INTERVAL; + jasmine.DEFAULT_TIMEOUT_INTERVAL = 60000; + }); + afterEach(function () { + jasmine.DEFAULT_TIMEOUT_INTERVAL = originalTimeout; + }); + + function hammer(mode, state) { + var callback = function () { + state.ran++; + }; + TNSTestNativeCallbacks.repeatPausingAfter(ITERATIONS, function () { + TNSTestNativeCallbacks.keepBlockReleaseMode(callback, mode); + }); + } + + // Releases land up to 2 ms after the loop, and enqueued operations run on + // later main-queue passes. + function settle(mode, state, done) { + var attempts = 200; + (function poll() { + if ((mode !== 2 || state.ran === ITERATIONS) || --attempts === 0) { + setTimeout(function () { + __collect(); + if (mode === 2) { + expect(state.ran).toBe(ITERATIONS); + } + done(); + }, 20); + return; + } + setTimeout(poll, 10); + })(); + } + + RELEASE_MODES.forEach(function (variant) { + it("survives releases from " + variant.name + " while JS runs on the main thread", function (done) { + var state = { ran: 0 }; + hammer(variant.mode, state); + settle(variant.mode, state, done); + }); + + it("survives releases from " + variant.name + " while JS runs on a background thread", function (done) { + var state = { ran: 0 }; + TNSTestNativeCallbacks.runOnBackgroundQueueCompletion( + function () { + hammer(variant.mode, state); + }, + function () { + settle(variant.mode, state, done); + } + ); + }); + }); +}); + +describe("JS block whose dispose is waiting for the isolate", function () { + // Drops the last native reference to fn's cached block on a background + // queue while this thread keeps the isolate locked, so the block's dispose + // stays parked on the Locker for the rest of the turn. + function strandDispose(fn) { + TNSTestNativeCallbacks.repeatPausingAfter(1, function () { + TNSTestNativeCallbacks.keepBlockForMilliseconds(fn, 1); + }); + TNSTestNativeCallbacks.sleepMilliseconds(30); + } + + it("is reported by interop.handleof while native code holds it", function () { + var fn = function () {}; + TNSTestNativeCallbacks.keepBlockForMilliseconds(fn, 1000); + expect(interop.handleof(fn) instanceof interop.Pointer).toBe(true); + }); + + it("is not handed out by interop.handleof", function () { + var fn = function () {}; + strandDispose(fn); + expect(function () { + interop.handleof(fn); + }).toThrow(); + }); + + it("is replaced by a fresh block when the function is marshalled again", function (done) { + var ran = 0; + var fn = function () { + ran++; + }; + strandDispose(fn); + NSOperationQueue.mainQueue.addOperationWithBlock(fn); + + var attempts = 100; + (function poll() { + if (ran === 1 || --attempts === 0) { + expect(ran).toBe(1); + done(); + return; + } + setTimeout(poll, 10); + })(); + }); +}); + +describe("JS block outliving its worker", function () { + var originalTimeout; + beforeEach(function () { + originalTimeout = jasmine.DEFAULT_TIMEOUT_INTERVAL; + jasmine.DEFAULT_TIMEOUT_INTERVAL = 10000; + }); + afterEach(function () { + jasmine.DEFAULT_TIMEOUT_INTERVAL = originalTimeout; + }); + + // The function is also registered through interop.FunctionReference, so + // the worker's teardown disposes it while native code still holds the + // block built from it; the block's own dispose runs after the isolate is + // gone. + it("is released after a teardown that disposed its function", function (done) { + var worker = new Worker("./blockFunctionReferenceWorker.js"); + 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(""); + done(); + }; + worker.postMessage(0); + }); +}); diff --git a/TestRunner/app/tests/blockFunctionReferenceWorker.js b/TestRunner/app/tests/blockFunctionReferenceWorker.js new file mode 100644 index 00000000..0ddf1a34 --- /dev/null +++ b/TestRunner/app/tests/blockFunctionReferenceWorker.js @@ -0,0 +1,8 @@ +// Hands native code a block built from a function that interop.FunctionReference +// also registered, and keeps it past this worker's teardown. +onmessage = function () { + var fn = function () {}; + new interop.FunctionReference(fn); + TNSTestNativeCallbacks.keepBlockForMilliseconds(fn, 300); + postMessage("kept"); +}; diff --git a/TestRunner/app/tests/index.js b/TestRunner/app/tests/index.js index 31bfe10a..f2f9e6fe 100644 --- a/TestRunner/app/tests/index.js +++ b/TestRunner/app/tests/index.js @@ -130,6 +130,7 @@ require("./MetadataTests"); require("./ApiTests"); require("./NsRuntimeTests"); require("./GCFinalizerTests"); +require("./BlockCacheRaceTests"); require("./WorkerConcurrentStartupTests"); require("./WorkerOptionsTests"); require("./WorkerResourceLimitsTests");