-
-
Notifications
You must be signed in to change notification settings - Fork 34.2k
vm: don't print out arrow message for custom error #7398
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
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( | ||
|
||
| 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. | ||
|
||
| try { | ||
| vm.runInThisContext(`throw ({ | ||
| name: 'MyCustomError', | ||
| message: 'This is a custom message' | ||
| })`, { filename: 'test.vm' }); | ||
| } catch (e) { | ||
| console.error('received error', e.name); | ||
| } | ||
|
Contributor
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?
Member
Author
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
enumkeyword is not strictly necessary (but it doesn't hurt and I realize we use it in other places.)