Service Worker: Update stale workers

content/browser/service_worker/service_worker_version.cc

Index: content/browser/service_worker/service_worker_version.cc
diff --git a/content/browser/service_worker/service_worker_version.cc b/content/browser/service_worker/service_worker_version.cc
index 55b6a0e5dc2822ab51e846b8525fa0765b1a0f2f..142069ab2b9d7251153d922a137b3739d25ddc59 100644
--- a/content/browser/service_worker/service_worker_version.cc
+++ b/content/browser/service_worker/service_worker_version.cc
@@ -60,6 +60,9 @@ const int kIdleWorkerTimeoutSeconds = 30;
 // Default delay for scheduled update.
 const int kUpdateDelaySeconds = 1;
 
+// Time to wait until updating a stale worker that's still running.
+const int kUpdateStaleWorkerSeconds = 30;

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

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.

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

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.

+
 // Timeout for waiting for a response to a ping.
 const int kPingTimeoutSeconds = 30;
 
@@ -415,7 +418,7 @@ class ServiceWorkerVersion::Metrics {
 // if the worker is not stalling.
 class ServiceWorkerVersion::PingController {
  public:
-  PingController(ServiceWorkerVersion* version) : version_(version) {}
+  explicit PingController(ServiceWorkerVersion* version) : version_(version) {}
   ~PingController() {}
 
   void Activate() { ping_state_ = PINGING; }
@@ -604,10 +607,9 @@ void ServiceWorkerVersion::ScheduleUpdate() {
     update_timer_.Reset();
     return;
   }
-  update_timer_.Start(
-      FROM_HERE, base::TimeDelta::FromSeconds(kUpdateDelaySeconds),
-      base::Bind(&ServiceWorkerVersion::StartUpdate,
-                 weak_factory_.GetWeakPtr()));
+  update_timer_.Start(FROM_HERE,
+                      base::TimeDelta::FromSeconds(kUpdateDelaySeconds),
+                      base::Bind(&ServiceWorkerVersion::StartUpdate, this));
 }
 
 void ServiceWorkerVersion::DeferScheduledUpdate() {
@@ -619,11 +621,9 @@ void ServiceWorkerVersion::StartUpdate() {
   update_timer_.Stop();
   if (!context_)
     return;
-  ServiceWorkerRegistration* registration =
-      context_->GetLiveRegistration(registration_id_);
-  if (!registration || !registration->GetNewestVersion())
-    return;
-  context_->UpdateServiceWorker(registration, false /* force_bypass_cache */);
+  context_->storage()->FindRegistrationForId(
+      registration_id_, scope_.GetOrigin(),
+      base::Bind(&ServiceWorkerVersion::FoundRegistrationForUpdate, this));

[nhiroki, 2015/06/22 05:16:55]

"this" should be weakptr because this version could be deleted during async
database access.

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

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));

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

Oh sorry, I thought you are passing a rawptr here. Your intention sounds
reasonable.
Bind() keeps |this|, so you don't have to have another parameter for that.

 }
 
 void ServiceWorkerVersion::DispatchMessageEvent(
@@ -1725,6 +1725,8 @@ void ServiceWorkerVersion::DidEnsureLiveRegistrationForStartWorker(
     return;
   }
 
+  MarkIfStale();
+
   switch (running_status()) {
     case RUNNING:
       RunSoon(base::Bind(callback, SERVICE_WORKER_OK));
@@ -1837,6 +1839,13 @@ void ServiceWorkerVersion::StartTimeoutTimer() {
 
 void ServiceWorkerVersion::StopTimeoutTimer() {
   timeout_timer_.Stop();
+
+  // Trigger update if worker is stale.
+  if (!stale_time_.is_null()) {
+    ClearTick(&stale_time_);
+    if (!update_timer_.IsRunning())
+      ScheduleUpdate();
+  }
 }
 
 void ServiceWorkerVersion::OnTimeoutTimer() {
@@ -1844,6 +1853,17 @@ void ServiceWorkerVersion::OnTimeoutTimer() {
          running_status() == STOPPING)
       << running_status();
 
+  MarkIfStale();

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

+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.

[nhiroki, 2015/06/22 05:16:55]

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

+
+  // Trigger update if worker is stale and we waited long enough for it to go
+  // idle.
+  if (GetTickDuration(stale_time_) >
+      base::TimeDelta::FromSeconds(kUpdateStaleWorkerSeconds)) {
+    ClearTick(&stale_time_);
+    if (!update_timer_.IsRunning())
+      ScheduleUpdate();
+  }
+
   // Starting a worker hasn't finished within a certain period.
   if (GetTickDuration(start_time_) >
       base::TimeDelta::FromMinutes(kStartWorkerTimeoutMinutes)) {
@@ -2056,4 +2076,28 @@ ServiceWorkerStatusCode ServiceWorkerVersion::DeduceStartWorkerFailureReason(
   return default_code;
 }
 
+void ServiceWorkerVersion::MarkIfStale() {
+  if (!context_)
+    return;
+  if (update_timer_.IsRunning() || !stale_time_.is_null())
+    return;
+  ServiceWorkerRegistration* registration =
+      context_->GetLiveRegistration(registration_id_);
+  if (!registration || registration->active_version() != this)
+    return;
+  base::TimeDelta time_since_last_check =
+      base::Time::Now() - registration->last_update_check();
+  if (time_since_last_check > base::TimeDelta::FromHours(24))

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

nit: Should this be a constant for readability?

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

Changed to use existing kRequestTimeoutMinutes.

+    RestartTick(&stale_time_);
+}
+
+void ServiceWorkerVersion::FoundRegistrationForUpdate(
+    ServiceWorkerStatusCode status,
+    const scoped_refptr<ServiceWorkerRegistration>& registration) {
+  if (status != SERVICE_WORKER_OK || registration->active_version() != this)
+    return;
+  context_->UpdateServiceWorker(registration.get(),
+                                false /* force_bypass_cache */);
+}
+
 }  // namespace content