Store |detail| as a hidden value of custom event wrappers

Store |detail| as a hidden value of custom event wrappers

We cannot hold strong references from Blink to V8. This means
that we shouldn't use ScriptValue in DOM impl objects.
To stop using ScriptValue in CustomEvent, store |detail|
as a hidden value of custom event wrappers and return it when
the getter of |detail| is called.

BUG=529941
Committed: https://crrev.com/bf91b3e9e1355a0d2dacd2efcdf4d971bd5c1e87
Cr-Commit-Position: refs/heads/master@{#351242}

[bashi, 2015-09-25 08:52:13]

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

[bashi, 2015-09-25 08:52:13]

Even an empty constructor (I commented out the custom constructor in the PS#1),
following code seems to cause leaks.

var e = new CustomEvent('custom', {'detail': new Error()});

Any ideas?

[bashi, 2015-09-25 09:42:34]

On 2015/09/25 08:52:13, bashi1 wrote:
> Even an empty constructor (I commented out the custom constructor in the
PS#1),
> following code seems to cause leaks.
> 
> var e = new CustomEvent('custom', {'detail': new Error()});
> 
> Any ideas?

Nevermind. I no longer observe leaks. Patch set #2 is ready for review.

[haraken, 2015-09-25 10:08:10]

Mostly looks good.

https://codereview.chromium.org/1372513002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/custom/V8CustomEventCustom.cpp
(right):

https://codereview.chromium.org/1372513002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8CustomEventCustom.cpp:92: }

Just to confirm: The line 82 - 92 is the only difference between auto-generated
constructor and the custom constructor, right?

https://codereview.chromium.org/1372513002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8CustomEventCustom.cpp:93:
wrapper = impl->associateWithWrapper(info.GetIsolate(),
&V8CustomEvent::wrapperTypeInfo, wrapper);

Remove this. You already execute this at line 80.

https://codereview.chromium.org/1372513002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8CustomEventCustom.cpp:115:
detail =
SerializedScriptValueFactory::instance().createAndSwallowExceptions(info.GetIsolate(),
mainWorldDetail)->deserialize();

At this point, wouldn't it be better to cache the serialized detail object onto
CustomEvent::m_serializedDetail?

[jochen (gone - plz use gerrit), 2015-09-25 11:23:06]

lgtm

[bashi, 2015-09-28 05:22:57]

https://codereview.chromium.org/1372513002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/custom/V8CustomEventCustom.cpp
(right):

https://codereview.chromium.org/1372513002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8CustomEventCustom.cpp:92: }
On 2015/09/25 10:08:10, haraken wrote:
> 
> Just to confirm: The line 82 - 92 is the only difference between
auto-generated
> constructor and the custom constructor, right?

Yes.

https://codereview.chromium.org/1372513002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8CustomEventCustom.cpp:93:
wrapper = impl->associateWithWrapper(info.GetIsolate(),
&V8CustomEvent::wrapperTypeInfo, wrapper);
On 2015/09/25 10:08:10, haraken wrote:
> 
> Remove this. You already execute this at line 80.

Done.

https://codereview.chromium.org/1372513002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8CustomEventCustom.cpp:115:
detail =
SerializedScriptValueFactory::instance().createAndSwallowExceptions(info.GetIsolate(),
mainWorldDetail)->deserialize();
On 2015/09/25 10:08:10, haraken wrote:
> 
> At this point, wouldn't it be better to cache the serialized detail object
onto
> CustomEvent::m_serializedDetail?

Right. I overlooked that there could be more than one isolated worlds. Done.

[haraken, 2015-09-28 05:28:41]

LGTM

[bashi, 2015-09-28 05:29:39]

The CQ bit was checked by bashi@chromium.org

[bashi, 2015-09-28 05:29:39]

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

[commit-bot: I haz the power, 2015-09-28 05:29:56]

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

[commit-bot: I haz the power, 2015-09-28 06:38:07]

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

[commit-bot: I haz the power, 2015-09-28 06:38:07]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[bashi, 2015-09-29 00:07:10]

https://codereview.chromium.org/1372513002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/js/constructor-length-expected.txt
(right):

https://codereview.chromium.org/1372513002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/js/constructor-length-expected.txt:11: FAIL
CustomEvent.length should be 1. Was 0.
Using custom constructor causes this regression. I think this is acceptable for
a short-term workaround. WDYT?

Or, we might want to make [CustomConstructor] have arguments like [Constructor]
so that we can calculate |length|. I didn't take this approach because it would
make this CL be difficult to merge branches.

[haraken, 2015-09-29 00:18:52]

https://codereview.chromium.org/1372513002/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/js/constructor-length-expected.txt
(right):

https://codereview.chromium.org/1372513002/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/js/constructor-length-expected.txt:11: FAIL
CustomEvent.length should be 1. Was 0.
On 2015/09/29 00:07:10, bashi1 wrote:
> Using custom constructor causes this regression. I think this is acceptable
for
> a short-term workaround. WDYT?
> 
> Or, we might want to make [CustomConstructor] have arguments like
[Constructor]
> so that we can calculate |length|. I didn't take this approach because it
would
> make this CL be difficult to merge branches.

constructor.length has been wrong for a long time (maybe it's still wrong in
some cases). So it would be reasonable to accept the regression in short term.

[bashi, 2015-09-29 00:21:42]

On 2015/09/29 00:18:52, haraken wrote:
>
https://codereview.chromium.org/1372513002/diff/60001/third_party/WebKit/Layo...
> File third_party/WebKit/LayoutTests/fast/js/constructor-length-expected.txt
> (right):
> 
>
https://codereview.chromium.org/1372513002/diff/60001/third_party/WebKit/Layo...
> third_party/WebKit/LayoutTests/fast/js/constructor-length-expected.txt:11:
FAIL
> CustomEvent.length should be 1. Was 0.
> On 2015/09/29 00:07:10, bashi1 wrote:
> > Using custom constructor causes this regression. I think this is acceptable
> for
> > a short-term workaround. WDYT?
> > 
> > Or, we might want to make [CustomConstructor] have arguments like
> [Constructor]
> > so that we can calculate |length|. I didn't take this approach because it
> would
> > make this CL be difficult to merge branches.
> 
> constructor.length has been wrong for a long time (maybe it's still wrong in
> some cases). So it would be reasonable to accept the regression in short term.

Thanks. Let me land this then.

[bashi, 2015-09-29 00:21:46]

The CQ bit was checked by bashi@chromium.org

[bashi, 2015-09-29 00:21:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1372513002/#ps60001
(title: " ")

[commit-bot: I haz the power, 2015-09-29 00:22:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1372513002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1372513002/60001

[commit-bot: I haz the power, 2015-09-29 02:27:54]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-09-29 02:28:41]

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

[Xianzhu, 2015-10-01 18:38:47]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1383603004/ by wangxianzhu@chromium.org.

The reason for reverting is: It caused release blocking bug 537567.

BUG=537567.