Skip to content

Commit

Permalink
module: fix column offsets in errors
Browse files Browse the repository at this point in the history
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: nodejs#2860
PR-URL: nodejs#2867
Reviewed-By: Colin Ihrig <[email protected]>
  • Loading branch information
tflanagan authored and cjihrig committed Nov 20, 2015
1 parent 94c3507 commit dfee4e3
Show file tree
Hide file tree
Showing 7 changed files with 78 additions and 6 deletions.
20 changes: 18 additions & 2 deletions doc/api/vm.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -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])

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions lib/module.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion src/node.js
Original file line number Diff line number Diff line change
Expand Up @@ -955,7 +955,7 @@
};

NativeModule.wrapper = [
'(function (exports, require, module, __filename, __dirname) { ',
'(function (exports, require, module, __filename, __dirname) {\n',
'\n});'
];

Expand Down
37 changes: 36 additions & 1 deletion src/node_contextify.cc
Original file line number Diff line number Diff line change
Expand Up @@ -504,13 +504,15 @@ class ContextifyScript : public BaseObject {
TryCatch try_catch;
Local<String> code = args[0]->ToString(env->isolate());
Local<String> filename = GetFilenameArg(args, 1);
Local<Integer> lineOffset = GetLineOffsetArg(args, 1);
Local<Integer> 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<UnboundScript> v8_script =
ScriptCompiler::CompileUnbound(env->isolate(), &source);
Expand Down Expand Up @@ -675,6 +677,39 @@ class ContextifyScript : public BaseObject {
}


static Local<Integer> GetLineOffsetArg(
const FunctionCallbackInfo<Value>& args,
const int i) {
Local<Integer> defaultLineOffset = Integer::New(args.GetIsolate(), 0);

if (!args[i]->IsObject()) {
return defaultLineOffset;
}

Local<String> key = FIXED_ONE_BYTE_STRING(args.GetIsolate(), "lineOffset");
Local<Value> value = args[i].As<Object>()->Get(key);

return value->IsUndefined() ? defaultLineOffset : value->ToInteger();
}


static Local<Integer> GetColumnOffsetArg(
const FunctionCallbackInfo<Value>& args,
const int i) {
Local<Integer> defaultColumnOffset = Integer::New(args.GetIsolate(), 0);

if (!args[i]->IsObject()) {
return defaultColumnOffset;
}

Local<String> key = FIXED_ONE_BYTE_STRING(args.GetIsolate(),
"columnOffset");
Local<Value> value = args[i].As<Object>()->Get(key);

return value->IsUndefined() ? defaultColumnOffset : value->ToInteger();
}


static bool EvalMachine(Environment* env,
const int64_t timeout,
const bool display_errors,
Expand Down
1 change: 1 addition & 0 deletions test/fixtures/test-error-first-line-offset.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
error
12 changes: 12 additions & 0 deletions test/parallel/test-vm-context.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
8 changes: 8 additions & 0 deletions test/sequential/test-module-loading.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');

0 comments on commit dfee4e3

Please sign in to comment.