From 6d8e58bd2ececf6e3830339f051b6ad2fb7fb08a Mon Sep 17 00:00:00 2001 From: Adrian Niculescu <15037449+adrian-niculescu@users.noreply.github.com> Date: Thu, 17 Sep 2026 13:55:40 +0300 Subject: [PATCH 1/3] fix(worker): isolate and wrapper lifetime fixes around startup and teardown A throwing onclose run from the entry script read the worker isolate before it was published and crashed. Terminate() used the worker isolate with no synchronization against the worker thread deleting its runtime. EndWrapperLifetime kept using the wrapper after dispatching nsworkerended, whose listeners can shut the runtime down and delete it. --- NativeScript/runtime/DataWrapper.h | 12 ++++- NativeScript/runtime/Worker.mm | 4 +- NativeScript/runtime/WorkerWrapper.mm | 48 ++++++++++++++----- TestRunner/app/tests/MessagingTests.js | 10 ++++ .../tests/messaging/throwingOncloseWorker.js | 6 +++ 5 files changed, 64 insertions(+), 16 deletions(-) create mode 100644 TestRunner/app/tests/messaging/throwingOncloseWorker.js diff --git a/NativeScript/runtime/DataWrapper.h b/NativeScript/runtime/DataWrapper.h index b0c04abf..84f6e7b4 100644 --- a/NativeScript/runtime/DataWrapper.h +++ b/NativeScript/runtime/DataWrapper.h @@ -553,12 +553,16 @@ class WorkerWrapper : public BaseDataWrapper { void Start(std::shared_ptr> poWorker, std::function func, std::optional qualityOfService = std::nullopt); - void CallOnErrorHandlers(v8::TryCatch& tc); + // Both reporters take the isolate from their caller, which is running on + // it: they are reachable while the entry script is still evaluating, before + // workerIsolate_ is published. + void CallOnErrorHandlers(v8::Isolate* isolate, v8::TryCatch& tc); // Reports a rejected entry-evaluation promise. A rejection carries a reason // rather than a TryCatch, so it cannot go through CallOnErrorHandlers, but it // follows the same web order: the worker scope's `onerror` first, then — only // if that did not handle it — the parent's Worker error event. - void ReportEntryEvaluationRejection(v8::Local context, + void ReportEntryEvaluationRejection(v8::Isolate* isolate, + v8::Local context, v8::Local reason); void PassUncaughtExceptionFromWorkerToMain(v8::Local context, v8::TryCatch& tc, @@ -629,7 +633,11 @@ class WorkerWrapper : public BaseDataWrapper { private: v8::Isolate* mainIsolate_; + // Written by the worker thread only: published once the worker's startup + // function returns, withdrawn before the worker's runtime is deleted. Any + // other thread reads and uses it under workerIsolateMutex_. v8::Isolate* workerIsolate_; + std::mutex workerIsolateMutex_; std::atomic isRunning_; std::atomic isClosing_; std::atomic isTerminating_; diff --git a/NativeScript/runtime/Worker.mm b/NativeScript/runtime/Worker.mm index 053ca7ba..14e5dc70 100644 --- a/NativeScript/runtime/Worker.mm +++ b/NativeScript/runtime/Worker.mm @@ -564,7 +564,7 @@ throw NativeScriptException( ? info[0] : Local(v8::Exception::Error(tns::ToV8String( iso, "Worker entry module evaluation rejected"))); - w->ReportEntryEvaluationRejection(ctx, reason); + w->ReportEntryEvaluationRejection(iso, ctx, reason); }; Local onFulfilled; Local onRejected; @@ -798,7 +798,7 @@ throw NativeScriptException( TryCatch tc(isolate); success = onCloseFunc->Call(context, v8::Undefined(isolate), 0, args).ToLocal(&result); if (!success && tc.HasCaught()) { - worker->CallOnErrorHandlers(tc); + worker->CallOnErrorHandlers(isolate, tc); } } } diff --git a/NativeScript/runtime/WorkerWrapper.mm b/NativeScript/runtime/WorkerWrapper.mm index 90ddcb57..209ec1f5 100644 --- a/NativeScript/runtime/WorkerWrapper.mm +++ b/NativeScript/runtime/WorkerWrapper.mm @@ -120,17 +120,26 @@ static void PostToLoop(const std::shared_ptr& loop, std::function worker = - this->poWorker_ != nullptr ? this->poWorker_->Get(this->mainIsolate_) : Local(); + // The dispatch below runs listeners, and a listener may shut the runtime + // down, whose teardown deletes this wrapper. Everything the dispatch needs is + // read first, and the liveness token says afterwards whether `this` is still + // there to unroot. + Isolate* isolate = this->mainIsolate_; + std::shared_ptr> selfRef = this->selfRef_; + Local worker = this->poWorker_ != nullptr ? this->poWorker_->Get(isolate) : Local(); if (!worker.IsEmpty() && worker->IsObject()) { - TryCatch tc(this->mainIsolate_); - Worker::EmitEnded(this->mainIsolate_, worker.As()); + TryCatch tc(isolate); + Worker::EmitEnded(isolate, worker.As()); if (tc.HasCaught()) { Local error = tc.Exception(); - Log(@"%s", tns::ToString(this->mainIsolate_, error).c_str()); - this->mainIsolate_->ThrowException(error); + Log(@"%s", tns::ToString(isolate, error).c_str()); + isolate->ThrowException(error); } } + if (selfRef->load(std::memory_order_acquire) == nullptr) { + // Deleted during the dispatch; that teardown released the Worker object. + return; + } this->UnrootWorkerObject(); } @@ -181,7 +190,7 @@ static void PostToLoop(const std::shared_ptr& loop, std::functiononMessage_(this->workerIsolate_, globalTarget, message); if (tc.HasCaught()) { - this->CallOnErrorHandlers(tc); + this->CallOnErrorHandlers(this->workerIsolate_, tc); } } @@ -232,7 +241,11 @@ static void PostThreadEndedNotification(Isolate* mainIsolate, std::weak_ptrworkerIsolate_ = func(); + Isolate* workerIsolate = func(); + { + std::lock_guard lock(this->workerIsolateMutex_); + this->workerIsolate_ = workerIsolate; + } this->DrainPendingTasks(); @@ -242,6 +255,14 @@ static void PostThreadEndedNotification(Isolate* mainIsolate, std::weak_ptr lock(this->workerIsolateMutex_); + this->workerIsolate_ = nullptr; + } + // The inspector must be gone before the Runtime (and with it the isolate) // is deleted below. this->DestroyInspector(); @@ -292,6 +313,9 @@ static void PostThreadEndedNotification(Isolate* mainIsolate, std::weak_ptrisTerminating_.exchange(true); if (!wasTerminating) { + // Held across the use, not just the read: the worker thread withdraws the + // isolate under the same mutex before deleting its runtime. + std::unique_lock isolateLock(this->workerIsolateMutex_); if (this->workerIsolate_ != nullptr) { // Flagged before the request so a pump that is between iterations sees // it on its next check, rather than only once V8 has some JS to @@ -307,6 +331,7 @@ static void PostThreadEndedNotification(Isolate* mainIsolate, std::weak_ptrworkerIsolate_->TerminateExecution(); } + isolateLock.unlock(); { // A worker paused at a breakpoint sits in the inspector's nested pause // loop, not in the CFRunLoop — kick it loose so TerminateExecution and @@ -414,11 +439,10 @@ static void PostThreadEndedNotification(Isolate* mainIsolate, std::weak_ptrisTerminating_) { return; } - Isolate* isolate = this->workerIsolate_; Local context = Caches::Get(isolate)->GetContext(); Local global = context->Global(); @@ -447,11 +471,11 @@ static void PostThreadEndedNotification(Isolate* mainIsolate, std::weak_ptrPassUncaughtExceptionFromWorkerToMain(context, tc); } -void WorkerWrapper::ReportEntryEvaluationRejection(Local context, Local reason) { +void WorkerWrapper::ReportEntryEvaluationRejection(Isolate* isolate, Local context, + Local reason) { if (this->isTerminating_) { return; } - Isolate* isolate = this->workerIsolate_; Local global = context->Global(); Local onErrorVal; diff --git a/TestRunner/app/tests/MessagingTests.js b/TestRunner/app/tests/MessagingTests.js index 59441b26..ceaf2fe8 100644 --- a/TestRunner/app/tests/MessagingTests.js +++ b/TestRunner/app/tests/MessagingTests.js @@ -252,6 +252,16 @@ describe("Messaging runtime edges", function () { } }; }); + + it("reports an error onclose threw while the entry script was still running", function (done) { + var worker = new Worker("./messaging/throwingOncloseWorker.js"); + worker.onerror = function (event) { + event.preventDefault(); + expect(event.message).toContain("boom from onclose"); + worker.terminate(); + done(); + }; + }); }); describe("AbortSignal handler attribute accounting", function () { diff --git a/TestRunner/app/tests/messaging/throwingOncloseWorker.js b/TestRunner/app/tests/messaging/throwingOncloseWorker.js new file mode 100644 index 00000000..fceedf0a --- /dev/null +++ b/TestRunner/app/tests/messaging/throwingOncloseWorker.js @@ -0,0 +1,6 @@ +// Closes from inside the entry script, so onclose runs before the entry has +// finished evaluating. +onclose = function () { + throw new Error("boom from onclose"); +}; +close(); From 1965a835bbd9e45ec1b6ade5766f8d210499503c Mon Sep 17 00:00:00 2001 From: Adrian Niculescu <15037449+adrian-niculescu@users.noreply.github.com> Date: Thu, 17 Sep 2026 14:27:33 +0300 Subject: [PATCH 2/3] fix(worker): serialize the message queue's run loop source with its teardown Push() and Signal() read the run loop source and run loop with no lock while Terminate(), on another thread, invalidates and releases the source. They now go through the mutex Terminate() holds, and the worker thread terminates its queue before it leaves, since the queue borrows that thread's run loop. Also removes the unused GetWorkerIsolate() getter, which handed out the isolate without the mutex that now guards it. --- NativeScript/runtime/ConcurrentQueue.cpp | 26 ++++++++---------------- NativeScript/runtime/ConcurrentQueue.h | 1 - NativeScript/runtime/DataWrapper.h | 1 - NativeScript/runtime/WorkerWrapper.mm | 6 ++++++ 4 files changed, 14 insertions(+), 20 deletions(-) diff --git a/NativeScript/runtime/ConcurrentQueue.cpp b/NativeScript/runtime/ConcurrentQueue.cpp index 6e34073f..7f331fc7 100644 --- a/NativeScript/runtime/ConcurrentQueue.cpp +++ b/NativeScript/runtime/ConcurrentQueue.cpp @@ -15,10 +15,6 @@ void ConcurrentQueue::Initialize(CFRunLoopRef runLoop, void (*performWork)(void* } void ConcurrentQueue::Push(std::shared_ptr message) { - if (this->runLoopTasksSource_ != nullptr && !CFRunLoopSourceIsValid(this->runLoopTasksSource_)) { - return; - } - { // Checked under the queue mutex, where Terminate() also flips it while // emptying the queue: a push that loses the race is dropped rather than @@ -30,7 +26,7 @@ void ConcurrentQueue::Push(std::shared_ptr message) { this->messagesQueue_.push(message); } - this->SignalAndWakeUp(); + this->Signal(); } std::vector> ConcurrentQueue::PopAll() { @@ -52,20 +48,14 @@ bool ConcurrentQueue::IsEmpty() { } void ConcurrentQueue::Signal() { - // Mirrors Push()'s validity handling instead of SignalAndWakeUp()'s - // assert: a retry racing Terminate() must be a silent no-op. - if (this->runLoopTasksSource_ == nullptr || - !CFRunLoopSourceIsValid(this->runLoopTasksSource_)) { - return; + // Serializes signaling and waking with Initialize() and Terminate(). + // Terminate() clears both pointers and invalidates and releases the source; + // the run loop is borrowed from the worker thread, which terminates the + // queue before it leaves. + std::unique_lock lock(initializationMutex_); + if (this->runLoopTasksSource_ != nullptr) { + CFRunLoopSourceSignal(this->runLoopTasksSource_); } - this->SignalAndWakeUp(); -} - -void ConcurrentQueue::SignalAndWakeUp() { - if (this->runLoopTasksSource_ != nullptr) { - tns::Assert(CFRunLoopSourceIsValid(this->runLoopTasksSource_)); - CFRunLoopSourceSignal(this->runLoopTasksSource_); - } if (this->runLoop_ != nullptr) { CFRunLoopWakeUp(this->runLoop_); diff --git a/NativeScript/runtime/ConcurrentQueue.h b/NativeScript/runtime/ConcurrentQueue.h index b8fe1694..33c78220 100644 --- a/NativeScript/runtime/ConcurrentQueue.h +++ b/NativeScript/runtime/ConcurrentQueue.h @@ -32,7 +32,6 @@ struct ConcurrentQueue { std::atomic terminated{false}; std::mutex mutex_; std::mutex initializationMutex_; - void SignalAndWakeUp(); }; } diff --git a/NativeScript/runtime/DataWrapper.h b/NativeScript/runtime/DataWrapper.h index 84f6e7b4..66bc91dc 100644 --- a/NativeScript/runtime/DataWrapper.h +++ b/NativeScript/runtime/DataWrapper.h @@ -627,7 +627,6 @@ class WorkerWrapper : public BaseDataWrapper { const inline v8::Isolate* GetMainIsolate() { return mainIsolate_; } // The only route from the worker thread to the parent: see mainLoop_. std::weak_ptr MainLoop() const { return mainLoop_; } - const inline v8::Isolate* GetWorkerIsolate() { return workerIsolate_; } const inline void MakeWeak() { isWeak_ = true; } const inline bool IsWeak() { return isWeak_; } diff --git a/NativeScript/runtime/WorkerWrapper.mm b/NativeScript/runtime/WorkerWrapper.mm index 209ec1f5..2cc9213d 100644 --- a/NativeScript/runtime/WorkerWrapper.mm +++ b/NativeScript/runtime/WorkerWrapper.mm @@ -255,6 +255,12 @@ static void PostThreadEndedNotification(Isolate* mainIsolate, std::weak_ptrqueue_.Terminate(); + // Withdrawn before the runtime and its isolate go away below. Terminate() // uses the isolate under this mutex, so a terminate that already read it has // finished with it by the time this returns, and a later one finds null. From 70769c4b99a826a055ff1721c6631bf096b52b19 Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Mon, 5 Oct 2026 15:03:16 -0300 Subject: [PATCH 3/3] refactor(worker): rename the queue's run loop mutex and scope the isolate lock initializationMutex_ guards the run loop source and run loop for the queue's whole life now that Signal() takes it, so it is named runLoopMutex_. Terminate() holds workerIsolateMutex_ in a scoped block, like the inspectorMutex_ block next to it, instead of a unique_lock released by hand. The comment in Push() said Terminate() flips the terminated flag under the queue mutex; it sets it under the run loop mutex, before emptying the queue. --- NativeScript/runtime/ConcurrentQueue.cpp | 18 ++++++------ NativeScript/runtime/ConcurrentQueue.h | 2 +- NativeScript/runtime/WorkerWrapper.mm | 35 ++++++++++++------------ 3 files changed, 28 insertions(+), 27 deletions(-) diff --git a/NativeScript/runtime/ConcurrentQueue.cpp b/NativeScript/runtime/ConcurrentQueue.cpp index 7f331fc7..982780c6 100644 --- a/NativeScript/runtime/ConcurrentQueue.cpp +++ b/NativeScript/runtime/ConcurrentQueue.cpp @@ -4,10 +4,10 @@ namespace tns { void ConcurrentQueue::Initialize(CFRunLoopRef runLoop, void (*performWork)(void*), void* info) { - std::unique_lock lock(initializationMutex_); - if (terminated) { - return; - } + std::unique_lock lock(runLoopMutex_); + if (terminated) { + return; + } this->runLoop_ = runLoop; CFRunLoopSourceContext sourceContext = { 0, info, 0, 0, 0, 0, 0, 0, 0, performWork }; this->runLoopTasksSource_ = CFRunLoopSourceCreate(kCFAllocatorDefault, 0, &sourceContext); @@ -16,9 +16,9 @@ void ConcurrentQueue::Initialize(CFRunLoopRef runLoop, void (*performWork)(void* void ConcurrentQueue::Push(std::shared_ptr message) { { - // Checked under the queue mutex, where Terminate() also flips it while - // emptying the queue: a push that loses the race is dropped rather than - // landing in a queue nothing will ever pop again. + // Terminate() sets the flag before it empties the queue under this + // mutex, so a push either sees it here or lands ahead of that sweep: + // never in a queue nothing will pop again. std::unique_lock mlock(this->mutex_); if (this->terminated) { return; @@ -52,7 +52,7 @@ void ConcurrentQueue::Signal() { // Terminate() clears both pointers and invalidates and releases the source; // the run loop is borrowed from the worker thread, which terminates the // queue before it leaves. - std::unique_lock lock(initializationMutex_); + std::unique_lock lock(runLoopMutex_); if (this->runLoopTasksSource_ != nullptr) { CFRunLoopSourceSignal(this->runLoopTasksSource_); } @@ -68,7 +68,7 @@ void ConcurrentQueue::Terminate() { // its sibling group's lock and posts to the sibling's loop. std::queue> dropped; { - std::unique_lock lock(initializationMutex_); + std::unique_lock lock(runLoopMutex_); terminated = true; CFRunLoopRef runLoop = this->runLoop_; CFRunLoopSourceRef source = this->runLoopTasksSource_; diff --git a/NativeScript/runtime/ConcurrentQueue.h b/NativeScript/runtime/ConcurrentQueue.h index 33c78220..8d4b7f0b 100644 --- a/NativeScript/runtime/ConcurrentQueue.h +++ b/NativeScript/runtime/ConcurrentQueue.h @@ -31,7 +31,7 @@ struct ConcurrentQueue { CFRunLoopRef runLoop_ = nullptr; std::atomic terminated{false}; std::mutex mutex_; - std::mutex initializationMutex_; + std::mutex runLoopMutex_; }; } diff --git a/NativeScript/runtime/WorkerWrapper.mm b/NativeScript/runtime/WorkerWrapper.mm index df03c7c3..5b3ba448 100644 --- a/NativeScript/runtime/WorkerWrapper.mm +++ b/NativeScript/runtime/WorkerWrapper.mm @@ -342,25 +342,26 @@ static void PostThreadEndedNotification(Isolate* mainIsolate, std::weak_ptrisTerminating_.exchange(true); if (!wasTerminating) { - // Held across the use, not just the read: the worker thread withdraws the - // isolate under the same mutex before deleting its runtime. - std::unique_lock isolateLock(this->workerIsolateMutex_); - if (this->workerIsolate_ != nullptr) { - // Flagged before the request so a pump that is between iterations sees - // it on its next check, rather than only once V8 has some JS to - // interrupt — which a parked graph never provides. - // - // NOTE: `workerIsolate_` is assigned only after the worker's ENTRY has - // finished evaluating, so a worker still parked in its entry is not - // reachable from here at all and terminate() does nothing for it. That - // is a pre-existing worker-lifecycle gap, not something this flag can - // close — see the follow-up filed for it. - if (Runtime* workerRuntime = Runtime::GetRuntime(this->workerIsolate_)) { - workerRuntime->RequestTermination(); + { + // Held across the use, not just the read: the worker thread withdraws + // the isolate under the same mutex before deleting its runtime. + std::lock_guard lock(this->workerIsolateMutex_); + if (this->workerIsolate_ != nullptr) { + // Flagged before the request so a pump that is between iterations sees + // it on its next check, rather than only once V8 has some JS to + // interrupt — which a parked graph never provides. + // + // NOTE: `workerIsolate_` is assigned only after the worker's ENTRY has + // finished evaluating, so a worker still parked in its entry is not + // reachable from here at all and terminate() does nothing for it. That + // is a pre-existing worker-lifecycle gap, not something this flag can + // close — see the follow-up filed for it. + if (Runtime* workerRuntime = Runtime::GetRuntime(this->workerIsolate_)) { + workerRuntime->RequestTermination(); + } + this->workerIsolate_->TerminateExecution(); } - this->workerIsolate_->TerminateExecution(); } - isolateLock.unlock(); { // A worker paused at a breakpoint sits in the inspector's nested pause // loop, not in the CFRunLoop — kick it loose so TerminateExecution and