diff --git a/NativeScript/runtime/ArgConverter.mm b/NativeScript/runtime/ArgConverter.mm index bae55621..80263ab9 100644 --- a/NativeScript/runtime/ArgConverter.mm +++ b/NativeScript/runtime/ArgConverter.mm @@ -293,11 +293,6 @@ Isolate* isolate = data->isolateWrapper_.Isolate(); - if (!data->isolateWrapper_.IsValid()) { - memset(retValue, 0, cif->rtype->size); - return; - } - // Declared before all V8 scopes: an ObjC exception must never unwind through a // live V8 scope (Locker/HandleScope/Context::Scope). A branded escape caught // below is captured here and @thrown only after the inner block closes every @@ -305,6 +300,15 @@ NSException* __strong pendingThrow = nil; { + // Outlives the Locker: a runtime torn down while this thread waits for the + // lock cannot dispose the isolate before the wait is over. Free when this + // thread has already entered the isolate. + IsolatePin pin = data->isolateWrapper_.Pin(); + if (!pin || !data->isolateWrapper_.IsValid()) { + memset(retValue, 0, cif->rtype->size); + return; + } + v8::Locker locker(isolate); // Checked again with the isolate locked: a runtime being destroyed holds // this lock while it removes its caches, so the check above can pass and diff --git a/NativeScript/runtime/Caches.cpp b/NativeScript/runtime/Caches.cpp index 13c86574..868c1b3b 100644 --- a/NativeScript/runtime/Caches.cpp +++ b/NativeScript/runtime/Caches.cpp @@ -12,7 +12,8 @@ namespace tns { Caches::Caches(Isolate* isolate, const int& isolateId) : PromiseRejections(std::make_unique(isolate)), isolate_(isolate), - isolateId_(isolateId) {} + isolateId_(isolateId), + gateId_(isolateId) {} Caches::~Caches() { // Subsystem state may hold v8 handles and reference the core caches below; diff --git a/NativeScript/runtime/Caches.h b/NativeScript/runtime/Caches.h index c7f497a5..01d0b642 100644 --- a/NativeScript/runtime/Caches.h +++ b/NativeScript/runtime/Caches.h @@ -117,6 +117,11 @@ class Caches { inline int getIsolateId() { return isolateId_; } + // The id assigned at Init; unlike getIsolateId() it survives + // InvalidateIsolate, so it still names this isolate's gate during teardown. + // -1 for the stand-in Get() returns once the isolate's Caches is gone. + inline int getGateId() const { return gateId_; } + inline void InvalidateIsolate() { isolateId_ = -1; } inline bool IsValid() { return isolateId_ != -1; } @@ -317,6 +322,7 @@ class Caches { v8::Isolate* isolate_; std::shared_ptr> context_; int isolateId_; + const int gateId_; }; } // namespace tns diff --git a/NativeScript/runtime/Interop.mm b/NativeScript/runtime/Interop.mm index 9cb484cf..998742e9 100644 --- a/NativeScript/runtime/Interop.mm +++ b/NativeScript/runtime/Interop.mm @@ -44,30 +44,36 @@ // resetting it never touches the finalizer drain's bookkeeping, // and a foreign-thread Locker into the block's own isolate is // legitimate now that extended class names are worker-scoped. - if (wrapper->isolateWrapper_.IsValid()) { - Isolate* isolate = wrapper->isolateWrapper_.Isolate(); - v8::Locker locker(isolate); - // Re-checked under the lock: ~Runtime holds it while it tears the - // isolate's caches down, so the check above can predate that. - if (wrapper->isolateWrapper_.IsValid()) { - Isolate::Scope isolate_scope(isolate); - HandleScope handle_scope(isolate); - Local callback = wrapper->callback_->Get(isolate); - if (!callback.IsEmpty() && callback->IsObject()) { - // 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); + // + // Validity is not the test here: the slot stays reachable after + // the isolate is invalidated, by JS still running under the Locker + // and by the teardown's own walk of registered objects. Until the + // gate closes, the wrapper is only freed once the slot is cleared. + { + IsolatePin pin = wrapper->isolateWrapper_.Pin(); + if (pin) { + Isolate* isolate = wrapper->isolateWrapper_.Isolate(); + v8::Locker locker(isolate); + if (!wrapper->isolateWrapper_.IsTornDown()) { + Isolate::Scope isolate_scope(isolate); + HandleScope handle_scope(isolate); + Local callback = wrapper->callback_->Get(isolate); + if (!callback.IsEmpty() && callback->IsObject()) { + // 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); + } } + // Unconditional: an already-detached callback still owns its + // node, and dropping the persistent without a reset would leave + // that node rooted forever. + wrapper->callback_->Reset(); } - // Unconditional: an already-detached callback still owns its - // node, and dropping the persistent without a reset would leave - // that node rooted forever. - wrapper->callback_->Reset(); } } - // Outside the isolate guard: once the isolate is gone the cache - // slot is unreachable and nothing else can free the wrapper. + // Outside the gate: once the teardown is done the cache slot is + // unreachable and nothing else can free the wrapper. delete blockWrapper; delete wrapper; ffi_closure_free(block->ffiClosure); @@ -570,8 +576,9 @@ inline bool isBool() { 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. + // until the isolate's teardown closes its gate, dispose clears this + // slot under the Locker this thread holds, before libclosure frees + // the block. blockPtr = CFAutorelease(wrapper->Block()); } } diff --git a/NativeScript/runtime/IsolateWrapper.cpp b/NativeScript/runtime/IsolateWrapper.cpp index e076907c..a7679221 100644 --- a/NativeScript/runtime/IsolateWrapper.cpp +++ b/NativeScript/runtime/IsolateWrapper.cpp @@ -7,12 +7,95 @@ // #include "IsolateWrapper.h" + +#include +#include + #include "Runtime.h" +#include "UnfairLock.h" +#include "robin_hood.h" namespace tns { bool IsolateWrapper::IsValid() const { - return Runtime::IsAlive(isolate_) && isolate_->GetData(tns::Constants::CACHES_ISOLATE_SLOT) != nullptr && GetCache()->getIsolateId() == isolateId_; + if (!Runtime::IsAlive(isolate_) || + isolate_->GetData(tns::Constants::CACHES_ISOLATE_SLOT) == nullptr) { + return false; + } + std::shared_ptr cache = GetCache(); + return cache->IsValid() && cache->getGateId() == gateId_; +} + +namespace IsolateGates { + +namespace { + +struct Gate { + uint32_t pins = 0; + bool closed = false; +}; + +// Trivially destructible, so exit() leaves it usable. +static UnfairMutex gatesMutex; + +// Never destroyed: exit() runs static destructors while other threads may +// still be pinning. +robin_hood::unordered_map& Gates() { + static auto* gates = new robin_hood::unordered_map(); + return *gates; +} + +} // namespace + +void Open(int id) { + std::lock_guard lock(gatesMutex); + Gates()[id] = Gate(); +} + +bool TryPin(int id) { + std::lock_guard lock(gatesMutex); + auto it = Gates().find(id); + if (it == Gates().end() || it->second.closed) { + return false; + } + it->second.pins++; + return true; +} + +void Unpin(int id) { + std::lock_guard lock(gatesMutex); + auto it = Gates().find(id); + if (it != Gates().end()) { + it->second.pins--; + } +} + +void Close(int id) { + std::lock_guard lock(gatesMutex); + auto it = Gates().find(id); + if (it != Gates().end()) { + it->second.closed = true; + } +} + +bool IsClosed(int id) { + std::lock_guard lock(gatesMutex); + auto it = Gates().find(id); + return it == Gates().end() || it->second.closed; +} + +bool RetireIfUnpinned(int id) { + std::lock_guard lock(gatesMutex); + auto it = Gates().find(id); + if (it == Gates().end()) { + return true; + } + if (it->second.pins > 0) { + return false; + } + Gates().erase(it); + return true; } +} // namespace IsolateGates } diff --git a/NativeScript/runtime/IsolateWrapper.h b/NativeScript/runtime/IsolateWrapper.h index 0eb56639..96407565 100644 --- a/NativeScript/runtime/IsolateWrapper.h +++ b/NativeScript/runtime/IsolateWrapper.h @@ -9,36 +9,86 @@ #ifndef IsolateWrapper_h #define IsolateWrapper_h -#include "v8.h" +#include + #include "Caches.h" #include "Constants.h" +#include "v8.h" namespace tns { +// One gate per isolate, keyed by Caches::getGateId() (ids are never reused). +// A thread that has not entered the isolate pins its gate before waiting for +// or holding the isolate's Locker. A pin taken before Close() defers +// Isolate::Dispose until it is released. Once the teardown has closed the +// gate, no wrapper can be reached from JS any more and pins are refused. A +// missing entry reads as closed. +namespace IsolateGates { +void Open(int id); +bool TryPin(int id); +void Unpin(int id); +// Called by the teardown while it holds the isolate's Locker, after the last +// walk that reads wrappers through JS objects. +void Close(int id); +bool IsClosed(int id); +// Drops the entry unless a pin is held; the isolate may be disposed only once +// this returns true. +bool RetireIfUnpinned(int id); +} // namespace IsolateGates + +// Owns a pin for its scope. Declare it before the Locker it guards, so the +// Locker is released first. A thread that has entered the isolate takes no +// pin: an entered isolate stays in use, which already defers its disposal. +class IsolatePin { + public: + IsolatePin(int id, bool entered) + : id_(id), + pinned_(!entered && IsolateGates::TryPin(id)), + usable_(entered || pinned_) {} + ~IsolatePin() { + if (pinned_) { + IsolateGates::Unpin(id_); + } + } + IsolatePin(const IsolatePin&) = delete; + IsolatePin& operator=(const IsolatePin&) = delete; + explicit operator bool() const { return usable_; } + + private: + int id_; + bool pinned_; + bool usable_; +}; + +// Kept trivially copyable: ObjC blocks that live as long as the process (the +// extended classes' synthesized methods) capture it by value. class IsolateWrapper { public: bool IsValid() const; inline std::shared_ptr GetCache() const { return tns::Caches::Get(isolate_); } - inline v8::Isolate* Isolate() { - return isolate_; - } - inline int IsolateId() { - return isolateId_; - } + inline v8::Isolate* Isolate() { return isolate_; } inline IsolateWrapper(v8::Isolate* isolate) { isolate_ = isolate; - isolateId_ = tns::Caches::Get(isolate_)->getIsolateId(); + gateId_ = tns::Caches::Get(isolate_)->getGateId(); + } + // Hold the returned pin across any wait for the isolate's Locker from a + // thread that is not already inside the isolate; an empty pin means the + // isolate may already be disposed and must not be touched at all. + inline IsolatePin Pin() const { + return IsolatePin(gateId_, v8::Isolate::TryGetCurrent() == isolate_); } - -private: + // True once the teardown has finished with every wrapper reachable from + // JS. Read it while pinned. + inline bool IsTornDown() const { return IsolateGates::IsClosed(gateId_); } + + private: v8::Isolate* isolate_; - int isolateId_; - - + int gateId_; }; +static_assert(std::is_trivially_copyable_v); } #endif /* IsolateWrapper_h */ diff --git a/NativeScript/runtime/Runtime.mm b/NativeScript/runtime/Runtime.mm index 216fb710..a358a46f 100644 --- a/NativeScript/runtime/Runtime.mm +++ b/NativeScript/runtime/Runtime.mm @@ -193,15 +193,18 @@ static void InitializeImportMetaObject(Local context, Local mod // CFNotificationCenterRemoveObserver(CFNotificationCenterGetLocalCenter(), this, // kCFTimeZoneSystemTimeZoneDidChangeNotification, NULL); -void DisposeIsolateWhenPossible(Isolate* isolate) { - // Disposal is deferred only while the isolate is still entered, which happens +void DisposeIsolateWhenPossible(Isolate* isolate, int gateId) { + // Disposal is deferred while the isolate is still entered, which happens // when the runtime is deleted by code running inside its own isolate: an - // embedder calling shutdownRuntime from a JS callback. exit() never deletes a - // runtime, so a process that is dying leaves its isolates to the OS. - if (isolate->IsInUse()) { + // embedder calling shutdownRuntime from a JS callback. It is also deferred + // while another thread holds a pin to wait for or use the isolate's Locker. + // Never block here instead: the pin holder may be waiting for a Locker this + // thread still holds further up the stack. exit() never deletes a runtime, + // so a process that is dying leaves its isolates to the OS. + if (isolate->IsInUse() || !IsolateGates::RetireIfUnpinned(gateId)) { dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(10.0 * NSEC_PER_MSEC)), dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_LOW, 0), ^{ - DisposeIsolateWhenPossible(isolate); + DisposeIsolateWhenPossible(isolate, gateId); }); } else { isolate->Dispose(); @@ -234,10 +237,11 @@ void DisposeIsolateWhenPossible(Isolate* isolate) { NativeScriptException::OnIsolateTeardown(isolate_); auto currentIsolate = this->isolate_; + int gateId = Caches::Get(this->isolate_)->getGateId(); { - // make sure we remove the isolate from the list of active isolates first - // this will make sure isAlive(isolate) will return false and prevent locking of the v8 isolate - // after it terminates execution + // First, so IsAlive turns false: native callbacks arriving from here on + // bail before taking the Locker. A block's dispose still pins and locks + // until the gate closes below. SpinLock lock(isolatesMutex_); Runtime::isolates_.erase( std::remove(Runtime::isolates_.begin(), Runtime::isolates_.end(), this->isolate_), @@ -299,9 +303,14 @@ void DisposeIsolateWhenPossible(Isolate* isolate) { Caches::Remove(this->isolate_); this->isolate_->SetData(Constants::RUNTIME_SLOT, nullptr); + + // Last, and under the Locker: from here on nothing reads a wrapper through + // a JS object, so threads that never pinned may free theirs without the + // lock, and threads already pinned see the gate closed once they get it. + IsolateGates::Close(gateId); } - DisposeIsolateWhenPossible(this->isolate_); + DisposeIsolateWhenPossible(this->isolate_, gateId); // Matched erase: only removes the registry entry while it still maps to // this runtime's loop, so a worker isolate that reuses this pointer after @@ -384,6 +393,8 @@ void DisposeIsolateWhenPossible(Isolate* isolate) { void Runtime::Init(Isolate* isolate, bool isWorker) { std::shared_ptr cache = Caches::Init(isolate, nextIsolateId.fetch_add(1, std::memory_order_relaxed)); + // Before anything can build an IsolateWrapper for this isolate. + IsolateGates::Open(cache->getGateId()); cache->isWorker = isWorker; cache->ObjectCtorInitializer = MetadataBuilder::GetOrCreateConstructorFunctionTemplate; cache->StructPrototypeInitializer = MetadataBuilder::GetOrCreateStructPrototype; diff --git a/TestRunner/app/tests/BlockCacheRaceTests.js b/TestRunner/app/tests/BlockCacheRaceTests.js index 2e0efac5..05fbb512 100644 --- a/TestRunner/app/tests/BlockCacheRaceTests.js +++ b/TestRunner/app/tests/BlockCacheRaceTests.js @@ -140,4 +140,20 @@ describe("JS block outliving its worker", function () { }; worker.postMessage(0); }); + + // The worker's teardown itself drops the block's last reference, while it + // still has the function left to dispose. + it("is released by the teardown that later disposes its function", function (done) { + var worker = new Worker("./blockTeardownReleaseWorker.js"); + worker.onmessage = function (msg) { + expect(msg.data).toBe("held"); + 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/blockTeardownReleaseWorker.js b/TestRunner/app/tests/blockTeardownReleaseWorker.js new file mode 100644 index 00000000..c60e22a5 --- /dev/null +++ b/TestRunner/app/tests/blockTeardownReleaseWorker.js @@ -0,0 +1,16 @@ +// Leaves a native object as the only owner of a block built from a function +// that interop.FunctionReference registered first. Teardown disposes registered +// objects newest first, so it releases the block (and runs its dispose) before +// it reaches the function whose slot still points at the block's wrapper. +onmessage = function () { + var fn = function () {}; + new interop.FunctionReference(fn); + var operation = NSBlockOperation.alloc().init(); + // The pool drains the reference the marshalling call autoreleased. + TNSTestNativeCallbacks.repeatPausingAfter(1, function () { + operation.addExecutionBlock(fn); + }); + globalThis.heldFunction = fn; + globalThis.heldOperation = operation; + postMessage("held"); +};