module: always decorate thrown errors

This provides more information when encountering a syntax or similar
error when executing a file with require().

Fixes: https://github.com/nodejs/node/issues/4286
PR-URL: https://github.com/nodejs/node/pull/4287
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

lib/internal/util.js

@@ -4,6 +4,7 @@ const binding = process.binding('util');
 const prefix = `(${process.release.name}:${process.pid}) `;
 
 exports.getHiddenValue = binding.getHiddenValue;
+exports.setHiddenValue = binding.setHiddenValue;
 
 // All the internal deprecations have to use this function only, as this will
 // prepend the prefix to the actual message.
@@ -76,13 +77,16 @@ exports._deprecate = function(fn, msg) {
 };
 
 exports.decorateErrorStack = function decorateErrorStack(err) {
-  if (!(exports.isError(err) && err.stack))
+  if (!(exports.isError(err) && err.stack) ||
+      exports.getHiddenValue(err, 'node:decorated') === true)
     return;
 
-  const arrow = exports.getHiddenValue(err, 'arrowMessage');
+  const arrow = exports.getHiddenValue(err, 'node:arrowMessage');
 
-  if (arrow)
+  if (arrow) {
     err.stack = arrow + err.stack;
+    exports.setHiddenValue(err, 'node:decorated', true);
+  }
 };
 
 exports.isError = function isError(e) {

lib/module.js

@@ -23,6 +23,16 @@ function hasOwnProperty(obj, prop) {
 }
 
 
+function tryWrapper(wrapper, opts) {
+  try {
+    return runInThisContext(wrapper, opts);
+  } catch (e) {
+    internalUtil.decorateErrorStack(e);
+    throw e;
+  }
+}
+
+
 function Module(id, parent) {
   this.id = id;
   this.exports = {};
@@ -371,8 +381,8 @@ Module.prototype._compile = function(content, filename) {
   // create wrapper function
   var wrapper = Module.wrap(content);
 
-  var compiledWrapper = runInThisContext(wrapper,
+  var compiledWrapper = tryWrapper(wrapper,
-                                      { filename: filename, lineOffset: 0 });
+                                   { filename: filename, lineOffset: 0 });
   if (global.v8debug) {
     if (!resolvedArgv) {
       // we enter the repl if we're not given a filename argument.

src/env.h

@@ -51,7 +51,7 @@ namespace node {
   V(alpn_buffer_string, "alpnBuffer")                                         \
   V(args_string, "args")                                                      \
   V(argv_string, "argv")                                                      \
-  V(arrow_message_string, "arrowMessage")                                     \
+  V(arrow_message_string, "node:arrowMessage")                                \
   V(async, "async")                                                           \
   V(async_queue_string, "_asyncQueue")                                        \
   V(atime_string, "atime")                                                    \
@@ -71,6 +71,7 @@ namespace node {
   V(cwd_string, "cwd")                                                        \
   V(debug_port_string, "debugPort")                                           \
   V(debug_string, "debug")                                                    \
+  V(decorated_string, "node:decorated")                                       \
   V(dest_string, "dest")                                                      \
   V(detached_string, "detached")                                              \
   V(dev_string, "dev")                                                        \

src/node.cc

@@ -1399,6 +1399,15 @@ ssize_t DecodeWrite(Isolate* isolate,
   return StringBytes::Write(isolate, buf, buflen, val, encoding, nullptr);
 }
 
+bool IsExceptionDecorated(Environment* env, Local<Value> er) {
+  if (!er.IsEmpty() && er->IsObject()) {
+    Local<Object> err_obj = er.As<Object>();
+    Local<Value> decorated = err_obj->GetHiddenValue(env->decorated_string());
+    return !decorated.IsEmpty() && decorated->IsTrue();
+  }
+  return false;
+}
+
 void AppendExceptionLine(Environment* env,
                          Local<Value> er,
                          Local<Message> message) {
@@ -1508,6 +1517,7 @@ static void ReportException(Environment* env,
 
   Local<Value> trace_value;
   Local<Value> arrow;
+  const bool decorated = IsExceptionDecorated(env, er);
 
   if (er->IsUndefined() || er->IsNull()) {
     trace_value = Undefined(env->isolate());
@@ -1522,7 +1532,7 @@ static void ReportException(Environment* env,
 
   // range errors have a trace member set to undefined
   if (trace.length() > 0 && !trace_value->IsUndefined()) {
-    if (arrow.IsEmpty() || !arrow->IsString()) {
+    if (arrow.IsEmpty() || !arrow->IsString() || decorated) {
       PrintErrorString("%s\n", *trace);
     } else {
       node::Utf8Value arrow_string(env->isolate(), arrow);
@@ -1554,7 +1564,7 @@ static void ReportException(Environment* env,
       node::Utf8Value name_string(env->isolate(), name);
       node::Utf8Value message_string(env->isolate(), message);
 
-      if (arrow.IsEmpty() || !arrow->IsString()) {
+      if (arrow.IsEmpty() || !arrow->IsString() || decorated) {
         PrintErrorString("%s: %s\n", *name_string, *message_string);
       } else {
         node::Utf8Value arrow_string(env->isolate(), arrow);

src/node_util.cc

@@ -52,6 +52,21 @@ static void GetHiddenValue(const FunctionCallbackInfo<Value>& args) {
   args.GetReturnValue().Set(obj->GetHiddenValue(name));
 }
 
+static void SetHiddenValue(const FunctionCallbackInfo<Value>& args) {
+  Environment* env = Environment::GetCurrent(args);
+
+  if (!args[0]->IsObject())
+    return env->ThrowTypeError("obj must be an object");
+
+  if (!args[1]->IsString())
+    return env->ThrowTypeError("name must be a string");
+
+  Local<Object> obj = args[0].As<Object>();
+  Local<String> name = args[1].As<String>();
+
+  args.GetReturnValue().Set(obj->SetHiddenValue(name, args[2]));
+}
+
 
 void Initialize(Local<Object> target,
                 Local<Value> unused,
@@ -63,6 +78,7 @@ void Initialize(Local<Object> target,
 #undef V
 
   env->SetMethod(target, "getHiddenValue", GetHiddenValue);
+  env->SetMethod(target, "setHiddenValue", SetHiddenValue);
 }
 
 }  // namespace util

test/parallel/test-util-decorate-error-stack.js

@@ -3,6 +3,8 @@
 const common = require('../common');
 const assert = require('assert');
 const internalUtil = require('internal/util');
+const spawnSync = require('child_process').spawnSync;
+const path = require('path');
 
 assert.doesNotThrow(function() {
   internalUtil.decorateErrorStack();
@@ -17,17 +19,37 @@ internalUtil.decorateErrorStack(obj);
 assert.strictEqual(obj.stack, undefined);
 
 // Verify that the stack is decorated when possible
+function checkStack(stack) {
+  const matches = stack.match(/var foo bar;/g);
+  assert.strictEqual(Array.isArray(matches), true);
+  assert.strictEqual(matches.length, 1);
+}
 let err;
+const badSyntaxPath =
+    path.resolve(__dirname, '..', 'fixtures', 'syntax', 'bad_syntax')
+        .replace(/\\/g, '\\\\');
 
 try {
-  require('../fixtures/syntax/bad_syntax');
+  require(badSyntaxPath);
 } catch (e) {
   err = e;
-  assert(!/var foo bar;/.test(err.stack));
-  internalUtil.decorateErrorStack(err);
 }
 
-assert(/var foo bar;/.test(err.stack));
+assert(typeof err, 'object');
+checkStack(err.stack);
+
+// Verify that the stack is only decorated once
+internalUtil.decorateErrorStack(err);
+internalUtil.decorateErrorStack(err);
+checkStack(err.stack);
+
+// Verify that the stack is only decorated once for uncaught exceptions
+const args = [
+  '-e',
+  `require('${badSyntaxPath}')`
+];
+const result = spawnSync(process.argv[0], args, { encoding: 'utf8' });
+checkStack(result.stderr);
 
 // Verify that the stack is unchanged when there is no arrow message
 err = new Error('foo');

test/parallel/test-util-internal.js

@@ -12,6 +12,12 @@ function getHiddenValue(obj, name) {
   };
 }
 
+function setHiddenValue(obj, name, val) {
+  return function() {
+    internalUtil.setHiddenValue(obj, name, val);
+  };
+}
+
 assert.throws(getHiddenValue(), /obj must be an object/);
 assert.throws(getHiddenValue(null, 'foo'), /obj must be an object/);
 assert.throws(getHiddenValue(undefined, 'foo'), /obj must be an object/);
@@ -22,12 +28,24 @@ assert.throws(getHiddenValue({}, null), /name must be a string/);
 assert.throws(getHiddenValue({}, []), /name must be a string/);
 assert.deepEqual(internalUtil.getHiddenValue({}, 'foo'), undefined);
 
+assert.throws(setHiddenValue(), /obj must be an object/);
+assert.throws(setHiddenValue(null, 'foo'), /obj must be an object/);
+assert.throws(setHiddenValue(undefined, 'foo'), /obj must be an object/);
+assert.throws(setHiddenValue('bar', 'foo'), /obj must be an object/);
+assert.throws(setHiddenValue(85, 'foo'), /obj must be an object/);
+assert.throws(setHiddenValue({}), /name must be a string/);
+assert.throws(setHiddenValue({}, null), /name must be a string/);
+assert.throws(setHiddenValue({}, []), /name must be a string/);
+const obj = {};
+assert.strictEqual(internalUtil.setHiddenValue(obj, 'foo', 'bar'), true);
+assert.strictEqual(internalUtil.getHiddenValue(obj, 'foo'), 'bar');
+
 let arrowMessage;
 
 try {
   require('../fixtures/syntax/bad_syntax');
 } catch (err) {
-  arrowMessage = internalUtil.getHiddenValue(err, 'arrowMessage');
+  arrowMessage = internalUtil.getHiddenValue(err, 'node:arrowMessage');
 }
 
 assert(/bad_syntax\.js:1/.test(arrowMessage));