diff options
author | Brian White <mscdex@mscdex.net> | 2015-12-15 03:55:35 -0500 |
---|---|---|
committer | Brian White <mscdex@mscdex.net> | 2015-12-19 14:15:38 -0500 |
commit | 18490d3d5af3131751791e501302bc57865337b3 (patch) | |
tree | 78cb704b8126394b9b632484ca7bde17a06914e3 | |
parent | 4b0b991bf5bbb41e204a7b98f62a8afd00faf855 (diff) | |
download | android-node-v8-18490d3d5af3131751791e501302bc57865337b3.tar.gz android-node-v8-18490d3d5af3131751791e501302bc57865337b3.tar.bz2 android-node-v8-18490d3d5af3131751791e501302bc57865337b3.zip |
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>
-rw-r--r-- | lib/internal/util.js | 10 | ||||
-rw-r--r-- | lib/module.js | 14 | ||||
-rw-r--r-- | src/env.h | 3 | ||||
-rw-r--r-- | src/node.cc | 14 | ||||
-rw-r--r-- | src/node_util.cc | 16 | ||||
-rw-r--r-- | test/parallel/test-util-decorate-error-stack.js | 30 | ||||
-rw-r--r-- | test/parallel/test-util-internal.js | 20 |
7 files changed, 94 insertions, 13 deletions
diff --git a/lib/internal/util.js b/lib/internal/util.js index 78254ceb82..21aafff218 100644 --- a/lib/internal/util.js +++ b/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) { diff --git a/lib/module.js b/lib/module.js index 8feba15b0f..82b1971e8b 100644 --- a/lib/module.js +++ b/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, - { 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. @@ -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") \ diff --git a/src/node.cc b/src/node.cc index f9797e664e..894c8f7641 100644 --- a/src/node.cc +++ b/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); diff --git a/src/node_util.cc b/src/node_util.cc index 1e0f214ae4..8475468c1f 100644 --- a/src/node_util.cc +++ b/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 diff --git a/test/parallel/test-util-decorate-error-stack.js b/test/parallel/test-util-decorate-error-stack.js index 24fee56df7..f289d52be4 100644 --- a/test/parallel/test-util-decorate-error-stack.js +++ b/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'); diff --git a/test/parallel/test-util-internal.js b/test/parallel/test-util-internal.js index ac7ff3da62..06fddb7e94 100644 --- a/test/parallel/test-util-internal.js +++ b/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)); |