fix(worker): keep the worker isolate alive while terminate() uses it

Terminate() read the worker isolate and then interrupted it with no lock, while the worker thread could clear the pointer and dispose that isolate in the same window: a worker that ends through its own close() while its parent calls terminate(), or a parent that terminates its children during its own shutdown. Terminate() now holds a mutex across the read and the use, and the worker thread takes it when it withdraws the isolate, before disposing it.

Author: adrian-niculescu

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

@@ -152,16 +152,23 @@ void WorkerWrapper::Terminate() {
         return;
     }
 
-    Isolate* isolate = workerIsolate_.load();
-    if (isolate != nullptr) {
-        // The only v8 call that is legal from another thread - interrupts any
-        // JS currently running on the worker (e.g. a busy loop).
-        isolate->TerminateExecution();
-        // A pump parked with nothing queued runs no JS, so the interrupt
-        // above never materializes for it - the loop's own flag ends it.
-        auto loop = NativeScriptPlatform::Instance()->LookupEventLoop(isolate);
-        if (loop != nullptr) {
-            loop->NoteTerminationRequested();
+    {
+        // Held across the use, not just the read: the worker thread withdraws
+        // the isolate under the same mutex before disposing it, so a terminate
+        // that already read it finishes with it first, and a later one finds
+        // null.
+        std::lock_guard<std::mutex> lock(workerIsolateMutex_);
+        Isolate* isolate = workerIsolate_.load();
+        if (isolate != nullptr) {
+            // The only v8 call that is legal from another thread - interrupts any
+            // JS currently running on the worker (e.g. a busy loop).
+            isolate->TerminateExecution();
+            // A pump parked with nothing queued runs no JS, so the interrupt
+            // above never materializes for it - the loop's own flag ends it.
+            auto loop = NativeScriptPlatform::Instance()->LookupEventLoop(isolate);
+            if (loop != nullptr) {
+                loop->NoteTerminationRequested();
+            }
         }
     }
 
@@ -656,7 +663,11 @@ void WorkerWrapper::BackgroundLooper(std::shared_ptr<WorkerWrapper> self) {
         // bootstrap failed between initWorkerRuntime and the workerIsolate_
         // publish (e.g. a JNI error while resolving the looper), the atomic is
         // still null while the isolate very much needs disposing.
-        workerIsolate_.store(nullptr);
+        {
+            // Waits out a Terminate() on another thread that is still using it.
+            std::lock_guard<std::mutex> lock(workerIsolateMutex_);
+            workerIsolate_.store(nullptr);
+        }
         Isolate* isolate = runtime_->GetIsolate();
         {
             v8::Locker locker(isolate);

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

@@ -178,7 +178,11 @@ class WorkerWrapper : public std::enable_shared_from_this<WorkerWrapper> {
     // The parent runtime's task queue; weak so a child outliving its parent
     // just drops its posts instead of touching a dead runtime.
     std::weak_ptr<EventLoop> parentTasks_;
+    // Written by the worker thread only: published once the runtime is up,
+    // withdrawn before the isolate is disposed. Any other thread reads and uses
+    // it under workerIsolateMutex_.
     std::atomic<v8::Isolate*> workerIsolate_;
+    std::mutex workerIsolateMutex_;
     Runtime* runtime_;
 
     const int workerId_;