inspector: avoid calling into JS from V8 interrupts by joyeecheung · Pull Request #65028 · nodejs/node

joyeecheung · 2026-08-04T23:13:34Z

Our inspector implementation dispatches inspector messages from a V8 interrupt handler, so they could be handled during an arbitrary point of JS execution where re-calling into another irrelevant JS code (which we currently do for creating the async hooks used for the async stack trace tracking) is not safe.

This patch tracks V8 interrupt state in this case and rewrite the async hook toggling as state reconciliation, so requests only record the desired state, which is applied once calling into JS is possible and safe, and the actual invocation is deferred to an immediate when inside an interrupt. This simplifies the previous mechanism and makes re-entrancy and early termination safer.

Drive-by: skip installing command line API extensions during teardown when calling into JS is no longer safe.

Refs: https://issues.chromium.org/u/1/issues/42212250
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/8173727
Refs: #26935

Previously in https://issues.chromium.org/u/1/issues/42212250 V8 was open to support arbitrary JS execution in an interrupt, but that was a long time ago. In a recent CL V8 is trying to disallow arbitrary JS execution in an interrupt again, which prompted this PR to make it safer.

nodejs-github-bot · 2026-08-04T23:13:39Z

Review requested:

@nodejs/inspector

codecov · 2026-08-05T00:52:40Z

Codecov Report

❌ Patch coverage is 58.97436% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.29%. Comparing base (c8fa0b1) to head (a4b4587).
⚠️ Report is 110 commits behind head on main.

Files with missing lines	Patch %	Lines
src/inspector_agent.cc	54.28%	10 Missing and 6 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #65028      +/-   ##
==========================================
- Coverage   92.04%   90.29%   -1.75%     
==========================================
  Files         399      759     +360     
  Lines      175810   247628   +71818     
  Branches    27119    46679   +19560     
==========================================
+ Hits       161816   223604   +61788     
- Misses      13682    15464    +1782     
- Partials      312     8560    +8248

Files with missing lines	Coverage Δ
src/env-inl.h	`95.01% <100.00%> (ø)`
src/env.cc	`85.21% <100.00%> (ø)`
src/env.h	`98.21% <ø> (ø)`
src/inspector_agent.h	`100.00% <ø> (ø)`
src/inspector_agent.cc	`80.79% <54.28%> (ø)`

... and 484 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Our inspector implementation dispatches inspector messages from a V8 interrupt handler, so they could be handled during an arbitrary point of JS execution where re-calling into another irrelevant JS code is not safe. This patch tracks V8 interrupt state in this case and rewrite the async hook toggling as state reconciliation, so requests only record the desired state, which is applied once calling into JS is possible and safe, and the actual invocation is deferred to an immediate when inside an interrupt. This simplifies the previous mechanism and makes re-entracy and early termination safer. Drive-by: skip installing command line API extensions during teardown when calling into JS is no longer safe. Signed-off-by: Joyee Cheung <joyeec9h3@gmail.com>

nodejs-github-bot · 2026-08-05T12:24:18Z

CI: https://ci.nodejs.org/job/node-test-pull-request/75517/

o- · 2026-08-06T09:27:22Z

Fyi, with https://chromium-review.googlesource.com/c/v8/v8/+/8173727 you can now try building with v8_disallow_js_in_api_interrupts_is_checked enabled, to get an assertion for the API contract.

legendecas · 2026-08-07T14:56:01Z

      return;
    }
    env->interrupt_data_.store(nullptr);
+    env->is_processing_v8_interrupt_ = true;


Would it be more reliable if V8 allows querying an isolate interrupt state? This flag at the moment only covers node's own interrupt requests.

I think for fixing inspector, only tracking what the interrupts requested from the Environment is sufficient - we only have this clash because Node.js dispatches inspector messages in an interrupt (that is trackable in the Environment). Technically that's not strictly necessary, at least Chromium only does that conditionally. But without rewriting the entire inspector message machinery, avoiding JS invocation in the interrupt is the simplest safeguard we can put in. Other interrupts are harmless, as they don't lead to the inspector message dispatch. For stricter checks, we could go v8_disallow_js_in_api_interrupts_is_checked and maybe an isolate-wide state check, but since we don't have any other sites that invoke JS in an interrupt currently (not that I could find), I think we can leave that to a followup.

nodejs-github-bot · 2026-08-11T14:35:13Z

CI: https://ci.nodejs.org/job/node-test-pull-request/75777/

…allbacks V8 now CHECKs that v8::Isolate::RequestInterrupt callbacks do not execute JavaScript, defaulting the check off only when build_with_node is set. Node's inspector dispatches protocol messages (and their JS handlers) from interrupt callbacks, which crashed test-inspector-*/test-worker-* with "Invoke in DisallowJavascriptExecutionScope". Electron embeds Node but does not define build_with_node, so turn the check off explicitly until nodejs/node#65028 lands. Ref: https://chromium-review.googlesource.com/c/v8/v8/+/8173727 Co-Authored-By: Claude <noreply@anthropic.com>

* chore: bump chromium in DEPS to 153.0.7988.0 * chore: bump chromium in DEPS to 153.0.7990.0 * chore: bump chromium in DEPS to 153.0.7992.0 * chore: bump chromium in DEPS to 153.0.7993.0 * chore: bump chromium in DEPS to 153.0.7995.0 * 8185860: Move SandboxedProcessLauncherDelegate to content/browser Ref: https://chromium-review.googlesource.com/c/chromium/src/+/8185860 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: John Kleinschmidt <jkleinsc@electronjs.org> * 5287320: Remove handling of esoteric run loop modes Upstream deleted ScopedPumpMessagesInPrivateModes and the run loop mode-mask machinery it relied on, and dropped its use in ui::ShowContextMenu. Electron's patch making the scoper ref-counted has nothing left to modify, so it is removed, and the remaining use in tray_icon_cocoa.mm is dropped to match upstream. Ref: https://chromium-review.googlesource.com/c/chromium/src/+/5287320 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: John Kleinschmidt <jkleinsc@electronjs.org> * 8164689: [SG Android] Establish passage for Dynamic routes in ProxyConfig Ref: https://chromium-review.googlesource.com/c/chromium/src/+/8164689 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: John Kleinschmidt <jkleinsc@electronjs.org> * 8189930: Support org.freedesktop.StatusNotifierItem for Linux status icons Ref: https://chromium-review.googlesource.com/c/chromium/src/+/8189930 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: John Kleinschmidt <jkleinsc@electronjs.org> * 7873086: [tracing] Avoid perfetto track aliasing in third_party/blink/renderer/core/workers Ref: https://chromium-review.googlesource.com/c/chromium/src/+/7873086 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: John Kleinschmidt <jkleinsc@electronjs.org> * 8169400: Clean up accessibility code Ref: https://chromium-review.googlesource.com/c/chromium/src/+/8169400 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: John Kleinschmidt <jkleinsc@electronjs.org> * 8174204: Remove Browser::app_name() in favor of BrowserInitState Ref: https://chromium-review.googlesource.com/c/chromium/src/+/8174204 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: John Kleinschmidt <jkleinsc@electronjs.org> * 8174085: Roll src/third_party/sqlite/src/ 0a5fe1b18..ba8c54c83 (80 commits) Ref: https://chromium-review.googlesource.com/c/chromium/src/+/8174085 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: John Kleinschmidt <jkleinsc@electronjs.org> * chore: update patches * 8202925: [api] Remove deprecated GetAlignedPointerFromEmbedderData and SetAlignedPointerInEmbedderData Adds v8::kEmbedderDataTypeTagDefault to the remaining tag-less Context::Set/GetAlignedPointerFromEmbedderData call sites in Node, matching the pattern already used in this patch. Ref: https://chromium-review.googlesource.com/c/v8/v8/+/8202925 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: John Kleinschmidt <jkleinsc@electronjs.org> * chore: update patches * 7904261: Refactor multi-network CCT to rely on net::UrlRequest's target_network Adds the new is_for_network_service parameter to Electron's WillCreateURLLoaderFactory override. Electron passes false at its one direct call site since the same builder is also finished against an existing SharedURLLoaderFactory and Electron does no network-bound request targeting. Ref: https://chromium-review.googlesource.com/c/chromium/src/+/7904261 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: John Kleinschmidt <jkleinsc@electronjs.org> * 8140059: Refactor ConfigureStubHostResolver to rely on InsecureDnsMode Ref: https://chromium-review.googlesource.com/c/chromium/src/+/8140059 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: John Kleinschmidt <jkleinsc@electronjs.org> * chore: bump chromium in DEPS to 153.0.7996.0 * chore: bump chromium in DEPS to 153.0.7997.0 * chore: bump chromium in DEPS to 153.0.7998.0 * chore: bump chromium in DEPS to 153.0.7999.0 * chore: update patches * chore: bump chromium in DEPS to 153.0.8001.0 * 8206965: Spellcheck: initialize renderers whose process is still launching The fix carried by fix_do_not_skip_still-launching_renderers_when_initializing_spellcheck.patch landed upstream; remove the patch. Ref: https://chromium-review.googlesource.com/c/chromium/src/+/8206965 Co-Authored-By: Claude <noreply@anthropic.com> * 8230238: Ruff: reformat build. Upstream reformatted build/**/*.py to 4-space indentation. Re-indent the ignore_rc_check.patch hunk in build/toolchain/win/rc/rc.py to match and refresh build/toolchain/apple/linker_driver.py context. Ref: https://chromium-review.googlesource.com/c/chromium/src/+/8230238 Co-Authored-By: Claude <noreply@anthropic.com> * 8129904: [policy] Specify static inputs for policy_templates grit target Ref: https://chromium-review.googlesource.com/c/chromium/src/+/8129904 Co-Authored-By: Claude <noreply@anthropic.com> * chore: update patches * 8224563: [network_hints] Store frame identity as GlobalRenderFrameHostId Ref: https://chromium-review.googlesource.com/c/chromium/src/+/8224563 Co-Authored-By: Claude <noreply@anthropic.com> * 7708809: Implement portal-based eye dropper for Linux eye_dropper_aura.cc now calls EyeDropperPortal::Create() on Wayland when USE_DBUS is set; build eye_dropper_portal.cc into //electron/chromium_src on Linux so the symbol resolves at link time. Ref: https://chromium-review.googlesource.com/c/chromium/src/+/7708809 Co-Authored-By: Claude <noreply@anthropic.com> * 8104602: WebUI: Enable use_typescript_go (TS v7) by default. ts_library() now invokes the host tsgo binary from //third_party/typescript/<host>/src, which DEPS gates on host_os == "mac". Our macOS checkouts are produced on Linux, so install the mac-arm64 / mac-amd64 CIPD package in fix-sync like the other host binaries. Ref: https://chromium-review.googlesource.com/c/chromium/src/+/8104602 Co-Authored-By: Claude <noreply@anthropic.com> * 8194387: Move fullscreen Browser methods into FullscreenController FullscreenController now notifies BrowserCommandController and BookmarkBarController directly. Electron builds neither, so compile the calls out in fix_adapt_exclusive_access_for_electron_needs.patch (they were already unreachable: Electron constructs ExclusiveAccessManager with null controllers). Fixes the Windows link. Ref: https://chromium-review.googlesource.com/c/chromium/src/+/8194387 Co-Authored-By: Claude <noreply@anthropic.com> * 8173727: [api] Prepare DisallowJavascriptExecution scope around api callbacks V8 now CHECKs that v8::Isolate::RequestInterrupt callbacks do not execute JavaScript, defaulting the check off only when build_with_node is set. Node's inspector dispatches protocol messages (and their JS handlers) from interrupt callbacks, which crashed test-inspector-*/test-worker-* with "Invoke in DisallowJavascriptExecutionScope". Electron embeds Node but does not define build_with_node, so turn the check off explicitly until nodejs/node#65028 lands. Ref: https://chromium-review.googlesource.com/c/v8/v8/+/8173727 Co-Authored-By: Claude <noreply@anthropic.com> --------- Signed-off-by: John Kleinschmidt <jkleinsc@electronjs.org> Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: electron-claude[bot] <279139701+electron-claude[bot]@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com> Co-authored-by: Alice Zhao <alicelovescake@anthropic.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Aug 4, 2026

joyeecheung force-pushed the fix-inspector-interrupt branch from bf00114 to a4b4587 Compare August 5, 2026 01:59

legendecas mentioned this pull request Aug 7, 2026

inspector: load network tracking modules eagerly #64379

Open

legendecas reviewed Aug 7, 2026

View reviewed changes

legendecas approved these changes Aug 8, 2026

View reviewed changes

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 11, 2026

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 11, 2026

Navigation Menu

Uh oh!

Uh oh!

inspector: avoid calling into JS from V8 interrupts - #65028

inspector: avoid calling into JS from V8 interrupts#65028
joyeecheung wants to merge 1 commit into
nodejs:mainfrom
joyeecheung:fix-inspector-interrupt

joyeecheung commented Aug 4, 2026 •

edited

Loading

Uh oh!

nodejs-github-bot commented Aug 4, 2026

Uh oh!

codecov Bot commented Aug 5, 2026 •

edited

Loading

Uh oh!

nodejs-github-bot commented Aug 5, 2026

Uh oh!

o- commented Aug 6, 2026

Uh oh!

legendecas Aug 7, 2026 •

edited

Loading

Uh oh!

joyeecheung Aug 7, 2026 •

edited

Loading

Uh oh!

nodejs-github-bot commented Aug 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Uh oh!

Conversation

joyeecheung commented Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Aug 4, 2026

Uh oh!

codecov Bot commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nodejs-github-bot commented Aug 5, 2026

Uh oh!

o- commented Aug 6, 2026

Uh oh!

legendecas Aug 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joyeecheung Aug 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Aug 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

joyeecheung commented Aug 4, 2026 •

edited

Loading

codecov Bot commented Aug 5, 2026 •

edited

Loading

legendecas Aug 7, 2026 •

edited

Loading

joyeecheung Aug 7, 2026 •

edited

Loading