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

RunMainThreadEntry read the target isolate from its entry, released the cache lock and only then took the isolate's Locker. A worker that ended in between removed the entry and disposed the isolate, and the main thread then locked freed memory. The main thread now holds the isolate from the moment it takes the entry until it releases the Locker, and a worker waits for those holds before disposing its isolate.

Author: adrian-niculescu

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);
@@ -1932,8 +1949,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