From 72e46c6d047c4d3ce4e761ec8922a77813e36327 Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Wed, 11 Nov 2026 19:13:01 -0300 Subject: [PATCH] fix(worker): drop __runOnMainThread callbacks from a closing worker The main thread read a worker's isolate from the callback cache, released the cache lock, then took a Locker on it. A worker shutting down in that window disposed the isolate first, and the main thread crashed in v8::Locker::Initialize. Each worker now has a gate the main thread holds from checking it until it owns the worker's Locker. Shutdown closes the gate before taking its teardown Locker, so a callback that arrives after that is dropped without touching the isolate, and the worker never waits on the main thread. A worker callback that throws under uncaughtErrorPolicy "throw" is now reported through the worker's own error path (its onerror, then the parent's Worker object) instead of being rethrown on the main thread, where it became a Java exception that resolved a handle from the worker isolate against the main one and crashed. --- .../runtime/src/main/cpp/CallbackHandlers.cpp | 31 ++++++++++++++++++- .../runtime/src/main/cpp/CallbackHandlers.h | 13 ++++++-- test-app/runtime/src/main/cpp/Runtime.cpp | 4 +-- .../runtime/src/main/cpp/WorkerWrapper.cpp | 6 ++++ test-app/runtime/src/main/cpp/WorkerWrapper.h | 17 ++++++++++ 5 files changed, 65 insertions(+), 6 deletions(-) diff --git a/test-app/runtime/src/main/cpp/CallbackHandlers.cpp b/test-app/runtime/src/main/cpp/CallbackHandlers.cpp index c4d1e7a2f..f6a903df6 100644 --- a/test-app/runtime/src/main/cpp/CallbackHandlers.cpp +++ b/test-app/runtime/src/main/cpp/CallbackHandlers.cpp @@ -741,10 +741,16 @@ void CallbackHandlers::RunOnMainThreadCallback(const FunctionCallbackInfo gate; + if (auto wrapper = WorkerWrapper::FromIsolate(isolate)) { + gate = wrapper->Gate(); + } + { std::lock_guard lock(cacheMutex_); bool inserted; - std::tie(std::ignore, inserted) = cache_.try_emplace(key, isolate, callback); + std::tie(std::ignore, inserted) = cache_.try_emplace(key, isolate, callback, + std::move(gate)); NS_DCHECK(inserted && "Main thread callback ID should not be duplicated"); } @@ -756,6 +762,7 @@ void CallbackHandlers::RunOnMainThreadCallback(const FunctionCallbackInfo gate; { std::lock_guard lock(cacheMutex_); auto it = cache_.find(key); @@ -763,9 +770,23 @@ void CallbackHandlers::RunMainThreadEntry(uint64_t key) { return; } isolate = it->second.isolate_; + gate = it->second.gate_; + } + + // A worker that has started shutting down may dispose its isolate at any + // moment, so its callback is dropped without touching the isolate. + std::unique_lock gateLock; + if (gate != nullptr) { + gateLock = std::unique_lock(gate->mutex); + if (!gate->open) { + return; + } } v8::Locker locker(isolate); + if (gateLock.owns_lock()) { + gateLock.unlock(); + } Isolate::Scope isolate_scope(isolate); HandleScope handle_scope(isolate); Local cb; @@ -788,6 +809,14 @@ void CallbackHandlers::RunMainThreadEntry(uint64_t key) { if (tc.HasCaught() && !NativeScriptException::ContainUncaughtCallbackException(isolate, tc)) { + if (gate != nullptr) { + // A worker's error belongs to the worker, as if the callback had + // run on its own thread: its onerror, then the parent's Worker + // object. Thrown from here it would become a Java exception on the + // main thread. + CallWorkerScopeOnErrorHandle(isolate, tc); + return; + } // surfaces via the event loop's guard as a pending Java exception throw NativeScriptException(tc); } diff --git a/test-app/runtime/src/main/cpp/CallbackHandlers.h b/test-app/runtime/src/main/cpp/CallbackHandlers.h index 112940060..43f040dd9 100644 --- a/test-app/runtime/src/main/cpp/CallbackHandlers.h +++ b/test-app/runtime/src/main/cpp/CallbackHandlers.h @@ -20,6 +20,11 @@ #include "NativeScriptAssert.h" #include "NativeScriptException.h" #include "Runtime.h" +#include + +namespace tns { + struct IsolateGate; +} namespace tns { class CallbackHandlers { @@ -218,9 +223,11 @@ namespace tns { struct CacheEntry { - CacheEntry(v8::Isolate* isolate, v8::Local callback) + CacheEntry(v8::Isolate* isolate, v8::Local callback, + std::shared_ptr gate) : isolate_(isolate), - callback_(isolate, callback) { + callback_(isolate, callback), + gate_(std::move(gate)) { } ~CacheEntry() { @@ -229,6 +236,8 @@ namespace tns { v8::Isolate* isolate_; v8::Global callback_; + // null for the main isolate, which is never disposed + std::shared_ptr gate_; }; static robin_hood::unordered_map cache_; diff --git a/test-app/runtime/src/main/cpp/Runtime.cpp b/test-app/runtime/src/main/cpp/Runtime.cpp index c7d36ae87..5aa20ac59 100644 --- a/test-app/runtime/src/main/cpp/Runtime.cpp +++ b/test-app/runtime/src/main/cpp/Runtime.cpp @@ -1338,9 +1338,7 @@ void Runtime::DestroyRuntime() { m_dispatchNativeUncaughtErrorFunc.Reset(); // Both hold v8::Global handles to JS callbacks, so their entries must be // dropped here rather than in ~Runtime, which runs after Isolate::Dispose -- - // resetting a Global then writes into a freed handle table. Doing it here - // also closes a window in which the main thread could take a Locker on this - // isolate (RunMainThreadEntry) after it had already been disposed. + // resetting a Global then writes into a freed handle table. CallbackHandlers::RemoveIsolateEntries(m_isolate); FrameCallbacks::RemoveIsolateEntries(m_isolate); diff --git a/test-app/runtime/src/main/cpp/WorkerWrapper.cpp b/test-app/runtime/src/main/cpp/WorkerWrapper.cpp index 6b257896a..2783cda86 100644 --- a/test-app/runtime/src/main/cpp/WorkerWrapper.cpp +++ b/test-app/runtime/src/main/cpp/WorkerWrapper.cpp @@ -718,6 +718,12 @@ void WorkerWrapper::BackgroundLooper(std::shared_ptr self) { } } + // No Locker is held here, as the gate requires. + { + std::lock_guard lock(gate_->mutex); + gate_->open = false; + } + // Terminate any workers this worker created (nested workers). Their // Worker object persistents live in this isolate, so they must be // released before the isolate is disposed below. Each child cascades to diff --git a/test-app/runtime/src/main/cpp/WorkerWrapper.h b/test-app/runtime/src/main/cpp/WorkerWrapper.h index 5d8784bab..732ff6af3 100644 --- a/test-app/runtime/src/main/cpp/WorkerWrapper.h +++ b/test-app/runtime/src/main/cpp/WorkerWrapper.h @@ -29,6 +29,21 @@ class Runtime; class WorkerInspectorClient; #endif +/* + * Lets a thread other than the worker's own take the worker's Locker without + * racing its disposal. A caller holds `mutex`, gives up if `open` is false, + * and releases `mutex` only once it owns the Locker. The worker's shutdown + * closes the gate before it takes its teardown Locker, so a closed gate means + * the isolate may already be gone. + * + * Never take `mutex` while holding the worker's Locker: a gated caller waits + * for that Locker with `mutex` held. + */ +struct IsolateGate { + std::mutex mutex; + bool open = true; +}; + /* * Owns a worker's native thread and its lifecycle, mirroring the iOS * runtime's WorkerWrapper. The thread is a std::thread that attaches to the @@ -53,6 +68,7 @@ class WorkerWrapper : public std::enable_shared_from_this { bool IsTerminating() const { return isTerminating_; } bool IsClosing() const { return isClosing_; } bool IsDisposed() const { return isDisposed_; } + const std::shared_ptr& Gate() const { return gate_; } /* * Spawns the (detached) worker thread. The thread holds a shared_ptr to @@ -224,6 +240,7 @@ class WorkerWrapper : public std::enable_shared_from_this { std::atomic_bool isClosing_; std::atomic_bool isTerminating_; std::atomic_bool isDisposed_; + const std::shared_ptr gate_ = std::make_shared(); // False until the entry script has finished evaluating // (EnableMessageQueue); DrainPendingTasks leaves the queue untouched while // disabled. Written and read on the worker thread only - the atomic is