Preserve message when rethrowing exception

Preserve message when rethrowing exception

A new message was always generated if there is a the top-most verbose
TryCatch, even when rethrowing an exception from a TryCatch that is going
out of scope, and we already have a message.

BUG=v8:3583
LOG=Y
R=yangguo@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=24152

[Jens Widell, 2014-09-19 15:32:28]

jl@opera.com changed reviewers:
+ mstarzinger@chromium.org, yangguo@chromium.org

[Jens Widell, 2014-09-19 15:32:29]

PTAL

The change in https://codereview.chromium.org/576323004/ caused a Blink layout
test to start failing due to an exception message being reported as having been
thrown on a different (more correct) line. This patch causes the same change in
behavior as that Blink patch.

[Yang, 2014-09-22 15:26:31]

On 2014/09/19 15:32:29, Jens Widell wrote:
> PTAL
> 
> The change in https://codereview.chromium.org/576323004/ caused a Blink layout
> test to start failing due to an exception message being reported as having
been
> thrown on a different (more correct) line. This patch causes the same change
in
> behavior as that Blink patch.

LGTM. Will land after I run a couple of tests.

[Jens Widell, 2014-09-23 06:13:35]

On 2014/09/22 15:26:31, Yang wrote:
> On 2014/09/19 15:32:29, Jens Widell wrote:
> > PTAL
> > 
> > The change in https://codereview.chromium.org/576323004/ caused a Blink
layout
> > test to start failing due to an exception message being reported as having
> been
> > thrown on a different (more correct) line. This patch causes the same change
> in
> > behavior as that Blink patch.
> 
> LGTM. Will land after I run a couple of tests.

Thanks!

Beware that this patch could cause more test failures similar to what I
encountered in https://codereview.chromium.org/576323004/, and thus might be
tricky to land. I'm not aware of any other cases where the changed behaviour
affects a test, though. I'd of course be happy to help out on the Blink side if
this patch does cause any kind of issues.

[Yang, 2014-09-23 10:31:28]

On 2014/09/23 06:13:35, Jens Widell wrote:
> On 2014/09/22 15:26:31, Yang wrote:
> > On 2014/09/19 15:32:29, Jens Widell wrote:
> > > PTAL
> > > 
> > > The change in https://codereview.chromium.org/576323004/ caused a Blink
> layout
> > > test to start failing due to an exception message being reported as having
> > been
> > > thrown on a different (more correct) line. This patch causes the same
change
> > in
> > > behavior as that Blink patch.
> > 
> > LGTM. Will land after I run a couple of tests.
> 
> Thanks!
> 
> Beware that this patch could cause more test failures similar to what I
> encountered in https://codereview.chromium.org/576323004/, and thus might be
> tricky to land. I'm not aware of any other cases where the changed behaviour
> affects a test, though. I'd of course be happy to help out on the Blink side
if
> this patch does cause any kind of issues.

I ran webkit layout tests and found the following two test failures:
inspector/sources/debugger/rethrow-error-from-bindings-crash.html
storage/websql/sql-error-codes.html

It would be great to make sure those are just wrong expectations. We could then
mark them as NEEDS_MANUAL_REBASELINE before landing it to V8 and then after V8
has been rolled, rebaseline.

[Jens Widell, 2014-09-23 10:36:11]

On 2014/09/23 10:31:28, Yang wrote:
> On 2014/09/23 06:13:35, Jens Widell wrote:
> > On 2014/09/22 15:26:31, Yang wrote:
> > > On 2014/09/19 15:32:29, Jens Widell wrote:
> > > > PTAL
> > > > 
> > > > The change in https://codereview.chromium.org/576323004/ caused a Blink
> > layout
> > > > test to start failing due to an exception message being reported as
having
> > > been
> > > > thrown on a different (more correct) line. This patch causes the same
> change
> > > in
> > > > behavior as that Blink patch.
> > > 
> > > LGTM. Will land after I run a couple of tests.
> > 
> > Thanks!
> > 
> > Beware that this patch could cause more test failures similar to what I
> > encountered in https://codereview.chromium.org/576323004/, and thus might be
> > tricky to land. I'm not aware of any other cases where the changed behaviour
> > affects a test, though. I'd of course be happy to help out on the Blink side
> if
> > this patch does cause any kind of issues.
> 
> I ran webkit layout tests and found the following two test failures:
> inspector/sources/debugger/rethrow-error-from-bindings-crash.html
> storage/websql/sql-error-codes.html
> 
> It would be great to make sure those are just wrong expectations. We could
then
> mark them as NEEDS_MANUAL_REBASELINE before landing it to V8 and then after V8
> has been rolled, rebaseline.

The sql-error-codes.html test should be fixed with
https://codereview.chromium.org/576323004/ which landed (in Blink) an hour ago.

I'll investigate rethrow-error-from-bindings-crash.html, but since that's a test
specifically about rethrowing errors, it seems likely they trigger code that
does that, and thus would be (correctly) affected by this fix.

[Jens Widell, 2014-09-23 11:51:04]

On 2014/09/23 10:36:11, Jens Widell wrote:
> I'll investigate rethrow-error-from-bindings-crash.html, but since that's a
test
> specifically about rethrowing errors, it seems likely they trigger code that
> does that, and thus would be (correctly) affected by this fix.

Here's what I think is going on with this test:

When Isolate::DoThrow() calls Debug::OnThrow(), the pending message stored in
the isolate is lost. Without this patch, the message is regenerated again (by
accident; not because it was lost) so the fact that it was lost made no
particular difference. With this patch, we do not generate a new message, so
then the fact that it was lost, means no message is logged to the console.

Advise on what to do about this?

Semi-unrelated:

The test tests two methods, Node.appendChild() and
Range.compareBoundaryPoints(). The generated bindings code for the latter has a
v8::TryCatch local (always rethrown) while the former does not. Only the latter
has a problem. And we are getting rid of these v8::TryCatch objects. I can work
around this test failure by landing a CL that removes the v8::TryCatch.

[Jens Widell, 2014-09-23 12:13:17]

This CL works around the test failure:
https://codereview.chromium.org/592293003/

Note though that the way it does that is by completely eliminating the
v8::TryCatch::ReThrow() usage that the test is supposed to be testing in the
first place, so we should probably not be happy with just that.

Note also that the CL is not just a workaround; it's the right thing to do and
would have been done eventually either way.

[Yang, 2014-09-23 13:32:06]

On 2014/09/23 12:13:17, Jens Widell wrote:
> This CL works around the test failure:
> https://codereview.chromium.org/592293003/
> 
> Note though that the way it does that is by completely eliminating the
> v8::TryCatch::ReThrow() usage that the test is supposed to be testing in the
> first place, so we should probably not be happy with just that.
> 
> Note also that the CL is not just a workaround; it's the right thing to do and
> would have been done eventually either way.


Not sure I'm following. If I understand correctly, calling Debug::OnThrow
somehow clears the pending message, and with this CL, we end up with no message
at the end, hence test failure.
How does https://codereview.chromium.org/592293003/ fix this issue?

In any case, I guess it would be fine to land this once the work around lands?

[Jens Widell, 2014-09-23 13:43:11]

On 2014/09/23 13:32:06, Yang wrote:
> On 2014/09/23 12:13:17, Jens Widell wrote:
> > This CL works around the test failure:
> > https://codereview.chromium.org/592293003/
> > 
> > Note though that the way it does that is by completely eliminating the
> > v8::TryCatch::ReThrow() usage that the test is supposed to be testing in the
> > first place, so we should probably not be happy with just that.
> > 
> > Note also that the CL is not just a workaround; it's the right thing to do
and
> > would have been done eventually either way.
> 
> 
> Not sure I'm following. If I understand correctly, calling Debug::OnThrow
> somehow clears the pending message, and with this CL, we end up with no
message
> at the end, hence test failure.

Yes, that's correct. (Or so I think, at least.)


> How does https://codereview.chromium.org/592293003/ fix this issue?

It removes a v8::TryCatch in the generated bindings code. With this TryCatch, we
get two calls to Isolate::DoThrow():

1) Initial: has no message to begin with, calls Debug::OnThrow(), then generates
the message and stores in v8::TryCatch.

2) via ReThrow(): has message already, calls Debug::OnThrow(), then avoids
generating another message since it's supposed to be rethrowing with the message
from 1.

Without that TryCatch, we get only the one call to Isolate::DoThrow():

1) Initial: has no message, calls Debug::OnThrow(), then generates the message.


> In any case, I guess it would be fine to land this once the work around lands?

Well... it won't break layout tests, and we will have exchanged one buggy
behavior with another, and it's a bit unclear which is worse.

The old buggy behavior was that exception messages were sometimes (far from
always, I think) regenerated so that the exception would appear to have been
thrown at the call to one native/blink callback on the call stack when in fact
it had been thrown somewhere else.

The new buggy behavior is that exceptions are sometimes not reported to the
console at all, but only with a debugger attached, and only (I think) when the
debugger is configured to intercept the exception in question. IOW, either the
debugger or the console gets to see the message, but not both. (And still only
if there is an "extra" v8::TryCatch somewhere in the bindings layer.)

[Yang, 2014-09-23 13:55:16]

On 2014/09/23 13:43:11, Jens Widell wrote:
> On 2014/09/23 13:32:06, Yang wrote:
> > On 2014/09/23 12:13:17, Jens Widell wrote:
> > > This CL works around the test failure:
> > > https://codereview.chromium.org/592293003/
> > > 
> > > Note though that the way it does that is by completely eliminating the
> > > v8::TryCatch::ReThrow() usage that the test is supposed to be testing in
the
> > > first place, so we should probably not be happy with just that.
> > > 
> > > Note also that the CL is not just a workaround; it's the right thing to do
> and
> > > would have been done eventually either way.
> > 
> > 
> > Not sure I'm following. If I understand correctly, calling Debug::OnThrow
> > somehow clears the pending message, and with this CL, we end up with no
> message
> > at the end, hence test failure.
> 
> Yes, that's correct. (Or so I think, at least.)
> 
> 
> > How does https://codereview.chromium.org/592293003/ fix this issue?
> 
> It removes a v8::TryCatch in the generated bindings code. With this TryCatch,
we
> get two calls to Isolate::DoThrow():
> 
> 1) Initial: has no message to begin with, calls Debug::OnThrow(), then
generates
> the message and stores in v8::TryCatch.
> 
> 2) via ReThrow(): has message already, calls Debug::OnThrow(), then avoids
> generating another message since it's supposed to be rethrowing with the
message
> from 1.
> 
> Without that TryCatch, we get only the one call to Isolate::DoThrow():
> 
> 1) Initial: has no message, calls Debug::OnThrow(), then generates the
message.
> 
> 
> > In any case, I guess it would be fine to land this once the work around
lands?
> 
> Well... it won't break layout tests, and we will have exchanged one buggy
> behavior with another, and it's a bit unclear which is worse.
> 
> The old buggy behavior was that exception messages were sometimes (far from
> always, I think) regenerated so that the exception would appear to have been
> thrown at the call to one native/blink callback on the call stack when in fact
> it had been thrown somewhere else.
> 
> The new buggy behavior is that exceptions are sometimes not reported to the
> console at all, but only with a debugger attached, and only (I think) when the
> debugger is configured to intercept the exception in question. IOW, either the
> debugger or the console gets to see the message, but not both. (And still only
> if there is an "extra" v8::TryCatch somewhere in the bindings layer.)

Alright. As long as both CLs are good individually, and no tests break, I'm fine
with landing it. Waiting for https://codereview.chromium.org/592293003/ to land
first though.

[Jens Widell, 2014-09-23 14:21:41]

On 2014/09/23 13:55:16, Yang wrote:
> As long as both CLs are good individually, and no tests break, I'm fine
> with landing it.

I'm fine with landing it too. I reported
http://code.google.com/p/v8/issues/detail?id=3592 about the remaining issue.
(Would CC you, but I'm not allowed to.)

> Waiting for https://codereview.chromium.org/592293003/ to land first though.

It has now landed in Blink.

[Yang, 2014-09-23 14:29:53]

Committed patchset #1 (id:1) manually as 24152 (presubmit successful).