Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Eval and vm module with custom errors #7397

Closed
kamn opened this issue Jun 24, 2016 · 6 comments
Closed

Eval and vm module with custom errors #7397

kamn opened this issue Jun 24, 2016 · 6 comments
Assignees
Labels
vm Issues and PRs related to the vm subsystem.

Comments

@kamn
Copy link

kamn commented Jun 24, 2016

  • Version: v6.2.2
  • Platform: Ubuntu 16.04
  • Subsystem: vm

I was playing around with dynamically running code and found a difference between standard JS eval and the vm module with custom error code. Here is a code snippet that can be run

var vm = require("vm");

var customError = `
function MyError(message) {
  this.name = this.constructor.name;
  this.message = message;
};
MyError.prototype = Error.prototype;
`;
var errorThrower = "throw new MyError('MyError');";


console.log("Eval - MyError");
try {
  var result = eval(customError + "\n" + errorThrower);
} catch (e) {
  console.log(e.message + "\n");
}


console.log("Good VM - Error");
try {
  var result = vm.runInThisContext("throw new Error('Not MyError');", "repl");
} catch (e) {
  console.log(e.message + "\n");
}

console.log("Bad VM - MyError");
try {
  var result = vm.runInThisContext(customError + "\n" + errorThrower, "repl");
} catch (e) {
  console.log(e.message + "\n");
}

console.log("Bad VM - MyError Again");
try {
  var result = vm.runInThisContext(customError + "\n" + errorThrower, "repl");
} catch (e) {
  console.log(e.message + "\n");
}

and the output

kamn@kamn:~/Dev/VmEvalTest$ node index.js 
Eval - MyError
MyError

Good VM - Error
Not MyError

Bad VM - MyError

repl:7
throw new MyError('MyError');
^
MyError

Bad VM - MyError Again
MyError

It seems like custom errors in vm output to stdin(?) the first time they happen. They are even caught afterwards. This is not true for a standard Error in vm and both Errors work in JS Eval. I am not sure this is a expected behavior but it does not seem like it. Any feedback would be appreciated.

Thanks!

@addaleax addaleax added the vm Issues and PRs related to the vm subsystem. label Jun 24, 2016
@addaleax addaleax self-assigned this Jun 24, 2016
@mscdex
Copy link
Contributor

mscdex commented Jun 24, 2016

The reason is that the vm.runInThisContext()'s displayErrors option is true by default. Explicitly setting it to false will get you:

Eval - MyError
MyError

Good VM - Error
Not MyError

Bad VM - MyError
MyError

Bad VM - MyError Again
MyError

The name displayErrors is a bit misleading, but what it actually does is decorate the exception message if true, which is what you're seeing (the filename is added and it points to the source line). When it's false, the error is not decorated, so you just get the exception message itself. Originally when this option was added it did print the exception to stderr IIRC (hence the 'display' part of the name), but that behavior changed some time ago.

@addaleax
Copy link
Member

@mscdex Well, there’s also a real bug here; none of the vm methods should print anything to stderr under normal circumstances.

The reason it does here is this line, and it shouldn’t be too hard to fix this.

addaleax added a commit to addaleax/node that referenced this issue Jun 24, 2016
In `AppendExceptionLine()`, which is used both by the `vm`
module and the uncaught exception handler, don’t print anything
to stderr when called from the `vm` module, even if the
thrown object is not a native error instance.

Fixes: nodejs#7397
@addaleax
Copy link
Member

PR: #7398

@kamn
Copy link
Author

kamn commented Jun 24, 2016

@mscdex: Thank you for the explanation and work around.
@addaleax: Thank you for the quick fix.

@adoyle-h
Copy link

There is another question.

if (env->printed_error())
return;
env->set_printed_error(true);

uv_tty_reset_mode();
PrintErrorString("\n%s", arrow);

Why print the error only once? The exception may occur repeatedly. If the output to stderr is allowed, I think it should be printed always rather than once.

@addaleax
Copy link
Member

Hm, maybe that made sense back in nodejs/node-v0.x-archive#7049, but I don’t see any real point right now either… @indutny you don’t happen to remember, do you?

Fishrock123 pushed a commit that referenced this issue Jul 5, 2016
In `AppendExceptionLine()`, which is used both by the `vm`
module and the uncaught exception handler, don’t print anything
to stderr when called from the `vm` module, even if the
thrown object is not a native error instance.

Fixes: #7397
PR-URL: #7398
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax added a commit to addaleax/node that referenced this issue Jul 11, 2016
In `AppendExceptionLine()`, which is used both by the `vm`
module and the uncaught exception handler, don’t print anything
to stderr when called from the `vm` module, even if the
thrown object is not a native error instance.

Fixes: nodejs#7397
PR-URL: nodejs#7398
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this issue Jul 12, 2016
In `AppendExceptionLine()`, which is used both by the `vm`
module and the uncaught exception handler, don’t print anything
to stderr when called from the `vm` module, even if the
thrown object is not a native error instance.

Fixes: #7397
PR-URL: #7398
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this issue Jul 12, 2016
In `AppendExceptionLine()`, which is used both by the `vm`
module and the uncaught exception handler, don’t print anything
to stderr when called from the `vm` module, even if the
thrown object is not a native error instance.

Fixes: #7397
PR-URL: #7398
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this issue Jul 14, 2016
In `AppendExceptionLine()`, which is used both by the `vm`
module and the uncaught exception handler, don’t print anything
to stderr when called from the `vm` module, even if the
thrown object is not a native error instance.

Fixes: #7397
PR-URL: #7398
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this issue Jul 14, 2016
In `AppendExceptionLine()`, which is used both by the `vm`
module and the uncaught exception handler, don’t print anything
to stderr when called from the `vm` module, even if the
thrown object is not a native error instance.

Fixes: #7397
PR-URL: #7398
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

No branches or pull requests

4 participants