Skip to content

Commit 3cf8944

Browse files
committed
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.
1 parent 55a357a commit 3cf8944

7 files changed

Lines changed: 67 additions & 9 deletions

File tree

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ shared.runRuntimeTests();
2020
shared.runWorkerTests();
2121
require("./tests/testWebAssembly");
2222
require("./tests/testMultithreadedJavascript");
23+
require("./tests/testWorkerTerminateDuringLoad");
2324
require("./tests/testInterfaceDefaultMethods");
2425
require("./tests/testInterfaceStaticMethods");
2526
require("./tests/testMetadata");
Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
describe("Worker terminate during module load", function () {
2+
var ITERATIONS = 3;
3+
// Long enough to outlast the worker's isolate setup, short enough to keep
4+
// the spec well inside the jasmine timeout.
5+
var TERMINATE_AFTER = 150;
6+
var SETTLE_AFTER = 250;
7+
8+
it("should not report an error or crash when terminate() interrupts the worker's module body", function (done) {
9+
var errors = [];
10+
11+
function iteration(remaining) {
12+
if (remaining === 0) {
13+
expect(errors).toEqual([]);
14+
done();
15+
return;
16+
}
17+
18+
var worker = new Worker("./workerTerminateDuringLoadWorker.js");
19+
worker.onerror = function (e) {
20+
errors.push(e.message);
21+
};
22+
23+
setTimeout(function () {
24+
worker.terminate();
25+
setTimeout(function () {
26+
iteration(remaining - 1);
27+
}, SETTLE_AFTER);
28+
}, TERMINATE_AFTER);
29+
}
30+
31+
iteration(ITERATIONS);
32+
});
33+
});
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
// Spins at module scope so a terminate() from the parent lands while this
2+
// module body is still executing, which is the window the test targets.
3+
var deadline = Date.now() + 5000;
4+
while (Date.now() < deadline) {
5+
}

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

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1496,12 +1496,14 @@ void CallbackHandlers::CallWorkerScopeOnErrorHandle(Isolate *isolate, TryCatch &
14961496
auto globalObject = context->Global();
14971497

14981498
// execute onerror handle if one is implemented
1499-
auto callback = globalObject->Get(context, ArgConverter::ConvertToV8String(isolate,
1500-
"onerror")).ToLocalChecked();
1501-
auto isEmpty = callback.IsEmpty();
1499+
Local<Value> callback;
1500+
if (!globalObject->Get(context, ArgConverter::ConvertToV8String(isolate, "onerror"))
1501+
.ToLocal(&callback)) {
1502+
return;
1503+
}
15021504
auto isFunction = callback->IsFunction();
15031505

1504-
if (!isEmpty && isFunction && !tc.Message().IsEmpty()) {
1506+
if (isFunction && !tc.Message().IsEmpty()) {
15051507
auto msg = tc.Message()->Get();
15061508
Local<Value> args1[] = {msg};
15071509

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

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -374,10 +374,11 @@ Local<Object> ModuleInternal::LoadModule(Isolate* isolate, const string& moduleP
374374
if (Util::EndsWith(modulePath, ".js")) {
375375
auto script = LoadScript(isolate, modulePath, fullRequiredModulePath);
376376

377-
moduleFunc = script->Run(context).ToLocalChecked().As<Function>();
378-
if (tc.HasCaught()) {
377+
Local<Value> moduleFuncValue;
378+
if (!script->Run(context).ToLocal(&moduleFuncValue) || tc.HasCaught()) {
379379
throw NativeScriptException(tc, "Error running script " + modulePath);
380380
}
381+
moduleFunc = moduleFuncValue.As<Function>();
381382
} else if (Util::EndsWith(modulePath, ".so")) {
382383
auto handle = dlopen(modulePath.c_str(), RTLD_LAZY);
383384
if (handle == nullptr) {
@@ -425,7 +426,11 @@ Local<Object> ModuleInternal::LoadModule(Isolate* isolate, const string& moduleP
425426

426427
auto thiz = Object::New(isolate);
427428
auto extendsName = ArgConverter::ConvertToV8String(isolate, "__extends");
428-
thiz->Set(context, extendsName, context->Global()->Get(context, extendsName).ToLocalChecked());
429+
Local<Value> extendsFunc;
430+
if (!context->Global()->Get(context, extendsName).ToLocal(&extendsFunc) || tc.HasCaught()) {
431+
throw NativeScriptException(tc, "Cannot read '__extends' while loading " + modulePath);
432+
}
433+
thiz->Set(context, extendsName, extendsFunc);
429434
moduleFunc->Call(context, thiz, sizeof(requireArgs) / sizeof(Local<Value> ), requireArgs);
430435

431436
if (tc.HasCaught()) {

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

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,19 @@ NativeScriptException::NativeScriptException(TryCatch& tc,
4444
const string& message)
4545
: m_javaException(JniLocalRef()) {
4646
auto isolate = Isolate::GetCurrent();
47-
m_javascriptException = new Persistent<Value>(isolate, tc.Exception());
4847
auto ex = tc.Exception();
48+
m_javascriptException =
49+
ex.IsEmpty() ? nullptr : new Persistent<Value>(isolate, ex);
50+
51+
// A terminated isolate carries no message object and no inspectable
52+
// exception - every accessor below hands back an empty handle. Resetting
53+
// does not cancel the isolate's pending termination.
54+
if (tc.HasTerminated() || tc.Message().IsEmpty()) {
55+
m_message = message.empty() ? "Execution terminated." : message;
56+
tc.Reset();
57+
return;
58+
}
59+
4960
m_message = GetErrorMessage(tc.Message(), ex, message);
5061
m_stackTrace = GetErrorStackTrace(tc.Message()->GetStackTrace());
5162
m_fullMessage = GetFullMessage(tc, m_message);

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -110,8 +110,9 @@ void Runtime::Init(JavaVM* vm, void* reserved) {
110110
// handle SIGABRT/SIGSEGV only on API level > 20 as the handling is not so
111111
// efficient in older versions
112112
if (m_androidVersion > 20) {
113-
struct sigaction action;
113+
struct sigaction action = {};
114114
action.sa_handler = SIG_handler;
115+
sigemptyset(&action.sa_mask);
115116
sigaction(SIGABRT, &action, NULL);
116117
sigaction(SIGSEGV, &action, NULL);
117118
}

0 commit comments

Comments
 (0)