-
-
Notifications
You must be signed in to change notification settings - Fork 35.6k
Fix REPL crash when V8 emits warnings during preview mode (Issue #63473) #63491
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1064,7 +1064,18 @@ void PerIsolateMessageListener(Local<Message> message, Local<Value> error) { | |||||||||||||||||||||||
| filename, | ||||||||||||||||||||||||
| message->GetLineNumber(env->context()).FromMaybe(-1), | ||||||||||||||||||||||||
| msg); | ||||||||||||||||||||||||
| USE(ProcessEmitWarningGeneric(env, warning, "V8")); | ||||||||||||||||||||||||
| // If we're inside a DisallowJavascriptExecutionScope (e.g., REPL preview), | ||||||||||||||||||||||||
| // defer the warning to the next event loop iteration when JS execution is | ||||||||||||||||||||||||
| // allowed. This prevents crashes when V8 emits warnings during code | ||||||||||||||||||||||||
| // evaluation with throwOnSideEffect. | ||||||||||||||||||||||||
| if (!env->can_call_into_js()) { | ||||||||||||||||||||||||
| std::string warning_str = warning; | ||||||||||||||||||||||||
| env->SetImmediate([warning_str](Environment* env) { | ||||||||||||||||||||||||
| ProcessEmitWarningGeneric(env, warning_str, "V8"); | ||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||
| USE(ProcessEmitWarningGeneric(env, warning, "V8")); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
Comment on lines
+1071
to
+1078
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||||
| break; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| case Isolate::MessageErrorLevel::kMessageError: | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. asm.js support is being removed from V8, please remove this test file. We can manually test the reproduction against this PR for now. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,67 @@ | ||
| 'use strict'; | ||
|
|
||
| // Regression test for issue #63473: | ||
| // When asm.js code is evaluated in the Node.js REPL with previews enabled, | ||
| // V8 emits a deprecation warning inside a DisallowJavascriptExecutionScope. | ||
| // This test verifies that the warning is deferred and the process does not crash. | ||
|
|
||
| const common = require('../common'); | ||
| const assert = require('assert'); | ||
| const spawn = require('child_process').spawn; | ||
|
|
||
| // asm.js code that may trigger a deprecation warning in V8 | ||
| const asmCode = ` | ||
| function asm(stdlib, foreign, heap) { | ||
| "use asm"; | ||
| function f() { return 1; } | ||
| return { f: f }; | ||
| } | ||
| asm(this, null, new ArrayBuffer(1024)); | ||
| `; | ||
|
|
||
| const child = spawn(process.execPath, ['-i'], { | ||
| stdio: ['pipe', 'pipe', 'pipe'], | ||
| }); | ||
|
|
||
| let stdout = ''; | ||
| let stderr = ''; | ||
| let crashed = false; | ||
|
|
||
| child.stdout.setEncoding('utf8'); | ||
| child.stdout.on('data', (chunk) => { | ||
| stdout += chunk; | ||
| process.stdout.write(chunk); | ||
| }); | ||
|
|
||
| child.stderr.setEncoding('utf8'); | ||
| child.stderr.on('data', (chunk) => { | ||
| stderr += chunk; | ||
| process.stderr.write(chunk); | ||
| // Check for crash indicators | ||
| if (chunk.includes('FATAL ERROR') || chunk.includes('Segmentation fault')) { | ||
| crashed = true; | ||
| } | ||
| }); | ||
|
|
||
| // Feed the asm.js code to REPL via stdin | ||
| child.stdout.once('data', function() { | ||
| child.stdin.write(asmCode); | ||
| child.stdin.write('\n'); | ||
| setTimeout(() => { | ||
| child.stdin.end(); | ||
| }, 500); | ||
| }); | ||
|
|
||
| child.on('close', common.mustCall((exitCode) => { | ||
| // Most important: process should not crash (exit code 0 or normal termination) | ||
| assert.strictEqual(crashed, false, 'Process should not crash'); | ||
| // In Node.js, exit code 0 is normal, but we also allow the process to exit | ||
| // naturally without a crash (which would have exit code 0) | ||
| // The main assertion is that it doesn't crash with FATAL ERROR or segfault | ||
| if (exitCode !== 0 && exitCode !== null) { | ||
| // Note: We only fail if there's clear evidence of a crash in stderr | ||
| if (stderr.includes('FATAL ERROR') || stderr.includes('Segmentation')) { | ||
| assert.fail(`Process crashed with exit code ${exitCode}\nstderr: ${stderr}`); | ||
| } | ||
| } | ||
| })); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're copy-capturing the string anyway, there is no need to make an additional copy.