Skip to content

inspector: defer JS execution from V8 interrupts - #65397

Closed
deepak1556 wants to merge 3 commits into
nodejs:mainfrom
deepak1556:robo/enable_js_in_api_interrupt_check
Closed

inspector: defer JS execution from V8 interrupts#65397
deepak1556 wants to merge 3 commits into
nodejs:mainfrom
deepak1556:robo/enable_js_in_api_interrupt_check

Conversation

@deepak1556

Copy link
Copy Markdown
Contributor

Followup to #65028 covering additional callsites, upstreaming from electron/electron#52960.

Backported the V8 feature to enable the scope check moving forward, node::RequestInterrupt already subjected this restriction with its scope check. The change should only affect addons that were to use v8::Isolate::RequestInterrupt, but its already a documented caution in the public header. Any reason the feature shouldn't be enabled ?

cc @joyeecheung

@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 lib / src Issues and PRs involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels Aug 19, 2026
deepak1556 and others added 3 commits August 19, 2026 17:04
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: deepak1556 <hop2deep@gmail.com>
Original commit message:

    [api] Prepare DisallowJavascriptExecution scope around api callbacks

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

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

    This CL prepares the infrastructure to enforce this contract.

    Bug: 540137721, 40067940

    TAG=agy
    CONV=78573798-a951-4bd3-9c6f-3e3b6775da4b

    Change-Id: I43cc83c3cc3ebd34052a927dde55bbf4d49d2d29
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/8173727
    Auto-Submit: Olivier Flückiger <olivf@chromium.org>
    Reviewed-by: Simon Zünd <szuend@chromium.org>
    Commit-Queue: Olivier Flückiger <olivf@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#109086}

Refs: v8/v8@b36cf9f
Signed-off-by: deepak1556 <hop2deep@gmail.com>
@deepak1556
deepak1556 force-pushed the robo/enable_js_in_api_interrupt_check branch from e9d757c to 788c38a Compare August 19, 2026 08:31
@codecov

codecov Bot commented Aug 19, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 61.05263% with 37 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.12%. Comparing base (55e4ca3) to head (788c38a).
⚠️ Report is 329 commits behind head on main.

Files with missing lines Patch % Lines
src/inspector_agent.cc 52.94% 24 Missing and 8 partials ⚠️
src/inspector_js_api.cc 78.26% 3 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #65397      +/-   ##
==========================================
- Coverage   90.14%   90.12%   -0.02%     
==========================================
  Files         752      752              
  Lines      251870   251889      +19     
  Branches    47365    47353      -12     
==========================================
- Hits       227037   227021      -16     
- Misses      16177    16205      +28     
- Partials     8656     8663       +7     
Files with missing lines Coverage Δ
src/inspector_agent.h 100.00% <100.00%> (ø)
src/inspector_js_api.cc 84.09% <78.26%> (+7.07%) ⬆️
src/inspector_agent.cc 81.81% <52.94%> (+0.89%) ⬆️

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

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 19, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 19, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@codebytere codebytere added author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue PRs queued for automated landing through the Commit Queue. and removed needs-ci PRs that need a full CI run. labels Sep 4, 2026
@nodejs-github-bot nodejs-github-bot added commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. and removed commit-queue PRs queued for automated landing through the Commit Queue. labels Sep 4, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Commit Queue failed

This pull request has multiple commits, but no landing policy was selected.

Add commit-queue-squash PRs the Commit Queue should land as one squashed commit. to land it as one commit, or commit-queue-rebase PRs the Commit Queue should land as multiple self-contained commits. to land the commits separately.

The pull request was removed from the Commit Queue and labeled commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. . After resolving the failure, remove that label and add commit-queue PRs queued for automated landing through the Commit Queue. to retry.

Full Commit Queue output
- Loading data for nodejs/node/pull/65397
✔  Done loading data for nodejs/node/pull/65397
----------------------------------- PR info ------------------------------------
Title      inspector: defer JS execution from V8 interrupts (#65397)
Author     Robo <hop2deep@gmail.com> (@deepak1556)
Branch     deepak1556:robo/enable_js_in_api_interrupt_check -> nodejs:main
Labels     lib / src, author ready, commit-queue
Commits    3
 - inspector: defer JS execution from V8 interrupts
 - deps: enable V8 API interrupt JS check
 - deps: V8: backport b36cf9f6ccd2
Committers 1
 - deepak1556 <hop2deep@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/65397
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/65397
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Wed, 19 Aug 2026 07:52:51 GMT
   ✔  Approvals: 5
   ✔  - Joyee Cheung (@joyeecheung) (TSC): https://github.com/nodejs/node/pull/65397#pullrequestreview-4972342519
   ✔  - Chengzhong Wu (@legendecas) (TSC): https://github.com/nodejs/node/pull/65397#pullrequestreview-4973929213
   ✔  - Juan José Arboleda (@juanarbol): https://github.com/nodejs/node/pull/65397#pullrequestreview-4975923033
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/65397#pullrequestreview-4978582219
   ✔  - Shelley Vohr (@codebytere): https://github.com/nodejs/node/pull/65397#pullrequestreview-5113448266
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2026-08-20T14:25:46Z: https://ci.nodejs.org/job/node-test-pull-request/76059/
- Querying data for job/node-test-pull-request/76059/
✔  Build data downloaded
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
   ddc0a0aa28..1e9fd9544e  main       -> origin/main
✔  origin/main is now up-to-date
main is out of sync with origin/main. Mismatched commits:
 - 0df3c02e8b build: add `--shared-abseil` configure flag
 - 1e9fd9544e build: add `--shared-highway` configure flag
--------------------------------------------------------------------------------
HEAD is now at 1e9fd9544e build: add `--shared-highway` configure flag
   ✔  Reset to origin/main
- Downloading patch for 65397
From https://github.com/nodejs/node
 * branch                  refs/pull/65397/merge -> FETCH_HEAD
✔  Fetched commits as 5ceeb6e31c63..788c38aa6746
--------------------------------------------------------------------------------
[main 3f12e91d5f] inspector: defer JS execution from V8 interrupts
 Author: deepak1556 <hop2deep@gmail.com>
 Date: Wed Aug 19 14:16:35 2026 +0900
 3 files changed, 145 insertions(+), 102 deletions(-)
Auto-merging common.gypi
Auto-merging tools/v8_gypfiles/features.gypi
[main 4f33538947] deps: enable V8 API interrupt JS check
 Author: deepak1556 <hop2deep@gmail.com>
 Date: Wed Aug 19 15:22:06 2026 +0900
 2 files changed, 11 insertions(+)
Auto-merging common.gypi
[main 63de161ced] deps: V8: backport b36cf9f6ccd2
 Author: Olivier Flückiger <olivf@chromium.org>
 Date: Thu Aug 6 07:57:24 2026 +0000
 11 files changed, 93 insertions(+), 2 deletions(-)
 create mode 100644 deps/v8/test/debugger/debug/futex-reentrant-wait.js
   ✔  Patches applied
There are 3 commits in the PR. Attempting autorebase.
(node:588) [DEP0190] DeprecationWarning: Passing args to a child process with shell option true can lead to security vulnerabilities, as the arguments are not escaped, only concatenated.
(Use `node --trace-deprecation ...` to show where the warning was created)
Rebasing (2/6)
Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
inspector: defer JS execution from V8 interrupts

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>
PR-URL: https://github.com/nodejs/node/pull/65397
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>
--------------------------------------------------------------------------------
[detached HEAD 2b887e9bd8] inspector: defer JS execution from V8 interrupts
 Author: deepak1556 <hop2deep@gmail.com>
 Date: Wed Aug 19 14:16:35 2026 +0900
 3 files changed, 145 insertions(+), 102 deletions(-)
Rebasing (3/6)
Rebasing (4/6)
Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
deps: enable V8 API interrupt JS check

Signed-off-by: deepak1556 <hop2deep@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/65397
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>
--------------------------------------------------------------------------------
[detached HEAD b70311f510] deps: enable V8 API interrupt JS check
 Author: deepak1556 <hop2deep@gmail.com>
 Date: Wed Aug 19 15:22:06 2026 +0900
 2 files changed, 11 insertions(+)
Rebasing (5/6)
Rebasing (6/6)
Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
deps: V8: backport b36cf9f6ccd2

Original commit message:

    [api] Prepare DisallowJavascriptExecution scope around api callbacks

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

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

    This CL prepares the infrastructure to enforce this contract.

    Bug: 540137721, 40067940

    TAG=agy
    CONV=78573798-a951-4bd3-9c6f-3e3b6775da4b

    Change-Id: I43cc83c3cc3ebd34052a927dde55bbf4d49d2d29
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/8173727
    Auto-Submit: Olivier Flückiger <olivf@chromium.org>
    Reviewed-by: Simon Zünd <szuend@chromium.org>
    Commit-Queue: Olivier Flückiger <olivf@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#109086}

Refs: https://github.com/v8/v8/commit/b36cf9f6ccd2931d1d0bde6a9f366d30667e4b75
Signed-off-by: deepak1556 <hop2deep@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/65397
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>
--------------------------------------------------------------------------------
[detached HEAD 52129cefe2] deps: V8: backport b36cf9f6ccd2
 Author: Olivier Flückiger <olivf@chromium.org>
 Date: Thu Aug 6 07:57:24 2026 +0000
 11 files changed, 93 insertions(+), 2 deletions(-)
 create mode 100644 deps/v8/test/debugger/debug/futex-reentrant-wait.js
Successfully rebased and updated refs/heads/main.
--------------------------------------------------------------------------------
   ℹ  Add `commit-queue-squash` label to land the PR as one commit, or `commit-queue-rebase` to land as separate commits.

View workflow run

@codebytere codebytere added commit-queue-rebase PRs the Commit Queue should land as multiple self-contained commits. commit-queue PRs queued for automated landing through the Commit Queue. and removed commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. labels Sep 4, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in 03e2b9b...ccd648e

nodejs-github-bot pushed a commit that referenced this pull request Sep 4, 2026
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>
PR-URL: #65397
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>
nodejs-github-bot pushed a commit that referenced this pull request Sep 4, 2026
Signed-off-by: deepak1556 <hop2deep@gmail.com>
PR-URL: #65397
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>
nodejs-github-bot pushed a commit that referenced this pull request Sep 4, 2026
Original commit message:

    [api] Prepare DisallowJavascriptExecution scope around api callbacks

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

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

    This CL prepares the infrastructure to enforce this contract.

    Bug: 540137721, 40067940

    TAG=agy
    CONV=78573798-a951-4bd3-9c6f-3e3b6775da4b

    Change-Id: I43cc83c3cc3ebd34052a927dde55bbf4d49d2d29
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/8173727
    Auto-Submit: Olivier Flückiger <olivf@chromium.org>
    Reviewed-by: Simon Zünd <szuend@chromium.org>
    Commit-Queue: Olivier Flückiger <olivf@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#109086}

Refs: v8/v8@b36cf9f
Signed-off-by: deepak1556 <hop2deep@gmail.com>
PR-URL: #65397
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Sep 4, 2026
@deepak1556
deepak1556 deleted the robo/enable_js_in_api_interrupt_check branch September 7, 2026 04:00
@aduh95 aduh95 added the backport-requested-v26.x PRs awaiting manual backport to the v26.x-staging branch. label Sep 7, 2026
@aduh95

aduh95 commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

This doesn't land cleanly on v26.x-staging, and would require a manual backport if we want it on that release line

@aduh95

aduh95 commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

This also breaks node-test-commit-v8-linux (which apparently was not run on this PR): https://ci.nodejs.org/job/node-test-commit-v8-linux/7346/

=== debugger/debug/futex-reentrant-wait ===
--- stdout ---
test/mjsunit/mjsunit.js:418: Failure: expected <not null> found <null>

Stack: MjsUnitAssertionError
    at assertNotNull test/mjsunit/mjsunit.js 583:7
    at               test/debugger/debug/futex-reentrant-wait.js 34:1
    throw new MjsUnitAssertionError(
    ^
MjsUnitAssertionError
    at assertNotNull test/mjsunit/mjsunit.js 583:7
    at               test/debugger/debug/futex-reentrant-wait.js 34:1
Command: out.gn/x64.release/d8 --test test/mjsunit/mjsunit.js test/debugger/test-api.js test/debugger/debug/futex-reentrant-wait.js --random-seed=-643241798 --nohard-abort --allow-natives-syntax --enable-inspector --allow-natives-syntax

===
=== 1 tests failed
===

I've opened a revert in #65907

@aduh95 aduh95 added dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. dont-land-on-v24.x PRs that should not land on the v24.x-staging branch and should not be released in v24.x. dont-land-on-v26.x PRs that should not land on the v26.x-staging branch and should not be released in v26.x. and removed author ready PRs with CI started, the required approvals, and no outstanding review comments. backport-requested-v26.x PRs awaiting manual backport to the v26.x-staging branch. labels Sep 8, 2026
@codebytere

Copy link
Copy Markdown
Member

cc @deepak1556 👆

@deepak1556

deepak1556 commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

Sorry for the breakage, the newly added test needs additional backport of https://chromium-review.googlesource.com/c/v8/v8/+/8086043 in this branch of V8

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

Labels

commit-queue-rebase PRs the Commit Queue should land as multiple self-contained commits. dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. dont-land-on-v24.x PRs that should not land on the v24.x-staging branch and should not be released in v24.x. dont-land-on-v26.x PRs that should not land on the v26.x-staging branch and should not be released in v26.x. lib / src Issues and PRs involving general changes in the lib/ or src/ directories.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants