Skip to content

Commit e7ac2fc

Browse files
mscdexmscdex
committed
module: always decorate thrown errors
This provides more information when encountering a syntax or similar error when executing a file with require().
1 parent 0b43c08 commit e7ac2fc

File tree

7 files changed

+94
-13
lines changed

7 files changed

+94
-13
lines changed

‎lib/internal/util.js‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ const binding = process.binding('util');
44
const prefix = `(${process.release.name}:${process.pid}) `;
55

66
exports.getHiddenValue = binding.getHiddenValue;
7+
exports.setHiddenValue = binding.setHiddenValue;
78

89
// All the internal deprecations have to use this function only, as this will
910
// prepend the prefix to the actual message.
@@ -76,13 +77,16 @@ exports._deprecate = function(fn, msg) {
7677
};
7778

7879
exports.decorateErrorStack = function decorateErrorStack(err) {
79-
if (!(exports.isError(err) && err.stack))
80+
if (!(exports.isError(err) && err.stack) ||
81+
exports.getHiddenValue(err, 'node:decorated') === true)
8082
return;
8183

82-
const arrow = exports.getHiddenValue(err, 'arrowMessage');
84+
const arrow = exports.getHiddenValue(err, 'node:arrowMessage');
8385

84-
if (arrow)
86+
if (arrow) {
8587
err.stack = arrow + err.stack;
88+
exports.setHiddenValue(err, 'node:decorated', true);
89+
}
8690
};
8791

8892
exports.isError = function isError(e) {

‎lib/module.js‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,16 @@ function hasOwnProperty(obj, prop) {
2323
}
2424

2525

26+
function tryWrapper(wrapper, opts) {
27+
try {
28+
return runInThisContext(wrapper, opts);
29+
} catch (e) {
30+
internalUtil.decorateErrorStack(e);
31+
throw e;
32+
}
33+
}
34+
35+
2636
function Module(id, parent) {
2737
this.id = id;
2838
this.exports = {};
@@ -371,8 +381,8 @@ Module.prototype._compile = function(content, filename) {
371381
// create wrapper function
372382
var wrapper = Module.wrap(content);
373383

374-
var compiledWrapper = runInThisContext(wrapper,
375-
{ filename: filename, lineOffset: 0 });
384+
var compiledWrapper = tryWrapper(wrapper,
385+
{ filename: filename, lineOffset: 0 });
376386
if (global.v8debug) {
377387
if (!resolvedArgv) {
378388
// we enter the repl if we're not given a filename argument.

‎src/env.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ namespace node {
4545
V(alpn_buffer_string, "alpnBuffer") \
4646
V(args_string, "args") \
4747
V(argv_string, "argv") \
48-
V(arrow_message_string, "arrowMessage") \
48+
V(arrow_message_string, "node:arrowMessage") \
4949
V(async, "async") \
5050
V(async_queue_string, "_asyncQueue") \
5151
V(atime_string, "atime") \
@@ -65,6 +65,7 @@ namespace node {
6565
V(cwd_string, "cwd") \
6666
V(debug_port_string, "debugPort") \
6767
V(debug_string, "debug") \
68+
V(decorated_string, "node:decorated") \
6869
V(dest_string, "dest") \
6970
V(detached_string, "detached") \
7071
V(dev_string, "dev") \

‎src/node.cc‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1399,6 +1399,15 @@ ssize_t DecodeWrite(Isolate* isolate,
13991399
return StringBytes::Write(isolate, buf, buflen, val, encoding, nullptr);
14001400
}
14011401

1402+
bool IsExceptionDecorated(Environment* env, Local<Value> er) {
1403+
if (!er.IsEmpty() && er->IsObject()) {
1404+
Local<Object> err_obj = er.As<Object>();
1405+
Local<Value> decorated = err_obj->GetHiddenValue(env->decorated_string());
1406+
return !decorated.IsEmpty() && decorated->IsTrue();
1407+
}
1408+
return false;
1409+
}
1410+
14021411
void AppendExceptionLine(Environment* env,
14031412
Local<Value> er,
14041413
Local<Message> message) {
@@ -1508,6 +1517,7 @@ static void ReportException(Environment* env,
15081517

15091518
Local<Value> trace_value;
15101519
Local<Value> arrow;
1520+
const bool decorated = IsExceptionDecorated(env, er);
15111521

15121522
if (er->IsUndefined() || er->IsNull()) {
15131523
trace_value = Undefined(env->isolate());
@@ -1522,7 +1532,7 @@ static void ReportException(Environment* env,
15221532

15231533
// range errors have a trace member set to undefined
15241534
if (trace.length() > 0 && !trace_value->IsUndefined()) {
1525-
if (arrow.IsEmpty() || !arrow->IsString()) {
1535+
if (arrow.IsEmpty() || !arrow->IsString() || decorated) {
15261536
PrintErrorString("%s\n", *trace);
15271537
} else {
15281538
node::Utf8Value arrow_string(env->isolate(), arrow);
@@ -1554,7 +1564,7 @@ static void ReportException(Environment* env,
15541564
node::Utf8Value name_string(env->isolate(), name);
15551565
node::Utf8Value message_string(env->isolate(), message);
15561566

1557-
if (arrow.IsEmpty() || !arrow->IsString()) {
1567+
if (arrow.IsEmpty() || !arrow->IsString() || decorated) {
15581568
PrintErrorString("%s: %s\n", *name_string, *message_string);
15591569
} else {
15601570
node::Utf8Value arrow_string(env->isolate(), arrow);

‎src/node_util.cc‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,21 @@ static void GetHiddenValue(const FunctionCallbackInfo<Value>& args) {
5252
args.GetReturnValue().Set(obj->GetHiddenValue(name));
5353
}
5454

55+
static void SetHiddenValue(const FunctionCallbackInfo<Value>& args) {
56+
Environment* env = Environment::GetCurrent(args);
57+
58+
if (!args[0]->IsObject())
59+
return env->ThrowTypeError("obj must be an object");
60+
61+
if (!args[1]->IsString())
62+
return env->ThrowTypeError("name must be a string");
63+
64+
Local<Object> obj = args[0].As<Object>();
65+
Local<String> name = args[1].As<String>();
66+
67+
args.GetReturnValue().Set(obj->SetHiddenValue(name, args[2]));
68+
}
69+
5570

5671
void Initialize(Local<Object> target,
5772
Local<Value> unused,
@@ -63,6 +78,7 @@ void Initialize(Local<Object> target,
6378
#undef V
6479

6580
env->SetMethod(target, "getHiddenValue", GetHiddenValue);
81+
env->SetMethod(target, "setHiddenValue", SetHiddenValue);
6682
}
6783

6884
} // namespace util

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

Lines changed: 26 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@
33
const common = require('../common');
44
const assert = require('assert');
55
const internalUtil = require('internal/util');
6+
const spawnSync = require('child_process').spawnSync;
7+
const path = require('path');
68

79
assert.doesNotThrow(function() {
810
internalUtil.decorateErrorStack();
@@ -17,17 +19,37 @@ internalUtil.decorateErrorStack(obj);
1719
assert.strictEqual(obj.stack, undefined);
1820

1921
// Verify that the stack is decorated when possible
22+
function checkStack(stack) {
23+
const matches = stack.match(/var foo bar;/g);
24+
assert.strictEqual(Array.isArray(matches), true);
25+
assert.strictEqual(matches.length, 1);
26+
}
2027
let err;
28+
const badSyntaxPath =
29+
path.resolve(__dirname, '..', 'fixtures', 'syntax', 'bad_syntax')
30+
.replace(/\\/g, '\\\\');
2131

2232
try {
23-
require('../fixtures/syntax/bad_syntax');
33+
require(badSyntaxPath);
2434
} catch (e) {
2535
err = e;
26-
assert(!/var foo bar;/.test(err.stack));
27-
internalUtil.decorateErrorStack(err);
2836
}
2937

30-
assert(/var foo bar;/.test(err.stack));
38+
assert(typeof err, 'object');
39+
checkStack(err.stack);
40+
41+
// Verify that the stack is only decorated once
42+
internalUtil.decorateErrorStack(err);
43+
internalUtil.decorateErrorStack(err);
44+
checkStack(err.stack);
45+
46+
// Verify that the stack is only decorated once for uncaught exceptions
47+
const args = [
48+
'-e',
49+
`require('${badSyntaxPath}')`
50+
];
51+
const result = spawnSync(process.argv[0], args, { encoding: 'utf8' });
52+
checkStack(result.stderr);
3153

3254
// Verify that the stack is unchanged when there is no arrow message
3355
err = new Error('foo');

‎test/parallel/test-util-internal.js‎

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,12 @@ function getHiddenValue(obj, name) {
1212
};
1313
}
1414

15+
function setHiddenValue(obj, name, val) {
16+
return function() {
17+
internalUtil.setHiddenValue(obj, name, val);
18+
};
19+
}
20+
1521
assert.throws(getHiddenValue(), /obj must be an object/);
1622
assert.throws(getHiddenValue(null, 'foo'), /obj must be an object/);
1723
assert.throws(getHiddenValue(undefined, 'foo'), /obj must be an object/);
@@ -22,12 +28,24 @@ assert.throws(getHiddenValue({}, null), /name must be a string/);
2228
assert.throws(getHiddenValue({}, []), /name must be a string/);
2329
assert.deepEqual(internalUtil.getHiddenValue({}, 'foo'), undefined);
2430

31+
assert.throws(setHiddenValue(), /obj must be an object/);
32+
assert.throws(setHiddenValue(null, 'foo'), /obj must be an object/);
33+
assert.throws(setHiddenValue(undefined, 'foo'), /obj must be an object/);
34+
assert.throws(setHiddenValue('bar', 'foo'), /obj must be an object/);
35+
assert.throws(setHiddenValue(85, 'foo'), /obj must be an object/);
36+
assert.throws(setHiddenValue({}), /name must be a string/);
37+
assert.throws(setHiddenValue({}, null), /name must be a string/);
38+
assert.throws(setHiddenValue({}, []), /name must be a string/);
39+
const obj = {};
40+
assert.strictEqual(internalUtil.setHiddenValue(obj, 'foo', 'bar'), true);
41+
assert.strictEqual(internalUtil.getHiddenValue(obj, 'foo'), 'bar');
42+
2543
let arrowMessage;
2644

2745
try {
2846
require('../fixtures/syntax/bad_syntax');
2947
} catch (err) {
30-
arrowMessage = internalUtil.getHiddenValue(err, 'arrowMessage');
48+
arrowMessage = internalUtil.getHiddenValue(err, 'node:arrowMessage');
3149
}
3250

3351
assert(/bad_syntax\.js:1/.test(arrowMessage));

0 commit comments

Comments
 (0)