Make 'any'-typed attributes of events available in isolated worlds

Make 'any'-typed attributes of events available in isolated worlds

Before r153701, event attributes of type 'any' were freely-shared between isolated worlds.
But since that patch, accessing those attributes from other worlds always returns null.

This patch re-enables isolated worlds to communicate via custom-constructed events
without leaking object identity by using SerializedScriptValue to clone 'any' attributes.

When an event is constructed in an isolated world, we eagerly make a clone of its 'any' attributes.
This is mostly for expediency: it would otherwise be tricky to find the original data, as it will
be hanging off some wrapper in some world, but we wouldn't know which world to look in.

But for the main world, since all these events are ScriptWrappable, we lazily clone.
This is both for performance reasons and to avoid any side-effects of cloning
(such as calling setters) until the event is actually accessed in another world.

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

[adamk, 2013-07-17 22:54:57]

Reviewers:

abarth for the isolated world-related stuff
marja for the change to ScriptWrappable

[abarth-chromium, 2013-07-17 23:06:44]

LGTM, but we should probably have Marja take a look.

https://codereview.chromium.org/19457002/diff/6001/LayoutTests/fast/events/in...
File LayoutTests/fast/events/init-custom-event-isolated-world.html (right):

https://codereview.chromium.org/19457002/diff/6001/LayoutTests/fast/events/in...
LayoutTests/fast/events/init-custom-event-isolated-world.html:6:
console.log("Tests that properties of custom events are cloned when accessed in
isolated worlds.");
Why not just put this in a <p> ?

https://codereview.chromium.org/19457002/diff/6001/Source/bindings/scripts/de...
File Source/bindings/scripts/deprecated_code_generator_v8.pm (right):

https://codereview.chromium.org/19457002/diff/6001/Source/bindings/scripts/de...
Source/bindings/scripts/deprecated_code_generator_v8.pm:2704:
event->${setter}(SerializedScriptValue::createSwallowExceptions(${attrName},
args.GetIsolate()));
createSwallowExceptions -> createAndSwallowExceptions ?

https://codereview.chromium.org/19457002/diff/6001/Source/bindings/v8/ScriptW...
File Source/bindings/v8/ScriptWrappable.h (right):

https://codereview.chromium.org/19457002/diff/6001/Source/bindings/v8/ScriptW...
Source/bindings/v8/ScriptWrappable.h:81: return
unsafePersistent().newLocal(isolate);
This part I'm not sure about.  /me needs to learn more about Marja's work.

https://codereview.chromium.org/19457002/diff/6001/Source/core/dom/CustomEvent.h
File Source/core/dom/CustomEvent.h (right):

https://codereview.chromium.org/19457002/diff/6001/Source/core/dom/CustomEven...
Source/core/dom/CustomEvent.h:57: void
setSerializedDetail(PassRefPtr<SerializedScriptValue> detail) {
m_serializedScriptValue = detail; }
Should we ASSERT That we don't have one already?

[adamk, 2013-07-17 23:45:10]

https://codereview.chromium.org/19457002/diff/6001/LayoutTests/fast/events/in...
File LayoutTests/fast/events/init-custom-event-isolated-world.html (right):

https://codereview.chromium.org/19457002/diff/6001/LayoutTests/fast/events/in...
LayoutTests/fast/events/init-custom-event-isolated-world.html:6:
console.log("Tests that properties of custom events are cloned when accessed in
isolated worlds.");
On 2013/07/17 23:06:44, abarth wrote:
> Why not just put this in a <p> ?

Done.

https://codereview.chromium.org/19457002/diff/6001/Source/bindings/scripts/de...
File Source/bindings/scripts/deprecated_code_generator_v8.pm (right):

https://codereview.chromium.org/19457002/diff/6001/Source/bindings/scripts/de...
Source/bindings/scripts/deprecated_code_generator_v8.pm:2704:
event->${setter}(SerializedScriptValue::createSwallowExceptions(${attrName},
args.GetIsolate()));
On 2013/07/17 23:06:44, abarth wrote:
> createSwallowExceptions -> createAndSwallowExceptions ?

Done.

https://codereview.chromium.org/19457002/diff/6001/Source/core/dom/CustomEvent.h
File Source/core/dom/CustomEvent.h (right):

https://codereview.chromium.org/19457002/diff/6001/Source/core/dom/CustomEven...
Source/core/dom/CustomEvent.h:57: void
setSerializedDetail(PassRefPtr<SerializedScriptValue> detail) {
m_serializedScriptValue = detail; }
On 2013/07/17 23:06:44, abarth wrote:
> Should we ASSERT That we don't have one already?

Done here and in the other two events.

[haraken, 2013-07-18 00:00:29]

https://codereview.chromium.org/19457002/diff/6001/Source/bindings/v8/Diction...
File Source/bindings/v8/Dictionary.cpp (right):

https://codereview.chromium.org/19457002/diff/6001/Source/bindings/v8/Diction...
Source/bindings/v8/Dictionary.cpp:187: value =
SerializedScriptValue::createSwallowExceptions(v8Value, m_isolate);

Would you elaborate on why you want to ignore exceptions here?

https://codereview.chromium.org/19457002/diff/6001/Source/bindings/v8/V8Bindi...
File Source/bindings/v8/V8Binding.cpp (right):

https://codereview.chromium.org/19457002/diff/6001/Source/bindings/v8/V8Bindi...
Source/bindings/v8/V8Binding.cpp:543: return
DOMWrapperWorld::isolatedWorld(v8::Context::GetCurrent());

Why did you make this change?

I guess your new code is correct, but I'd like to understand why the old code
was using Context::GetEntered() instead of Context::GetCurrent().

https://codereview.chromium.org/19457002/diff/6001/Source/bindings/v8/V8Bindi...
Source/bindings/v8/V8Binding.cpp:549: return !wrapper.IsEmpty() ?
wrapper->GetHiddenValue(key) : v8::Local<v8::Value>();

Nit: I'd prefer:

wrapper.Empty() ? v8::Local<v8::Value>() : wrapper->GetHiddenValue(key)

https://codereview.chromium.org/19457002/diff/6001/Source/bindings/v8/custom/...
File Source/bindings/v8/custom/V8CustomEventCustom.cpp (right):

https://codereview.chromium.org/19457002/diff/6001/Source/bindings/v8/custom/...
Source/bindings/v8/custom/V8CustomEventCustom.cpp:69:
event->setSerializedDetail(SerializedScriptValue::createSwallowExceptions(mainWorldDetail,
info.GetIsolate()));

What happens if SerializedScriptValue::createSwallowExceptions() return 0 ?

https://codereview.chromium.org/19457002/diff/6001/Source/bindings/v8/custom/...
Source/bindings/v8/custom/V8CustomEventCustom.cpp:73: result =
event->serializedScriptValue()->deserialize();

Would it be possible to avoid deserializing the SerializedScriptValue that has
been just serialized in the line 69 ?

[adamk, 2013-07-18 01:09:14]

https://codereview.chromium.org/19457002/diff/6001/Source/bindings/v8/Diction...
File Source/bindings/v8/Dictionary.cpp (right):

https://codereview.chromium.org/19457002/diff/6001/Source/bindings/v8/Diction...
Source/bindings/v8/Dictionary.cpp:187: value =
SerializedScriptValue::createSwallowExceptions(v8Value, m_isolate);
On 2013/07/18 00:00:29, haraken wrote:
> 
> Would you elaborate on why you want to ignore exceptions here?

Note that this file is now gone.

But in all the other callers, the reason is that this serialization should be as
invisible as possible to script. The whole patch is an attempt to simulate old
behavior (where the detail/state/data properties were directly accessible from
different worlds), and anything that makes the copying noticeable (beyond the
necessities, like calling getters on the object to-be-copied) should be avoided.

None of this behavior is specced; it's all an attempt to make extensions behave
sanely.

https://codereview.chromium.org/19457002/diff/6001/Source/bindings/v8/V8Bindi...
File Source/bindings/v8/V8Binding.cpp (right):

https://codereview.chromium.org/19457002/diff/6001/Source/bindings/v8/V8Bindi...
Source/bindings/v8/V8Binding.cpp:543: return
DOMWrapperWorld::isolatedWorld(v8::Context::GetCurrent());
On 2013/07/18 00:00:29, haraken wrote:
> 
> Why did you make this change?
> 
> I guess your new code is correct, but I'd like to understand why the old code
> was using Context::GetEntered() instead of Context::GetCurrent().

I can't explain why the old code was using GetEntered (that was just a series of
refactors that landed us at the code you see today), but I needed to change it
to GetCurrent() for this patch because I thought it might be possible to trigger
the code without having Entered a context.

Note that, since an isolated world shouldn't be able to communicate with any
other, _any_ context on the stack of contexts should be able to tell us what
world we're in.

https://codereview.chromium.org/19457002/diff/6001/Source/bindings/v8/V8Bindi...
Source/bindings/v8/V8Binding.cpp:549: return !wrapper.IsEmpty() ?
wrapper->GetHiddenValue(key) : v8::Local<v8::Value>();
On 2013/07/18 00:00:29, haraken wrote:
> 
> Nit: I'd prefer:
> 
> wrapper.Empty() ? v8::Local<v8::Value>() : wrapper->GetHiddenValue(key)

Will do on the next upload.

https://codereview.chromium.org/19457002/diff/6001/Source/bindings/v8/custom/...
File Source/bindings/v8/custom/V8CustomEventCustom.cpp (right):

https://codereview.chromium.org/19457002/diff/6001/Source/bindings/v8/custom/...
Source/bindings/v8/custom/V8CustomEventCustom.cpp:69:
event->setSerializedDetail(SerializedScriptValue::createSwallowExceptions(mainWorldDetail,
info.GetIsolate()));
On 2013/07/18 00:00:29, haraken wrote:
> 
> What happens if SerializedScriptValue::createSwallowExceptions() return 0 ?

I don't believe that's possible. Is it? Even if it were, though, the if
statement below would fail nicely and we'd end up with v8::Null as our answer.

https://codereview.chromium.org/19457002/diff/6001/Source/bindings/v8/custom/...
Source/bindings/v8/custom/V8CustomEventCustom.cpp:73: result =
event->serializedScriptValue()->deserialize();
On 2013/07/18 00:00:29, haraken wrote:
> 
> Would it be possible to avoid deserializing the SerializedScriptValue that has
> been just serialized in the line 69 ?

The entire point of the patch is to force a serialize/deserialize. See the CL
description, and bug https://code.google.com/p/chromium/issues/detail?id=85158
for more detail.

[haraken, 2013-07-18 01:28:00]

https://codereview.chromium.org/19457002/diff/6001/Source/bindings/v8/custom/...
File Source/bindings/v8/custom/V8CustomEventCustom.cpp (right):

https://codereview.chromium.org/19457002/diff/6001/Source/bindings/v8/custom/...
Source/bindings/v8/custom/V8CustomEventCustom.cpp:69:
event->setSerializedDetail(SerializedScriptValue::createSwallowExceptions(mainWorldDetail,
info.GetIsolate()));

I thought it might be possible in a situation where createSwallowException
throws an exception (but he in fact doesn't throw), but anyway the situation
will be nicely handled in the below v8::Null().

https://codereview.chromium.org/19457002/diff/6001/Source/bindings/v8/custom/...
Source/bindings/v8/custom/V8CustomEventCustom.cpp:73: result =
event->serializedScriptValue()->deserialize();

sorry, I was confused. Now got it! (You might want to add a comment about it.)

[marja, 2013-07-18 09:24:51]

I don't understand this CL well enough to review its correctness. However, I
trust abarth@'s review, so, just removing myself from the reviewer list.

One question though:

https://codereview.chromium.org/19457002/diff/16001/Source/bindings/v8/V8Bind...
File Source/bindings/v8/V8Binding.cpp (right):

https://codereview.chromium.org/19457002/diff/16001/Source/bindings/v8/V8Bind...
Source/bindings/v8/V8Binding.cpp:543: return
DOMWrapperWorld::isolatedWorld(v8::Context::GetCurrent());
What's the difference between GetCurrent() and GetEntered()?

[haraken, 2013-07-18 09:38:31]

>
https://codereview.chromium.org/19457002/diff/16001/Source/bindings/v8/V8Bind...
> Source/bindings/v8/V8Binding.cpp:543: return
> DOMWrapperWorld::isolatedWorld(v8::Context::GetCurrent());
> What's the difference between GetCurrent() and GetEntered()?

For example, imagine that a function in document A calls a function defined in
document B. In this case, GetCurrent() returns document B, whereas GetEntered()
returns document A.

GetCurrent() returns a context on which we're. On the other hand, GetEntered()
returns a context that we have entered last (e.g. context->Enter() or
Context::Scope(context) enters the context).

[marja, 2013-07-18 09:43:37]

On 2013/07/18 09:38:31, haraken wrote:
> >
>
https://codereview.chromium.org/19457002/diff/16001/Source/bindings/v8/V8Bind...
> > Source/bindings/v8/V8Binding.cpp:543: return
> > DOMWrapperWorld::isolatedWorld(v8::Context::GetCurrent());
> > What's the difference between GetCurrent() and GetEntered()?
> 
> For example, imagine that a function in document A calls a function defined in
> document B. In this case, GetCurrent() returns document B, whereas
GetEntered()
> returns document A.
> 
> GetCurrent() returns a context on which we're. On the other hand, GetEntered()
> returns a context that we have entered last (e.g. context->Enter() or
> Context::Scope(context) enters the context).

Great! I'm branching this discussion to e-mail, in order to not spam this code
review.

[adamk, 2013-07-18 16:48:31]

https://codereview.chromium.org/19457002/diff/6001/Source/bindings/v8/custom/...
File Source/bindings/v8/custom/V8CustomEventCustom.cpp (right):

https://codereview.chromium.org/19457002/diff/6001/Source/bindings/v8/custom/...
Source/bindings/v8/custom/V8CustomEventCustom.cpp:73: result =
event->serializedScriptValue()->deserialize();
On 2013/07/18 01:28:01, haraken wrote:
> 
> sorry, I was confused. Now got it! (You might want to add a comment about it.)

I think it's my fault, this CL came out of an in-person discussion with abarth
where we decided what the semantics would be. I didn't want to add comments
initially since I'd need to put it in three places. But now I think I ought to
just do that. Will comment before landing, thanks for the pair of fresh eyes.

[commit-bot: I haz the power, 2013-07-18 20:34:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/adamk@chromium.org/19457002/29001

[commit-bot: I haz the power, 2013-07-18 22:36:09]

Change committed as 154535

[haraken, 2014-05-01 14:22:27]

(Dragging up an old thread...)

adamk@: Let me ask a clarifying question about this CL.

https://codereview.chromium.org/19457002/diff/29001/Source/bindings/v8/custom...
File Source/bindings/v8/custom/V8PopStateEventCustom.cpp (right):

https://codereview.chromium.org/19457002/diff/29001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PopStateEventCustom.cpp:73: return;

This code implements lazy serialization in order to optimize a case where the
main world accesses |state|. However, I don't quite understand how it's
optimized.

For example, imagine the following flow:

(1) the main world sets |state|.
(2) the main world gets |state|.
(3) an isolated world gets |state|.

Currently, the serialization of |state| happens at (2), not (3). Is this
expected? I thought that the lazy serialization is intended to delay the
serialization until any isolated world gets |state| (i.e., (3)).

If my understanding is wrong and the lazy serialization is intended just to
delay the serialization until any world (including the main world) gets |state|
(i.e., (2)), would the optimization be worth doing?

[adamk, 2014-05-02 18:33:34]

https://codereview.chromium.org/19457002/diff/29001/Source/bindings/v8/custom...
File Source/bindings/v8/custom/V8PopStateEventCustom.cpp (right):

https://codereview.chromium.org/19457002/diff/29001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PopStateEventCustom.cpp:73: return;
On 2014/05/01 14:22:27, haraken wrote:
> 
> This code implements lazy serialization in order to optimize a case where the
> main world accesses |state|. However, I don't quite understand how it's
> optimized.
> 
> For example, imagine the following flow:
> 
> (1) the main world sets |state|.
> (2) the main world gets |state|.
> (3) an isolated world gets |state|.
> 
> Currently, the serialization of |state| happens at (2), not (3). Is this
> expected? I thought that the lazy serialization is intended to delay the
> serialization until any isolated world gets |state| (i.e., (3)).

I don't think this analysis is correct. Note that in the constructor, we store
the main world state as a HiddenValue on the main world wrapper (see
PopStateEventV8Internal::constructor). And then at the top of this function we
retrieve the HiddenValue. So no serialization is done if we're still in the main
world.

Do you have some reason to believe that setting/getting the hidden value is not
working as described?

> If my understanding is wrong and the lazy serialization is intended just to
> delay the serialization until any world (including the main world) gets
|state|
> (i.e., (2)), would the optimization be worth doing?

[haraken, 2014-05-03 12:35:09]

https://codereview.chromium.org/19457002/diff/29001/Source/bindings/v8/custom...
File Source/bindings/v8/custom/V8PopStateEventCustom.cpp (right):

https://codereview.chromium.org/19457002/diff/29001/Source/bindings/v8/custom...
Source/bindings/v8/custom/V8PopStateEventCustom.cpp:73: return;
On 2014/05/02 18:33:35, adamk wrote:
> On 2014/05/01 14:22:27, haraken wrote:
> > 
> > This code implements lazy serialization in order to optimize a case where
the
> > main world accesses |state|. However, I don't quite understand how it's
> > optimized.
> > 
> > For example, imagine the following flow:
> > 
> > (1) the main world sets |state|.
> > (2) the main world gets |state|.
> > (3) an isolated world gets |state|.
> > 
> > Currently, the serialization of |state| happens at (2), not (3). Is this
> > expected? I thought that the lazy serialization is intended to delay the
> > serialization until any isolated world gets |state| (i.e., (3)).
> 
> I don't think this analysis is correct. Note that in the constructor, we store
> the main world state as a HiddenValue on the main world wrapper (see
> PopStateEventV8Internal::constructor). And then at the top of this function we
> retrieve the HiddenValue. So no serialization is done if we're still in the
main
> world.

Makes sense, thanks for the clarification! I was overlooking the code at line 51
- 56.