Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Partial fix for issue 3452, syntax errors in vm.runInThisContext don't display proper line numbers #3570

Closed
wants to merge 1 commit into from

3 participants

@itaylor

This pull request (#3502) was already open and had been reviewed, but in the process of cleaning up my commit history, I took some action which inadvertently closed my old pull request. Sorry about that.

@Nodejs-Jenkins

Can one of the admins verify this patch?

@bnoordhuis bnoordhuis referenced this pull request from a commit in bnoordhuis/node
@apaprocki apaprocki vm: remove display_error hack, add regression test 055e4fb
@tjfontaine
Owner

Hi, thanks this is fixed in the current builds

@tjfontaine tjfontaine closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 28, 2012
  1. @itaylor

    Partial fix for issue 3452, syntax errors in vm.runInThisContext don'…

    itaylor authored
    …t display proper line numbers
This page is out of date. Refresh to see the latest.
View
32 src/node.cc
@@ -20,6 +20,7 @@
// USE OR OTHER DEALINGS IN THE SOFTWARE.
#include "node.h"
+#include "node_internals.h"
#include "req_wrap.h"
#include "handle_wrap.h"
@@ -1234,26 +1235,17 @@ void DisplayExceptionLine (TryCatch &try_catch) {
String::Utf8Value sourceline(message->GetSourceLine());
const char* sourceline_string = *sourceline;
- // HACK HACK HACK
- //
- // FIXME
- //
- // Because of how CommonJS modules work, all scripts are wrapped with a
+ // This removes from the printed error messages the module wrap prefix:
// "function (function (exports, __filename, ...) {"
- // to provide script local variables.
- //
- // When reporting errors on the first line of a script, this wrapper
- // function is leaked to the user. This HACK is to remove it. The length
- // of the wrapper is 62. That wrapper is defined in src/node.js
- //
- // If that wrapper is ever changed, then this number also has to be
- // updated. Or - someone could clean this up so that the two peices
- // don't need to be changed.
- //
+ // that is around all scripts that are run as modules.
+
// 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)) {
+ offset += sizeof(MODULE_WRAP_PREFIX) - 1;
+ }
fprintf(stderr, "%s\n", sourceline_string + offset);
// Print wavy underline (GetUnderline is deprecated).
int start = message->GetStartColumn();
@@ -2217,6 +2209,12 @@ Handle<Object> SetupProcessObject(int argc, char *argv[]) {
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));
+ process->Set(String::NewSymbol("_moduleWrapSuffix"), String::New(MODULE_WRAP_SUFFIX),
+ static_cast<PropertyAttribute>(DontEnum | DontDelete | ReadOnly));
+
// -e, --eval
if (eval_string) {
process->Set(String::NewSymbol("_eval"), String::New(eval_string));
View
4 src/node.js
@@ -590,8 +590,8 @@
};
NativeModule.wrapper = [
- '(function (exports, require, module, __filename, __dirname) { ',
- '\n});'
+ process._moduleWrapPrefix,
+ process._moduleWrapSuffix
];
NativeModule.prototype.compile = function() {
View
3  src/node_internals.h
@@ -62,6 +62,9 @@ inline static int snprintf(char* buf, unsigned int len, const char* fmt, ...) {
# define ROUND_UP(a, b) ((a) % (b) ? ((a) + (b)) - ((a) % (b)) : (a))
#endif
+#define MODULE_WRAP_PREFIX "(function (exports, require, module, __filename, __dirname) { "
+#define MODULE_WRAP_SUFFIX "\n});"
+
// this would have been a template function were it not for the fact that g++
// sometimes fails to resolve it...
#define THROW_ERROR(fun) \
View
6 src/node_script.cc
@@ -393,10 +393,12 @@ Handle<Value> WrappedScript::EvalMachine(const Arguments& args) {
script = output_flag == returnResult ? Script::Compile(code, filename)
: Script::New(code, filename);
if (script.IsEmpty()) {
- // FIXME UGLY HACK TO DISPLAY SYNTAX ERRORS.
+ //V8 TryCatch.ReThrow is broken. Calling it resets the stack trace to
+ //the place where ReThrow was called. This makes it impossible to get a stack
+ //trace after the an error has been rethrown. Until this V8 bug is fixed, this
+ //display_error argument makes it possible to log the location information.
if (display_error) DisplayExceptionLine(try_catch);
- // Hack because I can't get a proper stacktrace on SyntaxError
return try_catch.ReThrow();
}
} else {
View
12 test/message/vm_display_errors.js
@@ -0,0 +1,12 @@
+// Tests that when you add the display_error flag to a vm.createScript
+// call, that you will get the correct line / column displayed.
+// NOTE: This test should be deleted as soon as the V8 bug that causes rethrown
+// exceptions to get new stack traces is fixed. At that time it should be
+// replaced with a test that shows that stack traces in Syntax Errors
+// are reported normally
+var common = require('../common');
+common.error('before');
+
+require("vm").createScript('invalidJsHere ?= 1;', "myfilename.js", true);
+
+common.error('after');
View
19 test/message/vm_display_errors.out
@@ -0,0 +1,19 @@
+before
+
+myfilename.js:1
+invalidJsHere ?= 1;
+ ^
+
+vm.js:*
+ var ns = new binding.NodeScript(code, ctx, filename);
+ ^
+SyntaxError: Unexpected token =
+ at new Script (vm.js:*)
+ at Function.Script.createScript (vm.js:*)
+ at Object.<anonymous> (*/vm_display_errors.js:*)
+ at Module._compile (module.js:*)
+ at Object.Module._extensions..js (module.js:*)
+ at Module.load (module.js:*)
+ at Function.Module._load (module.js:*)
+ at Module.runMain (module.js:*)
+ at process.startup.processNextTick.process._tickCallback (node.js:*)
Something went wrong with that request. Please try again.