In V8Binding.cpp, throw all exceptions via ExceptionState& argument

In V8Binding.cpp, throw all exceptions via ExceptionState& argument

In those the conversion functions that take an ExceptionState& argument,
always throw via the ExceptionState object, and never directly to V8.
Various existing callers assume this, and fail to detect exceptions that
were thrown directly to V8.

This misbehavior is a regression from
  https://codereview.chromium.org/313033002/
which changed the definition of the TONATIVE_DEFAULT_EXCEPTIONSTATE()
macro, but these functions were somewhat tricky to use even before then,
since exceptions were sometimes thrown to V8 and sometimes not, while
always stored in the ExceptionState object.

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

[Jens Widell, 2014-09-09 15:06:37]

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

[Jens Widell, 2014-09-09 15:06:38]

PTAL

I chose not to introduce yet another TONATIVE_* macro variant for this use-case,
since it's not really a lot of code, and since macros don't necessarily help
readability or maintainability.

yhirano@: I claimed in the description that this fixes a regression from your
CL. As I recall it though, I suggested the macro changes that caused it, so...
my bad. :-)

https://codereview.chromium.org/559553003/diff/1/Source/bindings/core/v8/V8Bi...
File Source/bindings/core/v8/V8Binding.cpp (left):

https://codereview.chromium.org/559553003/diff/1/Source/bindings/core/v8/V8Bi...
Source/bindings/core/v8/V8Binding.cpp:331:
TONATIVE_DEFAULT_EXCEPTIONSTATE(int32_t, result, numberObject->Int32Value(),
exceptionState, 0);
I assumed here that since numberObject is a number already (was returned by
ToNumber()), Int32Value() can't actually throw an exception.

https://codereview.chromium.org/559553003/diff/1/Source/bindings/core/v8/V8Bi...
File Source/bindings/core/v8/V8Binding.cpp (right):

https://codereview.chromium.org/559553003/diff/1/Source/bindings/core/v8/V8Bi...
Source/bindings/core/v8/V8Binding.cpp:212: if (numberObject.IsEmpty()) {
Question: Can this be true? It would be if value->ToNumber() threw an exception,
but then we won't reach this line.

[Jens Widell, 2014-09-09 15:48:42]

With this fixed, we can follow up with this:
https://codereview.chromium.org/552143004

[haraken, 2014-09-10 01:09:24]

LGTM

https://codereview.chromium.org/559553003/diff/1/Source/bindings/core/v8/V8Bi...
File Source/bindings/core/v8/V8Binding.cpp (right):

https://codereview.chromium.org/559553003/diff/1/Source/bindings/core/v8/V8Bi...
Source/bindings/core/v8/V8Binding.cpp:212: if (numberObject.IsEmpty()) {
On 2014/09/09 15:06:38, Jens Widell wrote:
> Question: Can this be true? It would be if value->ToNumber() threw an
exception,
> but then we won't reach this line.

You're right. This should be ASSERT(!numberObject.IsEmpty()).

https://codereview.chromium.org/559553003/diff/1/Source/bindings/core/v8/V8Bi...
Source/bindings/core/v8/V8Binding.cpp:213: exceptionState.throwTypeError("Not
convertible to a number value (of type '" + String(typeName) + "'.");

Do we want to put this message into the exceptionState?

[Jens Widell, 2014-09-10 04:42:27]

https://codereview.chromium.org/559553003/diff/1/Source/bindings/core/v8/V8Bi...
File Source/bindings/core/v8/V8Binding.cpp (right):

https://codereview.chromium.org/559553003/diff/1/Source/bindings/core/v8/V8Bi...
Source/bindings/core/v8/V8Binding.cpp:213: exceptionState.throwTypeError("Not
convertible to a number value (of type '" + String(typeName) + "'.");
On 2014/09/10 01:09:23, haraken wrote:
> 
> Do we want to put this message into the exceptionState?

Don't know if I understand what you mean. Elaborate?

[haraken, 2014-09-10 04:49:49]

https://codereview.chromium.org/559553003/diff/1/Source/bindings/core/v8/V8Bi...
File Source/bindings/core/v8/V8Binding.cpp (right):

https://codereview.chromium.org/559553003/diff/1/Source/bindings/core/v8/V8Bi...
Source/bindings/core/v8/V8Binding.cpp:213: exceptionState.throwTypeError("Not
convertible to a number value (of type '" + String(typeName) + "'.");
On 2014/09/10 04:42:27, Jens Widell wrote:
> On 2014/09/10 01:09:23, haraken wrote:
> > 
> > Do we want to put this message into the exceptionState?
> 
> Don't know if I understand what you mean. Elaborate?

Sorry for not being clear.

There are two ways to throw the exception:

if (block.HasCaught()) {
    exceptionState.rethrowV8Exception(block.Exception());
    return 0;
}

or:

if (block.HasCaught()) {
    exceptionState.throwTypeError("Not convertible to a number value (of type '"
+ String(typeName) + "'.");
    return 0;
}

I thought that the latter would be more developer-friendly.

[Jens Widell, 2014-09-10 04:56:21]

On 2014/09/10 04:49:49, haraken wrote:
>
https://codereview.chromium.org/559553003/diff/1/Source/bindings/core/v8/V8Bi...
> File Source/bindings/core/v8/V8Binding.cpp (right):
> 
>
https://codereview.chromium.org/559553003/diff/1/Source/bindings/core/v8/V8Bi...
> Source/bindings/core/v8/V8Binding.cpp:213: exceptionState.throwTypeError("Not
> convertible to a number value (of type '" + String(typeName) + "'.");
> On 2014/09/10 04:42:27, Jens Widell wrote:
> > On 2014/09/10 01:09:23, haraken wrote:
> > > 
> > > Do we want to put this message into the exceptionState?
> > 
> > Don't know if I understand what you mean. Elaborate?
> 
> Sorry for not being clear.
> 
> There are two ways to throw the exception:
> 
> if (block.HasCaught()) {
>     exceptionState.rethrowV8Exception(block.Exception());
>     return 0;
> }
> 
> or:
> 
> if (block.HasCaught()) {
>     exceptionState.throwTypeError("Not convertible to a number value (of type
'"
> + String(typeName) + "'.");
>     return 0;
> }
> 
> I thought that the latter would be more developer-friendly.

Ah, right. I don't know. ToNumber() would typically throw if the value is an
object and its valueOf() (or toString()) throws. It might do that because of a
bug in the developer's code, and then replacing the exception here would make it
more difficult to diagnose.

[Jens Widell, 2014-09-10 05:33:57]

https://codereview.chromium.org/559553003/diff/1/Source/bindings/core/v8/V8Bi...
File Source/bindings/core/v8/V8Binding.cpp (right):

https://codereview.chromium.org/559553003/diff/1/Source/bindings/core/v8/V8Bi...
Source/bindings/core/v8/V8Binding.cpp:212: if (numberObject.IsEmpty()) {
On 2014/09/10 01:09:23, haraken wrote:
> On 2014/09/09 15:06:38, Jens Widell wrote:
> > Question: Can this be true? It would be if value->ToNumber() threw an
> exception,
> > but then we won't reach this line.
> 
> You're right. This should be ASSERT(!numberObject.IsEmpty()).

Done.

[haraken, 2014-09-10 07:04:19]

On 2014/09/10 04:56:21, Jens Widell wrote:
> On 2014/09/10 04:49:49, haraken wrote:
> >
>
https://codereview.chromium.org/559553003/diff/1/Source/bindings/core/v8/V8Bi...
> > File Source/bindings/core/v8/V8Binding.cpp (right):
> > 
> >
>
https://codereview.chromium.org/559553003/diff/1/Source/bindings/core/v8/V8Bi...
> > Source/bindings/core/v8/V8Binding.cpp:213:
exceptionState.throwTypeError("Not
> > convertible to a number value (of type '" + String(typeName) + "'.");
> > On 2014/09/10 04:42:27, Jens Widell wrote:
> > > On 2014/09/10 01:09:23, haraken wrote:
> > > > 
> > > > Do we want to put this message into the exceptionState?
> > > 
> > > Don't know if I understand what you mean. Elaborate?
> > 
> > Sorry for not being clear.
> > 
> > There are two ways to throw the exception:
> > 
> > if (block.HasCaught()) {
> >     exceptionState.rethrowV8Exception(block.Exception());
> >     return 0;
> > }
> > 
> > or:
> > 
> > if (block.HasCaught()) {
> >     exceptionState.throwTypeError("Not convertible to a number value (of
type
> '"
> > + String(typeName) + "'.");
> >     return 0;
> > }
> > 
> > I thought that the latter would be more developer-friendly.
> 
> Ah, right. I don't know. ToNumber() would typically throw if the value is an
> object and its valueOf() (or toString()) throws. It might do that because of a
> bug in the developer's code, and then replacing the exception here would make
it
> more difficult to diagnose.

Makes sense. LGTM.

[Jens Widell, 2014-09-10 07:05:22]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2014-09-10 07:06:13]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jl@opera.com/559553003/20001

[commit-bot: I haz the power, 2014-09-10 07:09:31]

Committed patchset #2 (id:20001) as 181709

[yhirano, 2014-09-10 07:19:01]

Sorry for being late, lgtm