Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

module: fix column offset on first line stack trace #25342

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions doc/api/vm.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,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: 3 additions & 1 deletion lib/module.js
Original file line number Diff line number Diff line change
Expand Up @@ -441,8 +441,10 @@ Module.prototype._compile = function(content, filename) {

// create wrapper function
var wrapper = Module.wrap(content);
var wrapperOffset = Module.wrapper[0].length;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be pulled out of the function to avoid calculating every time.


var compiledWrapper = runInThisContext(wrapper, { filename: filename });
var compiledWrapper = runInThisContext(wrapper,
{ filename: filename, columnOffset: -wrapperOffset });
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this pass make lint?

if (global.v8debug) {
if (!resolvedArgv) {
// we enter the repl if we're not given a filename argument.
Expand Down
49 changes: 48 additions & 1 deletion src/node_contextify.cc
Original file line number Diff line number Diff line change
Expand Up @@ -485,13 +485,15 @@ class ContextifyScript : public BaseObject {
TryCatch try_catch;
Local<String> code = args[0]->ToString();
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 @@ -658,6 +660,51 @@ 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]->IsUndefined()) {
return defaultLineOffset;
}
if (args[i]->IsInt32()) {
return args[i].As<Integer>();
}
if (!args[i]->IsObject()) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the only check you actually need here. The IsUndefined() and IsInt32() checks above can go. Same comment applies for your GetColumnOffsetArg() function.

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]->IsUndefined()) {
return defaultColumnOffset;
}
if (args[i]->IsInt32()) {
return args[i].As<Integer>();
}
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/module-err.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
s
15 changes: 15 additions & 0 deletions test/simple/test-module-loading.js
Original file line number Diff line number Diff line change
Expand Up @@ -306,3 +306,18 @@ 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'));

// #9445 Error on first line of a module have the correct column number.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can just be (might need to split this across lines if it's more than 80 characters):

// Error on the first line of a module should have the correct line and column number

var gh9445Exception;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you just name this err?

try {
var err_js = require('../fixtures/module-err.js');
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to assign this to a variable since you don't reference it anywhere.

}
catch (e) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move this to the previous line like } catch (e) {

gh9445Exception = e;
assert.ok(/module-err.js:1:1/.test(e.stack),
'expected appearance of proper offset in Error stack');
}
assert.ok(gh9445Exception,
'expected exception from runInContext offset test');


16 changes: 16 additions & 0 deletions test/simple/test-vm-context.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,22 @@ catch (e) {
assert.ok(gh1140Exception,
'expected exception from runInContext signature test');

// Issue GH-9445:
console.error('test runInContext offset');
var gh9445Exception;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just use err.

try {
vm.runInContext('throw new Error()', context, { filename: 'expected-filename.js',
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leave the opening curly brace, filename should go on the next line.

lineOffset: 32,
columnOffset: 123});
}
catch (e) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this up to the previous line.

gh9445Exception = e;
assert.ok(/expected-filename.js:33:130/.test(e.stack),
'expected appearance of proper offset in Error stack');
}
assert.ok(gh9445Exception,
'expected exception from runInContext offset test');

// GH-558, non-context argument segfaults / raises assertion
[undefined, null, 0, 0.0, '', {}, []].forEach(function(e) {
assert.throws(function() { script.runInContext(e); }, TypeError);
Expand Down