diff --git a/NativeScript/runtime/ConcurrentQueue.cpp b/NativeScript/runtime/ConcurrentQueue.cpp index 6e34073f..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); @@ -15,14 +15,10 @@ 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 - // 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; @@ -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(runLoopMutex_); + 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_); @@ -78,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 b8fe1694..8d4b7f0b 100644 --- a/NativeScript/runtime/ConcurrentQueue.h +++ b/NativeScript/runtime/ConcurrentQueue.h @@ -31,8 +31,7 @@ struct ConcurrentQueue { CFRunLoopRef runLoop_ = nullptr; std::atomic terminated{false}; std::mutex mutex_; - std::mutex initializationMutex_; - void SignalAndWakeUp(); + std::mutex runLoopMutex_; }; } diff --git a/NativeScript/runtime/DataWrapper.h b/NativeScript/runtime/DataWrapper.h index 792cddb8..c094b27e 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, @@ -622,7 +626,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_; } // Deletion is decided by who still holds the wrapper. The parent's Worker // object holds it from construction; Start() adds the worker thread. Each @@ -647,7 +650,11 @@ class WorkerWrapper : public BaseDataWrapper { enum class Holders : uint8_t { Parent, Both, WorkerThread }; 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 02234c18..5b3ba448 100644 --- a/NativeScript/runtime/WorkerWrapper.mm +++ b/NativeScript/runtime/WorkerWrapper.mm @@ -122,17 +122,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(); } @@ -183,7 +192,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); } } @@ -234,7 +243,11 @@ static void PostThreadEndedNotification(Isolate* mainIsolate, std::weak_ptrworkerIsolate_ = func(); + Isolate* workerIsolate = func(); + { + std::lock_guard lock(this->workerIsolateMutex_); + this->workerIsolate_ = workerIsolate; + } this->DrainPendingTasks(); @@ -244,6 +257,20 @@ 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. + { + std::lock_guard lock(this->workerIsolateMutex_); + this->workerIsolate_ = nullptr; + } + // The inspector must be gone before the Runtime (and with it the isolate) // is deleted below. this->DestroyInspector(); @@ -315,20 +342,25 @@ static void PostThreadEndedNotification(Isolate* mainIsolate, std::weak_ptrisTerminating_.exchange(true); if (!wasTerminating) { - 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(); } { // A worker paused at a breakpoint sits in the inspector's nested pause @@ -437,11 +469,10 @@ static void PostThreadEndedNotification(Isolate* mainIsolate, std::weak_ptrisTerminating_) { return; } - Isolate* isolate = this->workerIsolate_; Local context = Caches::Get(isolate)->GetContext(); Local global = context->Global(); @@ -470,11 +501,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();