Skip to content

Fix REPL crash when V8 emits warnings during preview mode (Issue #63473)#63491

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
DivyanshuX9:fix/issue-63473-repl-asm-warning
May 29, 2026
Merged

Fix REPL crash when V8 emits warnings during preview mode (Issue #63473)#63491
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
DivyanshuX9:fix/issue-63473-repl-asm-warning

Conversation

@DivyanshuX9
Copy link
Copy Markdown
Contributor

@DivyanshuX9 DivyanshuX9 commented May 22, 2026

Fix: Handle V8 Warnings in DisallowJavascriptExecutionScope


The Problem

When the Node.js REPL runs with preview mode enabled, certain code (such as
asm.js modules) causes V8 to emit deprecation warnings inside a
DisallowJavascriptExecutionScope : a V8 context where JavaScript execution
is temporarily forbidden.

Node's warning handler (PerIsolateMessageListener) was unconditionally
calling ProcessEmitWarningGeneric, which invokes process.emit() -> a JS
call which is inside this forbidden scope, causing a fatal crash:

Fatal error in , line 0

Invoke in DisallowJavascriptExecutionScope


What Changed

Modified PerIsolateMessageListener in src/node_errors.cc to check
can_call_into_js() before attempting to emit warnings.

When JavaScript cannot be called (i.e. we are inside a forbidden scope),
the warning is deferred via env->SetImmediate() until after the scope
exits at which point it is safe to call JavaScript again.

Files changed:

  • src/node_errors.cc -> added can_call_into_js() guard with deferred emission
  • test/parallel/test-repl-asm-warning-no-crash.js : new regression test

What Is Preserved

The fix does not degrade warning quality in any case:

Property Status
Full (node:PID) [DEPXXXX] DeprecationWarning: format Preserved
--redirect-warnings file routing Preserved
All existing warning behaviour in normal (non-forbidden) scopes Unchanged
No new allocations or memory leak risk Lambda captures string by value

Approach

Instead of falling back to raw fprintf(stderr, ...) (which loses structured
formatting and --redirect-warnings support), this fix uses env->SetImmediate()
to queue the warning for the next safe event loop iteration.

This is an established Node.js pattern for deferring work that requires
JavaScript execution. The lambda captures the warning string by value,
ensuring it remains valid until the callback fires.

// Before (crashes inside DisallowJavascriptExecutionScope)
ProcessEmitWarningGeneric(env, message, ...);

// After (defers safely when JS is forbidden)
if (!env->can_call_into_js()) {
  std::string deferred_msg = message;
  env->SetImmediate([deferred_msg](Environment* env) {
    ProcessEmitWarningGeneric(env, deferred_msg, ...);
  });
  return;
}
ProcessEmitWarningGeneric(env, message, ...);

How to Test

Run the regression test in isolation:

node test/parallel/test-repl-asm-warning-no-crash.js

Or as part of the test suite:

npm test -- test/parallel/test-repl-asm-warning-no-crash.js

Regression Test

test/parallel/test-repl-asm-warning-no-crash.js reproduces the issue by:

  1. Starting a Node.js REPL with usePreview: true (triggers the inspector path)
  2. Sending the asm.js reproduction case from V8 warnings crash process within DisallowJavascriptExecutionScope #63473
  3. Asserting the process exits cleanly with exit code 0
  4. Asserting no FATAL ERROR appears in stderr

Checklist

  • Regression test added
  • No breaking changes to existing behaviour or public API
  • --redirect-warnings routing preserved
  • No changes outside the two files listed above
  • Commit message follows Node.js format
  • No trailing whitespace or formatting changes to unrelated code

Fixes: #63473

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels May 22, 2026
@DivyanshuX9
Copy link
Copy Markdown
Contributor Author

@Renegade334 could you please review this fix?

Copy link
Copy Markdown
Member

@Renegade334 Renegade334 left a comment

Choose a reason for hiding this comment

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

Both this and #63486 seem to have borrowed from each other, but the core concept seems to have originated here, so I'm going to aim to get this one merged.

You need to fix your commit authorship information, and add a Signed-off-by: trailer as per

6. Your commit must contain the `Signed-off-by` line with your name and email
address as an acknowledgement that you agree to the [Developer Certificate of Origin][].
Bot generated commits are exempt from this requirement. If a commit has
multiple authors, the `Signed-off-by` line should be added for each author;
and at least one should match the author information in the commit metadata.
This rule does not apply to dependency updates (e.g. cherry-picks), release
commits, or backport commits.

Comment thread src/node_errors.cc Outdated
Comment thread src/node_errors.cc Outdated
Comment thread test/parallel/test-repl-asm-warning-no-crash.js Outdated
@DivyanshuX9 DivyanshuX9 force-pushed the fix/issue-63473-repl-asm-warning branch from 0d433ed to c1c87a7 Compare May 24, 2026 09:30
@DivyanshuX9
Copy link
Copy Markdown
Contributor Author

Removed the obsolete REPL asm warning regression test and updated node_errors.cc so deferred warnings capture warning directly in SetImmediate instead of copying to warning_str. Also added the Signed-off-by trailer and pushed the rewritten branch history.

@Renegade334 please check again

@DivyanshuX9 DivyanshuX9 requested a review from Renegade334 May 24, 2026 09:33
@DivyanshuX9 DivyanshuX9 force-pushed the fix/issue-63473-repl-asm-warning branch from c1c87a7 to 99b20d4 Compare May 24, 2026 09:41
Copy link
Copy Markdown
Member

@Renegade334 Renegade334 left a comment

Choose a reason for hiding this comment

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

Thanks for the changes.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 24, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.61%. Comparing base (3553a34) to head (6a9412f).
⚠️ Report is 88 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #63491      +/-   ##
==========================================
+ Coverage   90.13%   90.61%   +0.47%     
==========================================
  Files         718      730      +12     
  Lines      227914   236123    +8209     
  Branches    42811    46356    +3545     
==========================================
+ Hits       205435   213957    +8522     
+ Misses      14248    13932     -316     
- Partials     8231     8234       +3     
Files with missing lines Coverage Δ
src/node_errors.cc 64.00% <100.00%> (-0.18%) ⬇️

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

@DivyanshuX9
Copy link
Copy Markdown
Contributor Author

DivyanshuX9 commented May 24, 2026

should i make my PR message from
"Fixes: #62473"

to

"#63473"

to fix the one failing test , do i have to get review again or it will let it slide???

edit: its not letting me give the full link :( what to do

@DivyanshuX9

This comment was marked as off-topic.

@Renegade334
Copy link
Copy Markdown
Member

Please don't ping collaborators unnecessarily.

The easiest thing to do is to remove the Fixes: trailer from your commit entirely. It will be added by the tooling based on the PR description.

Defer non-critical warnings to the next event loop iteration when
can_call_into_js() returns false. This prevents crashes when V8
emits warnings during REPL preview evaluation or other contexts
where JavaScript execution is temporarily forbidden.

When a warning is emitted inside DisallowJavascriptExecutionScope,
ProcessEmitWarningGeneric cannot be called immediately. Instead,
use env->SetImmediate() to queue the warning emission for after
the scope exits. This preserves full warning formatting, deprecation
codes, and --redirect-warnings routing.

Signed-off-by: Divyanshu Sharma <Divyanshu88999@gmail.com>
@DivyanshuX9 DivyanshuX9 force-pushed the fix/issue-63473-repl-asm-warning branch from 99b20d4 to 6a9412f Compare May 24, 2026 16:04
@Renegade334 Renegade334 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels May 25, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 25, 2026
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

Copilot AI review requested due to automatic review settings May 29, 2026 14:55
@Renegade334 Renegade334 force-pushed the fix/issue-63473-repl-asm-warning branch from 7b20b8a to 6a9412f Compare May 29, 2026 15:02
@Renegade334
Copy link
Copy Markdown
Member

Please don't make unnecessary commits once the PR is in its final stages, it invalidates the existing CI.

In addition, merge commits aren't compatible with the tooling (https://github.com/nodejs/node/blob/main/doc/contributing/pull-requests.md#step-5-rebase). I have reverted to the branch state prior to the merge.

The CI was blocking on nodejs/build#4345 which has only just been resolved.

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

@Renegade334 Renegade334 added the commit-queue Add this label to land a pull request using GitHub Actions. label May 29, 2026
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels May 29, 2026
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/63491
✔  Done loading data for nodejs/node/pull/63491
----------------------------------- PR info ------------------------------------
Title      Fix REPL crash when V8 emits warnings during preview mode (Issue #63473) (#63491)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     DivyanshuX9:fix/issue-63473-repl-asm-warning -> nodejs:main
Labels     c++, author ready, needs-ci
Commits    1
 - errors: handle V8 warnings in DisallowJavascriptExecutionScope
Committers 1
 - Divyanshu Sharma <Divyanshu88999@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/63491
Fixes: https://github.com/nodejs/node/issues/63473
Reviewed-By: René <contact.9a5d6388@renegade334.me.uk>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/63491
Fixes: https://github.com/nodejs/node/issues/63473
Reviewed-By: René <contact.9a5d6388@renegade334.me.uk>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - errors: handle V8 warnings in DisallowJavascriptExecutionScope
   ℹ  This PR was created on Fri, 22 May 2026 17:18:29 GMT
   ✔  Approvals: 3
   ✔  - René (@Renegade334): https://github.com/nodejs/node/pull/63491#pullrequestreview-4354330688
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/63491#pullrequestreview-4351460626
   ✔  - Anna Henningsen (@addaleax): https://github.com/nodejs/node/pull/63491#pullrequestreview-4362601541
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2026-05-29T15:07:05Z: https://ci.nodejs.org/job/node-test-pull-request/73827/
- Querying data for job/node-test-pull-request/73827/
✔  Build data downloaded
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/26653249247

@Renegade334 Renegade334 added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels May 29, 2026
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 29, 2026
@nodejs-github-bot nodejs-github-bot merged commit 31965d6 into nodejs:main May 29, 2026
127 of 131 checks passed
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

Landed in 31965d6

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

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

V8 warnings crash process within DisallowJavascriptExecutionScope

5 participants