Skip to content
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

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 20 additions & 13 deletions src/node.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

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.)

if (message.IsEmpty())
return;

Expand Down Expand Up @@ -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(
Copy link
Member

Choose a reason for hiding this comment

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

Can you CHECK the return value?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

env->context(),
env->arrow_message_private_symbol(),
arrow_str);
}


Expand All @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion src/node_contextify.cc
Original file line number Diff line number Diff line change
Expand Up @@ -632,7 +632,7 @@ class ContextifyScript : public BaseObject {
if (IsExceptionDecorated(env, err_obj))
return;

AppendExceptionLine(env, exception, try_catch.Message());
AppendExceptionLine(env, exception, try_catch.Message(), CONTEXTIFY_ERROR);
Local<Value> stack = err_obj->Get(env->stack_string());
auto maybe_value =
err_obj->GetPrivate(
Expand Down
4 changes: 3 additions & 1 deletion src/node_internals.h
Original file line number Diff line number Diff line change
Expand Up @@ -137,9 +137,11 @@ constexpr size_t arraysize(const T(&)[N]) { return N; }

bool IsExceptionDecorated(Environment* env, v8::Local<v8::Value> er);

enum ErrorHandlingMode { FATAL_ERROR, CONTEXTIFY_ERROR };
void AppendExceptionLine(Environment* env,
v8::Local<v8::Value> er,
v8::Local<v8::Message> message);
v8::Local<v8::Message> message,
enum ErrorHandlingMode mode);

NO_RETURN void FatalError(const char* location, const char* message);

Expand Down
18 changes: 18 additions & 0 deletions test/message/vm_caught_custom_runtime_error.js
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.
Copy link
Contributor

Choose a reason for hiding this comment

The 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 catch won't print anything either.

Copy link
Member Author

Choose a reason for hiding this comment

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

@cjihrig Done, forgot to update this along with the catch block

try {
vm.runInThisContext(`throw ({
name: 'MyCustomError',
message: 'This is a custom message'
})`, { filename: 'test.vm' });
} catch (e) {
console.error('received error', e.name);
}
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

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

@Fishrock123 sounds good, done


console.error('end');
3 changes: 3 additions & 0 deletions test/message/vm_caught_custom_runtime_error.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
beginning
received error MyCustomError
end