src: attach error to stack on displayErrors

The vm module's displayErrors option attaches error arrow
messages as a hidden property. Later, core JavaScript code
can optionally decorate the error stack with the arrow message.
However, when user code catches an error, it has no way to
access the arrow message. This commit changes the behavior of
displayErrors to mean "decorate the error stack if an error
occurs."

Fixes: nodejs#4835
PR-URL: nodejs#4874
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Author: cjihrig

doc/api/vm.markdown

@@ -32,10 +32,10 @@ The options when creating a script are:
   displayed in stack traces
 - `columnOffset`: allows you to add an offset to the column number that is
   displayed in stack traces
-- `displayErrors`: whether or not to print any errors to stderr, with the
-  line of code that caused them highlighted, before throwing an exception.
-  Applies only to syntax errors compiling the code; errors while running the
-  code are controlled by the options to the script's methods.
+- `displayErrors`: if `true`, on error, attach the line of code that caused
+  the error to the stack trace. Applies only to syntax errors compiling the
+  code; errors while running the code are controlled by the options to the
+  script's methods.
 - `timeout`: a number of milliseconds to execute `code` before terminating
   execution. If execution is terminated, an [`Error`][] will be thrown.
 - `cachedData`: an optional `Buffer` with V8's code cache data for the supplied
@@ -150,10 +150,10 @@ The options for running a script are:
   displayed in stack traces
 - `columnOffset`: allows you to add an offset to the column number that is
   displayed in stack traces
-- `displayErrors`: whether or not to print any errors to stderr, with the
-  line of code that caused them highlighted, before throwing an exception.
-  Applies only to runtime errors executing the code; it is impossible to create
-  a `Script` instance with syntax errors, as the constructor will throw.
+- `displayErrors`: if `true`, on error, attach the line of code that caused
+  the error to the stack trace. Applies only to runtime errors executing the
+  code; it is impossible to create a `Script` instance with syntax errors, as
+  the constructor will throw.
 - `timeout`: a number of milliseconds to execute the script before terminating
   execution. If execution is terminated, an [`Error`][] will be thrown.
 
@@ -290,10 +290,10 @@ e.g. `(0,eval)('code')`. However, it also has the following additional options:
   displayed in stack traces
 - `columnOffset`: allows you to add an offset to the column number that is
   displayed in stack traces
-- `displayErrors`: whether or not to print any errors to stderr, with the
-  line of code that caused them highlighted, before throwing an exception.
-  Will capture both syntax errors from compiling `code` and runtime errors
-  thrown by executing the compiled code. Defaults to `true`.
+- `displayErrors`: if `true`, on error, attach the line of code that caused
+  the error to the stack trace. Will capture both syntax errors from compiling
+  `code` and runtime errors thrown by executing the compiled code. Defaults to
+  `true`.
 - `timeout`: a number of milliseconds to execute `code` before terminating
   execution. If execution is terminated, an [`Error`][] will be thrown.
 

lib/module.js

@@ -23,16 +23,6 @@ function hasOwnProperty(obj, prop) {
 }
 
 
-function tryWrapper(wrapper, opts) {
-  try {
-    return runInThisContext(wrapper, opts);
-  } catch (e) {
-    internalUtil.decorateErrorStack(e);
-    throw e;
-  }
-}
-
-
 function stat(filename) {
   filename = path._makeLong(filename);
   const cache = stat.cache;
@@ -394,8 +384,12 @@ Module.prototype._compile = function(content, filename) {
   // create wrapper function
   var wrapper = Module.wrap(content);
 
-  var compiledWrapper = tryWrapper(wrapper,
-                                   { filename: filename, lineOffset: 0 });
+  var compiledWrapper = runInThisContext(wrapper, {
+    filename: filename,
+    lineOffset: 0,
+    displayErrors: true
+  });
+
   if (global.v8debug) {
     if (!resolvedArgv) {
       // we enter the repl if we're not given a filename argument.

src/node.js

@@ -605,7 +605,7 @@
              'global.require = require;\n' +
              'return require("vm").runInThisContext(' +
              JSON.stringify(body) + ', { filename: ' +
-             JSON.stringify(name) + ' });\n';
+             JSON.stringify(name) + ', displayErrors: true });\n';
     // Defer evaluation for a tick.  This is a workaround for deferred
     // events not firing when evaluating scripts from the command line,
     // see https://github.com/nodejs/node/issues/1600.
@@ -988,7 +988,8 @@
 
     var fn = runInThisContext(source, {
       filename: this.filename,
-      lineOffset: 0
+      lineOffset: 0,
+      displayErrors: true
     });
     fn(this.exports, NativeModule.require, this, this.filename);
 

src/node_contextify.cc

@@ -541,7 +541,7 @@ class ContextifyScript : public BaseObject {
 
     if (v8_script.IsEmpty()) {
       if (display_errors) {
-        AppendExceptionLine(env, try_catch.Exception(), try_catch.Message());
+        DecorateErrorStack(env, try_catch);
       }
       try_catch.ReThrow();
       return;
@@ -640,6 +640,30 @@ class ContextifyScript : public BaseObject {
     }
   }
 
+  static void DecorateErrorStack(Environment* env, const TryCatch& try_catch) {
+    Local<Value> exception = try_catch.Exception();
+
+    if (!exception->IsObject())
+      return;
+
+    Local<Object> err_obj = exception.As<Object>();
+
+    if (IsExceptionDecorated(env, err_obj))
+      return;
+
+    AppendExceptionLine(env, exception, try_catch.Message());
+    Local<Value> stack = err_obj->Get(env->stack_string());
+    Local<Value> arrow = err_obj->GetHiddenValue(env->arrow_message_string());
+
+    if (!(stack->IsString() && arrow->IsString()))
+      return;
+
+    Local<String> decorated_stack = String::Concat(arrow.As<String>(),
+                                                   stack.As<String>());
+    err_obj->Set(env->stack_string(), decorated_stack);
+    err_obj->SetHiddenValue(env->decorated_string(), True(env->isolate()));
+  }
+
   static int64_t GetTimeoutArg(const FunctionCallbackInfo<Value>& args,
                                const int i) {
     if (args[i]->IsUndefined() || args[i]->IsString()) {
@@ -816,7 +840,7 @@ class ContextifyScript : public BaseObject {
     if (result.IsEmpty()) {
       // Error occurred during execution of the script.
       if (display_errors) {
-        AppendExceptionLine(env, try_catch.Exception(), try_catch.Message());
+        DecorateErrorStack(env, try_catch);
       }
       try_catch.ReThrow();
       return false;

src/node_internals.h

@@ -125,6 +125,8 @@ inline static int snprintf(char *buffer, size_t n, const char *format, ...) {
 # define NO_RETURN
 #endif
 
+bool IsExceptionDecorated(Environment* env, v8::Local<v8::Value> er);
+
 void AppendExceptionLine(Environment* env,
                          v8::Local<v8::Value> er,
                          v8::Local<v8::Message> message);

test/message/vm_display_syntax_error.js

@@ -4,6 +4,12 @@ var vm = require('vm');
 
 console.error('beginning');
 
+try {
+  vm.runInThisContext('var 4;', { filename: 'foo.vm', displayErrors: true });
+} catch (err) {
+  console.error(err.stack);
+}
+
 vm.runInThisContext('var 5;', { filename: 'test.vm' });
 
 console.error('end');

test/message/vm_display_syntax_error.out

@@ -1,5 +1,18 @@
 beginning
 
+foo.vm:1
+var 4;
+    ^
+SyntaxError: Unexpected number
+    at Object.exports.runInThisContext (vm.js:*)
+    at Object.<anonymous> (*test*message*vm_display_syntax_error.js:*)
+    at Module._compile (module.js:*)
+    at Object.Module._extensions..js (module.js:*)
+    at Module.load (module.js:*)
+    at Function.Module._load (module.js:*)
+    at Function.Module.runMain (module.js:*)
+    at startup (node.js:*)
+    at node.js:*
 test.vm:1
 var 5;
     ^