Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

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

…t display proper line numbers
  • Loading branch information...
commit 2f6bca67a69e9597aa2ae001bef0e668c3ebfff4 1 parent 2d0011f
@itaylor authored
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:*)
Please sign in to comment.
Something went wrong with that request. Please try again.