ServiceWorkerMessageEvent shouldn't hold ScriptValue

ServiceWorkerMessageEvent shouldn't hold ScriptValue

It causes memory leak as described in [1]. Use hidden value as
workaround.

[1] http://crbug.com/529941

Committed: https://crrev.com/261e718ee5bf304dbc160df0ae15c425d7fb4a0c
Cr-Commit-Position: refs/heads/master@{#374610}

[bashi, 2016-02-10 03:30:58]

bashi@chromium.org changed reviewers:
+ falken@chromium.org, haraken@chromium.org, nhiroki@chromium.org

[bashi, 2016-02-10 03:30:58]

PTAL?

context: https://codereview.chromium.org/1658073002/#msg24

[nhiroki, 2016-02-10 03:44:18]

Thank you for quickly making a fix! lgtm from SW's POV.

https://codereview.chromium.org/1688573002/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerMessageEvent.idl
(right):

https://codereview.chromium.org/1688573002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerMessageEvent.idl:8:
// TODO(bashi): Stop using CostomConstructor once we solve reference
s/Costom/Custom/

https://codereview.chromium.org/1688573002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerMessageEvent.idl:9:
// circulation between Blink and V8. http://crbug.com/529941
Apparently, this issue was already fixed. Should we file a new one or reopen
this?

[haraken, 2016-02-10 04:13:07]

LGTM

https://codereview.chromium.org/1688573002/diff/1/third_party/WebKit/Source/b...
File
third_party/WebKit/Source/bindings/modules/v8/custom/V8ServiceWorkerMessageEventCustom.cpp
(right):

https://codereview.chromium.org/1688573002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/modules/v8/custom/V8ServiceWorkerMessageEventCustom.cpp:73:
event->setSerializedData(SerializedScriptValueFactory::instance().createAndSwallowExceptions(info.GetIsolate(),
mainWorldData));

Not directly related to your CL, I think we should enter ScriptState::Scope (of
the main world) before serializing the ScriptValue.

A follow-up CL is fine.

[bashi, 2016-02-10 04:43:38]

https://codereview.chromium.org/1688573002/diff/1/third_party/WebKit/Source/b...
File
third_party/WebKit/Source/bindings/modules/v8/custom/V8ServiceWorkerMessageEventCustom.cpp
(right):

https://codereview.chromium.org/1688573002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/modules/v8/custom/V8ServiceWorkerMessageEventCustom.cpp:73:
event->setSerializedData(SerializedScriptValueFactory::instance().createAndSwallowExceptions(info.GetIsolate(),
mainWorldData));
On 2016/02/10 04:13:07, haraken wrote:
> 
> Not directly related to your CL, I think we should enter ScriptState::Scope
(of
> the main world) before serializing the ScriptValue.
> 
> A follow-up CL is fine.

I see. Will do that in a follow-up CL (including other Events like CustomEvent).

https://codereview.chromium.org/1688573002/diff/1/third_party/WebKit/Source/m...
File
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerMessageEvent.idl
(right):

https://codereview.chromium.org/1688573002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerMessageEvent.idl:8:
// TODO(bashi): Stop using CostomConstructor once we solve reference
On 2016/02/10 03:44:17, nhiroki (slow) wrote:
> s/Costom/Custom/

Done.

https://codereview.chromium.org/1688573002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/serviceworkers/ServiceWorkerMessageEvent.idl:9:
// circulation between Blink and V8. http://crbug.com/529941
On 2016/02/10 03:44:17, nhiroki (slow) wrote:
> Apparently, this issue was already fixed. Should we file a new one or reopen
> this?

Thanks for the catch. It should be http://crbug.com/501866. Changed.

[bashi, 2016-02-10 04:43:51]

The CQ bit was checked by bashi@chromium.org

[bashi, 2016-02-10 04:43:51]

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

[commit-bot: I haz the power, 2016-02-10 04:43:57]

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

[commit-bot: I haz the power, 2016-02-10 05:54:57]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-02-10 05:56:17]

Description was changed from

==========
ServiceWorkerMessageEvent shouldn't hold ScriptValue

It causes memory leak as described in [1]. Use hidden value as
workaround.

[1] http://crbug.com/529941
==========

to

==========
ServiceWorkerMessageEvent shouldn't hold ScriptValue

It causes memory leak as described in [1]. Use hidden value as
workaround.

[1] http://crbug.com/529941

Committed: https://crrev.com/261e718ee5bf304dbc160df0ae15c425d7fb4a0c
Cr-Commit-Position: refs/heads/master@{#374610}
==========

[commit-bot: I haz the power, 2016-02-10 05:56:18]

Patchset 2 (id:??) landed as
https://crrev.com/261e718ee5bf304dbc160df0ae15c425d7fb4a0c
Cr-Commit-Position: refs/heads/master@{#374610}