diff --git a/test-app/runtime/src/main/cpp/CallbackHandlers.cpp b/test-app/runtime/src/main/cpp/CallbackHandlers.cpp index 8d2b18135..1e3da91ce 100644 --- a/test-app/runtime/src/main/cpp/CallbackHandlers.cpp +++ b/test-app/runtime/src/main/cpp/CallbackHandlers.cpp @@ -730,7 +730,24 @@ void CallbackHandlers::RunMainThreadEntry(uint64_t key) { return; } isolate = it->second.isolate_; - } + // Taken with the entry, under the same lock RemoveIsolateEntries + // takes, so a worker either removes the entry first or waits for this + // hold before disposing the isolate. + ++heldIsolates_[isolate]; + } + // Declared before the Locker so that it lets go only after the Locker is + // released. + struct IsolateHold { + Isolate *isolate; + ~IsolateHold() { + std::lock_guard lock(cacheMutex_); + auto held = heldIsolates_.find(isolate); + if (--held->second == 0) { + heldIsolates_.erase(held); + isolateReleased_.notify_all(); + } + } + } hold{isolate}; v8::Locker locker(isolate); Isolate::Scope isolate_scope(isolate); @@ -755,8 +772,15 @@ void CallbackHandlers::RunMainThreadEntry(uint64_t key) { if (tc.HasCaught() && !NativeScriptException::ContainUncaughtCallbackException(isolate, tc)) { + NativeScriptException ex(tc); + if (!runtime->IsMainThread()) { + // Reported only after this function lets go of the isolate, which + // its worker may dispose by then, and from the main thread, which + // never enters it: only the message and stack can travel. + ex.ReleaseJsHandle(); + } // surfaces via the event loop's guard as a pending Java exception - throw NativeScriptException(tc); + throw ex; } } @@ -1932,8 +1956,17 @@ void CallbackHandlers::RemoveIsolateEntries(v8::Isolate *isolate) { } } } + +void CallbackHandlers::WaitForMainThreadCallbacks(v8::Isolate *isolate) { + std::unique_lock lock(cacheMutex_); + isolateReleased_.wait(lock, [isolate]() { + return heldIsolates_.find(isolate) == heldIsolates_.end(); + }); +} robin_hood::unordered_map CallbackHandlers::cache_; std::mutex CallbackHandlers::cacheMutex_; +robin_hood::unordered_map CallbackHandlers::heldIsolates_; +std::condition_variable CallbackHandlers::isolateReleased_; std::atomic_int64_t CallbackHandlers::count_ = {0}; diff --git a/test-app/runtime/src/main/cpp/CallbackHandlers.h b/test-app/runtime/src/main/cpp/CallbackHandlers.h index 86dba64a5..56c1a437a 100644 --- a/test-app/runtime/src/main/cpp/CallbackHandlers.h +++ b/test-app/runtime/src/main/cpp/CallbackHandlers.h @@ -3,6 +3,7 @@ #include #include +#include #include #include #include "JEnv.h" @@ -163,6 +164,14 @@ namespace tns { static void RemoveIsolateEntries(v8::Isolate *isolate); + /* + * Blocks until no __runOnMainThread callback still holds `isolate`. + * A worker calls it after releasing its Locker and before disposing + * the isolate: a callback that took the isolate before + * RemoveIsolateEntries ran may be waiting on that Locker. + */ + static void WaitForMainThreadCallbacks(v8::Isolate *isolate); + private: CallbackHandlers() { @@ -248,6 +257,10 @@ namespace tns { // thread (multithreaded JS, workers), each under a different // isolate's Locker, so the Lockers provide no mutual exclusion static std::mutex cacheMutex_; + // How many RunMainThreadEntry calls hold each isolate, from reading + // its entry until they release its Locker; guarded by cacheMutex_ + static robin_hood::unordered_map heldIsolates_; + static std::condition_variable isolateReleased_; }; diff --git a/test-app/runtime/src/main/cpp/Runtime.cpp b/test-app/runtime/src/main/cpp/Runtime.cpp index d7ab015e8..c1e08e211 100644 --- a/test-app/runtime/src/main/cpp/Runtime.cpp +++ b/test-app/runtime/src/main/cpp/Runtime.cpp @@ -1175,9 +1175,10 @@ 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. A + // RunMainThreadEntry that has not taken its entry yet finds it gone; one that + // already has is waited for before the isolate is disposed + // (CallbackHandlers::WaitForMainThreadCallbacks). 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 04abc68fd..b513e084d 100644 --- a/test-app/runtime/src/main/cpp/WorkerWrapper.cpp +++ b/test-app/runtime/src/main/cpp/WorkerWrapper.cpp @@ -667,6 +667,7 @@ void WorkerWrapper::BackgroundLooper(std::shared_ptr self) { isolate->RemoveNearHeapLimitCallback(WorkerWrapper::OnNearHeapLimit, 0); runtime_->DestroyRuntime(); } + CallbackHandlers::WaitForMainThreadCallbacks(isolate); isolate->Dispose(); // Dispose freed the isolate's memory, so its address can be reused by // a concurrent Isolate::New - drop the platform's loop entry now, not