src: only call FatalException if not verbose #12826

danbev · 2017-05-04T08:13:10Z

This commit attempts to address the TODO regarding not calling
FatalException if the try_catch is verbose.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

src

jasnell · 2017-05-04T12:28:50Z

@nodejs/v8
@bnoordhuis

cjihrig

Second commit LGTM

danbev · 2017-05-04T12:57:32Z

CI: https://ci.nodejs.org/job/node-test-pull-request/7849/

addaleax

For my understanding: There isn’t any behavioural change because in the verbose case V8 will call OnMessage, right? If so, LGTM.

danbev · 2017-05-09T08:33:14Z

For my understanding: There isn’t any behavioural change because in the verbose case V8 will call OnMessage, right? If so, LGTM.

Yes that is my understanding as well, if a TryCatch is used and set to verbose, the handler will be called.

bnoordhuis

LGTM if you bump the patch level in v8-version.h. Did you request a back-port to the 5.8 branch?

Original commit message: This commit adds a getter for the private is_verbose_ member. The use case for this comes from Node.js where the ability to avoid calling FatalException if the TryCatch is verbose would be nice to have. BUG= Review-Url: https://codereview.chromium.org/2840803002 Cr-Commit-Position: refs/heads/master@{nodejs#45018}

This commit attempts to address the TODO regarding not calling FatalException if the try_catch is verbose.

danbev · 2017-05-15T12:38:48Z

Rebased CI: https://ci.nodejs.org/job/node-test-pull-request/8082/

danbev · 2017-05-15T12:46:46Z

if you bump the patch level in v8-version.h

I've bumped the patch level now.

Did you request a back-port to the 5.8 branch?

I've not done so but would be happy to. Just to clarify, you mean create back-port this to 5.8 in V8 right? I've never done that before so if you have any pointers about how to go about doing that (what branch would I base the change set on)?

Original commit message: This commit adds a getter for the private is_verbose_ member. The use case for this comes from Node.js where the ability to avoid calling FatalException if the TryCatch is verbose would be nice to have. BUG= Review-Url: https://codereview.chromium.org/2840803002 Cr-Commit-Position: refs/heads/master@{nodejs#45018} PR-URL: nodejs#12826 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

This commit attempts to address the TODO regarding not calling FatalException if the try_catch is verbose. PR-URL: nodejs#12826 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

danbev · 2017-05-16T08:04:09Z

Landed in fefab90 and 986e1d2

bnoordhuis · 2017-05-16T13:37:05Z

@danbev Just so you know, the Author tag is "daniel.bevenius" in commit 986e1d2. git am-d from upstream, I assume?

danbev · 2017-05-17T06:45:53Z

@bnoordhuis Thanks, I forgot to change it. I'll see if I can change my name in V8 so they are the same just in case I do this again and forget.
While I have you there would you be able to comment about my question above regarding the backporting?

bnoordhuis · 2017-05-17T08:13:04Z

@danbev https://github.com/v8/v8/wiki/Merging-&-Patching#is-there-a-tldr

danbev · 2017-05-17T09:10:35Z

@bnoordhuis perfect, thanks!

danbev · 2017-05-18T05:19:31Z

V8 backport request

Original commit message: This commit adds a getter for the private is_verbose_ member. The use case for this comes from Node.js where the ability to avoid calling FatalException if the TryCatch is verbose would be nice to have. BUG= Review-Url: https://codereview.chromium.org/2840803002 Cr-Commit-Position: refs/heads/master@{nodejs#45018} PR-URL: nodejs#12826 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

This commit attempts to address the TODO regarding not calling FatalException if the try_catch is verbose. PR-URL: nodejs#12826 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Original commit message: This commit adds a getter for the private is_verbose_ member. The use case for this comes from Node.js where the ability to avoid calling FatalException if the TryCatch is verbose would be nice to have. BUG= Review-Url: https://codereview.chromium.org/2840803002 Cr-Commit-Position: refs/heads/master@{#45018} PR-URL: #12826 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins · 2017-06-22T19:12:04Z

I'm a bit confused as to why 986e1d2 is showing up in both the v8.0.0 proposal as well as the v8.2.0 proposal. It would appear that only one of the commits from this PR ended up making its way to the 8.0.0 release which weird me out

/cc @jasnell @addaleax

addaleax · 2017-06-22T19:21:48Z

@MylesBorins The commit you linked was being re-applied as part of the V8 5.9 upgrade, but both were part of 8.0.0 … does that make sense?

Original commit message: This commit adds a getter for the private is_verbose_ member. The use case for this comes from Node.js where the ability to avoid calling FatalException if the TryCatch is verbose would be nice to have. BUG= Review-Url: https://codereview.chromium.org/2840803002 Cr-Commit-Position: refs/heads/master@{#45018} PR-URL: #12826 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins · 2017-07-17T14:30:08Z

Should this be backported to v6.x?

Original commit message: This commit adds a getter for the private is_verbose_ member. The use case for this comes from Node.js where the ability to avoid calling FatalException if the TryCatch is verbose would be nice to have. BUG= Review-Url: https://codereview.chromium.org/2840803002 Cr-Commit-Position: refs/heads/master@{#45018} PR-URL: #12826 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Original commit message: This commit adds a getter for the private is_verbose_ member. The use case for this comes from Node.js where the ability to avoid calling FatalException if the TryCatch is verbose would be nice to have. BUG= Review-Url: https://codereview.chromium.org/2840803002 Cr-Commit-Position: refs/heads/master@{nodejs#45018} PR-URL: nodejs#12826 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Original commit message: This commit adds a getter for the private is_verbose_ member. The use case for this comes from Node.js where the ability to avoid calling FatalException if the TryCatch is verbose would be nice to have. BUG= Review-Url: https://codereview.chromium.org/2840803002 Cr-Commit-Position: refs/heads/master@{#45018} PR-URL: #12826 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

This commit attempts to address the TODO regarding not calling FatalException if the try_catch is verbose. PR-URL: #12826 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Original commit message: This commit adds a getter for the private is_verbose_ member. The use case for this comes from Node.js where the ability to avoid calling FatalException if the TryCatch is verbose would be nice to have. BUG= Review-Url: https://codereview.chromium.org/2840803002 Cr-Commit-Position: refs/heads/master@{#45018} PR-URL: #12826 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

This commit attempts to address the TODO regarding not calling FatalException if the try_catch is verbose. PR-URL: #12826 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. v8 engine Issues and PRs related to the V8 dependency. labels May 4, 2017

cjihrig approved these changes May 4, 2017

View reviewed changes

addaleax approved these changes May 7, 2017

View reviewed changes

bnoordhuis approved these changes May 15, 2017

View reviewed changes

danbev added 3 commits May 15, 2017 14:20

src: only call FatalException if not verbose

49abd1a

This commit attempts to address the TODO regarding not calling FatalException if the try_catch is verbose.

deps: bump V8_PATCH_LEVEL to 39

7af80be

danbev force-pushed the is-verbose-todo branch from 3162e34 to 7af80be Compare May 15, 2017 12:37

danbev closed this May 16, 2017

danbev deleted the is-verbose-todo branch May 16, 2017 08:05

jasnell mentioned this pull request May 28, 2017

8.0.0 Release Proposal #12220

Closed

addaleax mentioned this pull request Jun 17, 2017

v8.2.0 proposal #13744

Merged

MylesBorins added the lts-watch-v6.x label Jul 17, 2017

addaleax mentioned this pull request Jul 24, 2017

v8.3.0 proposal #14452

Closed

addaleax added the dont-land-on-v8.x label Aug 2, 2017

MylesBorins added land-on-v6.x and removed lts-watch-v6.x labels Aug 14, 2017

MylesBorins mentioned this pull request Aug 16, 2017

v6.11.3-proposal #14852

Merged

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: only call FatalException if not verbose #12826

src: only call FatalException if not verbose #12826

danbev commented May 4, 2017

jasnell commented May 4, 2017

cjihrig left a comment

danbev commented May 4, 2017

addaleax left a comment

danbev commented May 9, 2017

bnoordhuis left a comment

danbev commented May 15, 2017

danbev commented May 15, 2017

danbev commented May 16, 2017

bnoordhuis commented May 16, 2017

danbev commented May 17, 2017

bnoordhuis commented May 17, 2017

danbev commented May 17, 2017

danbev commented May 18, 2017

MylesBorins commented Jun 22, 2017

addaleax commented Jun 22, 2017

MylesBorins commented Jul 17, 2017

src: only call FatalException if not verbose #12826

src: only call FatalException if not verbose #12826

Conversation

danbev commented May 4, 2017

Checklist

Affected core subsystem(s)

jasnell commented May 4, 2017

cjihrig left a comment

Choose a reason for hiding this comment

danbev commented May 4, 2017

addaleax left a comment

Choose a reason for hiding this comment

danbev commented May 9, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

danbev commented May 15, 2017

danbev commented May 15, 2017

danbev commented May 16, 2017

bnoordhuis commented May 16, 2017

danbev commented May 17, 2017

bnoordhuis commented May 17, 2017

danbev commented May 17, 2017

danbev commented May 18, 2017

MylesBorins commented Jun 22, 2017

addaleax commented Jun 22, 2017

MylesBorins commented Jul 17, 2017