Service Worker: Update stale workers

Service Worker: Update stale workers

Before this patch we only update a worker on main resource navigation
inside its scope. This means a Push-only SW for which you don't navigate
to its scope is never updated.

This patch introduces a way to update "stale" workers.  A worker that
hasn't been checked for update from the network for over 24 hours is
considered stale. We check for staleness when a worker starts up and
periodically while it's running.  We try to update stale workers once
they stop (one reason is that skipWaiting() may kick out the worker
before it finished handling the event that triggered the staleness
check; also want to avoid network contention while the incumbent is
handling an event).  However if a worker is running too long, an update
may be triggered (otherwise, a constant stream of events keeping the
worker alive would prevent it from ever updating).

BUG=477598
TEST=ServiceWorkerVersionTest.StaleUpdate*

Committed: https://crrev.com/13053b7698140f33fc71dd7ab92b01ef3ec45551
Cr-Commit-Position: refs/heads/master@{#335901}

[falken, 2015-06-18 09:05:56]

falken@chromium.org changed reviewers:
+ kinuko@chromium.org, michaeln@chromium.org, nhiroki@chromium.org,
peter@chromium.org

[falken, 2015-06-18 09:05:57]

PTAL

nhiroki, kinuko: all
peter: FYI, does this solve your use case?
michaeln: FYI if you have time

https://codereview.chromium.org/1187623006/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1187623006/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:1856: MarkIfStale();
+nhiroki: I'm hoping while the worker is running I can assume there's a live
registration for it? Would be painful to read from storage on each heartbeat.

[Peter Beverloo, 2015-06-18 14:08:58]

Yes, this seems great from our POV. Thanks!

https://codereview.chromium.org/1187623006/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1187623006/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:64: const int
kUpdateStaleWorkerSeconds = 30;
It'd be great if we could get some UMA coverage to track when this number has
been exceeded.

Looking at the ServiceWorker.PushEvent.Time histogram, the 90th percentile of
push events are under 10 seconds, the 99th percentile under 28 seconds. Since we
handle all events in parallel, this should be a good number for Push, even
during start-up bursts.

https://codereview.chromium.org/1187623006/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:2090: if
(time_since_last_check > base::TimeDelta::FromHours(24))
nit: Should this be a constant for readability?

[nhiroki, 2015-06-22 05:16:56]

Looks good overall. Can you fix test failure on linux_asan bot?

https://codereview.chromium.org/1187623006/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1187623006/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:626:
base::Bind(&ServiceWorkerVersion::FoundRegistrationForUpdate, this));
"this" should be weakptr because this version could be deleted during async
database access.

https://codereview.chromium.org/1187623006/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:1856: MarkIfStale();
On 2015/06/18 09:05:57, falken wrote:
> +nhiroki: I'm hoping while the worker is running I can assume there's a live
> registration for it? Would be painful to read from storage on each heartbeat.

Yes, you can assume that because ServiceWorkerGlobalScope owns its registration
handle for "self.registration".

[falken, 2015-06-22 08:05:44]

Thanks for reviews!

https://codereview.chromium.org/1187623006/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1187623006/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:64: const int
kUpdateStaleWorkerSeconds = 30;
On 2015/06/18 14:08:58, Peter Beverloo wrote:
> It'd be great if we could get some UMA coverage to track when this number has
> been exceeded.
> 
> Looking at the ServiceWorker.PushEvent.Time histogram, the 90th percentile of
> push events are under 10 seconds, the 99th percentile under 28 seconds. Since
we
> handle all events in parallel, this should be a good number for Push, even
> during start-up bursts.

Actually I realized I should be using 5 minutes as the waiting value, it's the
time we allot to all events like Push to timeout, and if we're waiting 24 hours
for an update 30 sec vs 5 minutes doesn't matter.

I agree UMA on timing out the events would be nice, I don't think we have this
yet.

https://codereview.chromium.org/1187623006/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:626:
base::Bind(&ServiceWorkerVersion::FoundRegistrationForUpdate, this));
On 2015/06/22 05:16:55, nhiroki wrote:
> "this" should be weakptr because this version could be deleted during async
> database access.

I actually wanted to ensure the version stays alive once we schedule the update.
Otherwise I think you could have OnStopped -> StopTimeoutTimer() ->
ScheduleUpdate() -> then all refs to version are dropped since it's no longer
running, and the worker doesn't update.

Does Bind() keep |this| alive? Or would binding it as a parameter to
FoundRegistrationForUpdate keep it alive:

base::Bind(&SWVersion::FoundRegistrationForUpdate, this,
make_scoped_refptr(this));

https://codereview.chromium.org/1187623006/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:2090: if
(time_since_last_check > base::TimeDelta::FromHours(24))
On 2015/06/18 14:08:58, Peter Beverloo wrote:
> nit: Should this be a constant for readability?

Changed to use existing kRequestTimeoutMinutes.

[nhiroki, 2015-06-22 08:39:17]

lgtm

https://codereview.chromium.org/1187623006/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1187623006/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:626:
base::Bind(&ServiceWorkerVersion::FoundRegistrationForUpdate, this));
On 2015/06/22 08:05:44, falken wrote:
> On 2015/06/22 05:16:55, nhiroki wrote:
> > "this" should be weakptr because this version could be deleted during async
> > database access.
> 
> I actually wanted to ensure the version stays alive once we schedule the
update.
> Otherwise I think you could have OnStopped -> StopTimeoutTimer() ->
> ScheduleUpdate() -> then all refs to version are dropped since it's no longer
> running, and the worker doesn't update.

Oh sorry, I thought you are passing a rawptr here. Your intention sounds
reasonable.

> Does Bind() keep |this| alive? Or would binding it as a parameter to
> FoundRegistrationForUpdate keep it alive:
> 
> base::Bind(&SWVersion::FoundRegistrationForUpdate, this,
> make_scoped_refptr(this));

Bind() keeps |this|, so you don't have to have another parameter for that.

[falken, 2015-06-22 08:43:59]

On 2015/06/22 08:39:17, nhiroki wrote:
> lgtm
> 
>
https://codereview.chromium.org/1187623006/diff/20001/content/browser/service...
> File content/browser/service_worker/service_worker_version.cc (right):
> 
>
https://codereview.chromium.org/1187623006/diff/20001/content/browser/service...
> content/browser/service_worker/service_worker_version.cc:626:
> base::Bind(&ServiceWorkerVersion::FoundRegistrationForUpdate, this));
> On 2015/06/22 08:05:44, falken wrote:
> > On 2015/06/22 05:16:55, nhiroki wrote:
> > > "this" should be weakptr because this version could be deleted during
async
> > > database access.
> > 
> > I actually wanted to ensure the version stays alive once we schedule the
> update.
> > Otherwise I think you could have OnStopped -> StopTimeoutTimer() ->
> > ScheduleUpdate() -> then all refs to version are dropped since it's no
longer
> > running, and the worker doesn't update.
> 
> Oh sorry, I thought you are passing a rawptr here. Your intention sounds
> reasonable.
> 
> > Does Bind() keep |this| alive? Or would binding it as a parameter to
> > FoundRegistrationForUpdate keep it alive:
> > 
> > base::Bind(&SWVersion::FoundRegistrationForUpdate, this,
> > make_scoped_refptr(this));
> 
> Bind() keeps |this|, so you don't have to have another parameter for that.

Got it, thanks.

[falken, 2015-06-22 08:44:13]

The CQ bit was checked by falken@chromium.org

[commit-bot: I haz the power, 2015-06-22 08:44:20]

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

[commit-bot: I haz the power, 2015-06-22 09:57:01]

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

[commit-bot: I haz the power, 2015-06-22 09:57:03]

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

[kinuko, 2015-06-22 14:02:53]

https://codereview.chromium.org/1187623006/diff/60001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1187623006/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:609:
base::Bind(&ServiceWorkerVersion::StartUpdate, this));
Hmm. This one makes the timer's internal task hold a ref to the registration,
which owns the update timer, which seems to make a cycle.

When an internal timer task (which is owned by MessageLoop while it's queued) is
deleted it tries to stop the timer in its dtor by calling StopAndAbandon(),
which deletes the scheduled timer task, which unrefs the version, which tries to
delete the timer, which again calls StopAndAbandon() before the first one
finishes..

[kinuko, 2015-06-22 14:21:09]

https://codereview.chromium.org/1187623006/diff/60001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1187623006/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:609:
base::Bind(&ServiceWorkerVersion::StartUpdate, this));
Maybe we could rely on context or some external party to hold a ref during
update, or to schedule an update.  (I've been suspecting we may need something
like that to prolong version's lifetime after all ref's dropped in some corner
cases)

https://codereview.chromium.org/1187623006/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:2087: if
(time_since_last_check > base::TimeDelta::FromHours(24))
Could we use a constant or have a comment for '24'?

[falken, 2015-06-23 07:53:27]

https://codereview.chromium.org/1187623006/diff/60001/content/browser/service...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1187623006/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_version.cc:609:
base::Bind(&ServiceWorkerVersion::StartUpdate, this));
On 2015/06/22 14:21:09, kinuko wrote:
> Maybe we could rely on context or some external party to hold a ref during
> update, or to schedule an update.  (I've been suspecting we may need something
> like that to prolong version's lifetime after all ref's dropped in some corner
> cases)

Yeah I don't see a great way to do this, I'm thinking of just
SWContextCore::ProtectVersion/UnprotectVersion when we have to keep the version
alive. It feels hacky but moving the timers somewhere external like
SWContextCore seems ugly as well.

I tried to do a helper class that holds the version, but it looks to run into
the same cycle issue: someone has to keep the helper class alive and it can't be
itself or its timer.

[falken, 2015-06-23 09:31:58]

Patchset #6 (id:100001) has been deleted

[falken, 2015-06-23 09:32:05]

Patchset #5 (id:80001) has been deleted

[falken, 2015-06-23 09:48:52]

Updated to have context core keep the version alive. PTAL.

[kinuko, 2015-06-23 14:30:49]

lgtm

[nhiroki, 2015-06-24 05:14:42]

Still lgtm. Can you add TEST= line?

https://codereview.chromium.org/1187623006/diff/160001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1187623006/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:614:
context_->ProtectVersion(make_scoped_refptr(this));
Can you check |context| before accessing?

https://codereview.chromium.org/1187623006/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:2107:
context_->UnprotectVersion(make_scoped_refptr(this));
How about passing "version_id" instead of "make_scoped_refptr(this)"?

[falken, 2015-06-24 07:58:11]

Thanks, updated.

> Can you add TEST= line?

Added this time. A long time ago, TEST= was meant for instructions to QA team.
The line was misunderstood so it's been removed from the autogenerated CL
descriptions.

I think these days there's still no wide agreement on how to use the line:

https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/"TEST$...

https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/"TEST$...

https://codereview.chromium.org/1187623006/diff/160001/content/browser/servic...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/1187623006/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:614:
context_->ProtectVersion(make_scoped_refptr(this));
On 2015/06/24 05:14:41, nhiroki wrote:
> Can you check |context| before accessing?

Done.

https://codereview.chromium.org/1187623006/diff/160001/content/browser/servic...
content/browser/service_worker/service_worker_version.cc:2107:
context_->UnprotectVersion(make_scoped_refptr(this));
On 2015/06/24 05:14:41, nhiroki wrote:
> How about passing "version_id" instead of "make_scoped_refptr(this)"?

Sure, done.

[falken, 2015-06-24 07:58:37]

The CQ bit was checked by falken@chromium.org

[falken, 2015-06-24 07:58:39]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
nhiroki@chromium.org
Link to the patchset: https://codereview.chromium.org/1187623006/#ps180001
(title: "review comments")

[commit-bot: I haz the power, 2015-06-24 07:59:09]

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

[nhiroki, 2015-06-24 08:16:40]

On 2015/06/24 07:58:11, falken wrote:
> Thanks, updated.
> 
> > Can you add TEST= line?
> 
> Added this time. A long time ago, TEST= was meant for instructions to QA team.
> The line was misunderstood so it's been removed from the autogenerated CL
> descriptions.
> 
> I think these days there's still no wide agreement on how to use the line:
> 
>
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/%22TES...
> 
>
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/%22TES...,


Ah, I didn't notice it. I was frequently encouraged to add the line when I
became a commiter, so I was thinking it was still valid. Thanks for your
information!

[commit-bot: I haz the power, 2015-06-24 08:29:25]

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

[commit-bot: I haz the power, 2015-06-24 08:29:26]

Exceeded global retry quota

[falken, 2015-06-24 08:37:16]

The CQ bit was checked by falken@chromium.org

[commit-bot: I haz the power, 2015-06-24 08:37:53]

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

[commit-bot: I haz the power, 2015-06-24 08:45:12]

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

[commit-bot: I haz the power, 2015-06-24 08:45:13]

Exceeded global retry quota

[falken, 2015-06-24 10:00:29]

Patchset #9 (id:200001) has been deleted

[falken, 2015-06-24 10:02:52]

Updated the patch to fix DCHECK failures. Some tests call StartUpdate directly
and would attempt to UnprotectVersion that was never protected.

[falken, 2015-06-24 10:03:17]

The CQ bit was checked by falken@chromium.org

[falken, 2015-06-24 10:03:17]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org,
nhiroki@chromium.org
Link to the patchset: https://codereview.chromium.org/1187623006/#ps220001
(title: "fix DCHECK")

[commit-bot: I haz the power, 2015-06-24 10:03:23]

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

[commit-bot: I haz the power, 2015-06-24 11:02:09]

Committed patchset #9 (id:220001)

[commit-bot: I haz the power, 2015-06-24 11:03:12]

Patchset 9 (id:??) landed as
https://crrev.com/13053b7698140f33fc71dd7ab92b01ef3ec45551
Cr-Commit-Position: refs/heads/master@{#335901}