Skip to content

inspector: fix crash when pausing JS sessions - #65914

Open
christianaurichzm wants to merge 1 commit into
nodejs:mainfrom
christianaurichzm:inspector-allow-js-session-delivery
Open

inspector: fix crash when pausing JS sessions#65914
christianaurichzm wants to merge 1 commit into
nodejs:mainfrom
christianaurichzm:inspector-allow-js-session-delivery

Conversation

@christianaurichzm

Copy link
Copy Markdown
Contributor

parallel/test-inspector-multisession-ws has been failing across macOS, Linux, Windows ARM64, and s390x (nodejs/reliability#1654) with:

# Fatal error in , line 0
# Invoke in DisallowJavascriptExecutionScope

This started after #65397 enabled v8_disallow_js_in_api_interrupts_is_checked.

An in-process inspector.Session delivers protocol messages by calling into JavaScript. JSBindingsConnection::SendMessageToFrontend() already defers messages produced from GC callbacks and Node-managed V8 interrupts.

Debugger.pause schedules a separate Isolate::RequestInterrupt() inside V8. That interrupt is not reflected by
Environment::is_processing_v8_interrupt(). V8 then emits Debugger.paused before entering the AllowJavascriptExecutionScope used for the pause message loop, so the JS session callback is invoked while DisallowJavascriptExecution is active.

Add an AllowJavascriptExecutionScope only to the synchronous delivery path, after the existing guards. d8 does the same in InspectorFrontend::Send(). The queued delivery path from #65397 is unchanged.

The regression test fails 3/3 on main and passes with this change.

Tested locally with all parallel/test-inspector-*, the related sequential inspector/debugger/cpu-prof/heapdump tests, and --repeat=10 for the affected multisession tests and the new test.

test-inspector-multisession-ws and parallel.status are unchanged.

This is a process crash, not a test that should be marked flaky.

Refs: #65397
Refs: nodejs/reliability#1654

An in-process inspector.Session delivers protocol messages by calling
into JavaScript, and V8 produces some of them while it asserts that
JavaScript is not executed: Debugger.paused is broadcast from the
Isolate::RequestInterrupt handler that Debugger.pause schedules, before
V8 opens its own AllowJavascriptExecutionScope for the pause message
loop. So the process aborts with "Invoke in
DisallowJavascriptExecutionScope" as soon as a debugger client pauses it
while JavaScript is running.

Open an AllowJavascriptExecutionScope on the synchronous delivery path,
the same opt-out d8's own inspector channel uses for every protocol
message it delivers, see InspectorFrontend::Send() in
deps/v8/src/d8/d8.cc. The messages produced where calling into
JavaScript is really unsafe, such as GC callbacks, take the queueing
branch instead and are delivered from a point where it is allowed
again.

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

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/inspector

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol. needs-ci PRs that need a full CI run. labels Sep 8, 2026
@codecov

codecov Bot commented Sep 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.17%. Comparing base (b113d09) to head (e0fef75).
⚠️ Report is 29 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #65914      +/-   ##
==========================================
+ Coverage   90.16%   90.17%   +0.01%     
==========================================
  Files         771      771              
  Lines      265094   265170      +76     
  Branches    50355    50368      +13     
==========================================
+ Hits       239027   239124      +97     
+ Misses      17011    17001      -10     
+ Partials     9056     9045      -11     
Files with missing lines Coverage Δ
src/inspector_js_api.cc 84.13% <100.00%> (+0.04%) ⬆️

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

@legendecas legendecas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This does not fix the problem. The linked #65397 has stated that:

In v8::Isolate::RequestInterrupt the API contract says:

    API interrupt callbacks are forbidden from executing JavaScript
    on the interrupted Isolate.

This patch does not conform to the V8 API contract, rather, it contradicts the intentional DisallowJavascriptExecutionScope. An expected fix should fix the timing of calling into JS.

@christianaurichzm

Copy link
Copy Markdown
Contributor Author

Thanks. I took another look at #65028 after your comment. You pointed out there that Node's interrupt flag only covers its own RequestInterrupts, and that seems to be the missing case here: the debugger schedules a separate RequestInterrupt inside V8.

I tried the timing-based approach. Deferring Debugger.paused to runMessageLoopOnPause() works, but Node's current interrupt state doesn't cover that debugger interrupt. Deferring more broadly breaks existing inspector delivery; heap snapshots are one example. I also confirmed that Debugger.resumed hits the same issue, since didContinue() runs after the current V8 execution scope closes.

I also tried a small V8-side change that extends the existing inspector scope across the pause/resume notifications. It passes the full local suite with no src/ changes, but I'm not sure widening that scope is the right fix.

Would you prefer that I bring this upstream to V8 first?

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++. inspector Issues and PRs related to the V8 inspector protocol. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants