inspector: fix crash when pausing JS sessions

[christianaurichzm]

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

[nodejs-github-bot]

Review requested:

• @nodejs/inspector

[codecov]

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]

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]

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?

[christianaurichzm]

Since #65907 reverted the V8 check, this no longer reproduces on main, so I'm moving this to draft.

The gap is still in V8 upstream: handleProgramBreak() sends Debugger.paused before the AllowJavascriptExecutionScope added in v8/v8@b36cf9f6ccd2, and Debugger.resumed after it closes. I've uploaded a CL that extends that scope over both notifications: https://chromium-review.googlesource.com/c/v8/v8/+/8459373

@deepak1556 this will come back when the check is re-enabled. test-inspector-multisession-ws is what hit it in CI.

[deepak1556]

Thanks @christianaurichzm for looking into this, I am following up in https://chromium-review.googlesource.com/c/v8/v8/+/8173727/comments/48212f36_562dce9e to understand first the nature of the original scope addition.

[christianaurichzm]

@deepak1556 the V8 fix landed as v8/v8@35bd1582fe0a. I'd like to open the re-land of #65397 with a backport of it on top, keeping your two commits as they are (the defer one needed a conflict resolution with #65877). Does that work for you, or would you rather re-land it yourself?

[deepak1556]

Sounds good, please go ahead. Also ensure #65397 (comment) is backported. Its better to land them all in one PR.

[christianaurichzm]

Replaced by #66740, which backports the V8 fix and re-lands #65397 on top of it.