ServiceWorker: Prevent premature garbage collection on ServiceWorkerRegistration

ServiceWorker: Prevent premature garbage collection on ServiceWorkerRegistration

Before this patch, the ServiceWorkerRegistration object could be garbage
collected prematurely and it caused renderer crash.

This change keeps the registration object alive until stop() is called, and
notifies WebServiceWorkerRegistration of the stop event.


BUG=396400
TEST=https://codereview.chromium.org/466723002/

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

[nhiroki, 2014-08-13 09:06:23]

Ptal, thanks!

[falken, 2014-08-13 09:51:44]

lgtm as I think we need this to prevent the ServiceWorkerRegistration from being
gc'd preventing updatefound events from being dispatched, but...

ServiceWorkerRegistration doesn't have a terminal state for which we can flip
hasPendingActivity to false? It means a page can accumulate SWRegistrations that
won't be gc'd for the lifetime of the page.

[nhiroki, 2014-08-13 10:45:01]

On 2014/08/13 09:51:44, falken wrote:
> lgtm as I think we need this to prevent the ServiceWorkerRegistration from
being
> gc'd preventing updatefound events from being dispatched, but...
> 
> ServiceWorkerRegistration doesn't have a terminal state for which we can flip
> hasPendingActivity to false? It means a page can accumulate SWRegistrations
that
> won't be gc'd for the lifetime of the page.

Hmm... you are right. I wonder if we can flip the flag when unregistration is
complete, that is, ServiceWorkerDispatcher::OnUnregistered notifies
WebServiceWorkerRegistration of that via WebServiceWorkerRegistrationProxy.
Wdyt?

[falken, 2014-08-13 12:31:14]

On 2014/08/13 10:45:01, nhiroki wrote:
> On 2014/08/13 09:51:44, falken wrote:
> > lgtm as I think we need this to prevent the ServiceWorkerRegistration from
> being
> > gc'd preventing updatefound events from being dispatched, but...
> > 
> > ServiceWorkerRegistration doesn't have a terminal state for which we can
flip
> > hasPendingActivity to false? It means a page can accumulate SWRegistrations
> that
> > won't be gc'd for the lifetime of the page.
> 
> Hmm... you are right. I wonder if we can flip the flag when unregistration is
> complete, that is, ServiceWorkerDispatcher::OnUnregistered notifies
> WebServiceWorkerRegistration of that via WebServiceWorkerRegistrationProxy.
> Wdyt?

That might work but you must account for registration ressurection.

[nhiroki, 2014-08-14 08:42:05]

On 2014/08/13 12:31:14, falken wrote:
> On 2014/08/13 10:45:01, nhiroki wrote:
> > On 2014/08/13 09:51:44, falken wrote:
> > > lgtm as I think we need this to prevent the ServiceWorkerRegistration from
> > being
> > > gc'd preventing updatefound events from being dispatched, but...
> > > 
> > > ServiceWorkerRegistration doesn't have a terminal state for which we can
> flip
> > > hasPendingActivity to false? It means a page can accumulate
SWRegistrations
> > that
> > > won't be gc'd for the lifetime of the page.
> > 
> > Hmm... you are right. I wonder if we can flip the flag when unregistration
is
> > complete, that is, ServiceWorkerDispatcher::OnUnregistered notifies
> > WebServiceWorkerRegistration of that via WebServiceWorkerRegistrationProxy.
> > Wdyt?
> 
> That might work but you must account for registration ressurection.

Good point. Okay, let's revisit that later.

[nhiroki, 2014-08-14 08:43:43]

Hi kent-san, can you take a look at 
public/platform/WebServiceWorkerRegistration.h? Thanks.

[tkent, 2014-08-14 08:47:41]

lgtm

[nhiroki, 2014-08-14 09:02:18]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2014-08-14 09:03:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/nhiroki@chromium.org/467133002/40001

[commit-bot: I haz the power, 2014-08-14 13:43:25]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/2...)

[commit-bot: I haz the power, 2014-08-14 15:05:54]

Committed patchset #2 (40001) as 180265