-
Notifications
You must be signed in to change notification settings - Fork 29.1k
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
vm: don't print out arrow message for custom error #7398
Changes from 4 commits
7af968a
06a4039
4f2785d
1d83e93
3d9845b
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 |
---|---|---|
|
@@ -1423,7 +1423,8 @@ bool IsExceptionDecorated(Environment* env, Local<Value> er) { | |
|
||
void AppendExceptionLine(Environment* env, | ||
Local<Value> er, | ||
Local<Message> message) { | ||
Local<Message> message, | ||
enum ErrorHandlingMode mode) { | ||
if (message.IsEmpty()) | ||
return; | ||
|
||
|
@@ -1510,20 +1511,26 @@ void AppendExceptionLine(Environment* env, | |
|
||
Local<String> arrow_str = String::NewFromUtf8(env->isolate(), arrow); | ||
|
||
if (!arrow_str.IsEmpty() && !err_obj.IsEmpty() && err_obj->IsNativeError()) { | ||
err_obj->SetPrivate( | ||
env->context(), | ||
env->arrow_message_private_symbol(), | ||
arrow_str); | ||
const bool can_set_arrow = !arrow_str.IsEmpty() && !err_obj.IsEmpty(); | ||
// If allocating arrow_str failed, print it out. There's not much else to do. | ||
// If it's not an error, but something needs to be printed out because | ||
// it's a fatal exception, also print it out from here. | ||
// Otherwise, the arrow property will be attached to the object and handled | ||
// by the caller. | ||
if (!can_set_arrow || (mode == FATAL_ERROR && !err_obj->IsNativeError())) { | ||
if (env->printed_error()) | ||
return; | ||
env->set_printed_error(true); | ||
|
||
uv_tty_reset_mode(); | ||
PrintErrorString("\n%s", arrow); | ||
return; | ||
} | ||
|
||
// Allocation failed, just print it out. | ||
if (env->printed_error()) | ||
return; | ||
env->set_printed_error(true); | ||
uv_tty_reset_mode(); | ||
PrintErrorString("\n%s", arrow); | ||
err_obj->SetPrivate( | ||
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. Can you CHECK the return value? 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. Done |
||
env->context(), | ||
env->arrow_message_private_symbol(), | ||
arrow_str); | ||
} | ||
|
||
|
||
|
@@ -1532,7 +1539,7 @@ static void ReportException(Environment* env, | |
Local<Message> message) { | ||
HandleScope scope(env->isolate()); | ||
|
||
AppendExceptionLine(env, er, message); | ||
AppendExceptionLine(env, er, message, FATAL_ERROR); | ||
|
||
Local<Value> trace_value; | ||
Local<Value> arrow; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
'use strict'; | ||
require('../common'); | ||
const vm = require('vm'); | ||
|
||
console.error('beginning'); | ||
|
||
// Regression test for https://github.com/nodejs/node/issues/7397: | ||
// This should not print out anything to stderr due to the error being caught. | ||
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. This is the tiniest of nits, but maybe say vm.runInThisContext() should not print anything... As it currently stands, it seems like nothing should be printed at all, or the 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. @cjihrig Done, forgot to update this along with the |
||
try { | ||
vm.runInThisContext(`throw ({ | ||
name: 'MyCustomError', | ||
message: 'This is a custom message' | ||
})`, { filename: 'test.vm' }); | ||
} catch (e) { | ||
console.error('received error', e.name); | ||
} | ||
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. Should we also do another print from the catch to make sure it routes that way? 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. @Fishrock123 sounds good, done |
||
|
||
console.error('end'); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
beginning | ||
received error MyCustomError | ||
end |
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.
The
enum
keyword is not strictly necessary (but it doesn't hurt and I realize we use it in other places.)