Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

module: always decorate thrown errors #4287

Closed
wants to merge 1 commit into from
Closed

module: always decorate thrown errors #4287

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 7 additions & 3 deletions lib/internal/util.js
View file
Open in desktop
Expand Up @@ -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.
Expand Down Expand Up @@ -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) {
Expand Down
14 changes: 12 additions & 2 deletions lib/module.js
View file
Open in desktop
Expand Up @@ -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 = {};
Expand Down Expand Up @@ -371,8 +381,8 @@ Module.prototype._compile = function(content, filename) {
// create wrapper function
var wrapper = Module.wrap(content);

var compiledWrapper = runInThisContext(wrapper,
{ filename: filename, lineOffset: 0 });
var compiledWrapper = tryWrapper(wrapper,
{ filename: filename, lineOffset: 0 });
if (global.v8debug) {
if (!resolvedArgv) {
// we enter the repl if we're not given a filename argument.
Expand Down
3 changes: 2 additions & 1 deletion src/env.h
View file
Open in desktop
Expand Up @@ -45,7 +45,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") \
Expand All @@ -65,6 +65,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") \
Expand Down
14 changes: 12 additions & 2 deletions src/node.cc
View file
Open in desktop
Expand Up @@ -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) {
Expand Down Expand Up @@ -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());
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
16 changes: 16 additions & 0 deletions src/node_util.cc
View file
Open in desktop
Expand Up @@ -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,
Expand All @@ -63,6 +78,7 @@ void Initialize(Local<Object> target,
#undef V

env->SetMethod(target, "getHiddenValue", GetHiddenValue);
env->SetMethod(target, "setHiddenValue", SetHiddenValue);
}

} // namespace util
Expand Down
30 changes: 26 additions & 4 deletions test/parallel/test-util-decorate-error-stack.js
View file
Open in desktop
Expand Up @@ -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();
Expand All @@ -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');
Expand Down
20 changes: 19 additions & 1 deletion test/parallel/test-util-internal.js
View file
Open in desktop
Expand Up @@ -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/);
Expand All @@ -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));
Expand Down