Use ExceptionState to throw exceptions when converting arrays

Use ExceptionState to throw exceptions when converting arrays

In the to*NativeArray(), toImplArray() and toImplArguments() helpers,
throw all exceptions via an ExceptionState& argument.

To facilitate this, also add an ExceptionState& argument to the various
NativeValueTraits<>::nativeValue() implementations, and add a
V8StringValue::prepare() variant that takes an ExceptionState& argument.

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

[Jens Widell, 2014-09-11 13:22:30]

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

[Jens Widell, 2014-09-11 13:22:31]

PTAL

https://codereview.chromium.org/555133003/diff/1/Source/bindings/core/v8/Dict...
File Source/bindings/core/v8/DictionaryHelperForCore.cpp (right):

https://codereview.chromium.org/555133003/diff/1/Source/bindings/core/v8/Dict...
Source/bindings/core/v8/DictionaryHelperForCore.cpp:278: // FIXME (before
landing): We can't use a NonThrowableExceptionState here; figure out what to do
instead.
Need to resolve this. I thought I couldn't create a full ExceptionState here,
but maybe I can, using dictionary.v8value() for the |creationContext| argument?

Otherwise passing in an ExceptionState& to this method would be the natural
approach, I guess, but that will expand the patch even more, and I wasn't sure
if we really wanted to spend too much effort on Dictionary/DictionaryHelper at
this point.

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

https://codereview.chromium.org/555133003/diff/1/Source/bindings/core/v8/V8Bi...
Source/bindings/core/v8/V8Binding.cpp:937: V8StringResource<>
stringResource(origin.ResourceName());
It feels like maybe we could do something better here, especially given that
ResourceName(), if not empty, ought to be a string, and thus not really need
conversion logic at all. Or can the resource name be something other than a
string?

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

https://codereview.chromium.org/555133003/diff/1/Source/bindings/core/v8/V8Bi...
Source/bindings/core/v8/V8Binding.h:799:
result.uncheckedAppend(TraitsType::nativeValue(object->Get(i), isolate));
The change here (and in the other functions) to add a TryCatch and check it
after object->Get(i), fixes a crash (that could also have been fixed by checking
element.IsEmpty()):

var a=[];Object.defineProperty(a,0,{get:function(){throw
TypeError}});navigator.vibrate(a)

[haraken, 2014-09-11 13:38:12]

haraken@chromium.org changed reviewers:
+ bashi@chromium.org

[haraken, 2014-09-11 13:38:13]

I think bashi-san is solving a similar issue in a similar approach (how to
create an ExceptionState in Dictionary).

bashi-san: Would you comment on this patch?

[haraken, 2014-09-11 13:38:26]

On 2014/09/11 13:38:13, haraken wrote:
> I think bashi-san is solving a similar issue in a similar approach (how to
> create an ExceptionState in Dictionary).
> 
> bashi-san: Would you comment on this patch?

c.f., https://codereview.chromium.org/534133002/

[bashi, 2014-09-12 01:06:35]

On 2014/09/11 13:38:26, haraken wrote:
> On 2014/09/11 13:38:13, haraken wrote:
> > I think bashi-san is solving a similar issue in a similar approach (how to
> > create an ExceptionState in Dictionary).
> > 
> > bashi-san: Would you comment on this patch?
> 
> c.f., https://codereview.chromium.org/534133002/

I think this is a great change; I added nativeValueMayFail() functions in my CL,
but I was also thinking about adding an ExceptionState argument to nativeValue()
functions to avoid duplication. I think I can use this change when I revisit
DictionaryHelper refactoring:)

[Jens Widell, 2014-09-12 11:34:39]

https://codereview.chromium.org/555133003/diff/60001/Source/bindings/core/v8/...
File Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/555133003/diff/60001/Source/bindings/core/v8/...
Source/bindings/core/v8/V8Binding.h:625: bool
toV8Sequence(v8::Handle<v8::Value>, uint32_t& length, v8::Isolate*,
ExceptionState&);
Extended patch to cover toV8Sequence() too.

Its error handling was broken before: it returned a (non-empty) handle
containing undefined to signal to callers that they should throw a TypeError, or
a handle containing the exception, if one was thrown when accessing the object's
|length| property. And if all went well, toV8Sequence() returned a new handle
containing the input value (unchanged.)

What all callers did however was: If return value is an empty handle, throw a
TypeError, otherwise go ahead and use the "fetched" length, which would always
be zero if toV8Sequence() actually failed. No TypeError exceptions were thrown,
and no exceptions were propagated.

It now takes an ExceptionState& argument, and propagates exceptions via it. It
also signals success or failure via its boolean return value instead of
returning a handle. If it returns false (failed), callers need to check the
ExceptionState object: if it has an exception, then that's it, otherwise callers
should throw a TypeError.

https://codereview.chromium.org/555133003/diff/120001/Source/bindings/core/v8...
File Source/bindings/core/v8/DictionaryHelperForCore.cpp (right):

https://codereview.chromium.org/555133003/diff/120001/Source/bindings/core/v8...
Source/bindings/core/v8/DictionaryHelperForCore.cpp:635: bool
DictionaryHelper::convert(const Dictionary& dictionary,
Dictionary::ConversionContext& context, const String& key, MessagePortArray&
value)
So I realized the only caller of the problematic MessagePortArray get() variant
was this MessagePortArray convert() variant, which has an ExceptionState
available. Seemed reasonable then to do the conversion to MessagePortArray in
convert() instead, and drop the get() variant.

[Jens Widell, 2014-09-24 06:53:02]

haraken@: Friendly ping? :-)

[bashi, 2014-09-24 08:23:05]

LGTM (Please wait for Haraken-san's review though)

https://codereview.chromium.org/555133003/diff/160001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/555133003/diff/160001/Source/bindings/core/v8...
Source/bindings/core/v8/V8Binding.h:844: // The caller is responsible for
reporting a TypeError.
Now we have exceptionState. How about throwing a TypeError here and L864? I
think we can remove "if (!exceptionState.hadException())" blocks from call
sites.

[Jens Widell, 2014-09-24 08:29:50]

https://codereview.chromium.org/555133003/diff/160001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/555133003/diff/160001/Source/bindings/core/v8...
Source/bindings/core/v8/V8Binding.h:844: // The caller is responsible for
reporting a TypeError.
On 2014/09/24 08:23:05, bashi1 wrote:
> Now we have exceptionState. How about throwing a TypeError here and L864? I
> think we can remove "if (!exceptionState.hadException())" blocks from call
> sites.

Callers construct an error message containing either an argument index or a
property name, so to preserve behavior, we'd then need to pass that information
to this function instead.

[Jens Widell, 2014-09-24 08:29:51]

https://codereview.chromium.org/555133003/diff/160001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8Binding.h (right):

https://codereview.chromium.org/555133003/diff/160001/Source/bindings/core/v8...
Source/bindings/core/v8/V8Binding.h:844: // The caller is responsible for
reporting a TypeError.
On 2014/09/24 08:23:05, bashi1 wrote:
> Now we have exceptionState. How about throwing a TypeError here and L864? I
> think we can remove "if (!exceptionState.hadException())" blocks from call
> sites.

Callers construct an error message containing either an argument index or a
property name, so to preserve behavior, we'd then need to pass that information
to this function instead.

[bashi, 2014-09-24 08:34:37]

On 2014/09/24 08:29:51, Jens Widell wrote:
>
https://codereview.chromium.org/555133003/diff/160001/Source/bindings/core/v8...
> File Source/bindings/core/v8/V8Binding.h (right):
> 
>
https://codereview.chromium.org/555133003/diff/160001/Source/bindings/core/v8...
> Source/bindings/core/v8/V8Binding.h:844: // The caller is responsible for
> reporting a TypeError.
> On 2014/09/24 08:23:05, bashi1 wrote:
> > Now we have exceptionState. How about throwing a TypeError here and L864? I
> > think we can remove "if (!exceptionState.hadException())" blocks from call
> > sites.
> 
> Callers construct an error message containing either an argument index or a
> property name, so to preserve behavior, we'd then need to pass that
information
> to this function instead.

Thanks for explanation. Passing such information doesn't sound a good idea then.

[haraken, 2014-09-24 09:29:49]

Sorry about the review delay. LGTM.

https://codereview.chromium.org/555133003/diff/160001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8StringResource.h (right):

https://codereview.chromium.org/555133003/diff/160001/Source/bindings/core/v8...
Source/bindings/core/v8/V8StringResource.h:202: if (prepareSimple())

prepareFast ? (We normally add "Fast" for a fast path method.)

[Jens Widell, 2014-09-24 10:00:19]

Thank you very much for reviewing, haraken and bashi!

https://codereview.chromium.org/555133003/diff/160001/Source/bindings/core/v8...
File Source/bindings/core/v8/V8StringResource.h (right):

https://codereview.chromium.org/555133003/diff/160001/Source/bindings/core/v8...
Source/bindings/core/v8/V8StringResource.h:202: if (prepareSimple())
On 2014/09/24 09:29:49, haraken wrote:
> 
> prepareFast ? (We normally add "Fast" for a fast path method.)

Done.

[Jens Widell, 2014-09-24 11:25:49]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2014-09-24 11:26:34]

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

[commit-bot: I haz the power, 2014-09-24 13:04:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-24 13:04:52]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/28561)

[Jens Widell, 2014-09-24 13:06:30]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2014-09-24 13:06:45]

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

[commit-bot: I haz the power, 2014-09-24 14:53:31]

Committed patchset #11 (id:200001) as 182602