Structured cloning: improve DataCloneError reporting.

Structured cloning: improve DataCloneError reporting.

The spec tells us that cloning of neutered objects should throw
DataCloneError:

   http://www.whatwg.org/specs/web-apps/current-work/#safe-passing-of-structured-data

And if an attempt, like in the above, to re-transfer a neutered object
is made, DataCloneError should also be thrown:

  http://www.whatwg.org/specs/web-apps/current-work/#dom-window-postmessage
  http://www.whatwg.org/specs/web-apps/current-work/#dom-messageport-postmessage

The implementation used InvalidStateError instead.

Switch to throwing DataCloneError in SerializedScriptValue, both
during the handling of transferables and the actual cloning step. Hand
in hand with that switch away from the older InvalidStateError,
improve the quality of the error messages that go with the failed
cloning operations. i.e.,

 * SerializedScriptValue::create() now takes an ExceptionState for
   reporting exceptions. This allows context (e.g., what postMessage()
   that is attempting to clone) to be included in the error message.

   (As a result, removed the old method for signalling errors via a
   bool reference.)

 * Rephrase the unmarshalling/extraction of transferable arrays to
   also be reported over an incoming ExceptionState.

 * Make the error messages emitted by MessagePort.postMessage() a
   little bit more consistent.

R=mkwst@chromium.org,dslomov@chromium.org
BUG=327994
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=164106

[sof, 2013-12-12 21:30:38]

Please take a look when you next have a moment to spare.

dslomov: would appreciate if you could check if the SerializedScriptValue
changes are acceptable.

[Mike West, 2013-12-13 21:22:58]

This looks pretty reasonable to me, with the exception of the single question
below: LGTM if you either explain or revert the newly public setException
function.

I'll leave the final say to dslomov@, however. Please wait for him to weigh in
before landing the patch.

https://codereview.chromium.org/114363002/diff/1/Source/bindings/v8/Exception...
File Source/bindings/v8/ExceptionState.h (right):

https://codereview.chromium.org/114363002/diff/1/Source/bindings/v8/Exception...
Source/bindings/v8/ExceptionState.h:109: void
setException(v8::Handle<v8::Value>);
Why is this now public? I don't see where you need to use it outside
ExceptionState.

[sof, 2013-12-13 21:51:32]

https://codereview.chromium.org/114363002/diff/1/Source/bindings/v8/Exception...
File Source/bindings/v8/ExceptionState.h (right):

https://codereview.chromium.org/114363002/diff/1/Source/bindings/v8/Exception...
Source/bindings/v8/ExceptionState.h:109: void
setException(v8::Handle<v8::Value>);
On 2013/12/13 21:22:59, Mike West wrote:
> Why is this now public? I don't see where you need to use it outside
> ExceptionState.

It's used to transfer the exception from the TryCatch in
SerializedScriptValue::SerializedScriptValue() --

 https://codereview.chromium.org/114363002/patch/1/10024

It gave me pause to do so, but I thought it fairly reasonable to have a public
ExceptionState method for setting a pre-constructed exception value also.

[Dmitry Lomov (no reviews), 2013-12-16 09:14:38]

I like the direction where it is going, but I think it does not go far enough :)

For SerializedScriptValue, after this patch we will get into state where we have
both ExceptionState and ExceptionPolicy.
I suggest:
- Remove the ExceptionPolicy.
- For createAndSwallowExceptions, allocate an ExceptionState and ignore it (do
not call 'throwIfNeeded')
- Maybe remove 'create' overloads without ExceptionState.

https://codereview.chromium.org/114363002/diff/1/Source/bindings/v8/Exception...
File Source/bindings/v8/ExceptionState.h (right):

https://codereview.chromium.org/114363002/diff/1/Source/bindings/v8/Exception...
Source/bindings/v8/ExceptionState.h:109: void
setException(v8::Handle<v8::Value>);
On 2013/12/13 21:51:33, sof wrote:
> On 2013/12/13 21:22:59, Mike West wrote:
> > Why is this now public? I don't see where you need to use it outside
> > ExceptionState.
> 
> It's used to transfer the exception from the TryCatch in
> SerializedScriptValue::SerializedScriptValue() --
> 
>  https://codereview.chromium.org/114363002/patch/1/10024
> 
> It gave me pause to do so, but I thought it fairly reasonable to have a public
> ExceptionState method for setting a pre-constructed exception value also.

I think the naming of methods on ExceptionState is confusing. All
'throw*Exception' methods do not actually throw, they just set the state on
ExceptionState. The only method that actually throws is 'throwIfNeeded'. 
If we follow this, a bit confusing, state of the API, the good name for a public
'setException' would be 'rethrowV8Exception' (throwIfNeeded will just propagate
the exception).

In the long run, I suggest renaming all 'throw*Exception' => 'report*Exception'

[jsbell, 2013-12-16 17:00:28]

Drive-by comment:

Over in Source/modules/indexeddb/IDBObjectStore.cpp:170 there's a comment:

// FIXME: Make SerializedScriptValue::create etc take an ExceptionState or use
ScriptState::setDOMException.

It looks like it should be possible to switch to the new create() signature once
this lands, and check exceptionState.hadException() instead of didThrow.

Are there any other consumers of the create() signature with const bool&
didThrow ?

[sof, 2013-12-16 17:24:53]

On 2013/12/16 17:00:28, jsbell wrote:
> Drive-by comment:
> 
> Over in Source/modules/indexeddb/IDBObjectStore.cpp:170 there's a comment:
> 
> // FIXME: Make SerializedScriptValue::create etc take an ExceptionState or use
> ScriptState::setDOMException.
> 

Aha, great - that seems like a candidate.

> It looks like it should be possible to switch to the new create() signature
once
> this lands, and check exceptionState.hadException() instead of didThrow.
> 
> Are there any other consumers of the create() signature with const bool&
> didThrow ?

WebSerializedScriptValue is another, and avoiding it completely in generated
bindings. Doable to change both, I'll have a closer look if it can be done here
or in a followup. (I didn't want to make this CL too sprawling and held off
attempting to eradicate didThrow usage earlier.)

[jsbell, 2013-12-16 18:16:32]

On 2013/12/16 17:24:53, sof wrote:
> I'll have a closer look if it can be done here
> or in a followup. (I didn't want to make this CL too sprawling and held off
> attempting to eradicate didThrow usage earlier.)

Follow-up CL is great, thanks!

[sof, 2013-12-17 13:41:31]

On 2013/12/16 09:14:38, Dmitry Lomov (chromium) wrote:
> I like the direction where it is going, but I think it does not go far enough
:)
> 
> For SerializedScriptValue, after this patch we will get into state where we
have
> both ExceptionState and ExceptionPolicy.
> I suggest:
> - Remove the ExceptionPolicy.

Done.

> - For createAndSwallowExceptions, allocate an ExceptionState and ignore it (do
> not call 'throwIfNeeded')

Done.

> - Maybe remove 'create' overloads without ExceptionState.

Done. I removed the "didThrow" overloads + a few other bits (removed JsFailure,
improved precision of error messages.)

> 
>
https://codereview.chromium.org/114363002/diff/1/Source/bindings/v8/Exception...
> File Source/bindings/v8/ExceptionState.h (right):
> 
>
https://codereview.chromium.org/114363002/diff/1/Source/bindings/v8/Exception...
> Source/bindings/v8/ExceptionState.h:109: void
> setException(v8::Handle<v8::Value>);
> On 2013/12/13 21:51:33, sof wrote:
> > On 2013/12/13 21:22:59, Mike West wrote:
> > > Why is this now public? I don't see where you need to use it outside
> > > ExceptionState.
> > 
> > It's used to transfer the exception from the TryCatch in
> > SerializedScriptValue::SerializedScriptValue() --
> > 
> >  https://codereview.chromium.org/114363002/patch/1/10024
> > 
> > It gave me pause to do so, but I thought it fairly reasonable to have a
public
> > ExceptionState method for setting a pre-constructed exception value also.
> 
> I think the naming of methods on ExceptionState is confusing. All
> 'throw*Exception' methods do not actually throw, they just set the state on
> ExceptionState. The only method that actually throws is 'throwIfNeeded'. 
> If we follow this, a bit confusing, state of the API, the good name for a
public
> 'setException' would be 'rethrowV8Exception' (throwIfNeeded will just
propagate
> the exception).
> 

Done, a much better name.

> In the long run, I suggest renaming all 'throw*Exception' =>
'report*Exception'

If there's agreement on new names, this can be done. But not here :)

[Dmitry Lomov (no reviews), 2013-12-17 15:24:55]

Great work. LGTM.



On 2013/12/17 13:41:31, sof wrote:
> On 2013/12/16 09:14:38, Dmitry Lomov (chromium) wrote:
> > I like the direction where it is going, but I think it does not go far
enough
> :)
> > 
> > For SerializedScriptValue, after this patch we will get into state where we
> have
> > both ExceptionState and ExceptionPolicy.
> > I suggest:
> > - Remove the ExceptionPolicy.
> 
> Done.
> 
> > - For createAndSwallowExceptions, allocate an ExceptionState and ignore it
(do
> > not call 'throwIfNeeded')
> 
> Done.
> 
> > - Maybe remove 'create' overloads without ExceptionState.
> 
> Done. I removed the "didThrow" overloads + a few other bits (removed
JsFailure,
> improved precision of error messages.)
> 
> > 
> >
>
https://codereview.chromium.org/114363002/diff/1/Source/bindings/v8/Exception...
> > File Source/bindings/v8/ExceptionState.h (right):
> > 
> >
>
https://codereview.chromium.org/114363002/diff/1/Source/bindings/v8/Exception...
> > Source/bindings/v8/ExceptionState.h:109: void
> > setException(v8::Handle<v8::Value>);
> > On 2013/12/13 21:51:33, sof wrote:
> > > On 2013/12/13 21:22:59, Mike West wrote:
> > > > Why is this now public? I don't see where you need to use it outside
> > > > ExceptionState.
> > > 
> > > It's used to transfer the exception from the TryCatch in
> > > SerializedScriptValue::SerializedScriptValue() --
> > > 
> > >  https://codereview.chromium.org/114363002/patch/1/10024
> > > 
> > > It gave me pause to do so, but I thought it fairly reasonable to have a
> public
> > > ExceptionState method for setting a pre-constructed exception value also.
> > 
> > I think the naming of methods on ExceptionState is confusing. All
> > 'throw*Exception' methods do not actually throw, they just set the state on
> > ExceptionState. The only method that actually throws is 'throwIfNeeded'. 
> > If we follow this, a bit confusing, state of the API, the good name for a
> public
> > 'setException' would be 'rethrowV8Exception' (throwIfNeeded will just
> propagate
> > the exception).
> > 
> 
> Done, a much better name.
> 
> > In the long run, I suggest renaming all 'throw*Exception' =>
> 'report*Exception'
> 
> If there's agreement on new names, this can be done. But not here :)

Sure.

[commit-bot: I haz the power, 2013-12-17 15:31:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/114363002/40001

[commit-bot: I haz the power, 2013-12-17 15:51:43]

Retried try job too often on blink_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

[sof, 2013-12-17 16:48:42]

Rebased + reset one bindings/ test result (brought about by meeting
code_generator_v8.pm improvements from 783e6c8ddd )

Needs owner approval from web/ and modules/indexeddb/

[sof, 2013-12-18 07:10:07]

+jsbell, +jochen

Two call sites that had to be adjusted needs approval, could you have a look at
some point?

Source/web/WebSerializedScriptValue.cpp and
Source/modules/indexeddb/IDBObjectStore.cpp

[jochen (gone - plz use gerrit), 2013-12-18 08:01:32]

lgtm

[jsbell, 2013-12-18 16:49:30]

Source/modules/indexeddb change LGTM

[commit-bot: I haz the power, 2013-12-18 16:52:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/114363002/60001

[commit-bot: I haz the power, 2013-12-18 18:09:59]

Change committed as 164106