From dfee4e3712ac4673b5fc472a8f77ac65bdc65f87 Mon Sep 17 00:00:00 2001 From: Tristian Flanagan Date: Mon, 14 Sep 2015 18:26:59 -0400 Subject: [PATCH] module: fix column offsets in errors Because Node modules are wrapped, errors on the first line of a file leak the wrapper to the user and report the wrong column number. This commit adds a line break to the module wrapper so that the first line is treated the same as all other lines. To compensate for the additional line, a line offset of -1 is also applied to errors. Fixes: https://github.com/nodejs/node/issues/2860 PR-URL: https://github.com/nodejs/node/pull/2867 Reviewed-By: Colin Ihrig --- doc/api/vm.markdown | 20 +++++++++- lib/module.js | 4 +- src/node.js | 2 +- src/node_contextify.cc | 37 ++++++++++++++++++- test/fixtures/test-error-first-line-offset.js | 1 + test/parallel/test-vm-context.js | 12 ++++++ test/sequential/test-module-loading.js | 8 ++++ 7 files changed, 78 insertions(+), 6 deletions(-) create mode 100644 test/fixtures/test-error-first-line-offset.js diff --git a/doc/api/vm.markdown b/doc/api/vm.markdown index 870a9551215abf..10c8cbc4ce4fd2 100644 --- a/doc/api/vm.markdown +++ b/doc/api/vm.markdown @@ -26,10 +26,16 @@ The options when creating a script are: - `filename`: allows you to control the filename that shows up in any stack traces produced from this script. +- `lineOffset`: allows you to add an offset to the line number that is + displayed in stack traces +- `columnOffset`: allows you to add an offset to the column number that is + displayed in stack traces - `displayErrors`: whether or not to print any errors to stderr, with the line of code that caused them highlighted, before throwing an exception. Applies only to syntax errors compiling the code; errors while running the code are controlled by the options to the script's methods. +- `timeout`: a number of milliseconds to execute `code` before terminating + execution. If execution is terminated, an `Error` will be thrown. ### script.runInContext(contextifiedSandbox[, options]) @@ -124,8 +130,14 @@ multiple times: The options for running a script are: -- `displayErrors`: whether or not to print any runtime errors to stderr, with - the line of code that caused them highlighted, before throwing an exception. +- `filename`: allows you to control the filename that shows up in any stack + traces produced. +- `lineOffset`: allows you to add an offset to the line number that is + displayed in stack traces +- `columnOffset`: allows you to add an offset to the column number that is + displayed in stack traces +- `displayErrors`: whether or not to print any errors to stderr, with the + line of code that caused them highlighted, before throwing an exception. Applies only to runtime errors executing the code; it is impossible to create a `Script` instance with syntax errors, as the constructor will throw. - `timeout`: a number of milliseconds to execute the script before terminating @@ -252,6 +264,10 @@ e.g. `(0,eval)('code')`. However, it also has the following additional options: - `filename`: allows you to control the filename that shows up in any stack traces produced. +- `lineOffset`: allows you to add an offset to the line number that is + displayed in stack traces +- `columnOffset`: allows you to add an offset to the column number that is + displayed in stack traces - `displayErrors`: whether or not to print any errors to stderr, with the line of code that caused them highlighted, before throwing an exception. Will capture both syntax errors from compiling `code` and runtime errors diff --git a/lib/module.js b/lib/module.js index 507ca83c1bca17..8b9b59551ece76 100644 --- a/lib/module.js +++ b/lib/module.js @@ -47,7 +47,6 @@ Module.wrapper = NativeModule.wrapper; Module.wrap = NativeModule.wrap; Module._debug = util.debuglog('module'); - // We use this alias for the preprocessor that filters it out const debug = Module._debug; @@ -401,7 +400,8 @@ Module.prototype._compile = function(content, filename) { // create wrapper function var wrapper = Module.wrap(content); - var compiledWrapper = runInThisContext(wrapper, { filename: filename }); + var compiledWrapper = runInThisContext(wrapper, + { filename: filename, lineOffset: -1 }); if (global.v8debug) { if (!resolvedArgv) { // we enter the repl if we're not given a filename argument. diff --git a/src/node.js b/src/node.js index ad8f27e499afa5..6a7aa7221bd00f 100644 --- a/src/node.js +++ b/src/node.js @@ -955,7 +955,7 @@ }; NativeModule.wrapper = [ - '(function (exports, require, module, __filename, __dirname) { ', + '(function (exports, require, module, __filename, __dirname) {\n', '\n});' ]; diff --git a/src/node_contextify.cc b/src/node_contextify.cc index df328f265ebf6e..6675e2d8a08f70 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -504,13 +504,15 @@ class ContextifyScript : public BaseObject { TryCatch try_catch; Local code = args[0]->ToString(env->isolate()); Local filename = GetFilenameArg(args, 1); + Local lineOffset = GetLineOffsetArg(args, 1); + Local columnOffset = GetColumnOffsetArg(args, 1); bool display_errors = GetDisplayErrorsArg(args, 1); if (try_catch.HasCaught()) { try_catch.ReThrow(); return; } - ScriptOrigin origin(filename); + ScriptOrigin origin(filename, lineOffset, columnOffset); ScriptCompiler::Source source(code, origin); Local v8_script = ScriptCompiler::CompileUnbound(env->isolate(), &source); @@ -675,6 +677,39 @@ class ContextifyScript : public BaseObject { } + static Local GetLineOffsetArg( + const FunctionCallbackInfo& args, + const int i) { + Local defaultLineOffset = Integer::New(args.GetIsolate(), 0); + + if (!args[i]->IsObject()) { + return defaultLineOffset; + } + + Local key = FIXED_ONE_BYTE_STRING(args.GetIsolate(), "lineOffset"); + Local value = args[i].As()->Get(key); + + return value->IsUndefined() ? defaultLineOffset : value->ToInteger(); + } + + + static Local GetColumnOffsetArg( + const FunctionCallbackInfo& args, + const int i) { + Local defaultColumnOffset = Integer::New(args.GetIsolate(), 0); + + if (!args[i]->IsObject()) { + return defaultColumnOffset; + } + + Local key = FIXED_ONE_BYTE_STRING(args.GetIsolate(), + "columnOffset"); + Local value = args[i].As()->Get(key); + + return value->IsUndefined() ? defaultColumnOffset : value->ToInteger(); + } + + static bool EvalMachine(Environment* env, const int64_t timeout, const bool display_errors, diff --git a/test/fixtures/test-error-first-line-offset.js b/test/fixtures/test-error-first-line-offset.js new file mode 100644 index 00000000000000..9aeeab047d22c3 --- /dev/null +++ b/test/fixtures/test-error-first-line-offset.js @@ -0,0 +1 @@ +error diff --git a/test/parallel/test-vm-context.js b/test/parallel/test-vm-context.js index 45e19e6638892e..e1cd2ef01e5176 100644 --- a/test/parallel/test-vm-context.js +++ b/test/parallel/test-vm-context.js @@ -60,3 +60,15 @@ var ctx = {}; Object.defineProperty(ctx, 'b', { configurable: false }); ctx = vm.createContext(ctx); assert.equal(script.runInContext(ctx), false); + +// Error on the first line of a module should +// have the correct line and column number +assert.throws(function() { + vm.runInContext('throw new Error()', context, { + filename: 'expected-filename.js', + lineOffset: 32, + columnOffset: 123 + }); +}, function(err) { + return /expected-filename.js:33:130/.test(err.stack); +}, 'Expected appearance of proper offset in Error stack'); diff --git a/test/sequential/test-module-loading.js b/test/sequential/test-module-loading.js index d53de512f2510a..a8696f84cd2ce9 100644 --- a/test/sequential/test-module-loading.js +++ b/test/sequential/test-module-loading.js @@ -279,3 +279,11 @@ process.on('exit', function() { // #1440 Loading files with a byte order marker. assert.equal(42, require('../fixtures/utf8-bom.js')); assert.equal(42, require('../fixtures/utf8-bom.json')); + +// Error on the first line of a module should +// have the correct line and column number +assert.throws(function() { + require('../fixtures/test-error-first-line-offset.js'); +}, function(err) { + return /test-error-first-line-offset.js:1:1/.test(err.stack); +}, 'Expected appearance of proper offset in Error stack');