Skip to content

[v24.x backport] src: do not enable wasm trap handler if there's not enough vmem#64338

Open
joyeecheung wants to merge 2 commits into
nodejs:v24.x-stagingfrom
joyeecheung:backport-62132-to-24
Open

[v24.x backport] src: do not enable wasm trap handler if there's not enough vmem#64338
joyeecheung wants to merge 2 commits into
nodejs:v24.x-stagingfrom
joyeecheung:backport-62132-to-24

Conversation

@joyeecheung

@joyeecheung joyeecheung commented Jul 7, 2026

Copy link
Copy Markdown
Member

This is a backport of #62132 which would help closing microsoft/vscode#251777

joyeecheung and others added 2 commits July 7, 2026 15:53
@joyeecheung
deps: V8: backport bef0d9c1bc90
5c2cf58 
Original commit message:

    [api] Add V8::GetWasmMemoryReservationSizeInBytes()

    When the system does not have enough virtual memory for the wasm
    cage, installing the trap handler would cause any code allocating
    wasm memory to throw. Therefore it's useful for the embedder to
    know when the system doesn't have enough virtual address space
    to allocate enough wasm cages and in that case, skip the
    trap handler installation so that wasm code can at least work
    (even not at the maximal performance).

    Node.js previously has a command line option
    --disable-wasm-trap-handler to fully disable trap-based bound checks,
    this new API would allow it to adapt automatically while keeping the
    optimization in the happy path, since it's not always possible for
    end users to opt-into disabling trap-based bound checks (for example,
    when a VS Code Server is loaded in a remote server for debugging).

    Refs: nodejs#62132
    Refs: microsoft/vscode#251777
    Change-Id: I345c076af2b2b47700e5716b49c3133fdf8a0981
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/7638233
    Reviewed-by: Jakob Kummerow <[email protected]>
    Reviewed-by: Michael Lippautz <[email protected]>
    Commit-Queue: Joyee Cheung <[email protected]>
    Reviewed-by: Clemens Backes <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#105702}

Refs: v8/v8@bef0d9c
Co-authored-by: Joyee Cheung <[email protected]>
PR-URL: nodejs#62132
Refs: microsoft/vscode#251777
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/7638233
Reviewed-By: Aditi Singh <[email protected]>
Signed-off-by: Joyee Cheung <[email protected]>
@joyeecheung
src: do not enable wasm trap handler if there's not enough vmem
0eeb4f5 
PR-URL: nodejs#62132
Refs: microsoft/vscode#251777
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/7638233
Reviewed-By: Aditi Singh <[email protected]>
Signed-off-by: Joyee Cheung <[email protected]>
@nodejs-github-bot

nodejs-github-bot commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Review requested:

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

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. v24.x Issues that can be reproduced on v24.x or PRs targeting the v24.x-staging branch. labels Jul 7, 2026
@codecov

codecov Bot commented Jul 7, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 94.44444% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 89.87%. Comparing base (b881658) to head (5159d50).
⚠️ Report is 371 commits behind head on v24.x-staging.

Files with missing lines Patch % Lines
src/node.cc 94.44% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@                Coverage Diff                @@
##           v24.x-staging   #64338      +/-   ##
=================================================
- Coverage          89.92%   89.87%   -0.05%     
=================================================
  Files                686      694       +8     
  Lines             208389   212085    +3696     
  Branches           40077    40686     +609     
=================================================
+ Hits              187387   190616    +3229     
- Misses             13238    13578     +340     
- Partials            7764     7891     +127
Files with missing lines Coverage Δ
src/debug_utils.h 80.00% <ø> (ø)
src/node.cc 74.87% <94.44%> (+0.43%) ⬆️

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

@nodejs-github-bot

nodejs-github-bot commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

nodejs-github-bot commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

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

@joyeecheung

joyeecheung commented Jul 13, 2026

Copy link
Copy Markdown
Member Author

CI is green. @nodejs/releasers can we include this in the next v24 release please? This would help address a class of bugs that basically manifests as "vscode remote ssh mysteriously getting stuck when connecting to a server".

@joyeecheung joyeecheung added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels Jul 17, 2026
@joyeecheung
joyeecheung force-pushed the backport-62132-to-24 branch from 0eeb4f5 to 3048bdc Compare July 17, 2026 21:34
joyeecheung added a commit to joyeecheung/node that referenced this pull request Jul 17, 2026
@joyeecheung
deps: V8: backport bef0d9c1bc90
e50ddc9 
Original commit message:

    [api] Add V8::GetWasmMemoryReservationSizeInBytes()

    When the system does not have enough virtual memory for the wasm
    cage, installing the trap handler would cause any code allocating
    wasm memory to throw. Therefore it's useful for the embedder to
    know when the system doesn't have enough virtual address space
    to allocate enough wasm cages and in that case, skip the
    trap handler installation so that wasm code can at least work
    (even not at the maximal performance).

    Node.js previously has a command line option
    --disable-wasm-trap-handler to fully disable trap-based bound checks,
    this new API would allow it to adapt automatically while keeping the
    optimization in the happy path, since it's not always possible for
    end users to opt-into disabling trap-based bound checks (for example,
    when a VS Code Server is loaded in a remote server for debugging).

    Refs: nodejs#62132
    Refs: microsoft/vscode#251777
    Change-Id: I345c076af2b2b47700e5716b49c3133fdf8a0981
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/7638233
    Reviewed-by: Jakob Kummerow <[email protected]>
    Reviewed-by: Michael Lippautz <[email protected]>
    Commit-Queue: Joyee Cheung <[email protected]>
    Reviewed-by: Clemens Backes <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#105702}

Refs: v8/v8@bef0d9c
Co-authored-by: Joyee Cheung <[email protected]>
Refs: microsoft/vscode#251777
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/7638233
Signed-off-by: Joyee Cheung <[email protected]>
PR-URL: nodejs#64338
Reviewed-By: Richard Lau <[email protected]>
joyeecheung added a commit to joyeecheung/node that referenced this pull request Jul 17, 2026
@joyeecheung
src: do not enable wasm trap handler if there's not enough vmem
3048bdc 
Refs: microsoft/vscode#251777
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/7638233
Signed-off-by: Joyee Cheung <[email protected]>
PR-URL: nodejs#64338
Reviewed-By: Richard Lau <[email protected]>
joyeecheung added a commit to joyeecheung/node that referenced this pull request Jul 17, 2026
@joyeecheung
deps: V8: backport bef0d9c1bc90
652a989 
Original commit message:

    [api] Add V8::GetWasmMemoryReservationSizeInBytes()

    When the system does not have enough virtual memory for the wasm
    cage, installing the trap handler would cause any code allocating
    wasm memory to throw. Therefore it's useful for the embedder to
    know when the system doesn't have enough virtual address space
    to allocate enough wasm cages and in that case, skip the
    trap handler installation so that wasm code can at least work
    (even not at the maximal performance).

    Node.js previously has a command line option
    --disable-wasm-trap-handler to fully disable trap-based bound checks,
    this new API would allow it to adapt automatically while keeping the
    optimization in the happy path, since it's not always possible for
    end users to opt-into disabling trap-based bound checks (for example,
    when a VS Code Server is loaded in a remote server for debugging).

    Refs: nodejs#62132
    Refs: microsoft/vscode#251777
    Change-Id: I345c076af2b2b47700e5716b49c3133fdf8a0981
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/7638233
    Reviewed-by: Jakob Kummerow <[email protected]>
    Reviewed-by: Michael Lippautz <[email protected]>
    Commit-Queue: Joyee Cheung <[email protected]>
    Reviewed-by: Clemens Backes <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#105702}

Refs: v8/v8@bef0d9c
Co-authored-by: Joyee Cheung <[email protected]>
Refs: microsoft/vscode#251777
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/7638233
Signed-off-by: Joyee Cheung <[email protected]>
PR-URL: nodejs#64338
Reviewed-By: Richard Lau <[email protected]>
joyeecheung added a commit to joyeecheung/node that referenced this pull request Jul 17, 2026
@joyeecheung
src: do not enable wasm trap handler if there's not enough vmem
5159d50 
Refs: microsoft/vscode#251777
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/7638233
Signed-off-by: Joyee Cheung <[email protected]>
PR-URL: nodejs#64338
Reviewed-By: Richard Lau <[email protected]>
@joyeecheung
joyeecheung force-pushed the backport-62132-to-24 branch from 3048bdc to 5159d50 Compare July 17, 2026 21:38
@joyeecheung

joyeecheung commented Jul 17, 2026

Copy link
Copy Markdown
Member Author

CI was green. Force pushed to sign the commits, but it seems I cannot land it myself - maybe it needs to be someone from @nodejs/releasers ?

@aduh95 aduh95 removed commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels Jul 21, 2026
@aduh95
aduh95 force-pushed the backport-62132-to-24 branch from 5159d50 to 0eeb4f5 Compare July 21, 2026 14:47
@aduh95 aduh95 changed the title [v24.x] src: do not enable wasm trap handler if there's not enough vmem [v24.x backport] src: do not enable wasm trap handler if there's not enough vmem Jul 21, 2026
aduh95 pushed a commit that referenced this pull request Jul 21, 2026
@joyeecheung @aduh95
deps: V8: backport bef0d9c1bc90
38e03df 
Original commit message:

    [api] Add V8::GetWasmMemoryReservationSizeInBytes()

    When the system does not have enough virtual memory for the wasm
    cage, installing the trap handler would cause any code allocating
    wasm memory to throw. Therefore it's useful for the embedder to
    know when the system doesn't have enough virtual address space
    to allocate enough wasm cages and in that case, skip the
    trap handler installation so that wasm code can at least work
    (even not at the maximal performance).

    Node.js previously has a command line option
    --disable-wasm-trap-handler to fully disable trap-based bound checks,
    this new API would allow it to adapt automatically while keeping the
    optimization in the happy path, since it's not always possible for
    end users to opt-into disabling trap-based bound checks (for example,
    when a VS Code Server is loaded in a remote server for debugging).

    Refs: #62132
    Refs: microsoft/vscode#251777
    Change-Id: I345c076af2b2b47700e5716b49c3133fdf8a0981
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/7638233
    Reviewed-by: Jakob Kummerow <[email protected]>
    Reviewed-by: Michael Lippautz <[email protected]>
    Commit-Queue: Joyee Cheung <[email protected]>
    Reviewed-by: Clemens Backes <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#105702}

Refs: v8/v8@bef0d9c
Co-authored-by: Joyee Cheung <[email protected]>
PR-URL: #62132
Backport-PR-URL: #64338
Refs: microsoft/vscode#251777
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/7638233
Signed-off-by: Joyee Cheung <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
aduh95 pushed a commit that referenced this pull request Jul 21, 2026
@joyeecheung @aduh95
src: do not enable wasm trap handler if there's not enough vmem
b459d10 
PR-URL: #62132
Backport-PR-URL: #64338
Refs: microsoft/vscode#251777
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/7638233
Signed-off-by: Joyee Cheung <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@richardlau richardlau richardlau approved these changes

Assignees

No one assigned

Labels

lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. v24.x Issues that can be reproduced on v24.x or PRs targeting the v24.x-staging branch.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@joyeecheung @nodejs-github-bot @richardlau @aduh95