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

vm: never abort on caught syntax error #17394

Closed
wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Nov 29, 2017

Fixes: #13258

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

vm

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Nov 29, 2017
constructor(code, options) {
// Calling `ReThrow()` on a native TryCatch does not generate a new
// abort-on-uncaught-exception check. A dummy try/catch in JS land
// protects against that.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nodejs/v8 Is this a bug in V8?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah a ReThrow is not actually a new throw. It just "re-activates" the exception.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hashseed Does that mean one could get the right behavior by exiting the TryCatch scope, then using isolate->ThrowException()?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. Though the message you get with this kind of "rethrow" will show the stack trace of the new throw location.

@addaleax addaleax added vm Issues and PRs related to the vm subsystem. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Nov 29, 2017
@addaleax
Copy link
Member Author

Hm – looks like message tests fail, because the throw in JS makes us drop the filename from the output… going to need to look into this a bit more I guess

@addaleax
Copy link
Member Author

Okay, I think I fixed it with some … cleverness.

I’d appreciate it if @bnoordhuis could take a look?

try {
super(code, options);
} catch (e) {
throw e; /* node-do-not-add-exception-line */
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any idea for how to avoid this? Or: Is this even actually a bad thing?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would it be a bad thing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure - it seems like something that people might find out, start to use, and be unhappy when they find out we changed/removed/etc. it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what "it" refers to. The line in the (decorated) stack trace? What info could they glean from that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, sorry, I should have been clearer - I was talking about the node-do-not-add-exception-line magic string and its effect of not appending the filename + arrow string in that case

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hah, okay. I read your question as "is it a bad thing for the throw to show up." I wouldn't say so, and yes, I'd say it's only a matter of time before people find out about the magic comment. :-)

If you want to exclude stack frames, you can filter based on the script id. Probably overkill for this, though.

@addaleax
Copy link
Member Author

This is unnecessary since we only run `AppendExceptionLine()`
once per exception at this point anyway.
Keep track of C++ `TryCatch` state to avoid aborting when
an exception is thrown inside one, and re-throw in JS
to make sure the exception is being picked up a second time by
a second uncaught exception handler, if necessary.

Add a bit of a hack to `AppendExceptionLine` to avoid overriding
the line responsible for re-throwing the exception.

Fixes: nodejs#13258
@addaleax
Copy link
Member Author

addaleax commented Dec 5, 2017

Landed in aeddc36, b73e66e

@addaleax addaleax closed this Dec 5, 2017
@addaleax addaleax deleted the vm-no-abort branch December 5, 2017 22:50
addaleax added a commit that referenced this pull request Dec 5, 2017
This is unnecessary since we only run `AppendExceptionLine()`
once per exception at this point anyway.

PR-URL: #17394
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Dec 5, 2017
Keep track of C++ `TryCatch` state to avoid aborting when
an exception is thrown inside one, and re-throw in JS
to make sure the exception is being picked up a second time by
a second uncaught exception handler, if necessary.

Add a bit of a hack to `AppendExceptionLine` to avoid overriding
the line responsible for re-throwing the exception.

PR-URL: #17394
Fixes: #13258
Reviewed-By: James M Snell <jasnell@gmail.com>
danbev added a commit to danbev/node that referenced this pull request Dec 7, 2017
Currently the following warning is show when building:
../src/node_contextify.cc:642:10:
warning: unused variable 'display_errors' [-Wunused-variable]

    bool display_errors = maybe_display_errors.ToChecked();

This commit removes the unused variable which became unused in commit
aeddc36 ("src: remove tracking for
exception arrow data").

Refs: nodejs#17394
danbev added a commit that referenced this pull request Dec 8, 2017
Currently the following warning is show when building:
../src/node_contextify.cc:642:10:
warning: unused variable 'display_errors' [-Wunused-variable]

    bool display_errors = maybe_display_errors.ToChecked();

This commit removes the unused variable which became unused in commit
aeddc36 ("src: remove tracking for
exception arrow data").

PR-URL: #17491
Refs: #17394
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins
Copy link
Contributor

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

MylesBorins pushed a commit that referenced this pull request Dec 20, 2017
Currently the following warning is show when building:
../src/node_contextify.cc:642:10:
warning: unused variable 'display_errors' [-Wunused-variable]

    bool display_errors = maybe_display_errors.ToChecked();

This commit removes the unused variable which became unused in commit
aeddc36 ("src: remove tracking for
exception arrow data").

PR-URL: #17491
Refs: #17394
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins
Copy link
Contributor

Got it to land !!!

MylesBorins pushed a commit that referenced this pull request Dec 21, 2017
This is unnecessary since we only run `AppendExceptionLine()`
once per exception at this point anyway.

PR-URL: #17394
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2017
Keep track of C++ `TryCatch` state to avoid aborting when
an exception is thrown inside one, and re-throw in JS
to make sure the exception is being picked up a second time by
a second uncaught exception handler, if necessary.

Add a bit of a hack to `AppendExceptionLine` to avoid overriding
the line responsible for re-throwing the exception.

PR-URL: #17394
Fixes: #13258
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2017
Currently the following warning is show when building:
../src/node_contextify.cc:642:10:
warning: unused variable 'display_errors' [-Wunused-variable]

    bool display_errors = maybe_display_errors.ToChecked();

This commit removes the unused variable which became unused in commit
aeddc36 ("src: remove tracking for
exception arrow data").

PR-URL: #17491
Refs: #17394
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins MylesBorins mentioned this pull request Jan 10, 2018
@MylesBorins
Copy link
Contributor

MylesBorins commented Jan 22, 2018

does this make sense for lts?

edit: if backported it should come with #17491

@MylesBorins
Copy link
Contributor

ping re: backport

@MylesBorins
Copy link
Contributor

MylesBorins commented May 22, 2018

one more backport ping

should come with #17491

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vm: behavior with --abort-on-uncaught-exception
7 participants