From a9a5ea44006dd3021fbeb7ea46aea30f50c5a087 Mon Sep 17 00:00:00 2001 From: Adrian Niculescu <15037449+adrian-niculescu@users.noreply.github.com> Date: Tue, 6 Oct 2026 23:53:48 +0300 Subject: [PATCH 1/3] 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. --- .../runtime/src/main/cpp/WorkerWrapper.cpp | 33 ++++++++++++------- test-app/runtime/src/main/cpp/WorkerWrapper.h | 4 +++ 2 files changed, 26 insertions(+), 11 deletions(-) diff --git a/test-app/runtime/src/main/cpp/WorkerWrapper.cpp b/test-app/runtime/src/main/cpp/WorkerWrapper.cpp index 04abc68fd..18cf6ecb5 100644 --- a/test-app/runtime/src/main/cpp/WorkerWrapper.cpp +++ b/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 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 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 lock(workerIsolateMutex_); + workerIsolate_.store(nullptr); + } Isolate* isolate = runtime_->GetIsolate(); { v8::Locker locker(isolate); diff --git a/test-app/runtime/src/main/cpp/WorkerWrapper.h b/test-app/runtime/src/main/cpp/WorkerWrapper.h index 5dd3b2fa8..b79941d87 100644 --- a/test-app/runtime/src/main/cpp/WorkerWrapper.h +++ b/test-app/runtime/src/main/cpp/WorkerWrapper.h @@ -178,7 +178,11 @@ class WorkerWrapper : public std::enable_shared_from_this { // 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 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 workerIsolate_; + std::mutex workerIsolateMutex_; Runtime* runtime_; const int workerId_; From 921209ff4d01a647c0c9a046dd7dc456bf7fcb35 Mon Sep 17 00:00:00 2001 From: Adrian Niculescu <15037449+adrian-niculescu@users.noreply.github.com> Date: Wed, 7 Oct 2026 00:06:39 +0300 Subject: [PATCH 2/3] fix(worker): let terminate() stop a worker that called close() close() only ends the worker once its running callback returns, and terminate() returned early for a closing worker, so a worker that kept running after close() could not be stopped. terminate() now interrupts it like any other running worker. --- test-app/app/src/main/assets/app/mainpage.js | 1 + .../tests/testWorkerTerminateAfterClose.js | 30 +++++++++++++++++++ .../app/tests/workerCloseThenSpinWorker.js | 9 ++++++ .../runtime/src/main/cpp/WorkerWrapper.cpp | 7 ++--- 4 files changed, 43 insertions(+), 4 deletions(-) create mode 100644 test-app/app/src/main/assets/app/tests/testWorkerTerminateAfterClose.js create mode 100644 test-app/app/src/main/assets/app/tests/workerCloseThenSpinWorker.js diff --git a/test-app/app/src/main/assets/app/mainpage.js b/test-app/app/src/main/assets/app/mainpage.js index 62aeb52eb..7b56d7ba8 100644 --- a/test-app/app/src/main/assets/app/mainpage.js +++ b/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"); diff --git a/test-app/app/src/main/assets/app/tests/testWorkerTerminateAfterClose.js b/test-app/app/src/main/assets/app/tests/testWorkerTerminateAfterClose.js new file mode 100644 index 000000000..800e74a92 --- /dev/null +++ b/test-app/app/src/main/assets/app/tests/testWorkerTerminateAfterClose.js @@ -0,0 +1,30 @@ +describe("Worker terminate after close", function () { + var SETTLE_AFTER = 300; + + it("stops a worker that keeps running after it called close()", function (done) { + var counter = new Int32Array(new SharedArrayBuffer(4)); + 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 > 5000) { + fail("the worker never started running"); + done(); + return; + } + setTimeout(waitForSpin, 20); + return; + } + worker.terminate(); + setTimeout(function () { + var afterTerminate = Atomics.load(counter, 0); + setTimeout(function () { + expect(Atomics.load(counter, 0)).toBe(afterTerminate); + done(); + }, SETTLE_AFTER); + }, SETTLE_AFTER); + })(); + }); +}); diff --git a/test-app/app/src/main/assets/app/tests/workerCloseThenSpinWorker.js b/test-app/app/src/main/assets/app/tests/workerCloseThenSpinWorker.js new file mode 100644 index 000000000..bd1fb23ea --- /dev/null +++ b/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. +onmessage = function (event) { + var counter = new Int32Array(event.data); + close(); + for (;;) { + Atomics.add(counter, 0, 1); + } +}; diff --git a/test-app/runtime/src/main/cpp/WorkerWrapper.cpp b/test-app/runtime/src/main/cpp/WorkerWrapper.cpp index 18cf6ecb5..e7b54799c 100644 --- a/test-app/runtime/src/main/cpp/WorkerWrapper.cpp +++ b/test-app/runtime/src/main/cpp/WorkerWrapper.cpp @@ -142,8 +142,7 @@ void WorkerWrapper::PostMessageToParent(std::shared_ptr message } void WorkerWrapper::Terminate() { - if (isClosing_ || isDisposed_) { - // The worker is already shutting down on its own; nothing to do. + if (isDisposed_) { return; } @@ -160,8 +159,8 @@ void WorkerWrapper::Terminate() { std::lock_guard 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). + // 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. From 44f3c8cf3dc9843f8dddb6d259a6484b322cd644 Mon Sep 17 00:00:00 2001 From: Adrian Niculescu <15037449+adrian-niculescu@users.noreply.github.com> Date: Wed, 7 Oct 2026 00:20:12 +0300 Subject: [PATCH 3/3] test(worker): stop the close-then-spin worker on cleanup and fit the spec in its timeout A run where terminate() failed left the worker spinning past the spec. The loop now also checks a stop flag the spec raises in afterEach, the spec gets its own Jasmine timeout with a shorter start deadline, and the start-deadline branch fails through an expectation rather than the fail() global this Jasmine does not provide. --- .../tests/testWorkerTerminateAfterClose.js | 38 ++++++++++++++++--- .../app/tests/workerCloseThenSpinWorker.js | 8 ++-- 2 files changed, 36 insertions(+), 10 deletions(-) diff --git a/test-app/app/src/main/assets/app/tests/testWorkerTerminateAfterClose.js b/test-app/app/src/main/assets/app/tests/testWorkerTerminateAfterClose.js index 800e74a92..4eff24878 100644 --- a/test-app/app/src/main/assets/app/tests/testWorkerTerminateAfterClose.js +++ b/test-app/app/src/main/assets/app/tests/testWorkerTerminateAfterClose.js @@ -1,26 +1,52 @@ 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) { - var counter = new Int32Array(new SharedArrayBuffer(4)); + 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 > 5000) { - fail("the worker never started running"); + if (Date.now() - started > START_DEADLINE) { + expect("the worker never started running").toBeNull(); done(); return; } - setTimeout(waitForSpin, 20); + later(waitForSpin, 20); return; } worker.terminate(); - setTimeout(function () { + later(function () { var afterTerminate = Atomics.load(counter, 0); - setTimeout(function () { + later(function () { expect(Atomics.load(counter, 0)).toBe(afterTerminate); done(); }, SETTLE_AFTER); diff --git a/test-app/app/src/main/assets/app/tests/workerCloseThenSpinWorker.js b/test-app/app/src/main/assets/app/tests/workerCloseThenSpinWorker.js index bd1fb23ea..00cd2905a 100644 --- a/test-app/app/src/main/assets/app/tests/workerCloseThenSpinWorker.js +++ b/test-app/app/src/main/assets/app/tests/workerCloseThenSpinWorker.js @@ -1,9 +1,9 @@ // close() lets the running callback finish, so this one never returns unless -// terminate() interrupts it. +// terminate() interrupts it, or the spec raises the stop flag to clean up. onmessage = function (event) { - var counter = new Int32Array(event.data); + var shared = new Int32Array(event.data); close(); - for (;;) { - Atomics.add(counter, 0, 1); + while (Atomics.load(shared, 1) === 0) { + Atomics.add(shared, 0, 1); } };