| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: d5b4b866-c726-4c0c-8df7-e1dadb667a37 📥 CommitsReviewing files that changed from the base of the PR and between ae2d748 and 61ed64b. 📒 Files selected for processing (3)
📝 Walkthrough WalkthroughThe 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 Sequence Diagram(s)sequenceDiagram
participant Test as Termination test
participant Worker as Worker module
participant Runtime as Native runtime
participant ErrorHandler as Worker error handler
Test->>Worker: Start worker with delayed module initialization
Test->>Worker: Terminate worker
Runtime->>Runtime: Handle terminated execution
Runtime->>ErrorHandler: Safely inspect worker onerror
Test->>Test: Assert no error messages
Possibly related PRs
Suggested reviewers: nathanwalker Poem Poem 🚥 Pre-merge checks | ✅ 5 ✅ Passed checks (5 passed)
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. ❤️ ShareComment @coderabbitai help to get the list of available commands. |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agentsVerify 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.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: efe533d5-6e95-4c7f-ac10-7e64e7fae01d
📥 CommitsReviewing files that changed from the base of the PR and between c08a91b and ae2d748.
📒 Files selected for processing (7)
Sorry, something went wrong.
| 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 AgentsVerify 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.
Sorry, something went wrong.
| 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 AgentsVerify 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.
Sorry, something went wrong.
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.
| Back | FazBrowse Home | New Git URL |
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 on main before 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. Once TerminateExecution() is armed, every V8 entry that runs JS returns an empty MaybeLocal — and three call sites on exactly this path unwrapped without checking. The proven signature (symbolized from a device tombstone) is Fatal error in v8::ToLocalChecked / Empty MaybeLocal on a worker thread, aborting from ModuleInternal::LoadModule.
Fixes
No behavior change for healthy paths: termination reporting was already correctly suppressed (isTerminating_ is set before TerminateExecution() and guarded in BackgroundLooper/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 truncated SIGABRT 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 so terminate() deterministically lands inside the module-function call (the wide window; the script->Run() window is microseconds and can't be hit reliably), asserting no onerror fires and the process survives. It cannot fail spuriously. It lives in the app's tests/ because shared/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