Make exception throwing more robust in V8 out-of-stack situations

Make exception throwing more robust in V8 out-of-stack situations

If V8 throws an exception when Blink tries to create an exception to
throw, return V8's exception instead of undefined, since it makes sense
in that case to propagate V8's exception.

BUG=454607
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189489

[Jens Widell, 2015-02-04 09:37:32]

jl@opera.com changed reviewers:
+ haraken@chromium.org

[Jens Widell, 2015-02-04 09:37:33]

PTAL

I guess we might also want to RELEASE_ASSERT() that we're actually throwing an
exception when we mean to. We could do that by addressing the FIXME in
ExceptionState::setException() for instance. WDYT?

[haraken, 2015-02-04 09:41:42]

https://codereview.chromium.org/871403007/diff/1/Source/bindings/core/v8/V8Th...
File Source/bindings/core/v8/V8ThrowException.cpp (right):

https://codereview.chromium.org/871403007/diff/1/Source/bindings/core/v8/V8Th...
Source/bindings/core/v8/V8ThrowException.cpp:82: }

I might be misunderstanding but why is this not like:

if (tryCatch.HasCaught()) {
  ASSERT(exception.IsEmpty());
  return tryCatch.Exception();
}
ASSERT(!exception.IsEmpty());

?

[Jens Widell, 2015-02-04 09:48:02]

https://codereview.chromium.org/871403007/diff/1/Source/bindings/core/v8/V8Th...
File Source/bindings/core/v8/V8ThrowException.cpp (right):

https://codereview.chromium.org/871403007/diff/1/Source/bindings/core/v8/V8Th...
Source/bindings/core/v8/V8ThrowException.cpp:82: }
On 2015/02/04 09:41:41, haraken wrote:
> 
> I might be misunderstanding but why is this not like:
> 
> if (tryCatch.HasCaught()) {
>   ASSERT(exception.IsEmpty());
>   return tryCatch.Exception();
> }
> ASSERT(!exception.IsEmpty());
> 
> ?

Oups, this was meant to be "if (exception.IsEmpty())" still. The added negation
is a left-over from a slightly different change that I made and then decided
against, and failed to undo completely.

I have no reason to doubt those are accurate assertions. I'll change to that.

[Jens Widell, 2015-02-04 09:50:52]

https://codereview.chromium.org/871403007/diff/1/Source/bindings/core/v8/V8Th...
File Source/bindings/core/v8/V8ThrowException.cpp (right):

https://codereview.chromium.org/871403007/diff/1/Source/bindings/core/v8/V8Th...
Source/bindings/core/v8/V8ThrowException.cpp:82: }
On 2015/02/04 09:48:01, Jens Widell wrote:
> On 2015/02/04 09:41:41, haraken wrote:
> > 
> > I might be misunderstanding but why is this not like:
> > 
> > if (tryCatch.HasCaught()) {
> >   ASSERT(exception.IsEmpty());
> >   return tryCatch.Exception();
> > }
> > ASSERT(!exception.IsEmpty());
> > 
> > ?
> 
> Oups, this was meant to be "if (exception.IsEmpty())" still. The added
negation
> is a left-over from a slightly different change that I made and then decided
> against, and failed to undo completely.
> 
> I have no reason to doubt those are accurate assertions. I'll change to that.

Done.

[haraken, 2015-02-04 10:52:09]

LGTM

[Jens Widell, 2015-02-04 10:53:27]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2015-02-04 10:53:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/871403007/10003

[commit-bot: I haz the power, 2015-02-04 11:54:56]

Committed patchset #2 (id:10003) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189489

[leviw_travelin_and_unemployed, 2015-02-04 20:07:33]

leviw@chromium.org changed reviewers:
+ leviw@chromium.org

[leviw_travelin_and_unemployed, 2015-02-04 20:07:33]

Why no test?