node.cc: break on uncaught exceptions

[bajtos]

All TryCatch blocks used in node.cc are now verbose, so that V8 debugger
can break on uncaught exceptions.

See comment in deps/v8/include/v8.h for explanation:

By default, exceptions that are caught by an external exception
handler are not reported. Call SetVerbose with true on an
external exception handler to have exceptions caught by the
handler reported as if they were not caught.

The flag is used by Isolate::ShouldReportException(), which is called
by Isolate::DoThrow() to decide whether an exception is considered uncaught.

/cc: @bnoordhuis

[Nodejs-Jenkins]

Thank you for contributing this pull request! Here are a few pointers to make sure your submission will be considered for inclusion.

Commit bajtos/node@8b7cfe0 has the following error(s):

• Commit message line too long: 12

You can fix all these things without opening another issue.

Please see CONTRIBUTING.md for more information

[bajtos]

@bnoordhuis I have submitted a first draft for review. I am not happy with duplicating SetVerbose(true) all over the place.

Perhaps I could add a lightweight descendant of TryCatch called NodeTryCatch that will call SetVerbose(true) in the constructor?

Any better idea?

[bnoordhuis]

Perhaps I could add a lightweight descendant of TryCatch called NodeTryCatch that will call SetVerbose(true) in the constructor?

That doesn't really bother me. There are only a few TryCatch call sites (you missed one in src/node_script.cc by the way) and there should only be fewer over time.

I'm not sure what the repercussions are of this change but I guess we can land it in master and try it out. Is this the final version of your PR?

[bajtos]

Perhaps I could add a lightweight descendant of TryCatch called NodeTryCatch that will call SetVerbose(true) in the constructor?

That doesn't really bother me. There are only a few TryCatch call sites (you missed one in src/node_script.cc by the way) and there should only be fewer over time.

Our usual disagreement about coding style, heh? My concern is that it was already easy to miss one of the six places required to be changed, so I'd rather make sure the next person doing a similar change can avoid this risk.

Anyway, you are the boss.

I'm not sure what the repercussions are of this change but I guess we can land it in master and try it out.

Yeah, I am not entirely sure either. My understanding is that this change affects only Isolate::DoThrow() - see the condition

// Generate the message if required.
if (report_exception || try_catch_needs_message) {

Since try_catch_need_message is true by default, the condition is fullfilled regardless of report_exception (that was false before this change and it is true now). Hopefully this means that my change does not introduce any performance regression when running without a debugger.

Is this the final version of your PR?

I added the missing bit in src/node_script.cc and rebased on master's HEAD, the PR is ready for merge now.

[bnoordhuis]

My concern is that it was already easy to miss one of the six places required to be changed, so I'd rather make sure the next person doing a similar change can avoid this risk.

It's nothing git grep -w TryCatch src/ won't catch.

This PR breaks a number of tests (see here.) The tests that break locally for me all seem to involve child processes, oddly enough. Might just be coincidence.

[bajtos]

This PR breaks a number of tests.

Oh well. There are two additional lines printed for unhandled exception now. Apparently the verbose flag is used on other places too.

E.g. for test/message/timeout_throw.js

Old output:

/Users/bajtos/src/node/test/message/timeout_throw.js:26
  undefined_reference_error_maker;
  ^
ReferenceError: undefined_reference_error_maker is not defined
    at null._onTimeout (/Users/bajtos/src/node/test/message/timeout_throw.js:26:3)
    at Timer.listOnTimeout [as ontimeout] (timers.js:105:15)

New output:

/Users/bajtos/src/node/test/message/timeout_throw.js:1290: Uncaught ReferenceError: undefined_reference_error_maker is not defined

/Users/bajtos/src/node/test/message/timeout_throw.js:26
  undefined_reference_error_maker;
  ^
ReferenceError: undefined_reference_error_maker is not defined
    at null._onTimeout (/Users/bajtos/src/node/test/message/timeout_throw.js:26:3)
    at Timer.listOnTimeout [as ontimeout] (timers.js:105:15)

The line-numer 1290 is weird, since timeout_throw.js has only 27 lines.

[bajtos]

It's V8 reporting the exception to stdout/stderr, I guess that was the original intention behind SetVerbose flag.

See deps/v8/src/isolate.cc:1392:

// Save the message for reporting if the the exception remains uncaught.
thread_local_top()->has_pending_message_ = report_exception;

I am not sure what is the correct solution here.

• Supress logging of uncaught exceptions in V8, since we are logging them ourselves? (But then are we sure the only uncaught exceptions are those coming from node.js code?)
• Add yet another flag to TryCatch to distinguish between "this exception should be considered as uncaught by debugger" and "this exception should be logged to stdout/stderr"?
• Any other options?

What are your thoughts, Ben?

[bnoordhuis]

While you're here... style: no space between function name and parenthesis.

[bajtos]

Thanks for the review, I addressed the issues in 1ebae5c.

@bnoordhuis: What's your opinion on FatalException() checking if TryCatch is verbose?

The current state is this:

• Existing native modules have is_verbose_ false, so they are not affected.
• The person adding SetVerbose(true) should be aware that he must not call FatalException
• FatalException is called twice only if the person above makes a mistake and calls both SetVerbose(true) and FatalException on error.

Given that, is it worth the effort (and extra code) to detect such mistakes and do not log the error second time? Does it happen often that a native module adds it's own TryCatch block?

[bnoordhuis]

Landed in c16963b, thanks.

Does it happen often that a native module adds it's own TryCatch block?

Depends on your definition of 'often'. It's not common but it's not extremely rare either.

Given that, is it worth the effort (and extra code) to detect such mistakes and do not log the error second time?

Probably not. Let's take the reactive approach and not do anything until someone complains.