Implement v8::ValueSerializer::Delegate to throw DOMException for DataCloneError.

Implement v8::ValueSerializer::Delegate to throw DOMException for DataCloneError.

This allows V8 to indirectly throw a DOMException, as is required to handle
certain kinds of errors encountered during serialization.

BUG=chromium:148757
Committed: https://crrev.com/15b0869d2e654f0fa8c9b37ba1cca9a2a942f66c
Cr-Commit-Position: refs/heads/master@{#417184}

[jbroman, 2016-09-06 20:36:04]

The CQ bit was checked by jbroman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-06 20:36:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbroman, 2016-09-06 20:38:39]

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

[jbroman, 2016-09-06 20:38:40]

We have to throw and clear the exception so that V8 sees the exception pending
on the isolate. It later gets caught and rethrown by Blink.

[jbroman, 2016-09-06 20:39:27]

https://codereview.chromium.org/2316763002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/ExceptionState.h (left):

https://codereview.chromium.org/2316763002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ExceptionState.h:53:
STACK_ALLOCATED();
This change was made because the blink-gc plugin won't permit holding a pointer
to ExceptionState in a class that implements a non-GC interface. This object
doesn't hold pointers to Oilpan objects anyhow, so it doesn't need this.

[commit-bot: I haz the power, 2016-09-06 23:00:48]

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

[commit-bot: I haz the power, 2016-09-06 23:00:48]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-09-07 00:27:54]

https://codereview.chromium.org/2316763002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/ExceptionState.h (left):

https://codereview.chromium.org/2316763002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ExceptionState.h:53:
STACK_ALLOCATED();
On 2016/09/06 20:39:27, jbroman wrote:
> This change was made because the blink-gc plugin won't permit holding a
pointer
> to ExceptionState in a class that implements a non-GC interface. This object
> doesn't hold pointers to Oilpan objects anyhow, so it doesn't need this.

Hmm, I'd like to keep the STACK_ALLOCATED. STACK_ALLOCATED is not only for
oilpan but also for asserting that the class should be used on the stack
(otherwise it's likely to be a bug).

Any idea to restructure the class design around V8ScriptValueSerializer and keep
the STACK_ALLOCATED as is?

[jbroman, 2016-09-07 01:03:34]

https://codereview.chromium.org/2316763002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/ExceptionState.h (left):

https://codereview.chromium.org/2316763002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ExceptionState.h:53:
STACK_ALLOCATED();
On 2016/09/07 at 00:27:54, haraken wrote:
> On 2016/09/06 20:39:27, jbroman wrote:
> > This change was made because the blink-gc plugin won't permit holding a
pointer
> > to ExceptionState in a class that implements a non-GC interface. This object
> > doesn't hold pointers to Oilpan objects anyhow, so it doesn't need this.
> 
> Hmm, I'd like to keep the STACK_ALLOCATED. STACK_ALLOCATED is not only for
oilpan but also for asserting that the class should be used on the stack
(otherwise it's likely to be a bug).
> 
> Any idea to restructure the class design around V8ScriptValueSerializer and
keep the STACK_ALLOCATED as is?

Isn't that what DISALLOW_NEW is for? If not, what is the distinction between
STACK_ALLOCATED and DISALLOW_NEW?

I don't think it's very clear whether STACK_ALLOCATED is supposed to mean "this
should be on the stack" or "this is an Oilpan object that only exists on the
stack".

If it is supposed to be the former, then I don't think the restriction that it
only have STACK_ALLOCATED base classes makes sense, because it prevents any pure
interface base class. (And I don't understand the restriction; shouldn't that
restriction be "no base class should be GarbageCollected"?)

V8ScriptValueSerializer could be split into two classes to work around this, but
I'd rather not complicate the code to satisfy blink_gc_plugin, especially when
there's nothing Oilpan-related going on here.

[jbroman, 2016-09-07 01:13:16]

https://codereview.chromium.org/2316763002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/ExceptionState.h (left):

https://codereview.chromium.org/2316763002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ExceptionState.h:53:
STACK_ALLOCATED();
Also, the comment says:

// Classes that contain references to garbage-collected objects but aren't
// themselves garbaged allocated, have some extra macros available which
// allows their use to be restricted to cases where the garbage collector
// is able to discover their references.

So if this is intended also for use on classes that don't use GC objects,
maybe the comment _and_ blink_gc_plugin should be revised?

[haraken, 2016-09-07 01:25:55]

On 2016/09/07 01:03:34, jbroman wrote:
>
https://codereview.chromium.org/2316763002/diff/1/third_party/WebKit/Source/b...
> File third_party/WebKit/Source/bindings/core/v8/ExceptionState.h (left):
> 
>
https://codereview.chromium.org/2316763002/diff/1/third_party/WebKit/Source/b...
> third_party/WebKit/Source/bindings/core/v8/ExceptionState.h:53:
> STACK_ALLOCATED();
> On 2016/09/07 at 00:27:54, haraken wrote:
> > On 2016/09/06 20:39:27, jbroman wrote:
> > > This change was made because the blink-gc plugin won't permit holding a
> pointer
> > > to ExceptionState in a class that implements a non-GC interface. This
object
> > > doesn't hold pointers to Oilpan objects anyhow, so it doesn't need this.
> > 
> > Hmm, I'd like to keep the STACK_ALLOCATED. STACK_ALLOCATED is not only for
> oilpan but also for asserting that the class should be used on the stack
> (otherwise it's likely to be a bug).
> > 
> > Any idea to restructure the class design around V8ScriptValueSerializer and
> keep the STACK_ALLOCATED as is?
> 
> Isn't that what DISALLOW_NEW is for? If not, what is the distinction between
> STACK_ALLOCATED and DISALLOW_NEW?

STACK_ALLOCATED = The object must be used on the stack.

DISALLOW_NEW = The object must be used on the stack or as a part of (maybe
heap-allocated) object.

DISALLOW_NEW_EXCEPT_PLACEMENT_NEW = The object must be used on the stack, as a
part of (maybe heap-allocated) object, or inside Vector/HashTable.

> 
> I don't think it's very clear whether STACK_ALLOCATED is supposed to mean
"this
> should be on the stack" or "this is an Oilpan object that only exists on the
> stack".

I'm intending to mean the former. STACK_ALLOCATED is not only for Oilpan. For
example, objects that hold v8::Local<v8::Value> should have STACK_ALLOCATED to
prevent developers from allocating the objects outside the stack.

Does this make sense?

> 
> If it is supposed to be the former, then I don't think the restriction that it
> only have STACK_ALLOCATED base classes makes sense, because it prevents any
pure
> interface base class.

Would you elaborate?

> (And I don't understand the restriction; shouldn't that
> restriction be "no base class should be GarbageCollected"?)
> 
> V8ScriptValueSerializer could be split into two classes to work around this,
but
> I'd rather not complicate the code to satisfy blink_gc_plugin, especially when
> there's nothing Oilpan-related going on here.

Yes, I need to update the comment on Allocator.h and the error message of the gc
plugin :)

[jbroman, 2016-09-07 01:37:15]

On 2016/09/07 at 01:25:55, haraken wrote:
> On 2016/09/07 01:03:34, jbroman wrote:
> >
https://codereview.chromium.org/2316763002/diff/1/third_party/WebKit/Source/b...
> > File third_party/WebKit/Source/bindings/core/v8/ExceptionState.h (left):
> > 
> >
https://codereview.chromium.org/2316763002/diff/1/third_party/WebKit/Source/b...
> > third_party/WebKit/Source/bindings/core/v8/ExceptionState.h:53:
> > STACK_ALLOCATED();
> > On 2016/09/07 at 00:27:54, haraken wrote:
> > > On 2016/09/06 20:39:27, jbroman wrote:
> > > > This change was made because the blink-gc plugin won't permit holding a
> > pointer
> > > > to ExceptionState in a class that implements a non-GC interface. This
object
> > > > doesn't hold pointers to Oilpan objects anyhow, so it doesn't need this.
> > > 
> > > Hmm, I'd like to keep the STACK_ALLOCATED. STACK_ALLOCATED is not only for
> > oilpan but also for asserting that the class should be used on the stack
> > (otherwise it's likely to be a bug).
> > > 
> > > Any idea to restructure the class design around V8ScriptValueSerializer
and
> > keep the STACK_ALLOCATED as is?
> > 
> > Isn't that what DISALLOW_NEW is for? If not, what is the distinction between
> > STACK_ALLOCATED and DISALLOW_NEW?
> 
> STACK_ALLOCATED = The object must be used on the stack.
> 
> DISALLOW_NEW = The object must be used on the stack or as a part of (maybe
heap-allocated) object.
> 
> DISALLOW_NEW_EXCEPT_PLACEMENT_NEW = The object must be used on the stack, as a
part of (maybe heap-allocated) object, or inside Vector/HashTable.
> 
> > 
> > I don't think it's very clear whether STACK_ALLOCATED is supposed to mean
"this
> > should be on the stack" or "this is an Oilpan object that only exists on the
> > stack".
> 
> I'm intending to mean the former. STACK_ALLOCATED is not only for Oilpan. For
example, objects that hold v8::Local<v8::Value> should have STACK_ALLOCATED to
prevent developers from allocating the objects outside the stack.

Well, except that we can and do put v8::Local outside the stack (for instance,
in vectors and maps).

> Does this make sense?
> 
> > 
> > If it is supposed to be the former, then I don't think the restriction that
it
> > only have STACK_ALLOCATED base classes makes sense, because it prevents any
pure
> > interface base class.
> 
> Would you elaborate?

Sure. The reason I removed this is because I got an error that said (I'm
reconstructing the message as I don't have it in front of me):

[blink-gc] Stack-allocated class blink::V8ScriptValueSerializer derives class
v8::ValueSerializer::Delegate which is not stack allocated.

But not only am I not doing anything GC-related (and yet have [blink-gc] in an
error message), but there is no bug that occurs by implementing an interface in
a stack-allocated class.

I tried to read the CL which added the check, but it wasn't clear to me what
it's trying to achieve. My best guess is that it's trying to ensure you don't
have:

A : public GarbageCollected<A>
B : public A (where B is STACK_ALLOCATED)

Because it obviously doesn't make sense for an object to both live on the Oilpan
heap and live on the stack. But it should be fine to implement a base, cc, v8,
etc. interface in a stack-allocated object. If this is what it's trying to
prevent, though, it would seem to suffice to look for
IsGarbageCollectedTypeMarker and IsGarbageCollectedMixinTypeMarker (as the type
traits do), or similar.

> > (And I don't understand the restriction; shouldn't that
> > restriction be "no base class should be GarbageCollected"?)
> > 
> > V8ScriptValueSerializer could be split into two classes to work around this,
but
> > I'd rather not complicate the code to satisfy blink_gc_plugin, especially
when
> > there's nothing Oilpan-related going on here.
> 
> Yes, I need to update the comment on Allocator.h and the error message of the
gc plugin :)

[haraken, 2016-09-07 01:52:31]

On 2016/09/07 01:37:15, jbroman wrote:
> On 2016/09/07 at 01:25:55, haraken wrote:
> > On 2016/09/07 01:03:34, jbroman wrote:
> > >
>
https://codereview.chromium.org/2316763002/diff/1/third_party/WebKit/Source/b...
> > > File third_party/WebKit/Source/bindings/core/v8/ExceptionState.h (left):
> > > 
> > >
>
https://codereview.chromium.org/2316763002/diff/1/third_party/WebKit/Source/b...
> > > third_party/WebKit/Source/bindings/core/v8/ExceptionState.h:53:
> > > STACK_ALLOCATED();
> > > On 2016/09/07 at 00:27:54, haraken wrote:
> > > > On 2016/09/06 20:39:27, jbroman wrote:
> > > > > This change was made because the blink-gc plugin won't permit holding
a
> > > pointer
> > > > > to ExceptionState in a class that implements a non-GC interface. This
> object
> > > > > doesn't hold pointers to Oilpan objects anyhow, so it doesn't need
this.
> > > > 
> > > > Hmm, I'd like to keep the STACK_ALLOCATED. STACK_ALLOCATED is not only
for
> > > oilpan but also for asserting that the class should be used on the stack
> > > (otherwise it's likely to be a bug).
> > > > 
> > > > Any idea to restructure the class design around V8ScriptValueSerializer
> and
> > > keep the STACK_ALLOCATED as is?
> > > 
> > > Isn't that what DISALLOW_NEW is for? If not, what is the distinction
between
> > > STACK_ALLOCATED and DISALLOW_NEW?
> > 
> > STACK_ALLOCATED = The object must be used on the stack.
> > 
> > DISALLOW_NEW = The object must be used on the stack or as a part of (maybe
> heap-allocated) object.
> > 
> > DISALLOW_NEW_EXCEPT_PLACEMENT_NEW = The object must be used on the stack, as
a
> part of (maybe heap-allocated) object, or inside Vector/HashTable.
> > 
> > > 
> > > I don't think it's very clear whether STACK_ALLOCATED is supposed to mean
> "this
> > > should be on the stack" or "this is an Oilpan object that only exists on
the
> > > stack".
> > 
> > I'm intending to mean the former. STACK_ALLOCATED is not only for Oilpan.
For
> example, objects that hold v8::Local<v8::Value> should have STACK_ALLOCATED to
> prevent developers from allocating the objects outside the stack.
> 
> Well, except that we can and do put v8::Local outside the stack (for instance,
> in vectors and maps).

I agree that the STACK_ALLOCATED check is not perfect but it has actually caught
many programming mistakes, so I think it's worth keeping.

> 
> > Does this make sense?
> > 
> > > 
> > > If it is supposed to be the former, then I don't think the restriction
that
> it
> > > only have STACK_ALLOCATED base classes makes sense, because it prevents
any
> pure
> > > interface base class.
> > 
> > Would you elaborate?
> 
> Sure. The reason I removed this is because I got an error that said (I'm
> reconstructing the message as I don't have it in front of me):
> 
> [blink-gc] Stack-allocated class blink::V8ScriptValueSerializer derives class
> v8::ValueSerializer::Delegate which is not stack allocated.
> 
> But not only am I not doing anything GC-related (and yet have [blink-gc] in an
> error message), but there is no bug that occurs by implementing an interface
in
> a stack-allocated class.
> 
> I tried to read the CL which added the check, but it wasn't clear to me what
> it's trying to achieve. My best guess is that it's trying to ensure you don't
> have:
> 
> A : public GarbageCollected<A>
> B : public A (where B is STACK_ALLOCATED)
> 
> Because it obviously doesn't make sense for an object to both live on the
Oilpan
> heap and live on the stack. But it should be fine to implement a base, cc, v8,
> etc. interface in a stack-allocated object. If this is what it's trying to
> prevent, though, it would seem to suffice to look for
> IsGarbageCollectedTypeMarker and IsGarbageCollectedMixinTypeMarker (as the
type
> traits do), or similar.

Ah, agreed. It doesn't make sense to have that restriction about the base class
:)

Historically, STACK_ALLOCATED was introduced for Oilpan-related verification but
later the scope was extended to other non-Oilpan-related usages (e.g.,
v8::Local<v8::Value>).


> 
> > > (And I don't understand the restriction; shouldn't that
> > > restriction be "no base class should be GarbageCollected"?)
> > > 
> > > V8ScriptValueSerializer could be split into two classes to work around
this,
> but
> > > I'd rather not complicate the code to satisfy blink_gc_plugin, especially
> when
> > > there's nothing Oilpan-related going on here.
> > 
> > Yes, I need to update the comment on Allocator.h and the error message of
the
> gc plugin :)

[jbroman, 2016-09-07 02:14:56]

Alright, what do you think about me filing a bug for this, and marking
V8ScriptValueSerializer GC_PLUGIN_IGNORE in the meantime (assuming that works)?

Then I should be able to restore STACK_ALLOCATED to ExceptionState.

[haraken, 2016-09-07 02:22:45]

On 2016/09/07 02:14:56, jbroman wrote:
> Alright, what do you think about me filing a bug for this, and marking
> V8ScriptValueSerializer GC_PLUGIN_IGNORE in the meantime (assuming that
works)?
> 
> Then I should be able to restore STACK_ALLOCATED to ExceptionState.

Sounds good.

[jbroman, 2016-09-07 13:38:44]

The CQ bit was checked by jbroman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 13:38:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbroman, 2016-09-07 13:39:43]

PTAL.

[haraken, 2016-09-07 14:49:17]

https://codereview.chromium.org/2316763002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp
(right):

https://codereview.chromium.org/2316763002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:92:
m_exceptionState->clearException();

BTW, it looks nasty to set ExceptionState and then clear just to pass it to
V8ThrowException::throwException.

Maybe can you use V8ThrowException::createDOMException and pass it to
V8ThrowException::throwException? Then you won't need the ExceptionState here.

[jbroman, 2016-09-07 15:05:25]

https://codereview.chromium.org/2316763002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp
(right):

https://codereview.chromium.org/2316763002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:92:
m_exceptionState->clearException();
On 2016/09/07 at 14:49:17, haraken wrote:
> BTW, it looks nasty to set ExceptionState and then clear just to pass it to
V8ThrowException::throwException.
> 
> Maybe can you use V8ThrowException::createDOMException and pass it to
V8ThrowException::throwException? Then you won't need the ExceptionState here.

It loses some of the other nice things that ExceptionState gives you, like
automatically prefixing the message with the lovely "Failed to execute
'postMessage' on 'Window':" (or similar) that mkwst@ added in late 2013. That
uses context that is stored in the ExceptionState.

I can add that back by making ExceptionState::addExceptionContext public and
calling it (effectively taking the logic that ExceptionState::throwDOMException
uses today). Roughly:

String processedMessage = m_exceptionState->addExceptionContext(message);
v8::Local<v8::Value> exception = V8ThrowException::createDOMException(isolate,
DataCloneError, processedMessage);
V8ThrowException::throwException(isolate, exception);

Would that be better?

[commit-bot: I haz the power, 2016-09-07 15:15:07]

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

[commit-bot: I haz the power, 2016-09-07 15:15:08]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-09-07 15:39:07]

https://codereview.chromium.org/2316763002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp
(right):

https://codereview.chromium.org/2316763002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:91:
V8ThrowException::throwException(m_scriptState->isolate(),
m_exceptionState->getException());

BTW, is this a right place to throw a V8 exception?

I'm wondering if we should just call throwDOMException here and let the line 44
throw the V8 exception.

https://codereview.chromium.org/2316763002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:92:
m_exceptionState->clearException();
On 2016/09/07 15:05:24, jbroman wrote:
> On 2016/09/07 at 14:49:17, haraken wrote:
> > BTW, it looks nasty to set ExceptionState and then clear just to pass it to
> V8ThrowException::throwException.
> > 
> > Maybe can you use V8ThrowException::createDOMException and pass it to
> V8ThrowException::throwException? Then you won't need the ExceptionState here.
> 
> It loses some of the other nice things that ExceptionState gives you, like
> automatically prefixing the message with the lovely "Failed to execute
> 'postMessage' on 'Window':" (or similar) that mkwst@ added in late 2013. That
> uses context that is stored in the ExceptionState.
> 
> I can add that back by making ExceptionState::addExceptionContext public and
> calling it (effectively taking the logic that
ExceptionState::throwDOMException
> uses today). Roughly:
> 
> String processedMessage = m_exceptionState->addExceptionContext(message);
> v8::Local<v8::Value> exception = V8ThrowException::createDOMException(isolate,
> DataCloneError, processedMessage);
> V8ThrowException::throwException(isolate, exception);
> 
> Would that be better?

Ah, thanks for the clarification! Makes sense.

[jbroman, 2016-09-07 15:43:38]

https://codereview.chromium.org/2316763002/diff/20001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp
(right):

https://codereview.chromium.org/2316763002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:91:
V8ThrowException::throwException(m_scriptState->isolate(),
m_exceptionState->getException());
On 2016/09/07 at 15:39:07, haraken wrote:
> BTW, is this a right place to throw a V8 exception?
> 
> I'm wondering if we should just call throwDOMException here and let the line
44 throw the V8 exception.

Doing so would cause the V8 API methods to return Nothing, even though there is
no pending exception in the isolate. (That will at present cause assertions to
fire in V8, since I have as you requested restored that property.)

https://codereview.chromium.org/2316763002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:92:
m_exceptionState->clearException();
On 2016/09/07 at 15:39:07, haraken wrote:
> On 2016/09/07 15:05:24, jbroman wrote:
> > On 2016/09/07 at 14:49:17, haraken wrote:
> > > BTW, it looks nasty to set ExceptionState and then clear just to pass it
to
> > V8ThrowException::throwException.
> > > 
> > > Maybe can you use V8ThrowException::createDOMException and pass it to
> > V8ThrowException::throwException? Then you won't need the ExceptionState
here.
> > 
> > It loses some of the other nice things that ExceptionState gives you, like
> > automatically prefixing the message with the lovely "Failed to execute
> > 'postMessage' on 'Window':" (or similar) that mkwst@ added in late 2013.
That
> > uses context that is stored in the ExceptionState.
> > 
> > I can add that back by making ExceptionState::addExceptionContext public and
> > calling it (effectively taking the logic that
ExceptionState::throwDOMException
> > uses today). Roughly:
> > 
> > String processedMessage = m_exceptionState->addExceptionContext(message);
> > v8::Local<v8::Value> exception =
V8ThrowException::createDOMException(isolate,
> > DataCloneError, processedMessage);
> > V8ThrowException::throwException(isolate, exception);
> > 
> > Would that be better?
> 
> Ah, thanks for the clarification! Makes sense.

So which is better, the set/clear weirdness or calling addExceptionContext
explicitly?

[haraken, 2016-09-07 15:58:57]

On 2016/09/07 15:43:38, jbroman wrote:
>
https://codereview.chromium.org/2316763002/diff/20001/third_party/WebKit/Sour...
> File
>
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp
> (right):
> 
>
https://codereview.chromium.org/2316763002/diff/20001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:91:
> V8ThrowException::throwException(m_scriptState->isolate(),
> m_exceptionState->getException());
> On 2016/09/07 at 15:39:07, haraken wrote:
> > BTW, is this a right place to throw a V8 exception?
> > 
> > I'm wondering if we should just call throwDOMException here and let the line
> 44 throw the V8 exception.
> 
> Doing so would cause the V8 API methods to return Nothing, even though there
is
> no pending exception in the isolate. (That will at present cause assertions to
> fire in V8, since I have as you requested restored that property.)

Ah, got it. You're right.

> 
>
https://codereview.chromium.org/2316763002/diff/20001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/bindings/core/v8/serialization/V8ScriptValueSerializer.cpp:92:
> m_exceptionState->clearException();
> On 2016/09/07 at 15:39:07, haraken wrote:
> > On 2016/09/07 15:05:24, jbroman wrote:
> > > On 2016/09/07 at 14:49:17, haraken wrote:
> > > > BTW, it looks nasty to set ExceptionState and then clear just to pass it
> to
> > > V8ThrowException::throwException.
> > > > 
> > > > Maybe can you use V8ThrowException::createDOMException and pass it to
> > > V8ThrowException::throwException? Then you won't need the ExceptionState
> here.
> > > 
> > > It loses some of the other nice things that ExceptionState gives you, like
> > > automatically prefixing the message with the lovely "Failed to execute
> > > 'postMessage' on 'Window':" (or similar) that mkwst@ added in late 2013.
> That
> > > uses context that is stored in the ExceptionState.
> > > 
> > > I can add that back by making ExceptionState::addExceptionContext public
and
> > > calling it (effectively taking the logic that
> ExceptionState::throwDOMException
> > > uses today). Roughly:
> > > 
> > > String processedMessage = m_exceptionState->addExceptionContext(message);
> > > v8::Local<v8::Value> exception =
> V8ThrowException::createDOMException(isolate,
> > > DataCloneError, processedMessage);
> > > V8ThrowException::throwException(isolate, exception);
> > > 
> > > Would that be better?
> > 
> > Ah, thanks for the clarification! Makes sense.
> 
> So which is better, the set/clear weirdness or calling addExceptionContext
> explicitly?

I'm fine with the set/clear weirdness, but let's add a comment about what it's
doing.

LGTM.

Another idea would be to add a copy constructor to ExceptionState and use a
dedicated ExceptionState for SerializedValue. Then you don't need to clear.
However, I don't think it's a massive improvement.

[jbroman, 2016-09-07 18:29:48]

The CQ bit was checked by jbroman@chromium.org

[jbroman, 2016-09-07 18:29:48]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2316763002/#ps40001
(title: "explanatory comment")

[commit-bot: I haz the power, 2016-09-07 18:30:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbroman, 2016-09-07 18:34:05]

The CQ bit was unchecked by jbroman@chromium.org

[jbroman, 2016-09-07 19:45:07]

The CQ bit was checked by jbroman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-07 19:45:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbroman, 2016-09-07 19:49:00]

On second thought, there is an advantage to using
V8ThrowException::createDOMException, which is that it lets us actually throw a
V8 exception even if the ExceptionState is a TrackExceptionState, so I've taken
that approach instead.

(We'll end up catching and suppressing that exception later on, but it keeps
V8's invariants cleaner.)

PTAL to confirm.

[commit-bot: I haz the power, 2016-09-07 21:20:53]

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

[commit-bot: I haz the power, 2016-09-07 21:20:54]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-09-07 23:21:00]

LGTM

[jbroman, 2016-09-08 03:57:37]

The CQ bit was checked by jbroman@chromium.org

[commit-bot: I haz the power, 2016-09-08 03:58:04]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 04:01:25]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-08 04:02:50]

Description was changed from

==========
Implement v8::ValueSerializer::Delegate to throw DOMException for
DataCloneError.

This allows V8 to indirectly throw a DOMException, as is required to handle
certain kinds of errors encountered during serialization.

BUG=chromium:148757
==========

to

==========
Implement v8::ValueSerializer::Delegate to throw DOMException for
DataCloneError.

This allows V8 to indirectly throw a DOMException, as is required to handle
certain kinds of errors encountered during serialization.

BUG=chromium:148757

Committed: https://crrev.com/15b0869d2e654f0fa8c9b37ba1cca9a2a942f66c
Cr-Commit-Position: refs/heads/master@{#417184}
==========

[commit-bot: I haz the power, 2016-09-08 04:02:51]

Patchset 4 (id:??) landed as
https://crrev.com/15b0869d2e654f0fa8c9b37ba1cca9a2a942f66c
Cr-Commit-Position: refs/heads/master@{#417184}