IDL: Add any support to IDL dictionary and use it in CustomEventInit

IDL: Add any support to IDL dictionary and use it in CustomEventInit

We need "any" support to implement some EventInit dictionary.
Add CustomEventInit which has "details" member of type "any".

We need to be careful when initializing an event with "any" type
properties. It could be leaked from an isolated world to another isolated
world if we just keep them in an instance of DOM class and return it when
callback is called. See [1] for details.

Currently, we have special handling to avoid this problem.  Suppose we
have an event interface E which has P property of type "any".  We
annotate E with [EventConstructor] and P with
[InitializedByEventConstructor]. When these are specified, the code
generator generates a special constructor. This constructor sets the
value of P as a hidden value to a wrapper object and stores serialized
value to a DOM object. When the custom getter callback assicated
with P is called, the callback looks up the hidden value for P and
returns it if it exists. If lookup failed, the callback "clones" the
value of P from the serialized value, store it as a hidden value, then
returns it.

Since using EventInit dictionary doesn't work with [EventConstructor],
we need another solution. This CL adds ScriptValue::v8ValueFor(ScriptState*).
It is similar to ScriptValue::v8Value(), but returns "cloned" V8 value
when a given ScriptState is different from the ScriptState which is associated
with the ScriptValue. Getter callbacks for any type properties can use it
to make sure it won't leak V8 value to other worlds.

[1] https://docs.google.com/document/d/1AtnKpzQaSY3Mo1qTm68mt_3DkcZzrp_jcGS92a3a1UU/edit

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

[bashi, 2015-05-13 09:44:34]

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

[bashi, 2015-05-13 09:44:35]

Haraken-san, what do you think about this CL?

A developer asked me a question about implementing ServiceWorkerMessageEvent[1]
and it turns out that we need to support any type in IDL dictionary if we want
to avoid adding further [EventConstructor].

[1]
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#serviceworke...

https://codereview.chromium.org/1130763006/diff/1/LayoutTests/fast/events/con...
File LayoutTests/fast/events/constructors/custom-event-constructor-expected.txt
(right):

https://codereview.chromium.org/1130763006/diff/1/LayoutTests/fast/events/con...
LayoutTests/fast/events/constructors/custom-event-constructor-expected.txt:16:
PASS new CustomEvent('eventType', { detail: undefined }).detail is null
This behavior matches what the spec requires (and Firefox).

https://codereview.chromium.org/1130763006/diff/1/Source/bindings/core/v8/Scr...
File Source/bindings/core/v8/ScriptValue.cpp (right):

https://codereview.chromium.org/1130763006/diff/1/Source/bindings/core/v8/Scr...
Source/bindings/core/v8/ScriptValue.cpp:74:
ASSERT(!m_scriptState->world().isWorkerWorld());
I thought it's not safe to compare world between workers, but it may be wrong.

[haraken, 2015-05-13 14:58:36]

(I might not have time to look at this until the weekend :-)

[haraken, 2015-05-18 01:59:44]

https://codereview.chromium.org/1130763006/diff/1/Source/bindings/core/v8/cus...
File Source/bindings/core/v8/custom/V8CustomEventCustom.cpp (right):

https://codereview.chromium.org/1130763006/diff/1/Source/bindings/core/v8/cus...
Source/bindings/core/v8/custom/V8CustomEventCustom.cpp:66: if
(SerializedScriptValue* serializedValue = event->serializedDetail())

This will be problematic in the following scenario:

1) An isolated world sets a detail. Then the serialized detail is set.
2) The main world sets a detail.
3) The main world accesses event.detail(). Then we expect that no
deserialization happens during detailAttributeGetterCustom, but it actually
happens (the serialized detail set in 1) is deserialized and returned).

Actually we want to avoid introducing unnecessary complexity here. What we need
is (just) not to regress performance of event.detail() called in the main world.

Can we probably change CustomEvent like:

  class CustomEvent {
    ScriptValue m_detailForMainWorld;
    RefPtr<SerializedScriptValue> m_serializedDetail;
  };

?

Then the detailAttributeGetterCustom() can do something like:

  if (DOMWrapperWorld::current().isMainWorld()) {
    return event->detailForMainWorld();
  }
  return event->serializedDetail()->deserialize();

https://codereview.chromium.org/1130763006/diff/1/Source/bindings/scripts/v8_...
File Source/bindings/scripts/v8_dictionary.py (right):

https://codereview.chromium.org/1130763006/diff/1/Source/bindings/scripts/v8_...
Source/bindings/scripts/v8_dictionary.py:169: is_object = idl_type.name ==
'Object'

Do we need to distinguish is_any from is_object?

https://codereview.chromium.org/1130763006/diff/1/Source/core/events/CustomEv...
File Source/core/events/CustomEvent.h (right):

https://codereview.chromium.org/1130763006/diff/1/Source/core/events/CustomEv...
Source/core/events/CustomEvent.h:58: ScriptValue detail() { return m_detail; }

Add const.

[bashi, 2015-05-20 00:22:01]

https://codereview.chromium.org/1130763006/diff/1/Source/bindings/core/v8/cus...
File Source/bindings/core/v8/custom/V8CustomEventCustom.cpp (right):

https://codereview.chromium.org/1130763006/diff/1/Source/bindings/core/v8/cus...
Source/bindings/core/v8/custom/V8CustomEventCustom.cpp:66: if
(SerializedScriptValue* serializedValue = event->serializedDetail())
On 2015/05/18 01:59:44, haraken wrote:
> 
> This will be problematic in the following scenario:
> 
> 1) An isolated world sets a detail. Then the serialized detail is set.
> 2) The main world sets a detail.
> 3) The main world accesses event.detail(). Then we expect that no
> deserialization happens during detailAttributeGetterCustom, but it actually
> happens (the serialized detail set in 1) is deserialized and returned).
> 
> Actually we want to avoid introducing unnecessary complexity here. What we
need
> is (just) not to regress performance of event.detail() called in the main
world.
> 
> Can we probably change CustomEvent like:
> 
>   class CustomEvent {
>     ScriptValue m_detailForMainWorld;
>     RefPtr<SerializedScriptValue> m_serializedDetail;
>   };
> 
> ?
> 
> Then the detailAttributeGetterCustom() can do something like:
> 
>   if (DOMWrapperWorld::current().isMainWorld()) {
>     return event->detailForMainWorld();
>   }
>   return event->serializedDetail()->deserialize();
> 

As chatted offline, this CL doesn't invoke unnecessary
serialization/deserialization.
- 'detail' is a readonly attribute and is set only once (in constructor).
- When we create CustomEvent in a world W1 (no matter the world is main or an
isolated), 'detail' is set on a DOM object as a ScriptValue.
- If the world W1 accesses event.detail, we just use a V8 value which the
ScriptValue holds.
- If another world W2 accesses event.detail, we 'clone' the V8 value via
v8ValueFor().
- In both cases, once the getter callback is invoked, it caches the result as a
hidden property on the wrapper object and reuses it in following event.detail
access.

I prefer the current approach if relying on ScriptValue::scriptState() is safe
(I believe so).
- main/isolated world is a concept which should be handled by binding layer. It
would be better not to have world-specific members in DOM classes.
- Some other Events have 'any' attributes. We would want to have a common way to
avoid leaking V8 values between worlds.

WDYT?

https://codereview.chromium.org/1130763006/diff/1/Source/bindings/scripts/v8_...
File Source/bindings/scripts/v8_dictionary.py (right):

https://codereview.chromium.org/1130763006/diff/1/Source/bindings/scripts/v8_...
Source/bindings/scripts/v8_dictionary.py:169: is_object = idl_type.name ==
'Object'
On 2015/05/18 01:59:44, haraken wrote:
> 
> Do we need to distinguish is_any from is_object?

No. Actually, we don't need to put |is_any| and |is_object| into a context dict.
Removed.

https://codereview.chromium.org/1130763006/diff/1/Source/core/events/CustomEv...
File Source/core/events/CustomEvent.h (right):

https://codereview.chromium.org/1130763006/diff/1/Source/core/events/CustomEv...
Source/core/events/CustomEvent.h:58: ScriptValue detail() { return m_detail; }
On 2015/05/18 01:59:44, haraken wrote:
> 
> Add const.

Done.

[haraken, 2015-05-20 00:43:42]

LGTM

https://codereview.chromium.org/1130763006/diff/20001/Source/bindings/core/v8...
File Source/bindings/core/v8/ScriptValue.cpp (right):

https://codereview.chromium.org/1130763006/diff/20001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptValue.cpp:75:
ASSERT(!targetScriptState->world().isWorkerWorld());

Are these isWorkerWorld checks useful?

https://codereview.chromium.org/1130763006/diff/20001/Source/bindings/core/v8...
File Source/bindings/core/v8/ScriptValue.h (right):

https://codereview.chromium.org/1130763006/diff/20001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptValue.h:173: v8::Local<v8::Value> v8ValueUnsafe()
const;

Not related to this CL, this is used only in one place. Can we remove this in a
follow-up?

[bashi, 2015-05-20 00:52:22]

https://codereview.chromium.org/1130763006/diff/20001/Source/bindings/core/v8...
File Source/bindings/core/v8/ScriptValue.cpp (right):

https://codereview.chromium.org/1130763006/diff/20001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptValue.cpp:75:
ASSERT(!targetScriptState->world().isWorkerWorld());
On 2015/05/20 00:43:42, haraken wrote:
> 
> Are these isWorkerWorld checks useful?

I thought someone may misuse v8ValueFor() in workers, but it seems it unlikely
happens. Removed.

https://codereview.chromium.org/1130763006/diff/20001/Source/bindings/core/v8...
File Source/bindings/core/v8/ScriptValue.h (right):

https://codereview.chromium.org/1130763006/diff/20001/Source/bindings/core/v8...
Source/bindings/core/v8/ScriptValue.h:173: v8::Local<v8::Value> v8ValueUnsafe()
const;
On 2015/05/20 00:43:42, haraken wrote:
> 
> Not related to this CL, this is used only in one place. Can we remove this in
a
> follow-up?

Acknowledged.

[bashi, 2015-05-20 00:52:42]

The CQ bit was checked by bashi@chromium.org

[bashi, 2015-05-20 00:52:42]

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/1130763006/#ps40001
(title: " ")

[commit-bot: I haz the power, 2015-05-20 01:02:02]

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

[commit-bot: I haz the power, 2015-05-20 02:28:22]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=195573