fix: don't abort on worker termination during module load - #1993
fix: don't abort on worker termination during module load#1993edusperoni wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe runtime now checks V8 operations, handles terminated isolates and missing messages, and initializes signal-action state safely. The application adds a test that terminates workers during module loading and verifies that no error is reported. ChangesWorker termination handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
3cf8944 to
ae2d748
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test-app/app/src/main/assets/app/tests/testWorkerTerminateDuringLoad.js`:
- Around line 18-28: Synchronize the termination timer in the test’s worker
lifecycle: update workerTerminateDuringLoadWorker.js to post a “module entered”
message immediately before its busy loop, then change the parent’s Worker
handling so the TERMINATE_AFTER timeout starts only from the corresponding
message event. Keep the existing worker.onerror collection and post-termination
iteration flow unchanged.
In `@test-app/runtime/src/main/cpp/CallbackHandlers.cpp`:
- Around line 1499-1503: Update the onerror lookup in the worker exception
handling path around CallbackHandlers so a failed Get does not return before
reporting non-terminating worker exceptions; preserve the terminating-worker
early exit, and fall through to extract and propagate the original exception
captured in innerTc. Add a regression test covering a global onerror getter that
throws and verifies the worker exception is reported.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: efe533d5-6e95-4c7f-ac10-7e64e7fae01d
📒 Files selected for processing (7)
test-app/app/src/main/assets/app/mainpage.jstest-app/app/src/main/assets/app/tests/testWorkerTerminateDuringLoad.jstest-app/app/src/main/assets/app/tests/workerTerminateDuringLoadWorker.jstest-app/runtime/src/main/cpp/CallbackHandlers.cpptest-app/runtime/src/main/cpp/ModuleInternal.cpptest-app/runtime/src/main/cpp/NativeScriptException.cpptest-app/runtime/src/main/cpp/Runtime.cpp
| 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); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Find relevant test and worker files:"
git ls-files | rg 'testWorkerTerminateDuringLoad|workerTerminateDuringLoadWorker|assets/app/tests'
echo
echo "Show test file:"
cat -n test-app/app/src/main/assets/app/tests/testWorkerTerminateDuringLoad.js
echo
echo "Locate workerTerminateDuringLoadWorker.js:"
fd -a 'workerTerminateDuringLoadWorker\.js$' . | sed 's#^\./##'
echo
echo "Show worker file(s):"
while IFS= read -r f; do
echo "--- $f"
cat -n "$f"
done < <(fd 'workerTerminateDuringLoadWorker\.js$' .)
echo
echo "Search related constants/usages:"
rg -n "TERMINATE_AFTER|SETTLE_AFTER|testWorkerTerminateDuringLoad|workerTerminateDuringLoadWorker" test-app/app/src/main/assets/app/tests test-app -S || trueRepository: NativeScript/android
Length of output: 6129
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Show workerTerminateDuringLoadWorker.js:"
cat -n test-app/app/src/main/assets/app/tests/workerTerminateDuringLoadWorker.js
echo
echo "Search related references:"
rg -n "TERMINATE_AFTER|SETTLE_AFTER|message|postMessage|terminate|" test-app/app/src/main/assets/app/tests/testWorkerTerminateDuringLoad.js test-app/app/src/main/assets/app/tests/workerTerminateDuringLoadWorker.js -S || trueRepository: NativeScript/android
Length of output: 4390
Synchronize termination with module entry.
workerTerminateDuringLoadWorker.js starts the spinning loop at top level, but the parent starts the TERMINATE_AFTER timer from new Worker(). If worker startup takes more than 150 ms, terminate() can run before the worker’s module body begins, so the test may pass without exercising termination during module evaluation.
Have the worker send a “module entered” message immediately before the busy loop, and start termination only after the parent receives that message.
🧰 Tools
🪛 ast-grep (0.45.0)
[error] 22-27: React's useState should not be directly called
Context: setTimeout(function () {
worker.terminate();
setTimeout(function () {
iteration(remaining - 1);
}, SETTLE_AFTER);
}, TERMINATE_AFTER)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(usestate-direct-usage)
[error] 24-26: React's useState should not be directly called
Context: setTimeout(function () {
iteration(remaining - 1);
}, SETTLE_AFTER)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(usestate-direct-usage)
[warning] 22-27: Avoid using the initial state variable in setState
Context: setTimeout(function () {
worker.terminate();
setTimeout(function () {
iteration(remaining - 1);
}, SETTLE_AFTER);
}, TERMINATE_AFTER)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
[warning] 24-26: Avoid using the initial state variable in setState
Context: setTimeout(function () {
iteration(remaining - 1);
}, SETTLE_AFTER)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test-app/app/src/main/assets/app/tests/testWorkerTerminateDuringLoad.js`
around lines 18 - 28, Synchronize the termination timer in the test’s worker
lifecycle: update workerTerminateDuringLoadWorker.js to post a “module entered”
message immediately before its busy loop, then change the parent’s Worker
handling so the TERMINATE_AFTER timeout starts only from the corresponding
message event. Keep the existing worker.onerror collection and post-termination
iteration flow unchanged.
| Local<Value> callback; | ||
| if (!globalObject->Get(context, ArgConverter::ConvertToV8String(isolate, "onerror")) | ||
| .ToLocal(&callback)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)CallbackHandlers\.cpp$|(^|/)CallbackHandlers\.h$|test' || true
echo "== locate relevant snippet =="
rg -n "onerror|innerTc|ToLocal\\(\\)|ToLocalChecked|CallbackHandlers\\.cpp" -S . || true
echo "== inspect CallbackHandlers.cpp around snippet =="
file="$PWD/test-app/runtime/src/main/cpp/CallbackHandlers.cpp"
if [ -f "$file" ]; then
wc -l "$file"
sed -n '1450,1545p' "$file" | cat -n -v
fiRepository: NativeScript/android
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== CallbackHandlers.cpp outline and relevant sections =="
file="test-app/runtime/src/main/cpp/CallbackHandlers.cpp"
wc -l "$file"
sed -n '1400,1580p' "$file" | cat -n -v
echo "== related to worker error termination flags/constants =="
rg -n "terminate|isTerminating|OnError|ThrowError|HasCaught|innerTc|globalObject->Get|onerror" test-app/runtime/src/main/cpp/CallbackHandlers.cpp test-app/runtime/src/main/cpp -S || true
echo "== relevant tests mentioning onerror/worker error/error events =="
rg -n "onerror|workerTerminateDuringLoad|WorkerEvent|error.*worker|worker.*error|terminate" test-app/app/src/main/assets/app/test-app tests -S || trueRepository: NativeScript/android
Length of output: 25669
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== WorkerWrapper PassUncaughtExceptionFromWorkerToParent implementation =="
file="test-app/runtime/src/main/cpp/WorkerWrapper.cpp"
sed -n '250,320p' "$file" | cat -n -v
echo "== GiveWorkerOnErrorAChance implementation and usage =="
sed -n '620,760p' test-app/runtime/src/main/cpp/NativeScriptException.cpp | cat -n -v
echo "== Search for non-termination reporting path after CallWorkerScopeOnErrorHandle =="
rg -n "PassUncaughtExceptionFromWorkerToParent|CallWorkerScopeOnErrorHandle\\(.*tc\\)|CallWorkerScopeOnErrorHandle\\(.*message" test-app/runtime/src/main/cpp/CallbackHandlers.cpp test-app/runtime/src/main/cpp/WorkerWrapper.cpp test-app/runtime/src/main/cpp/NativeScriptException.cpp test-app/runtime/src/main/cpp/ModuleInternal.cpp -S || true
echo "== deterministic JS semantics for try/catch + throwing getter and early return reportability =="
node - <<'JS'
let caught;
try {
const o = {};
Object.defineProperty(o, "onerror", {
get() {
throw new Error("throwing onerror getter");
}
});
let callback;
try {
callback = o.onerror;
} catch (e) {
caught = e.message;
return false;
}
if (false === callback) {
console.log("no lookup failure was suppressed");
}
} catch (e) {
console.log("outer catch", e && e.message);
}
console.log("caught?", caught);
JSRepository: NativeScript/android
Length of output: 12625
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== WorketThreadCloseCallback context around original tc =="
sed -n '1368,1435p' test-app/runtime/src/main/cpp/CallbackHandlers.cpp | cat -n -v
echo "== WorkerObjectPostMessageCallback context around original tc =="
sed -n '1255,1305p' test-app/runtime/src/main/cpp/CallbackHandlers.cpp | cat -n -v
echo "== deterministic JS semantics for throw in getter caught by TryCatch and lookup failure suppressing later reporting =="
node - <<'JS'
let onerrorCalled = false;
let reported = false;
const o = {};
Object.defineProperty(o, "onerror", {
get() {
throw new Error("throwing onerror getter");
}
});
let tc;
try {
tc = { exception: null };
let callback;
try {
callback = o.onerror;
} catch (e) {
tc.exception = e;
}
if (!tc.exception) return;
reported = false;
} catch (e) {
return;
}
try {
{
callback = o.onerror;
}
} catch (e) {
reported = false;
}
console.log("onerrorCalled", onerrorCalled, "reported", reported, "tc.exception.message", tc.exception.message);
JSRepository: NativeScript/android
Length of output: 6099
Do not suppress worker exceptions when onerror lookup fails.
A throwing global onerror getter captures its exception in innerTc, and Line 1503 returns before the existing reporting path. Non-terminating worker failures should fall through to extract and pass the original worker exception unless the worker is terminating. Add a regression test with a throwing global onerror getter.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test-app/runtime/src/main/cpp/CallbackHandlers.cpp` around lines 1499 - 1503,
Update the onerror lookup in the worker exception handling path around
CallbackHandlers so a failed Get does not return before reporting
non-terminating worker exceptions; preserve the terminating-worker early exit,
and fall through to extract and propagate the original exception captured in
innerTc. Add a regression test covering a global onerror getter that throws and
verifies the worker exception is reported.
1 participant
Description
Targets
main(rebased after the V8 14.9 upgrade, #1987, merged); the bugs themselves are long-standing (verified — every fixed site was byte-identical onmainbefore the upgrade).Intermittent process aborts/crashes (~20% of cold full-suite runs on an emulator, always in the worker-heavy early phase) were root-caused to
worker.terminate()landing while the worker is still loading its script. OnceTerminateExecution()is armed, every V8 entry that runs JS returns an emptyMaybeLocal— and three call sites on exactly this path unwrapped without checking. The proven signature (symbolized from a device tombstone) isFatal error in v8::ToLocalChecked / Empty MaybeLocalon a worker thread, aborting fromModuleInternal::LoadModule.Fixes
ModuleInternal.cpp—script->Run()was unwrapped withToLocalChecked()one line before thetc.HasCaught()guard meant to handle it; now checked first (matching the sibling compile sites in the same function). Same treatment for the unconditional__extendsglobal lookup a few lines down.NativeScriptException.cpp— theTryCatchconstructor now bails out early ontc.HasTerminated() || tc.Message().IsEmpty()with a fallback message, and no longer builds aPersistentfrom an empty exception handle (which leftReThrowToV8dereferencing an emptyLocal).CallbackHandlers.cpp—CallWorkerScopeOnErrorHandleunwrapped the globalonerrorlookup withToLocalChecked(); it runs precisely when a worker script fails to load. NowToLocal()+ early return.Runtime.cpp—struct sigactionwas never zero-initialized, leavingsa_mask/sa_flagsas stack garbage for both signal handlers (a source of run-to-run nondeterminism in crash behavior).No behavior change for healthy paths: termination reporting was already correctly suppressed (
isTerminating_is set beforeTerminateExecution()and guarded inBackgroundLooper/CallWorkerScopeOnErrorHandle); no existing spec asserts an error for terminate-during-load.Known remaining flake (out of scope): while re-verifying the reproduction on the unfixed build, one crash of a different signature was captured — a bionic
Pointer tag ... was truncatedSIGABRT on a worker thread right after isolate creation (heap corruption, not an empty-handle abort). It was not observed in 27 cold runs of the fixed build, but this PR does not claim to fix it; it's an open follow-up.Related Pull Requests
Does your pull request have unit tests?
Yes —
tests/testWorkerTerminateDuringLoad.js: the worker spins at module scope soterminate()deterministically lands inside the module-function call (the wide window; thescript->Run()window is microseconds and can't be hit reliably), asserting noonerrorfires and the process survives. It cannot fail spuriously. It lives in the app'stests/becauseshared/Workers/is a submodule shared with iOS. Suite: 606 specs, 0 failures (605 baseline + 1).Statistical verification: unfixed build reproduces at ~20% per cold full-suite run (re-armed before fixing: 1/5); fixed build ran 27 cold full-suite runs with zero crashes and a clean dropbox/tombstone sweep.
Summary by CodeRabbit
Bug Fixes
Tests