GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — fix(worker): keep a worker isolate alive while a __runOnMainThread callback locks it by adrian-niculescu · Pull Request #2068 · NativeScript/android

Conversation

adrian-niculescu commented Oct 6, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

A worker that calls __runOnMainThread(fn) and ends shortly after, for example through close(), can crash the app: the main thread locks the worker's isolate after the worker has disposed it, and dies with a SIGSEGV in v8::Locker::Initialize.

CallbackHandlers::RunMainThreadEntry reads the isolate from its cache entry, releases cacheMutex_, and only then constructs the v8::Locker. Worker teardown removes the entry in DestroyRuntime and disposes the isolate right after, so an entry the main thread already took can outlive its isolate. The main thread now records a hold on the isolate in the same cacheMutex_ section where it reads the entry, and drops it only after its Locker is released. The worker waits for those holds after it has released its own Locker and before it disposes the isolate. Waiting with the worker's Locker still held would deadlock against the main thread's Locker. A callback that took the entry before RemoveIsolateEntries ran then finds the entry gone under a live isolate and returns, and one that did not take it never sees the isolate.

A callback that throws has the same problem in another form: the exception kept a handle into the worker isolate after the hold was gone, and the event loop resolved that handle against the main thread's isolate when it reported it. For a worker's callback the exception now drops the handle while the isolate is still held, so the report carries its message and stack, as a failed runtime initialization already does. A callback on the main isolate keeps the handle, so its report is unchanged.

The window is a few instructions wide, so there is no spec for it. With a sleep injected between reading the entry and taking the Locker, a worker that closes right after posting its callback crashes the main thread on main and does not with this change. The full device suite passes.

Summary by CodeRabbit

Bug Fixes
- Improved worker shutdown reliability by waiting for pending main-thread callbacks to finish before disposing of the worker runtime.
- Prevented callback activity from accessing an isolate during disposal.
- Fixed cleanup of JavaScript handles when worker callbacks throw unhandled exceptions.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          fix(worker): keep a worker isolate alive while a __runOnMainThread ca…

168ff7c

…llback locks it

RunMainThreadEntry read the target isolate from its entry, released the cache lock and only then took the isolate's Locker. A worker that ended in between removed the entry and disposed the isolate, and the main thread then locked freed memory. The main thread now holds the isolate from the moment it takes the entry until it releases the Locker, and a worker waits for those holds before disposing its isolate.

coderabbitai Bot commented Oct 6, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: a85a81dc-a1f2-4474-a08e-478e168b15f9

📥 Commits

Reviewing files that changed from the base of the PR and between 9b12329 and a340e10.

📒 Files selected for processing (4)

test-app/runtime/src/main/cpp/CallbackHandlers.cpp
test-app/runtime/src/main/cpp/CallbackHandlers.h
test-app/runtime/src/main/cpp/Runtime.cpp
test-app/runtime/src/main/cpp/WorkerWrapper.cpp

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.

📝 Walkthrough

Walkthrough

CallbackHandlers tracks active main-thread callbacks by isolate and provides a wait method. BackgroundLooper waits for callbacks after destroying the worker runtime and before disposing its isolate. Worker callback exception handling releases its JavaScript handle before rethrowing.

Changes

Worker callback lifecycle

Layer / File(s)	Summary
Track callback holds `test-app/runtime/src/main/cpp/CallbackHandlers.h`, `test-app/runtime/src/main/cpp/CallbackHandlers.cpp`	CallbackHandlers tracks active callback holds per isolate and adds `WaitForMainThreadCallbacks`. For worker runtimes, the callback exception path releases the JavaScript handle before throwing the exception onward.
Wait before isolate disposal `test-app/runtime/src/main/cpp/WorkerWrapper.cpp`, `test-app/runtime/src/main/cpp/Runtime.cpp`	BackgroundLooper waits for main-thread callbacks after `DestroyRuntime()` and before `isolate->Dispose()`. The Runtime.cpp comment describes the removal of untaken entries and waiting for entries already taken.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~15 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant BackgroundLooper
  participant Runtime
  participant CallbackHandlers
  participant MainThreadCallback
  participant Isolate
  BackgroundLooper->>Runtime: DestroyRuntime()
  Runtime->>CallbackHandlers: RemoveIsolateEntries()
  BackgroundLooper->>CallbackHandlers: WaitForMainThreadCallbacks(isolate)
  MainThreadCallback->>CallbackHandlers: RunMainThreadEntry()
  CallbackHandlers->>Isolate: Hold isolate while callback runs
  CallbackHandlers-->>CallbackHandlers: Release hold after isolate locker
  CallbackHandlers-->>BackgroundLooper: Return when no holds remain
  BackgroundLooper->>Isolate: Dispose()

Suggested reviewers: edusperoni

Merge Risk: ⚪ Minimal · up to a340e

No merge-blocking issue is established; the worker callback shutdown change is ready for normal merge checks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 4 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: keeping a worker isolate alive while a `__runOnMainThread` callback acquires its locker.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

Fix all pre-merge checks with AI

Autopilot · 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.

❤️ Share

_{A rabbit counts each callback’s stay
Then waits until the holds give way
The worker rests; its tasks are through
The isolate waits for the all-clear too
A carrot marks the shutdown queue}

_{Comment @coderabbitai help to get the list of available commands.}

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          fix(worker): report a worker's __runOnMainThread exception without it…

a340e10

…s isolate handle

The exception a worker's callback threw kept a handle into the worker isolate after RunMainThreadEntry let go of it, so the isolate could be disposed before the event loop reported it, and the report resolved the handle against the main thread's isolate. For a worker's callback the exception now drops the handle while the isolate is still held, and only its message and stack reach Java.

adrian-niculescu marked this pull request as ready for review

October 6, 2026 21:53

edusperoni mentioned this pull request

Nov 11, 2026

fix(worker): drop callbacks from closing workers #2108

Open

This branch has not been deployed

No deployments

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

1 participant

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.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

adrian-niculescu commented Oct 6, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Oct 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

❌ Failed checks (1 warning)

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

adrian-niculescu commented Oct 6, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

coderabbitai Bot commented Oct 6, 2026 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.