From 3cf8944dd85a918608a0ef0d33d4db6aa125f25d Mon Sep 17 00:00:00 2001 From: Eduardo Speroni Date: Fri, 31 Jul 2026 01:09:01 -0300 Subject: [PATCH] fix: don't abort on worker termination during module load worker.terminate() calls Isolate::TerminateExecution() on the worker isolate from the parent thread, after which every V8 entry that runs JS hands back an empty handle. Three call sites in the worker's script-load path unwrapped those handles without checking: - ModuleInternal::LoadModule called script->Run(...).ToLocalChecked() one line before the tc.HasCaught() guard meant to handle exactly that, so a terminate landing mid-load killed the process with "Fatal error in v8::ToLocalChecked / Empty MaybeLocal". - The same function unwrapped the __extends lookup unconditionally. - CallWorkerScopeOnErrorHandle, which runs precisely when a worker script fails to load, unwrapped the global "onerror" lookup. NativeScriptException's TryCatch constructor then dereferenced tc.Message() unconditionally. A terminated TryCatch exposes no message object, so building the error to report turned the abort into a SIGSEGV, which the runtime's own signal handler converted into an opaque "JNI Exception occurred (SIGSEGV)" and no tombstone. All four now test before unwrapping, matching the sibling compile sites in LoadModule. Reporting already suppresses termination -- BackgroundLooper guards on isTerminating_ and CallWorkerScopeOnErrorHandle returns early for a terminating wrapper -- so a terminate during load unwinds as a normal shutdown. Also zero-initialises the sigaction struct used to install the SIGABRT and SIGSEGV handlers, whose sa_mask and sa_flags were stack garbage. The device suite hit this on roughly 20% of cold runs (pm clear + launch) on an arm64 emulator, always in a worker spawned by the Workers suite within the first seconds of the run; the faulting frame symbolised to ModuleInternal::LoadModule. 27 cold runs on the fixed build are clean. --- test-app/app/src/main/assets/app/mainpage.js | 1 + .../tests/testWorkerTerminateDuringLoad.js | 33 +++++++++++++++++++ .../tests/workerTerminateDuringLoadWorker.js | 5 +++ .../runtime/src/main/cpp/CallbackHandlers.cpp | 10 +++--- .../runtime/src/main/cpp/ModuleInternal.cpp | 11 +++++-- .../src/main/cpp/NativeScriptException.cpp | 13 +++++++- test-app/runtime/src/main/cpp/Runtime.cpp | 3 +- 7 files changed, 67 insertions(+), 9 deletions(-) create mode 100644 test-app/app/src/main/assets/app/tests/testWorkerTerminateDuringLoad.js create mode 100644 test-app/app/src/main/assets/app/tests/workerTerminateDuringLoadWorker.js diff --git a/test-app/app/src/main/assets/app/mainpage.js b/test-app/app/src/main/assets/app/mainpage.js index 2ff19f79d..863f6c0e4 100644 --- a/test-app/app/src/main/assets/app/mainpage.js +++ b/test-app/app/src/main/assets/app/mainpage.js @@ -20,6 +20,7 @@ shared.runRuntimeTests(); shared.runWorkerTests(); require("./tests/testWebAssembly"); require("./tests/testMultithreadedJavascript"); +require("./tests/testWorkerTerminateDuringLoad"); require("./tests/testInterfaceDefaultMethods"); require("./tests/testInterfaceStaticMethods"); require("./tests/testMetadata"); diff --git a/test-app/app/src/main/assets/app/tests/testWorkerTerminateDuringLoad.js b/test-app/app/src/main/assets/app/tests/testWorkerTerminateDuringLoad.js new file mode 100644 index 000000000..29ede32be --- /dev/null +++ b/test-app/app/src/main/assets/app/tests/testWorkerTerminateDuringLoad.js @@ -0,0 +1,33 @@ +describe("Worker terminate during module load", function () { + var ITERATIONS = 3; + // Long enough to outlast the worker's isolate setup, short enough to keep + // the spec well inside the jasmine timeout. + var TERMINATE_AFTER = 150; + var SETTLE_AFTER = 250; + + it("should not report an error or crash when terminate() interrupts the worker's module body", function (done) { + var errors = []; + + function iteration(remaining) { + if (remaining === 0) { + expect(errors).toEqual([]); + done(); + return; + } + + var worker = new Worker("./workerTerminateDuringLoadWorker.js"); + worker.onerror = function (e) { + errors.push(e.message); + }; + + setTimeout(function () { + worker.terminate(); + setTimeout(function () { + iteration(remaining - 1); + }, SETTLE_AFTER); + }, TERMINATE_AFTER); + } + + iteration(ITERATIONS); + }); +}); diff --git a/test-app/app/src/main/assets/app/tests/workerTerminateDuringLoadWorker.js b/test-app/app/src/main/assets/app/tests/workerTerminateDuringLoadWorker.js new file mode 100644 index 000000000..a6c291263 --- /dev/null +++ b/test-app/app/src/main/assets/app/tests/workerTerminateDuringLoadWorker.js @@ -0,0 +1,5 @@ +// Spins at module scope so a terminate() from the parent lands while this +// module body is still executing, which is the window the test targets. +var deadline = Date.now() + 5000; +while (Date.now() < deadline) { +} diff --git a/test-app/runtime/src/main/cpp/CallbackHandlers.cpp b/test-app/runtime/src/main/cpp/CallbackHandlers.cpp index de1d0b345..28583568b 100644 --- a/test-app/runtime/src/main/cpp/CallbackHandlers.cpp +++ b/test-app/runtime/src/main/cpp/CallbackHandlers.cpp @@ -1496,12 +1496,14 @@ void CallbackHandlers::CallWorkerScopeOnErrorHandle(Isolate *isolate, TryCatch & auto globalObject = context->Global(); // execute onerror handle if one is implemented - auto callback = globalObject->Get(context, ArgConverter::ConvertToV8String(isolate, - "onerror")).ToLocalChecked(); - auto isEmpty = callback.IsEmpty(); + Local callback; + if (!globalObject->Get(context, ArgConverter::ConvertToV8String(isolate, "onerror")) + .ToLocal(&callback)) { + return; + } auto isFunction = callback->IsFunction(); - if (!isEmpty && isFunction && !tc.Message().IsEmpty()) { + if (isFunction && !tc.Message().IsEmpty()) { auto msg = tc.Message()->Get(); Local args1[] = {msg}; diff --git a/test-app/runtime/src/main/cpp/ModuleInternal.cpp b/test-app/runtime/src/main/cpp/ModuleInternal.cpp index ef9a2faae..86b3e3a47 100644 --- a/test-app/runtime/src/main/cpp/ModuleInternal.cpp +++ b/test-app/runtime/src/main/cpp/ModuleInternal.cpp @@ -374,10 +374,11 @@ Local ModuleInternal::LoadModule(Isolate* isolate, const string& moduleP if (Util::EndsWith(modulePath, ".js")) { auto script = LoadScript(isolate, modulePath, fullRequiredModulePath); - moduleFunc = script->Run(context).ToLocalChecked().As(); - if (tc.HasCaught()) { + Local moduleFuncValue; + if (!script->Run(context).ToLocal(&moduleFuncValue) || tc.HasCaught()) { throw NativeScriptException(tc, "Error running script " + modulePath); } + moduleFunc = moduleFuncValue.As(); } else if (Util::EndsWith(modulePath, ".so")) { auto handle = dlopen(modulePath.c_str(), RTLD_LAZY); if (handle == nullptr) { @@ -425,7 +426,11 @@ Local ModuleInternal::LoadModule(Isolate* isolate, const string& moduleP auto thiz = Object::New(isolate); auto extendsName = ArgConverter::ConvertToV8String(isolate, "__extends"); - thiz->Set(context, extendsName, context->Global()->Get(context, extendsName).ToLocalChecked()); + Local extendsFunc; + if (!context->Global()->Get(context, extendsName).ToLocal(&extendsFunc) || tc.HasCaught()) { + throw NativeScriptException(tc, "Cannot read '__extends' while loading " + modulePath); + } + thiz->Set(context, extendsName, extendsFunc); moduleFunc->Call(context, thiz, sizeof(requireArgs) / sizeof(Local ), requireArgs); if (tc.HasCaught()) { diff --git a/test-app/runtime/src/main/cpp/NativeScriptException.cpp b/test-app/runtime/src/main/cpp/NativeScriptException.cpp index 2f02ba685..03ddce967 100644 --- a/test-app/runtime/src/main/cpp/NativeScriptException.cpp +++ b/test-app/runtime/src/main/cpp/NativeScriptException.cpp @@ -44,8 +44,19 @@ NativeScriptException::NativeScriptException(TryCatch& tc, const string& message) : m_javaException(JniLocalRef()) { auto isolate = Isolate::GetCurrent(); - m_javascriptException = new Persistent(isolate, tc.Exception()); auto ex = tc.Exception(); + m_javascriptException = + ex.IsEmpty() ? nullptr : new Persistent(isolate, ex); + + // A terminated isolate carries no message object and no inspectable + // exception - every accessor below hands back an empty handle. Resetting + // does not cancel the isolate's pending termination. + if (tc.HasTerminated() || tc.Message().IsEmpty()) { + m_message = message.empty() ? "Execution terminated." : message; + tc.Reset(); + return; + } + m_message = GetErrorMessage(tc.Message(), ex, message); m_stackTrace = GetErrorStackTrace(tc.Message()->GetStackTrace()); m_fullMessage = GetFullMessage(tc, m_message); diff --git a/test-app/runtime/src/main/cpp/Runtime.cpp b/test-app/runtime/src/main/cpp/Runtime.cpp index 04d106f6e..c24431731 100644 --- a/test-app/runtime/src/main/cpp/Runtime.cpp +++ b/test-app/runtime/src/main/cpp/Runtime.cpp @@ -110,8 +110,9 @@ void Runtime::Init(JavaVM* vm, void* reserved) { // handle SIGABRT/SIGSEGV only on API level > 20 as the handling is not so // efficient in older versions if (m_androidVersion > 20) { - struct sigaction action; + struct sigaction action = {}; action.sa_handler = SIG_handler; + sigemptyset(&action.sa_mask); sigaction(SIGABRT, &action, NULL); sigaction(SIGSEGV, &action, NULL); }