Reporting uncaught errors at the boundary between C++ and JS instead of tryin...

Reporting uncaught errors at the boundary between C++ and JS instead of trying to guess whether they get caught at the time of the throw.

Committed: http://code.google.com/p/v8/source/detail?r=915

[Mads Ager (chromium), 2008-12-03 15:44:48]

Overall, this looks good to me.  I'm puzzled by the checking going on after one
callback though.  We have many more callbacks in the code.  Why is the checking
only needed for one of them?

I think it would be good if Søren could have a look at this change as well since
he did the latest changes in this area.

http://codereview.chromium.org/12901/diff/1/5
File src/builtins.cc (right):

http://codereview.chromium.org/12901/diff/1/5#newcode389
Line 389: Top::ReportPendingMessages();
I'm not sure I understand why we need to check here?  There are a lot of places
in the code where we invoke a c++ callback function.  If we need to check here,
don't we need to check in the other places as well?

http://codereview.chromium.org/12901/diff/1/6
File src/execution.cc (right):

http://codereview.chromium.org/12901/diff/1/6#newcode102
Line 102: Top::setup_external_caught();
Why is setup_external_caught needed in the case that there was no exception?

http://codereview.chromium.org/12901/diff/1/3
File src/top.cc (right):

http://codereview.chromium.org/12901/diff/1/3#newcode859
Line 859: = thread_local_.pending_exception_;
We normally have the '=' on the previous line.

http://codereview.chromium.org/12901/diff/1/2
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/12901/diff/1/2#newcode3000
Line 3000: Script::Compile(v8_str("(function() { try { throw ''; } finally {
return; } })()"))->Run();
These two lines seem too long?  Break them by using the C++ concatenation of
adjacent strings?

[Søren Thygesen Gjesse, 2008-12-04 07:53:53]

Deferring the reporting is probably the only way to avoid reporting uncaught
exceptions which are actually canceled by  return from within finally. The
complexity of the exception handling makes it difficult to reason about changes
so I guess we will have to rely on our tests.

http://codereview.chromium.org/12901/diff/1/6
File src/execution.cc (right):

http://codereview.chromium.org/12901/diff/1/6#newcode101
Line 101: Top::clear_pending_message();
If there is no pending exception clearing it should not be needed.

http://codereview.chromium.org/12901/diff/1/3
File src/top.cc (right):

http://codereview.chromium.org/12901/diff/1/3#newcode742
Line 742: bool Top::IsUncaughtException(bool* is_caught_externally) {
This function does not only check whether the exception is uncaught as if it is
caught by a C++ TryCatch it also checks whether that handler is verbose. This
should be taken into account with the name change (perhaps adding another
reference parameter) and the comments should also be changed as they still
mention reporting the exception.

http://codereview.chromium.org/12901/diff/1/3#newcode838
Line 838: // NOTE: Notifying the debugger may have caused new exceptions.
Generating the message without reporting it can also cause a new exception.

http://codereview.chromium.org/12901/diff/1/2
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/12901/diff/1/2#newcode2991
Line 2991: TEST(TryCatchFinallyUsingMessaging) {
I suggest to change this test into one or more message test written in
JavaScript. We already have most of the try-catch-finally covered by a set of
message tests, with exception cancellation by returning from finally missing.

[olehougaard, 2008-12-04 08:44:35]

http://codereview.chromium.org/12901/diff/1/5
File src/builtins.cc (right):

http://codereview.chromium.org/12901/diff/1/5#newcode389
Line 389: Top::ReportPendingMessages();
On 2008/12/03 15:44:48, Mads Ager wrote:
> I'm not sure I understand why we need to check here?  There are a lot of
places
> in the code where we invoke a c++ callback function.  If we need to check
here,
> don't we need to check in the other places as well?

This was a remnant from an earlier effort to get the tests working. Removed.

http://codereview.chromium.org/12901/diff/1/6
File src/execution.cc (right):

http://codereview.chromium.org/12901/diff/1/6#newcode101
Line 101: Top::clear_pending_message();
On 2008/12/04 07:53:53, Søren Gjesse wrote:
> If there is no pending exception clearing it should not be needed.

Removed.

http://codereview.chromium.org/12901/diff/1/6#newcode102
Line 102: Top::setup_external_caught();
On 2008/12/03 15:44:48, Mads Ager wrote:
> Why is setup_external_caught needed in the case that there was no exception?

It isn't. Removed.

http://codereview.chromium.org/12901/diff/1/3
File src/top.cc (right):

http://codereview.chromium.org/12901/diff/1/3#newcode742
Line 742: bool Top::IsUncaughtException(bool* is_caught_externally) {
On 2008/12/04 07:53:53, Søren Gjesse wrote:
> This function does not only check whether the exception is uncaught as if it
is
> caught by a C++ TryCatch it also checks whether that handler is verbose. This
> should be taken into account with the name change (perhaps adding another
> reference parameter) and the comments should also be changed as they still
> mention reporting the exception.

Reverting to the original name.

http://codereview.chromium.org/12901/diff/1/3#newcode838
Line 838: // NOTE: Notifying the debugger may have caused new exceptions.
On 2008/12/04 07:53:53, Søren Gjesse wrote:
> Generating the message without reporting it can also cause a new exception.

Updated the comment.

http://codereview.chromium.org/12901/diff/1/3#newcode859
Line 859: = thread_local_.pending_exception_;
On 2008/12/03 15:44:48, Mads Ager wrote:
> We normally have the '=' on the previous line.

Fixed.

http://codereview.chromium.org/12901/diff/1/2
File test/cctest/test-api.cc (right):

http://codereview.chromium.org/12901/diff/1/2#newcode2991
Line 2991: TEST(TryCatchFinallyUsingMessaging) {
On 2008/12/04 07:53:53, Søren Gjesse wrote:
> I suggest to change this test into one or more message test written in
> JavaScript. We already have most of the try-catch-finally covered by a set of
> message tests, with exception cancellation by returning from finally missing.

Fixed.

http://codereview.chromium.org/12901/diff/1/2#newcode3000
Line 3000: Script::Compile(v8_str("(function() { try { throw ''; } finally {
return; } })()"))->Run();
On 2008/12/03 15:44:48, Mads Ager wrote:
> These two lines seem too long?  Break them by using the C++ concatenation of
> adjacent strings?

Fixed