Skip to content

deps: re-land V8 API interrupt JS check - #66740

Open
christianaurichzm wants to merge 4 commits into
nodejs:mainfrom
christianaurichzm:v8-interrupt-js-reland
Open

christianaurichzm wants to merge 4 commits into
nodejs:mainfrom
christianaurichzm:v8-interrupt-js-reland

Conversation

@christianaurichzm

Copy link
Copy Markdown
Contributor

This re-lands #65397 together with the V8 fix for the inspector crash it exposed.

#65907 reverted it because debugger/debug/futex-reentrant-wait failed on node-test-commit-v8-linux. That test depends on https://chromium-review.googlesource.com/c/v8/v8/+/8086043, which V8 14.6 didn't have. deps/v8 is on 15.5 now, which has it.

The check also made test-inspector-multisession-ws abort with Invoke in DisallowJavascriptExecutionScope (nodejs/reliability#1654). When V8 pauses from an API interrupt, it sent Debugger.paused and Debugger.resumed outside the AllowJavascriptExecutionScope it opens for the pause loop, and an in-process inspector.Session handles both in JavaScript. v8/v8@35bd1582fe0a fixes that, and the first commit backports it. V8 15.5 has no FunctionCallbackInfo::DataV2(), so the unittest uses Data() instead.

@deepak1556's two commits are unchanged apart from a conflict with #65877 in src/inspector_agent.{cc,h}. As in #65397, messages for an in-process inspector.Session that arrive while Node handles another client's message in an interrupt are delivered at the next microtask checkpoint instead of synchronously.

test-inspector-pause-with-js-session aborts 5/5 without the V8 fix and passes 100/100 with it. test-inspector-multisession-ws passes 100/100 as well, but it also passed without the fix here, so it only hits the crash by timing. make lint, parallel and sequential pass on Linux x64. This should also get a node-test-commit-v8-linux run.

Refs: #65397
Refs: #65907
Refs: #65914
Refs: nodejs/reliability#1654

christianaurichzm and others added 4 commits October 14, 2026 07:18
Original commit message:

    [inspector] Allow JS in pause notifications from API interrupts

    Debugger.pause and OOM breaks are scheduled with RequestInterrupt(), so
    handleProgramBreak() can run inside InvokeApiInterruptCallbacks() with
    JavaScript execution disallowed. b36cf9f6ccd2 allowed JavaScript around
    runMessageLoopOnPause(), but Debugger.paused is sent right before that
    scope and Debugger.resumed right after it. An embedder channel that
    delivers notifications to JavaScript on the same isolate, like the
    in-process inspector.Session in Node.js, crashes with "Invoke in
    DisallowJavascriptExecutionScope" when such a break happens.

    Extend the existing scope over both notifications. d8's InspectorFrontend
    opens its own AllowJavascriptExecutionScope, so no existing test covered
    this; add a unittest with a channel that calls into JavaScript.

    R=olivf@chromium.org, szuend@chromium.org

    Refs: nodejs#65914
    Bug: 40067940, 540137721
    Change-Id: I0b60fb40cf9e753f7444d69ee8c40b4dd5d843ec
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/8459373
    Reviewed-by: Simon Zünd <szuend@chromium.org>
    Commit-Queue: Olivier Flückiger <olivf@chromium.org>
    Reviewed-by: Olivier Flückiger <olivf@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#110692}

V8 15.5 has no FunctionCallbackInfo::DataV2(), so the unittest reads
the session pointer through Data() instead.

Refs: v8/v8@35bd158
Signed-off-by: Christian Aurich Zanettini Martins <christian.aurichzm@gmail.com>
Inspector messages for a main thread connection are dispatched
from a V8 interrupt handler. Defer frontend messages in this
case using a microtask. This preserves the existing timing
contract for main thread inspector connections, where
notifications are observable after a microtask checkpoint.

Starting the inspector I/O thread and toggling network tracking
can also call into JS from an interrupt.

Signed-off-by: deepak1556 <hop2deep@gmail.com>
Signed-off-by: Christian Aurich Zanettini Martins <christian.aurichzm@gmail.com>
Signed-off-by: deepak1556 <hop2deep@gmail.com>
Signed-off-by: Christian Aurich Zanettini Martins <christian.aurichzm@gmail.com>
Debugger.pause breaks from an API interrupt, and V8 sends
Debugger.paused and Debugger.resumed to every session from there. An
in-process inspector.Session receives them by calling into
JavaScript, which aborts with "Invoke in
DisallowJavascriptExecutionScope" unless V8 allows JavaScript around
both notifications.

Signed-off-by: Christian Aurich Zanettini Martins <christian.aurichzm@gmail.com>
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/inspector
  • @nodejs/security-wg
  • @nodejs/v8-update

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Oct 14, 2026
@christianaurichzm

Copy link
Copy Markdown
Contributor Author

@legendecas @joyeecheung since you reviewed #65397, could you take a look at the re-land when you get a chance? It also needs a node-test-commit-v8-linux run, since that is what caught the futex failure last time. Thanks!

@legendecas legendecas added v8 engine Issues and PRs related to the V8 dependency. dependencies PRs that add, update, or configure Node.js dependencies. labels Oct 14, 2026
@codecov

codecov Bot commented Oct 14, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 56.84211% with 41 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.48%. Comparing base (dd7235c) to head (960398c).
⚠️ Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
src/inspector_agent.cc 47.05% 27 Missing and 9 partials ⚠️
src/inspector_js_api.cc 78.26% 3 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #66740      +/-   ##
==========================================
- Coverage   90.48%   90.48%   -0.01%     
==========================================
  Files         791      791              
  Lines      277204   277250      +46     
  Branches    53311    53314       +3     
==========================================
+ Hits       250841   250871      +30     
- Misses      16760    16786      +26     
+ Partials     9603     9593      -10     
Files with missing lines Coverage Δ
src/inspector_agent.h 100.00% <100.00%> (ø)
src/inspector_js_api.cc 84.26% <78.26%> (+6.98%) ⬆️
src/inspector_agent.cc 83.75% <47.05%> (+0.34%) ⬆️

... and 32 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.

@deepak1556

Copy link
Copy Markdown
Contributor

Oh another case found,

  1. DOMStorageAgent::getWebStorage() (inspector: auto collect webstorage data #62145) looks up globalThis.localStorage / globalThis.sessionStorage via lazy JS accessors.
  2. When Storage::GetAll() opens a malformed database file (src: throw on a malformed localStorage file #65879), Storage::Open() creates a JS Error only for DOMStorageAgent::getDOMStorageItems() to catch it with v8::TryCatch and extract the message string.

Rather than deferring DOMStorage.getDOMStorageItems (which would prevent inspecting storage while paused in NodeInspectorClient::runMessageLoopOnPause), could try something like
0001-inspector-avoid-JS-execution-in-DOMStorage.getDOMSto.patch that keeps everything in C++

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. dependencies PRs that add, update, or configure Node.js dependencies. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants