vm: Correctly pass through syntax errors.

[apaprocki]

My V8 patch to fix C++ TryThrow to restore message script location when ReThrow-ing landed in 704fd8f. Now that this is in place, the display_error hack in node_script.cc can be removed since syntax errors within vm calls are correctly reported as a SyntaxError at the actual error site instead of at the outer-most vm call.

Fixes #3452

[apaprocki]

Should fix #5677 as well.
This fix supercedes #3570, which can be closed after this is merged.

[apaprocki]

Should fix #1310 as well.

[apaprocki]

/cc @bnoordhuis This cleans things up and adds a test case for the syntax error -- can you review?

[bnoordhuis]

LGTM with the caveat that I had to fix up some of the test/message tests. The test you added is still failing for me:

$ python tools/test.py message
[00:02|%  92|+  13|-   0]: release vm_syntax_error length differs.
expect=6
actual=14
patterns:
pattern = ^bar\:1$
pattern = ^invalid\ \?\=\ 1$
pattern = ^\ \ \ \ \ \ \ \ \ \^$
pattern = ^SyntaxError\:\ Unexpected\ token\ \=$
pattern = ^\ \ \ \ at\ context\.run\ \(.*test.*message.*vm\_syntax\_error\.js\:.*\:.*\)$
pattern = ^\ \ \ \ at\ foo\:1\:1$
outlines:
outline = /Users/bnoordhuis/src/master/test/message/vm_syntax_error.js:31
outline = vm.runInNewContext('run()', context, 'foo');
outline =    ^
outline = SyntaxError: Unexpected token =
outline =     at context.run (/Users/bnoordhuis/src/master/test/message/vm_syntax_error.js:28:12)
outline =     at foo:1:1
outline =     at Object.<anonymous> (/Users/bnoordhuis/src/master/test/message/vm_syntax_error.js:31:4)
outline =     at Module._compile (module.js:449:26)
outline =     at Object.Module._extensions..js (module.js:467:10)
outline =     at Module.load (module.js:349:32)
outline =     at Function.Module._load (module.js:305:12)
outline =     at Function.Module.runMain (module.js:490:10)
outline =     at startup (node.js:119:16)
outline =     at node.js:819:3  
=== release vm_syntax_error === 
Path: message/vm_syntax_error   
/Users/bnoordhuis/src/master/test/message/vm_syntax_error.js:31
vm.runInNewContext('run()', context, 'foo');
   ^
SyntaxError: Unexpected token = 
    at context.run (/Users/bnoordhuis/src/master/test/message/vm_syntax_error.js:28:12)
    at foo:1:1
    at Object.<anonymous> (/Users/bnoordhuis/src/master/test/message/vm_syntax_error.js:31:4)
    at Module._compile (module.js:449:26)
    at Object.Module._extensions..js (module.js:467:10)
    at Module.load (module.js:349:32)
    at Function.Module._load (module.js:305:12)
    at Function.Module.runMain (module.js:490:10)
    at startup (node.js:119:16)
    at node.js:819:3
Command: out/Release/node /Users/bnoordhuis/src/master/test/message/vm_syntax_error.js
[00:02|% 100|+  13|-   1]: Done

Can you check that? Here is my fix-up branch.

[apaprocki]

Hmm.. that is strange. That is the error you would get if the V8 fix was not present. I'll take a look.

[domenic]

Hmm, I tried to remove the DisplayError stuff in vm2 but it lost the error and gave the bad output. If you have any ideas let me know.

[apaprocki]

@domenic Yeah this is frustrating -- when I landed the V8 changes for this, it was working perfectly fine in my copy of node/V8. At some point since it worked its way into the actual V8 release and landed in node, it stopped working. I spent a bit of time poking at it but haven't tracked down what change was responsible. I'll keep at it..

[isaacs]

So, the V8 change is apparently still not there in 3.20.17. Is it perhaps in 3.21?

Removing the display_error param from the vm.Script constructor is trivial enough, and we all agree it's a great idea, but this pull req doesn't merge cleanly since the vm2 contextify changes. Let's revisit when V8 can handle ReThrow without losing information.

[isaacs]

@apaprocki Can you point me to your V8 change that's supposed to address this?

[apaprocki]

@isaacs The rev I indicated in the description, 704fd8f, landed it in Node.

My original changeset is here: https://codereview.chromium.org/15669003/

Yang changed it a bit and landed it here: https://codereview.chromium.org/17694002/

This definitely worked in the state of Node+V8 at the time of my original changeset. With the patch as it landed in V8, I didn't see it working as intended in the latest Node. I dug into it a little bit but didn't get to the bottom of it. I'm going to revisit it when I get a solid block of time to switch contexts.