fix(worker): keep a worker isolate alive while a __runOnMainThread callback locks it

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<std::mutex> 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<std::mutex> lock(cacheMutex_);
+    isolateReleased_.wait(lock, [isolate]() {
+        return heldIsolates_.find(isolate) == heldIsolates_.end();
+    });
+}
 robin_hood::unordered_map<uint64_t, CallbackHandlers::CacheEntry> CallbackHandlers::cache_;
 std::mutex CallbackHandlers::cacheMutex_;
+robin_hood::unordered_map<v8::Isolate *, int> CallbackHandlers::heldIsolates_;
+std::condition_variable CallbackHandlers::isolateReleased_;
 
 
 std::atomic_int64_t CallbackHandlers::count_ = {0};

test-app/runtime/src/main/cpp/CallbackHandlers.h

@@ -3,6 +3,7 @@
 
 #include <string>
 #include <map>
+#include <condition_variable>
 #include <mutex>
 #include <vector>
 #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<v8::Isolate *, int> heldIsolates_;
+        static std::condition_variable isolateReleased_;
 
 
     };

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);
 

test-app/runtime/src/main/cpp/WorkerWrapper.cpp

@@ -667,6 +667,7 @@ void WorkerWrapper::BackgroundLooper(std::shared_ptr<WorkerWrapper> 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