vm: never abort on caught syntax error #17394

addaleax · 2017-11-29T20:21:19Z

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

addaleax · 2017-11-29T20:22:06Z

lib/vm.js

+  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.


@nodejs/v8 Is this a bug in V8?

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

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

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

addaleax · 2017-11-29T20:35:28Z

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 · 2017-11-29T23:07:18Z

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

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

addaleax · 2017-11-29T23:07:58Z

lib/vm.js

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


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

Why would it be a bad thing?

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

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

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

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 · 2017-11-29T23:13:09Z

CI: https://ci.nodejs.org/job/node-test-commit/14468/

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 · 2017-12-05T22:50:05Z

Landed in aeddc36, b73e66e

This is unnecessary since we only run `AppendExceptionLine()` once per exception at this point anyway. PR-URL: #17394 Reviewed-By: James M Snell <[email protected]>

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 <[email protected]>

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

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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

MylesBorins · 2017-12-12T07:13:13Z

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.

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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

MylesBorins · 2017-12-21T19:44:53Z

Got it to land !!!

This is unnecessary since we only run `AppendExceptionLine()` once per exception at this point anyway. PR-URL: #17394 Reviewed-By: James M Snell <[email protected]>

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 <[email protected]>

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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

MylesBorins · 2018-01-22T08:36:56Z

does this make sense for lts?

edit: if backported it should come with #17491

MylesBorins · 2018-02-27T03:04:14Z

ping re: backport

MylesBorins · 2018-05-22T15:56:17Z

one more backport ping

should come with #17491

addaleax requested a review from cjihrig November 29, 2017 20:21

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

addaleax commented Nov 29, 2017

View reviewed changes

addaleax force-pushed the vm-no-abort branch from 4e7ba98 to 250ab47 Compare November 29, 2017 20:23

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 force-pushed the vm-no-abort branch from 250ab47 to 62233c9 Compare November 29, 2017 23:07

addaleax commented Nov 29, 2017

View reviewed changes

addaleax requested a review from bnoordhuis November 29, 2017 23:08

addaleax added 2 commits December 1, 2017 22:47

src: remove tracking for exception arrow data

ac1fea0

This is unnecessary since we only run `AppendExceptionLine()` once per exception at this point anyway.

addaleax force-pushed the vm-no-abort branch from 62233c9 to 2dbb612 Compare December 1, 2017 21:47

jasnell approved these changes Dec 5, 2017

View reviewed changes

addaleax closed this Dec 5, 2017

addaleax deleted the vm-no-abort branch December 5, 2017 22:50

addaleax added a commit that referenced this pull request Dec 5, 2017

src: remove tracking for exception arrow data

aeddc36

This is unnecessary since we only run `AppendExceptionLine()` once per exception at this point anyway. PR-URL: #17394 Reviewed-By: James M Snell <[email protected]>

bnoordhuis mentioned this pull request Dec 6, 2017

src: remove unused variable in node_contextify #17491

Closed

2 tasks

MylesBorins added the backport-requested-v9.x label Dec 12, 2017

MylesBorins removed the backport-requested-v9.x label Dec 21, 2017

MylesBorins pushed a commit that referenced this pull request Dec 21, 2017

src: remove tracking for exception arrow data

df6acf9

This is unnecessary since we only run `AppendExceptionLine()` once per exception at this point anyway. PR-URL: #17394 Reviewed-By: James M Snell <[email protected]>

MylesBorins mentioned this pull request Jan 10, 2018

v9.4.0 proposal #18069

Merged

MylesBorins added lts-watch-v6.x labels Jan 22, 2018

MylesBorins added the backport-requested-v8.x label May 22, 2018

BethGriggs removed the lts-watch-v8.x label Sep 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vm: never abort on caught syntax error #17394

vm: never abort on caught syntax error #17394

addaleax commented Nov 29, 2017 •

edited

Loading

addaleax Nov 29, 2017

hashseed Nov 29, 2017

addaleax Nov 29, 2017

hashseed Nov 29, 2017

addaleax commented Nov 29, 2017

addaleax commented Nov 29, 2017

addaleax Nov 29, 2017

bnoordhuis Nov 30, 2017

addaleax Nov 30, 2017

bnoordhuis Nov 30, 2017

addaleax Nov 30, 2017

bnoordhuis Nov 30, 2017

addaleax commented Nov 29, 2017

addaleax commented Dec 5, 2017

MylesBorins commented Dec 12, 2017

MylesBorins commented Dec 21, 2017

MylesBorins commented Jan 22, 2018 •

edited

Loading

MylesBorins commented Feb 27, 2018

MylesBorins commented May 22, 2018 •

edited

Loading

vm: never abort on caught syntax error #17394

vm: never abort on caught syntax error #17394

Conversation

addaleax commented Nov 29, 2017 • edited Loading

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Nov 29, 2017

addaleax commented Nov 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Nov 29, 2017

addaleax commented Dec 5, 2017

MylesBorins commented Dec 12, 2017

MylesBorins commented Dec 21, 2017

MylesBorins commented Jan 22, 2018 • edited Loading

MylesBorins commented Feb 27, 2018

MylesBorins commented May 22, 2018 • edited Loading

addaleax commented Nov 29, 2017 •

edited

Loading

MylesBorins commented Jan 22, 2018 •

edited

Loading

MylesBorins commented May 22, 2018 •

edited

Loading