From 7af968abd2a33a9dfa4ff9df71403750e1f6c9b8 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 24 Jun 2016 05:50:00 +0200 Subject: [PATCH 1/5] vm: don't print out arrow message for custom error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In `AppendExceptionLine()`, which is used both by the `vm` module and the uncaught exception handler, don’t print anything to stderr when called from the `vm` module, even if the thrown object is not a native error instance. Fixes: https://github.com/nodejs/node/issues/7397 --- src/node.cc | 10 ++++++---- src/node_internals.h | 3 ++- test/message/vm_caught_custom_runtime_error.js | 17 +++++++++++++++++ test/message/vm_caught_custom_runtime_error.out | 2 ++ 4 files changed, 27 insertions(+), 5 deletions(-) create mode 100644 test/message/vm_caught_custom_runtime_error.js create mode 100644 test/message/vm_caught_custom_runtime_error.out diff --git a/src/node.cc b/src/node.cc index b6fa4606f5fae0..cf316a5bc3dfa6 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1423,7 +1423,8 @@ bool IsExceptionDecorated(Environment* env, Local er) { void AppendExceptionLine(Environment* env, Local er, - Local message) { + Local message, + bool handlingFatalError) { if (message.IsEmpty()) return; @@ -1510,7 +1511,8 @@ void AppendExceptionLine(Environment* env, Local arrow_str = String::NewFromUtf8(env->isolate(), arrow); - if (!arrow_str.IsEmpty() && !err_obj.IsEmpty() && err_obj->IsNativeError()) { + if (!arrow_str.IsEmpty() && !err_obj.IsEmpty() && + (!handlingFatalError || err_obj->IsNativeError())) { err_obj->SetPrivate( env->context(), env->arrow_message_private_symbol(), @@ -1518,7 +1520,7 @@ void AppendExceptionLine(Environment* env, return; } - // Allocation failed, just print it out. + // Allocation failed when called from ReportException(), just print it out. if (env->printed_error()) return; env->set_printed_error(true); @@ -1532,7 +1534,7 @@ static void ReportException(Environment* env, Local message) { HandleScope scope(env->isolate()); - AppendExceptionLine(env, er, message); + AppendExceptionLine(env, er, message, true); Local trace_value; Local arrow; diff --git a/src/node_internals.h b/src/node_internals.h index b92c19734f99c9..acf2110116e179 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -139,7 +139,8 @@ bool IsExceptionDecorated(Environment* env, v8::Local er); void AppendExceptionLine(Environment* env, v8::Local er, - v8::Local message); + v8::Local message, + bool handlingFatalError = false); NO_RETURN void FatalError(const char* location, const char* message); diff --git a/test/message/vm_caught_custom_runtime_error.js b/test/message/vm_caught_custom_runtime_error.js new file mode 100644 index 00000000000000..c8621f65282354 --- /dev/null +++ b/test/message/vm_caught_custom_runtime_error.js @@ -0,0 +1,17 @@ +'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('end'); diff --git a/test/message/vm_caught_custom_runtime_error.out b/test/message/vm_caught_custom_runtime_error.out new file mode 100644 index 00000000000000..886c14b77eb802 --- /dev/null +++ b/test/message/vm_caught_custom_runtime_error.out @@ -0,0 +1,2 @@ +beginning +end From 06a4039226ed546052b03027b85a1695dfcd9819 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 24 Jun 2016 11:31:03 +0200 Subject: [PATCH 2/5] [squash] add console.error() statement to catch block --- test/message/vm_caught_custom_runtime_error.js | 1 + test/message/vm_caught_custom_runtime_error.out | 1 + 2 files changed, 2 insertions(+) diff --git a/test/message/vm_caught_custom_runtime_error.js b/test/message/vm_caught_custom_runtime_error.js index c8621f65282354..062b6d9dc8217a 100644 --- a/test/message/vm_caught_custom_runtime_error.js +++ b/test/message/vm_caught_custom_runtime_error.js @@ -12,6 +12,7 @@ try { message: 'This is a custom message' })`, { filename: 'test.vm' }); } catch (e) { + console.error('received error', e.name); } console.error('end'); diff --git a/test/message/vm_caught_custom_runtime_error.out b/test/message/vm_caught_custom_runtime_error.out index 886c14b77eb802..9aa1e6c6480e3b 100644 --- a/test/message/vm_caught_custom_runtime_error.out +++ b/test/message/vm_caught_custom_runtime_error.out @@ -1,2 +1,3 @@ beginning +received error MyCustomError end From 4f2785d5f907bbdde39c94c43aceca9ea92942dc Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 24 Jun 2016 16:29:12 +0200 Subject: [PATCH 3/5] [squash] use enum to indicate AppendExceptionLine mode --- src/node.cc | 8 ++++---- src/node_contextify.cc | 2 +- src/node_internals.h | 3 ++- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/node.cc b/src/node.cc index cf316a5bc3dfa6..2c7103d022857d 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1424,7 +1424,7 @@ bool IsExceptionDecorated(Environment* env, Local er) { void AppendExceptionLine(Environment* env, Local er, Local message, - bool handlingFatalError) { + enum ErrorHandlingMode mode) { if (message.IsEmpty()) return; @@ -1511,8 +1511,8 @@ void AppendExceptionLine(Environment* env, Local arrow_str = String::NewFromUtf8(env->isolate(), arrow); - if (!arrow_str.IsEmpty() && !err_obj.IsEmpty() && - (!handlingFatalError || err_obj->IsNativeError())) { + const bool can_set_arrow = !arrow_str.IsEmpty() && !err_obj.IsEmpty(); + if (can_set_arrow && (mode != FATAL_ERROR || err_obj->IsNativeError())) { err_obj->SetPrivate( env->context(), env->arrow_message_private_symbol(), @@ -1534,7 +1534,7 @@ static void ReportException(Environment* env, Local message) { HandleScope scope(env->isolate()); - AppendExceptionLine(env, er, message, true); + AppendExceptionLine(env, er, message, FATAL_ERROR); Local trace_value; Local arrow; diff --git a/src/node_contextify.cc b/src/node_contextify.cc index a4a769359412ef..4b72658c35cbf8 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -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 stack = err_obj->Get(env->stack_string()); auto maybe_value = err_obj->GetPrivate( diff --git a/src/node_internals.h b/src/node_internals.h index acf2110116e179..1420e2edb3cbe0 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -137,10 +137,11 @@ constexpr size_t arraysize(const T(&)[N]) { return N; } bool IsExceptionDecorated(Environment* env, v8::Local er); +enum ErrorHandlingMode { FATAL_ERROR, CONTEXTIFY_ERROR }; void AppendExceptionLine(Environment* env, v8::Local er, v8::Local message, - bool handlingFatalError = false); + enum ErrorHandlingMode mode); NO_RETURN void FatalError(const char* location, const char* message); From 1d83e93f9dbdb84292700b6dbba22a4570e63e8e Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 24 Jun 2016 17:04:34 +0200 Subject: [PATCH 4/5] [squash] invert and explain arrow-printing logic in AppendExceptionLine() --- src/node.cc | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/src/node.cc b/src/node.cc index 2c7103d022857d..4aba6d27364df4 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1512,20 +1512,25 @@ void AppendExceptionLine(Environment* env, Local arrow_str = String::NewFromUtf8(env->isolate(), arrow); const bool can_set_arrow = !arrow_str.IsEmpty() && !err_obj.IsEmpty(); - if (can_set_arrow && (mode != FATAL_ERROR || err_obj->IsNativeError())) { - err_obj->SetPrivate( - env->context(), - env->arrow_message_private_symbol(), - arrow_str); + // 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 when called from ReportException(), 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); } From 3d9845b8d19f87f96c791e7569a4334c363975ef Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 24 Jun 2016 17:24:51 +0200 Subject: [PATCH 5/5] [squash] address review comments --- src/node.cc | 8 ++++---- test/message/vm_caught_custom_runtime_error.js | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/node.cc b/src/node.cc index 4aba6d27364df4..fd8b8bb4cbafc1 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1527,10 +1527,10 @@ void AppendExceptionLine(Environment* env, return; } - err_obj->SetPrivate( - env->context(), - env->arrow_message_private_symbol(), - arrow_str); + CHECK(err_obj->SetPrivate( + env->context(), + env->arrow_message_private_symbol(), + arrow_str).FromMaybe(false)); } diff --git a/test/message/vm_caught_custom_runtime_error.js b/test/message/vm_caught_custom_runtime_error.js index 062b6d9dc8217a..237e8e3a105436 100644 --- a/test/message/vm_caught_custom_runtime_error.js +++ b/test/message/vm_caught_custom_runtime_error.js @@ -5,7 +5,7 @@ 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. +// vm.runInThisContext() should not print out anything to stderr by itself. try { vm.runInThisContext(`throw ({ name: 'MyCustomError',