Skip to content

fix: don't abort on worker termination during module load #1993

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
edusperoni wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: don't abort on worker termination during module load #1993

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions test-app/app/src/main/assets/app/mainpage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down
33 changes: 33 additions & 0 deletions test-app/app/src/main/assets/app/tests/testWorkerTerminateDuringLoad.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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);
Comment on lines +18 to +28

@coderabbitai coderabbitai Bot Aug 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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 || true

Repository: 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 || true

Repository: 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.

}

iteration(ITERATIONS);
});
});
5 changes: 5 additions & 0 deletions test-app/app/src/main/assets/app/tests/workerTerminateDuringLoadWorker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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) {
}
10 changes: 6 additions & 4 deletions test-app/runtime/src/main/cpp/CallbackHandlers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Value> callback;
if (!globalObject->Get(context, ArgConverter::ConvertToV8String(isolate, "onerror"))
.ToLocal(&callback)) {
return;
}
Comment on lines +1499 to +1503

@coderabbitai coderabbitai Bot Aug 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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
fi

Repository: 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 || true

Repository: 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);
JS

Repository: 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);
JS

Repository: 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.

auto isFunction = callback->IsFunction();

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

Expand Down
11 changes: 8 additions & 3 deletions test-app/runtime/src/main/cpp/ModuleInternal.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,10 +374,11 @@ Local<Object> ModuleInternal::LoadModule(Isolate* isolate, const string& moduleP
if (Util::EndsWith(modulePath, ".js")) {
auto script = LoadScript(isolate, modulePath, fullRequiredModulePath);

moduleFunc = script->Run(context).ToLocalChecked().As<Function>();
if (tc.HasCaught()) {
Local<Value> moduleFuncValue;
if (!script->Run(context).ToLocal(&moduleFuncValue) || tc.HasCaught()) {
throw NativeScriptException(tc, "Error running script " + modulePath);
}
moduleFunc = moduleFuncValue.As<Function>();
} else if (Util::EndsWith(modulePath, ".so")) {
auto handle = dlopen(modulePath.c_str(), RTLD_LAZY);
if (handle == nullptr) {
Expand Down Expand Up @@ -425,7 +426,11 @@ Local<Object> 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<Value> 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<Value> ), requireArgs);

if (tc.HasCaught()) {
Expand Down
13 changes: 12 additions & 1 deletion test-app/runtime/src/main/cpp/NativeScriptException.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,19 @@ NativeScriptException::NativeScriptException(TryCatch& tc,
const string& message)
: m_javaException(JniLocalRef()) {
auto isolate = Isolate::GetCurrent();
m_javascriptException = new Persistent<Value>(isolate, tc.Exception());
auto ex = tc.Exception();
m_javascriptException =
ex.IsEmpty() ? nullptr : new Persistent<Value>(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);
Expand Down
3 changes: 2 additions & 1 deletion test-app/runtime/src/main/cpp/Runtime.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down