node: do not override message/stack of error

Put the `...^` arrow string to the hidden property of the object, and use it only when printing error to the stderr. Fix: #2104 PR-URL: #2108 Reviewed-By: Trevor Norris <[email protected]>
nodejs · Aug 4, 2015 · df9cdd8 · df9cdd8
1 parent 35c7938
commit df9cdd8
Show file tree

Hide file tree

Showing 4 changed files with 57 additions and 19 deletions.
diff --git a/src/env.h b/src/env.h
@@ -44,6 +44,7 @@ namespace node {
   V(address_string, "address")                                                \
   V(args_string, "args")                                                      \
   V(argv_string, "argv")                                                      \
+  V(arrow_message_string, "arrowMessage")                                     \
   V(async, "async")                                                           \
   V(async_queue_string, "_asyncQueue")                                        \
   V(atime_string, "atime")                                                    \

diff --git a/src/node.cc b/src/node.cc
@@ -1413,16 +1413,7 @@ void AppendExceptionLine(Environment* env,
   if (arrow_str.IsEmpty() || err_obj.IsEmpty() || !err_obj->IsNativeError())
     goto print;
 
-  msg = err_obj->Get(env->message_string());
-  stack = err_obj->Get(env->stack_string());
-
-  if (msg.IsEmpty() || stack.IsEmpty())
-    goto print;
-
-  err_obj->Set(env->message_string(),
-               String::Concat(arrow_str, msg->ToString(env->isolate())));
-  err_obj->Set(env->stack_string(),
-               String::Concat(arrow_str, stack->ToString(env->isolate())));
+  err_obj->SetHiddenValue(env->arrow_message_string(), arrow_str);
   return;
 
  print:
@@ -1442,17 +1433,27 @@ static void ReportException(Environment* env,
   AppendExceptionLine(env, er, message);
 
   Local<Value> trace_value;
+  Local<Value> arrow;
 
-  if (er->IsUndefined() || er->IsNull())
+  if (er->IsUndefined() || er->IsNull()) {
     trace_value = Undefined(env->isolate());
-  else
-    trace_value = er->ToObject(env->isolate())->Get(env->stack_string());
+  } else {
+    Local<Object> err_obj = er->ToObject(env->isolate());
+
+    trace_value = err_obj->Get(env->stack_string());
+    arrow = err_obj->GetHiddenValue(env->arrow_message_string());
+  }
 
   node::Utf8Value trace(env->isolate(), trace_value);
 
   // range errors have a trace member set to undefined
   if (trace.length() > 0 && !trace_value->IsUndefined()) {
-    fprintf(stderr, "%s\n", *trace);
+    if (arrow.IsEmpty() || !arrow->IsString()) {
+      fprintf(stderr, "%s\n", *trace);
+    } else {
+      node::Utf8Value arrow_string(env->isolate(), arrow);
+      fprintf(stderr, "%s\n%s\n", *arrow_string, *trace);
+    }
   } else {
     // this really only happens for RangeErrors, since they're the only
     // kind that won't have all this info in the trace, or when non-Error
@@ -1476,7 +1477,17 @@ static void ReportException(Environment* env,
     } else {
       node::Utf8Value name_string(env->isolate(), name);
       node::Utf8Value message_string(env->isolate(), message);
-      fprintf(stderr, "%s: %s\n", *name_string, *message_string);
+
+      if (arrow.IsEmpty() || !arrow->IsString()) {
+        fprintf(stderr, "%s: %s\n", *name_string, *message_string);
+      } else {
+        node::Utf8Value arrow_string(env->isolate(), arrow);
+        fprintf(stderr,
+                "%s\n%s: %s\n",
+                *arrow_string,
+                *name_string,
+                *message_string);
+      }
     }
   }
 

diff --git a/test/parallel/test-vm-syntax-error-message.js b/test/parallel/test-vm-syntax-error-message.js
@@ -0,0 +1,26 @@
+'use strict';
+var common = require('../common');
+var assert = require('assert');
+var child_process = require('child_process');
+
+var p = child_process.spawn(process.execPath, [
+  '-e',
+  'vm = require("vm");' +
+      'context = vm.createContext({});' +
+      'try { vm.runInContext("throw new Error(\'boo\')", context); } ' +
+      'catch (e) { console.log(e.message); }'
+]);
+
+p.stderr.on('data', function(data) {
+  assert(false, 'Unexpected stderr data: ' + data);
+});
+
+var output = '';
+
+p.stdout.on('data', function(data) {
+  output += data;
+});
+
+process.on('exit', function() {
+  assert.equal(output.replace(/[\r\n]+/g, ''), 'boo');
+});
diff --git a/test/sequential/test-vm-syntax-error-stderr.js b/test/sequential/test-vm-syntax-error-stderr.js
@@ -8,17 +8,17 @@ var wrong_script = path.join(common.fixturesDir, 'cert.pem');
 
 var p = child_process.spawn(process.execPath, [
   '-e',
-  'try { require(process.argv[1]); } catch (e) { console.log(e.stack); }',
+  'require(process.argv[1]);',
   wrong_script
 ]);
 
-p.stderr.on('data', function(data) {
-  assert(false, 'Unexpected stderr data: ' + data);
+p.stdout.on('data', function(data) {
+  assert(false, 'Unexpected stdout data: ' + data);
 });
 
 var output = '';
 
-p.stdout.on('data', function(data) {
+p.stderr.on('data', function(data) {
   output += data;
 });