Oilpan: dispose ServiceWorker (and registration) objects earlier.

Oilpan: dispose ServiceWorker (and registration) objects earlier.

It is unsafe to keep a ServiceWorker registered with the embedder once
it has been determined to be unreachable by the garbage collector, but
not yet finalized, as it cannot be assumed to be in a valid state beyond
that point.

Hence, we detach and unregister the object during a pre-finalization step.

The same lifetime issue applies to ServiceWorkerRegistration, so extend
the same treatment to it also.

R=haraken
BUG=451692
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=188949

[sof, 2015-01-24 19:26:42]

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

[sof, 2015-01-24 19:26:43]

Please take a look.

Option #2 (https://code.google.com/p/chromium/issues/detail?id=451692#c1) seems
the right way forward here, given the objects in question. i.e., the number of
them + their object ownership pattern.

[haraken, 2015-01-26 01:39:41]

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

[haraken, 2015-01-26 01:39:42]

LGTM

https://codereview.chromium.org/874003002/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/ServiceWorker.cpp (right):

https://codereview.chromium.org/874003002/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/ServiceWorker.cpp:269: {

Can we add ASSERT(!m_outerWorker)?

https://codereview.chromium.org/874003002/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/ServiceWorkerRegistration.cpp (right):

https://codereview.chromium.org/874003002/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/ServiceWorkerRegistration.cpp:183:
m_outerRegistration.clear();

Can we add ASSERT(!m_outerRegistration) to ~ServiceWorkerRegistration()?

https://codereview.chromium.org/874003002/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/ServiceWorkerRegistration.h (right):

https://codereview.chromium.org/874003002/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/ServiceWorkerRegistration.h:34:
WILL_BE_USING_PRE_FINALIZER(ServiceWorkerRegistration, dispose);

Can we drop WILL_BE_, since ServiceWorkerRegistration is already on-heap in
non-oilpan builds?

https://codereview.chromium.org/874003002/diff/1/Source/platform/heap/ThreadS...
File Source/platform/heap/ThreadState.h (right):

https://codereview.chromium.org/874003002/diff/1/Source/platform/heap/ThreadS...
Source/platform/heap/ThreadState.h:167: #define
WILL_UNREGISTER_PREFINALIZER(objectPointer)
ThreadState::current()->unregisterPreFinalizer(*objectPointer)

I'm not quite sure how helpful these macros are though.

[sof, 2015-01-26 06:21:41]

https://codereview.chromium.org/874003002/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/ServiceWorkerRegistration.h (right):

https://codereview.chromium.org/874003002/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/ServiceWorkerRegistration.h:34:
WILL_BE_USING_PRE_FINALIZER(ServiceWorkerRegistration, dispose);
On 2015/01/26 01:39:42, haraken wrote:
> 
> Can we drop WILL_BE_, since ServiceWorkerRegistration is already on-heap in
> non-oilpan builds?

But it doesn't use lazy sweeping?

[haraken, 2015-01-26 06:29:03]

https://codereview.chromium.org/874003002/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/ServiceWorkerRegistration.h (right):

https://codereview.chromium.org/874003002/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/ServiceWorkerRegistration.h:34:
WILL_BE_USING_PRE_FINALIZER(ServiceWorkerRegistration, dispose);
On 2015/01/26 06:21:41, sof wrote:
> On 2015/01/26 01:39:42, haraken wrote:
> > 
> > Can we drop WILL_BE_, since ServiceWorkerRegistration is already on-heap in
> > non-oilpan builds?
> 
> But it doesn't use lazy sweeping?

Conceptually, whether we should enable lazy sweeping or not should be orthogonal
with whether oilpan is enabled or disabled. (i.e., we might need to enable lazy
sweeping in non-oilpan builds at some point.) So I'd prefer dropping WILL_BE_
here.

[sof, 2015-01-26 06:32:47]

https://codereview.chromium.org/874003002/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/ServiceWorker.cpp (right):

https://codereview.chromium.org/874003002/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/ServiceWorker.cpp:269: {
On 2015/01/26 01:39:42, haraken wrote:
> 
> Can we add ASSERT(!m_outerWorker)?

Non-Oilpan you clearly could.

[sof, 2015-01-26 10:06:57]

https://codereview.chromium.org/874003002/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/ServiceWorkerRegistration.cpp (right):

https://codereview.chromium.org/874003002/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/ServiceWorkerRegistration.cpp:183:
m_outerRegistration.clear();
On 2015/01/26 01:39:42, haraken wrote:
> 
> Can we add ASSERT(!m_outerRegistration) to ~ServiceWorkerRegistration()?

Done.

https://codereview.chromium.org/874003002/diff/1/Source/platform/heap/ThreadS...
File Source/platform/heap/ThreadState.h (right):

https://codereview.chromium.org/874003002/diff/1/Source/platform/heap/ThreadS...
Source/platform/heap/ThreadState.h:167: #define
WILL_UNREGISTER_PREFINALIZER(objectPointer)
ThreadState::current()->unregisterPreFinalizer(*objectPointer)
On 2015/01/26 01:39:42, haraken wrote:
> 
> I'm not quite sure how helpful these macros are though.

ok, i'm fine with waiting until a later juncture with adding these
definition-level macros. done.

[sof, 2015-01-26 10:07:01]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-01-26 10:07:44]

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

[commit-bot: I haz the power, 2015-01-26 11:29:00]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=188949