This repository has been archived by the owner. It is now read-only.

Partial fix for issue 3452, missing filename and error location on syntax error in vm functions #3502

Merged
merged 0 commits into from Jun 28, 2012

Conversation

Projects
None yet
2 participants

itaylor commented Jun 21, 2012

I was unable to completely fix #3452 due to a bug with V8's TryCatch::ReThrow where the rethrown exceptions have their stack trace reset. I was, however able to make it so that the DisplayErrorLine is able to display the correct error line when a script is not a wrapped module. This makes it so that the undocumented display_errors argument works correctly.

To do this, I moved the declaration of the wrapper out of node.js and into node.h, and I then pass it to node.js via the process variable when the node.js script has it's constructor called. This seemed to fit with the pattern of how other things are being inited in the node.js file.

I'm also working on a pure V8 test of ReThrow's broken behavior that I'll submit to the V8 project, which, if fixed, would eliminate the need for the display_errors argument and fix this bug entirely.

Test:

var myFileName = "/path/to/some/file";
require("vm").createScript('invalidJsHere ?= 1;', myFileName, true); //using display_errors = true

Output before this patch:

/path/to/some/file:1

^

vm.js:32
  var ns = new binding.NodeScript(code, ctx, filename);
           ^
SyntaxError: Unexpected token =
    at new Script (vm.js:32:12)
    at Function.Script.createScript (vm.js:48:10)
    at Object.<anonymous> (/Users/itaylor/temp/test.js:2:15)
    at Module._compile (module.js:449:26)
    at Object.Module._extensions..js (module.js:467:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Module.runMain (module.js:487:10)
    at process.startup.processNextTick.process._tickCallback (node.js:244:9)

After this change:

/path/to/some/file:1
invalidJsHere ?= 1;
               ^

vm.js:32
  var ns = new binding.NodeScript(code, ctx, filename);
           ^
SyntaxError: Unexpected token =
    at new Script (vm.js:32:12)
    at Function.Script.createScript (vm.js:48:10)
    at Object.<anonymous> (/Users/itaylor/temp/test.js:2:15)
    at Module._compile (module.js:449:26)
    at Object.Module._extensions..js (module.js:467:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Module.runMain (module.js:487:10)
    at process.startup.processNextTick.process._tickCallback (node.js:244:9)

@bnoordhuis bnoordhuis commented on an outdated diff Jun 21, 2012

deps/v8/src/messages.js
@@ -1254,4 +1254,4 @@ InstallFunctions($Error.prototype, DONT_ENUM, ['toString', ErrorToString]);
// Boilerplate for exceptions for stack overflows. Used from
// Isolate::StackOverflow().
-var kStackOverflowBoilerplate = MakeRangeError('stack_overflow', []);
+var kStackOverflowBoilerplate = MakeRangeError('stack_overflow', []);
@bnoordhuis

bnoordhuis Jun 21, 2012

Member

Whitespace-only change, please undo.

@bnoordhuis bnoordhuis commented on an outdated diff Jun 21, 2012

@@ -118,7 +118,6 @@
static Persistent<String> exit_symbol;
static Persistent<String> disposed_symbol;
-

@bnoordhuis bnoordhuis commented on an outdated diff Jun 21, 2012

@@ -175,6 +174,7 @@
static int64_t tick_times[RPM_SAMPLES];
static int tick_time_head;
+
@bnoordhuis

bnoordhuis Jun 21, 2012

Member

...and ditto.

@bnoordhuis bnoordhuis commented on an outdated diff Jun 21, 2012

// Even better would be to get support into V8 for wrappers that
// shouldn't be reported to users.
- int offset = linenum == 1 ? 62 : 0;
-
+ int offset = 0;
+ if(linenum == 1 && 0 == strncmp(sourceline_string, MODULE_WRAP_PREFIX, strlen(MODULE_WRAP_PREFIX))){
@bnoordhuis

bnoordhuis Jun 21, 2012

Member

Missing whitespace. :-)

@bnoordhuis bnoordhuis commented on an outdated diff Jun 21, 2012

// Even better would be to get support into V8 for wrappers that
// shouldn't be reported to users.
- int offset = linenum == 1 ? 62 : 0;
-
+ int offset = 0;
+ if(linenum == 1 && 0 == strncmp(sourceline_string, MODULE_WRAP_PREFIX, strlen(MODULE_WRAP_PREFIX))){
+ offset += strlen(MODULE_WRAP_PREFIX);
@bnoordhuis

bnoordhuis Jun 21, 2012

Member

Use sizeof(MODULE_WRAP_PREFIX) - 1 here. Same for the previous line.

@bnoordhuis bnoordhuis commented on an outdated diff Jun 21, 2012

@@ -128,6 +128,10 @@ void SetPrototypeMethod(target_t target,
enum encoding {ASCII, UTF8, BASE64, UCS2, BINARY, HEX};
enum encoding ParseEncoding(v8::Handle<v8::Value> encoding_v,
enum encoding _default = BINARY);
+
+#define MODULE_WRAP_PREFIX "(function (exports, require, module, __filename, __dirname) { "
+#define MODULE_WRAP_SUFFIX "\n});"
@bnoordhuis

bnoordhuis Jun 21, 2012

Member

Move this to node_internals.h.

itaylor commented Jun 21, 2012

Thanks for the review Ben... I'll get these changes made today.

@bnoordhuis bnoordhuis commented on an outdated diff Jun 21, 2012

@@ -2215,6 +2205,10 @@ static void DebugPortSetter(Local<String> property,
process->Set(String::NewSymbol("pid"), Integer::New(getpid()));
process->Set(String::NewSymbol("features"), GetFeatures());
+ //Add in the module wrap signatures
+ process->Set(String::NewSymbol("_moduleWrapPrefix"), String::New(MODULE_WRAP_PREFIX));
+ process->Set(String::NewSymbol("_moduleWrapSuffix"), String::New(MODULE_WRAP_SUFFIX));
@bnoordhuis

bnoordhuis Jun 21, 2012

Member

I don't really want to extend the process object with properties like that, they're used only once and it'll encourage people to rely on implementation details. Make them invisible (and invincible) with static_cast<PropertyAttributes>(DONT_ENUM | DONT_DELETE | READ_ONLY).

Member

bnoordhuis commented Jun 21, 2012

Couple of nits but generally LGTM. Can another committer review this too?

itaylor commented Jun 21, 2012

I've addressed Ben's comments, should be ready for someone else to look at.

@bnoordhuis bnoordhuis commented on an outdated diff Jun 21, 2012

// Even better would be to get support into V8 for wrappers that
// shouldn't be reported to users.
- int offset = linenum == 1 ? 62 : 0;
-
+ int offset = 0;
+ if (linenum == 1 && 0 == strncmp(sourceline_string, MODULE_WRAP_PREFIX, sizeof(MODULE_WRAP_PREFIX) - 1)){
@bnoordhuis

bnoordhuis Jun 21, 2012

Member

Still missing a whitespace. :-) Also, it should wrap at 80 characters.

@bnoordhuis bnoordhuis commented on an outdated diff Jun 21, 2012

@@ -2215,6 +2206,12 @@ static void DebugPortSetter(Local<String> property,
process->Set(String::NewSymbol("pid"), Integer::New(getpid()));
process->Set(String::NewSymbol("features"), GetFeatures());
+ //Add in the module wrap signatures
+ process->Set(String::NewSymbol("_moduleWrapPrefix"), String::New(MODULE_WRAP_PREFIX),
+ static_cast<PropertyAttribute>(DontEnum | DontDelete | ReadOnly));
@bnoordhuis

bnoordhuis Jun 21, 2012

Member

Style: needs to line up with String::NewSymbol(...)

Member

bnoordhuis commented Jun 21, 2012

@itaylor While you're at it, can you add a test if we don't already have one?

itaylor commented Jun 21, 2012

@bnoordhuis I've added a test case for this. I followed the pattern of undefined_reference_in_new_context.js, but both of these are kind of shitty tests, because they'll break if the stack trace changes due to a refactoring. I couldn't see another easy way of catching the output of an unhandled exception in the current test suite, though.

Really, undefined_reference_in_new_context.js documents the incorrect behavior we currently have for showing stack traces that caused me to file 3452 in the first place. In that test, you see the stack trace of where the call to the bad code was made, not the bad code itself... I still think we need to figure out some way to do a better job of showing the place in code that the actual exception occurs. I wish I would have been able to fix 3452 for real, instead of just adding a band-aid to it... Oh well, band-aids are better than bleeding all over, I guess :-)

I still have hope that someday we can get V8 team to fix ReThrow so, and this problem will just go away.

Member

bnoordhuis commented Jun 28, 2012

Ian, can you rebase your PR against the master branch and squash your work into one or two commits? It won't apply cleanly now.

Also, can you sign the CLA if you haven't done so already?

@itaylor itaylor merged commit 2d0011f into nodejs:master Jun 28, 2012

itaylor commented Jun 28, 2012

@bnoordhuis I've signed the CLA and opened a new pull request with a clean commit history.
joyent#3570

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.