vm: don't print out arrow message for custom error - #7398
Conversation
Sorry, something went wrong.
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
| message: 'This is a custom message' | ||
| })`, { filename: 'test.vm' }); | ||
| } catch (e) { | ||
| } |
There was a problem hiding this comment.
Should we also do another print from the catch to make sure it routes that way?
Sorry, something went wrong.
There was a problem hiding this comment.
@Fishrock123 sounds good, done
Sorry, something went wrong.
| Local<Value> er, | ||
| Local<Message> message) { | ||
| Local<Message> message, | ||
| bool handlingFatalError) { |
There was a problem hiding this comment.
Style: snake_case, not camelCase for parameters (i.e., handling_fatal_error.)
Also, an enum might be nicer because it gives a clear hint what the parameter does at the call site. With bools, I usually use a local variable to make it obvious:
const bool handling_fatal_error = true;
AppendExceptionLine(env, err, message, handling_fatal_error);
Sorry, something went wrong.
There was a problem hiding this comment.
Aside: is_fatal_error is a bit more succinct.
Sorry, something went wrong.
There was a problem hiding this comment.
I’ve updated with an enum, that’s a good idea.
Sorry, something went wrong.
| Local<Value> er, | ||
| Local<Message> message) { | ||
| Local<Message> message, | ||
| enum ErrorHandlingMode mode) { |
There was a problem hiding this comment.
The enum keyword is not strictly necessary (but it doesn't hurt and I realize we use it in other places.)
Sorry, something went wrong.
|
LGTM with comments. |
Sorry, something went wrong.
| console.error('beginning'); | ||
|
|
||
| // Regression test for https://github.com/nodejs/node/issues/7397: | ||
| // This should not print out anything to stderr due to the error being caught. |
There was a problem hiding this comment.
This is the tiniest of nits, but maybe say vm.runInThisContext() should not print anything... As it currently stands, it seems like nothing should be printed at all, or the catch won't print anything either.
Sorry, something went wrong.
There was a problem hiding this comment.
@cjihrig Done, forgot to update this along with the catch block
Sorry, something went wrong.
|
LGTM I suppose. |
Sorry, something went wrong.
Sorry, something went wrong.
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>
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 this is not landing cleanly, would you be willing to backport to v4.x? |
Sorry, something went wrong.
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>
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>
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>
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>
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>
Checklist
Affected core subsystem(s)
vm
Description of change
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