Fix REPL crash when V8 emits warnings during preview mode (Issue #63473)#63491
Conversation
|
@Renegade334 could you please review this fix? |
Renegade334
left a comment
There was a problem hiding this comment.
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
node/doc/contributing/pull-requests.md
Lines 202 to 208 in 985b608
0d433ed to
c1c87a7
Compare
|
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 |
c1c87a7 to
99b20d4
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 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
🚀 New features to boost your workflow:
|
This comment was marked as off-topic.
This comment was marked as off-topic.
|
Please don't ping collaborators unnecessarily. The easiest thing to do is to remove the |
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>
99b20d4 to
6a9412f
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
7b20b8a to
6a9412f
Compare
|
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. |
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/.ncuhttps://github.com/nodejs/node/actions/runs/26653249247 |
|
Landed in 31965d6 |
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 executionis temporarily forbidden.
Node's warning handler (
PerIsolateMessageListener) was unconditionallycalling
ProcessEmitWarningGeneric, which invokesprocess.emit()-> a JScall which is inside this forbidden scope, causing a fatal crash:
Fatal error in , line 0
Invoke in DisallowJavascriptExecutionScope
What Changed
Modified
PerIsolateMessageListenerinsrc/node_errors.ccto checkcan_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 scopeexits at which point it is safe to call JavaScript again.
Files changed:
src/node_errors.cc-> addedcan_call_into_js()guard with deferred emissiontest/parallel/test-repl-asm-warning-no-crash.js: new regression testWhat Is Preserved
The fix does not degrade warning quality in any case:
(node:PID) [DEPXXXX] DeprecationWarning:format--redirect-warningsfile routingApproach
Instead of falling back to raw
fprintf(stderr, ...)(which loses structuredformatting and
--redirect-warningssupport), this fix usesenv->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.
How to Test
Run the regression test in isolation:
Or as part of the test suite:
npm test -- test/parallel/test-repl-asm-warning-no-crash.jsRegression Test
test/parallel/test-repl-asm-warning-no-crash.jsreproduces the issue by:usePreview: true(triggers the inspector path)FATAL ERRORappears in stderrChecklist
--redirect-warningsrouting preservedFixes: #63473