You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(worker): drop callbacks from closing workers - #2108
Supersedes #2068 with a different approach to the same two crashes.
Race on worker shutdown.RunMainThreadEntry read a worker's isolate from the callback cache, released cacheMutex_, and only then took a v8::Locker on it. A worker shutting down in that window (DestroyRuntime → Dispose) freed the isolate first, and the main thread crashed in v8::Locker::Initialize.
#2068 fixed this by making the worker wait for in-flight main-thread holds before disposing. That keeps the isolate alive for a caller that will then only find its entry gone: the main thread still locks and enters a worker that was told to terminate, and teardown waits on main-thread progress. This PR drops the call instead:
Each worker owns an IsolateGate (std::mutex + open), held by shared_ptr in the cache entry so it outlives the isolate.
The main thread takes the gate's mutex, returns if the gate is closed, and releases it only once it owns the worker's Locker.
Worker shutdown closes the gate before it takes its teardown Locker, while holding no Locker. Once closed, the main thread never touches the isolate. The worker never waits for the main thread beyond the Locker contention that already existed.
The gate is per worker and never taken while holding that worker's Locker, so there is no lock-order cycle. A global mutex would deadlock against a worker that calls __runOnMainThread while holding its own Locker.
Main-isolate entries (multithreaded JS) carry no gate and are unchanged.
Throwing worker callbacks. Under uncaughtErrorPolicy: "throw", a worker callback's uncaught error was rethrown on the main thread as a Java exception. That exception resolved a handle from the worker isolate against the main one and crashed (SIGSEGV in NativeScriptException::ReThrowToJava). Worse, it would have taken the app down for an error that belongs to the worker. It now goes through the worker's own error path (CallWorkerScopeOnErrorHandle: the worker's onerror, then the parent's Worker object), as the worker's message dispatch already does. Under the default report policy nothing changes.
Does your commit message include the wording below to reference a specific issue in this repo?
No new spec. The race window is a few instructions wide, and the throw policy can't run in the device suite because it crashes the app on purpose (see testUncaughtErrorPolicy.js). Verified manually with a focused repro on an arm64 API 35 emulator, with usleep(200000) injected in RunMainThreadEntry right after the entry lookup:
Repro
main
this PR
Worker posts __runOnMainThread(cb), busy-waits 50 ms, then close() (x5)
The full device suite passes (runtestsAndVerifyResults: 1545/1545, on top of #2067, #2069 and #2070).
Summary by CodeRabbit
Bug Fixes
Worker callbacks are now safely discarded when a worker is shutting down, preventing them from running against a disposed runtime.
Uncaught exceptions from worker callbacks now follow the worker’s error-handling path instead of being raised as Java exceptions on the main thread. Callbacks from the main runtime retain their existing exception behavior.
No actionable comments were generated in the recent review.
Walkthrough
Worker callback entries now retain a shared isolate gate. Shutdown closes the gate, and main-thread callback execution checks it before accessing the isolate. Uncaught worker callback exceptions now use the worker’s onerror path.
Changes: Worker callback lifecycle and error routing
WorkerWrapper owns and exposes an IsolateGate, which BackgroundLooper closes during shutdown. Queued worker callbacks retain the gate, and RunMainThreadEntry checks it before isolate access. Uncaught worker callback exceptions are routed through the worker onerror handler.
NativeScript/android#2089: Changes how worker errors reach the worker onerror path and parent, which relates to this PR’s worker callback error routing.
Suggested reviewers:nathanwalker
Merge Risk: Minimal · up to aebab
The change closes a worker-shutdown race by dropping callbacks that arrive after a worker begins closing, and it routes worker callback errors to the worker's error handler. No concrete merge-blocking risk was identified.
Pre-merge checks: 4
Check name
Status
Explanation
Title check
The title "fix(worker): drop callbacks from closing workers" matches the main change. The changeset adds an IsolateGate to worker callback handling so that main-thread callbacks for a worker that is s…
Linked Issues check
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
Check skipped because no linked issues were found for this pull request.
Description Check
Check skipped - CodeRabbit’s high-level summary is enabled.
Full details: Title check
Explanation
The title "fix(worker): drop callbacks from closing workers" matches the main change. The changeset adds an IsolateGate to worker callback handling so that main-thread callbacks for a worker that is shutting down are dropped instead of touching a disposed isolate. The title names the fix, the affected component (worker), and the dropped-callback behavior. It is short, specific, and uses a conventional commit prefix. The summary also mentions routing worker callback errors through onerror, which the title does not mention, but that is a secondary aspect and the title does not need to cover every detail.
Finishing Touches
Generate unit tests (beta)
Commit to this branch
Create a new PR
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review.
The main thread read a worker's isolate from the callback cache, released
the cache lock, then took a Locker on it. A worker shutting down in that
window disposed the isolate first, and the main thread crashed in
v8::Locker::Initialize.
Each worker now has a gate the main thread holds from checking it until it
owns the worker's Locker. Shutdown closes the gate before taking its
teardown Locker, so a callback that arrives after that is dropped without
touching the isolate, and the worker never waits on the main thread.
A worker callback that throws under uncaughtErrorPolicy "throw" is now
reported through the worker's own error path (its onerror, then the
parent's Worker object) instead of being rethrown on the main thread,
where it became a Java exception that resolved a handle from the worker
isolate against the main one and crashed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Supersedes #2068 with a different approach to the same two crashes.
Race on worker shutdown.
RunMainThreadEntryread a worker's isolate from the callback cache, releasedcacheMutex_, and only then took av8::Lockeron it. A worker shutting down in that window (DestroyRuntime→Dispose) freed the isolate first, and the main thread crashed inv8::Locker::Initialize.#2068 fixed this by making the worker wait for in-flight main-thread holds before disposing. That keeps the isolate alive for a caller that will then only find its entry gone: the main thread still locks and enters a worker that was told to terminate, and teardown waits on main-thread progress. This PR drops the call instead:
IsolateGate(std::mutex+open), held byshared_ptrin the cache entry so it outlives the isolate.Locker.Locker, while holding noLocker. Once closed, the main thread never touches the isolate. The worker never waits for the main thread beyond theLockercontention that already existed.Locker, so there is no lock-order cycle. A global mutex would deadlock against a worker that calls__runOnMainThreadwhile holding its ownLocker.Main-isolate entries (multithreaded JS) carry no gate and are unchanged.
Throwing worker callbacks. Under
uncaughtErrorPolicy: "throw", a worker callback's uncaught error was rethrown on the main thread as a Java exception. That exception resolved a handle from the worker isolate against the main one and crashed (SIGSEGVinNativeScriptException::ReThrowToJava). Worse, it would have taken the app down for an error that belongs to the worker. It now goes through the worker's own error path (CallWorkerScopeOnErrorHandle: the worker'sonerror, then the parent'sWorkerobject), as the worker's message dispatch already does. Under the defaultreportpolicy nothing changes.Does your commit message include the wording below to reference a specific issue in this repo?
No issue; supersedes #2068.
Related Pull Requests
Does your pull request have unit tests?
No new spec. The race window is a few instructions wide, and the
throwpolicy can't run in the device suite because it crashes the app on purpose (seetestUncaughtErrorPolicy.js). Verified manually with a focused repro on an arm64 API 35 emulator, withusleep(200000)injected inRunMainThreadEntryright after the entry lookup:main__runOnMainThread(cb), busy-waits 50 ms, thenclose()(x5)SIGSEGVinv8::Locker::InitializecbdroppeduncaughtErrorPolicy: "throw", worker's__runOnMainThreadcallback throwsSIGSEGVinNativeScriptException::ReThrowToJavaWorker.onerrorgetsUncaught Error: boom-from-worker-callback, app aliveThe full device suite passes (
runtestsAndVerifyResults: 1545/1545, on top of #2067, #2069 and #2070).Summary by CodeRabbit