fix(worker): keep the worker isolate alive while terminate() uses it, and let it stop a closing worker

test-app/app/src/main/assets/app/mainpage.js

@@ -27,6 +27,7 @@ require("./tests/testWebAssembly");
 require("./tests/testEventLoop");
 require("./tests/testMultithreadedJavascript");
 require("./tests/testWorkerTerminateDuringLoad");
+require("./tests/testWorkerTerminateAfterClose");
 require("./tests/testWorkerOptions");
 require("./tests/testWorkerResourceLimits");
 require("./tests/testInterfaceDefaultMethods");

test-app/app/src/main/assets/app/tests/testWorkerTerminateAfterClose.js

@@ -0,0 +1,56 @@
+describe("Worker terminate after close", function () {
+	var START_DEADLINE = 3000;
+	var SETTLE_AFTER = 300;
+	// [0] counts the worker's loop iterations; [1] stops the loop, so a worker
+	// that terminate() failed to stop does not outlive the spec.
+	var shared;
+	var timers = [];
+	var originalTimeout;
+
+	beforeEach(function () {
+		originalTimeout = jasmine.DEFAULT_TIMEOUT_INTERVAL;
+		jasmine.DEFAULT_TIMEOUT_INTERVAL = 10000;
+	});
+
+	afterEach(function () {
+		timers.forEach(clearTimeout);
+		timers = [];
+		if (shared) {
+			Atomics.store(shared, 1, 1);
+			shared = null;
+		}
+		jasmine.DEFAULT_TIMEOUT_INTERVAL = originalTimeout;
+	});
+
+	function later(fn, ms) {
+		timers.push(setTimeout(fn, ms));
+	}
+
+	it("stops a worker that keeps running after it called close()", function (done) {
+		shared = new Int32Array(new SharedArrayBuffer(8));
+		var counter = shared;
+		var worker = new Worker("./workerCloseThenSpinWorker.js");
+		worker.postMessage(counter.buffer);
+		var started = Date.now();
+
+		(function waitForSpin() {
+			if (Atomics.load(counter, 0) === 0) {
+				if (Date.now() - started > START_DEADLINE) {
+					expect("the worker never started running").toBeNull();
+					done();
+					return;
+				}
+				later(waitForSpin, 20);
+				return;
+			}
+			worker.terminate();
+			later(function () {
+				var afterTerminate = Atomics.load(counter, 0);
+				later(function () {
+					expect(Atomics.load(counter, 0)).toBe(afterTerminate);
+					done();
+				}, SETTLE_AFTER);
+			}, SETTLE_AFTER);
+		})();
+	});
+});

test-app/app/src/main/assets/app/tests/workerCloseThenSpinWorker.js

@@ -0,0 +1,9 @@
+// close() lets the running callback finish, so this one never returns unless
+// terminate() interrupts it, or the spec raises the stop flag to clean up.
+onmessage = function (event) {
+	var shared = new Int32Array(event.data);
+	close();
+	while (Atomics.load(shared, 1) === 0) {
+		Atomics.add(shared, 0, 1);
+	}
+};

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

@@ -142,8 +142,7 @@ void WorkerWrapper::PostMessageToParent(std::shared_ptr<worker::Message> message
 }
 
 void WorkerWrapper::Terminate() {
-    if (isClosing_ || isDisposed_) {
-        // The worker is already shutting down on its own; nothing to do.
+    if (isDisposed_) {
         return;
     }
 
@@ -152,16 +151,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) {
+            // Legal from any thread: interrupts any JS currently running on the
+            // worker (e.g. a busy loop, including one that follows close()).
+            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 +662,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_;