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 b0c04abf..66bc91dc 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, @@ -623,13 +627,16 @@ 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_; } 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..2cc9213d 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,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(); @@ -292,6 +319,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 +337,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 +445,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 +477,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();