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