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..0fb598b1667aaf9d4462dc53c7f9185af7604b14 100644
--- a/content/browser/service_worker/service_worker_version.cc
+++ b/content/browser/service_worker/service_worker_version.cc
@@ -415,7 +415,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 +604,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));

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

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]

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)

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

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.

 }
 
 void ServiceWorkerVersion::DeferScheduledUpdate() {
@@ -619,11 +618,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));
 }
 
 void ServiceWorkerVersion::DispatchMessageEvent(
@@ -1725,6 +1722,8 @@ void ServiceWorkerVersion::DidEnsureLiveRegistrationForStartWorker(
     return;
   }
 
+  MarkIfStale();
+
   switch (running_status()) {
     case RUNNING:
       RunSoon(base::Bind(callback, SERVICE_WORKER_OK));
@@ -1837,6 +1836,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 +1850,17 @@ void ServiceWorkerVersion::OnTimeoutTimer() {
          running_status() == STOPPING)
       << running_status();
 
+  MarkIfStale();
+
+  // Trigger update if worker is stale and we waited long enough for it to go
+  // idle.
+  if (GetTickDuration(stale_time_) >
+      base::TimeDelta::FromMinutes(kRequestTimeoutMinutes)) {
+    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 +2073,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))

[kinuko, 2015/06/22 14:21:08]

Could we use a constant or have a comment for '24'?

+    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