Oilpan: move ServiceWorkerGlobalScopeProxy to the heap.

Oilpan: move ServiceWorkerGlobalScopeProxy to the heap.

The object maintains two references to heap objects, so by having it
also be on the Oilpan heap, these refs can be correctly traced & handled.

R=haraken
BUG=553613
Committed: https://crrev.com/dc3fa19f4b47fab944866a2e2f13089292d45f16
Cr-Commit-Position: refs/heads/master@{#360381}

[sof, 2015-11-17 12:16:35]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org

[haraken, 2015-11-17 13:04:35]

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

[haraken, 2015-11-17 13:04:36]

https://codereview.chromium.org/1448353002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.h (right):

https://codereview.chromium.org/1448353002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.h:104:
WebEmbeddedWorkerImpl& m_embeddedWorker;

WebEmbeddedWorkerImpl is still off the heap. Doesn't the m_embeddedWorker have a
risk of becoming a stale pointer? (in a period from when the
WebEmbeddedWorkerImpl gets destructed to when the next GC is triggered)

https://codereview.chromium.org/1448353002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.h:108:
WebServiceWorkerContextClient& m_client;

Similarly, is this raw pointer safe?

[sof, 2015-11-17 13:43:04]

https://codereview.chromium.org/1448353002/diff/1/third_party/WebKit/Source/w...
File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.h (right):

https://codereview.chromium.org/1448353002/diff/1/third_party/WebKit/Source/w...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.h:104:
WebEmbeddedWorkerImpl& m_embeddedWorker;
On 2015/11/17 13:04:36, haraken wrote:
> 
> WebEmbeddedWorkerImpl is still off the heap. Doesn't the m_embeddedWorker have
a
> risk of becoming a stale pointer? (in a period from when the
> WebEmbeddedWorkerImpl gets destructed to when the next GC is triggered)

This object would have to be used by a worker thread that we terminate before
finalizing WebEmbeddedWorkerImpl, so it is not likely?

That said, we could dispose/detach this proxy as part of finalizing
WebEmbedddWorkerImpl.

[haraken, 2015-11-17 14:00:54]

On 2015/11/17 13:43:04, sof wrote:
>
https://codereview.chromium.org/1448353002/diff/1/third_party/WebKit/Source/w...
> File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.h (right):
> 
>
https://codereview.chromium.org/1448353002/diff/1/third_party/WebKit/Source/w...
> third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.h:104:
> WebEmbeddedWorkerImpl& m_embeddedWorker;
> On 2015/11/17 13:04:36, haraken wrote:
> > 
> > WebEmbeddedWorkerImpl is still off the heap. Doesn't the m_embeddedWorker
have
> a
> > risk of becoming a stale pointer? (in a period from when the
> > WebEmbeddedWorkerImpl gets destructed to when the next GC is triggered)
> 
> This object would have to be used by a worker thread that we terminate before
> finalizing WebEmbeddedWorkerImpl, so it is not likely?
> 
> That said, we could dispose/detach this proxy as part of finalizing
> WebEmbedddWorkerImpl.

Yeah, that should be safer.

[sof, 2015-11-17 15:11:53]

On 2015/11/17 14:00:54, haraken wrote:
> On 2015/11/17 13:43:04, sof wrote:
> >
>
https://codereview.chromium.org/1448353002/diff/1/third_party/WebKit/Source/w...
> > File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.h (right):
> > 
> >
>
https://codereview.chromium.org/1448353002/diff/1/third_party/WebKit/Source/w...
> > third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.h:104:
> > WebEmbeddedWorkerImpl& m_embeddedWorker;
> > On 2015/11/17 13:04:36, haraken wrote:
> > > 
> > > WebEmbeddedWorkerImpl is still off the heap. Doesn't the m_embeddedWorker
> have
> > a
> > > risk of becoming a stale pointer? (in a period from when the
> > > WebEmbeddedWorkerImpl gets destructed to when the next GC is triggered)
> > 
> > This object would have to be used by a worker thread that we terminate
before
> > finalizing WebEmbeddedWorkerImpl, so it is not likely?
> > 
> > That said, we could dispose/detach this proxy as part of finalizing
> > WebEmbedddWorkerImpl.
> 
> Yeah, that should be safer.

Now done; opted for "detach".

[haraken, 2015-11-17 23:54:00]

LGTM

https://codereview.chromium.org/1448353002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp (right):

https://codereview.chromium.org/1448353002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp:83: {

Can we assert that detach() has been called?

https://codereview.chromium.org/1448353002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp:258:
WebServiceWorkerContextClient& ServiceWorkerGlobalScopeProxy::client() const

Nit: I'd inline these getters.

[sof, 2015-11-18 08:57:39]

Description was changed from

==========
Oilpan: move ServiceWorkerGlobalScopeProxy to the heap.

The object maintains two references to heap objects, so by having it
also be on the Oilpan heap, these refs can be correctly traced & handled.

R=
BUG=553613
==========

to

==========
Oilpan: move ServiceWorkerGlobalScopeProxy to the heap.

The object maintains two references to heap objects, so by having it
also be on the Oilpan heap, these refs can be correctly traced & handled.

R=haraken
BUG=553613
==========

[sof, 2015-11-18 08:57:54]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2015-11-18 08:57:55]

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/1448353002/#ps60001
(title: "assert for clean detachment")

[sof, 2015-11-18 08:58:09]

https://codereview.chromium.org/1448353002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp (right):

https://codereview.chromium.org/1448353002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ServiceWorkerGlobalScopeProxy.cpp:83: {
On 2015/11/17 23:53:59, haraken wrote:
> 
> Can we assert that detach() has been called?

Done; asserting for !m_embeddedWorker.

[commit-bot: I haz the power, 2015-11-18 08:58:19]

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

[commit-bot: I haz the power, 2015-11-18 10:08:54]

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

[commit-bot: I haz the power, 2015-11-18 10:08:55]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sof, 2015-11-18 10:49:34]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-11-18 10:50:00]

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

[commit-bot: I haz the power, 2015-11-18 12:33:21]

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

[commit-bot: I haz the power, 2015-11-18 12:33:21]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sof, 2015-11-18 18:16:52]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-11-18 18:18:53]

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

[commit-bot: I haz the power, 2015-11-18 19:08:20]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-11-18 19:09:25]

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