Service Worker: Update stale workers

content/browser/service_worker/service_worker_version.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/service_worker/service_worker_version.h"
 
 #include "base/command_line.h"
 #include "base/location.h"
 #include "base/memory/ref_counted.h"
 #include "base/metrics/histogram_macros.h"
@@ skipping 397 matching lines @@
  private:
   size_t fired_events = 0;
   size_t handled_events = 0;
   DISALLOW_COPY_AND_ASSIGN(Metrics);
 };
 
 // A controller for periodically sending a ping to the worker to see
 // 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; }
 
   void Deactivate() {
     ClearTick(&ping_time_);
     ping_state_ = NOT_PINGING;
   }
 
   void OnPongReceived() { ClearTick(&ping_time_); }
@@ skipping 168 matching lines @@
     }
   }
   stop_callbacks_.push_back(callback);
 }
 
 void ServiceWorkerVersion::ScheduleUpdate() {
   if (update_timer_.IsRunning()) {
     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() {
   if (update_timer_.IsRunning())
     update_timer_.Reset();
 }
 
 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(
     const base::string16& message,
     const std::vector<TransferredMessagePort>& sent_message_ports,
     const StatusCallback& callback) {
   for (const TransferredMessagePort& port : sent_message_ports) {
     MessagePortService::GetInstance()->HoldMessages(port.id);
   }
 
@@ skipping 1081 matching lines @@
     RecordStartWorkerResult(status);
     RunSoon(base::Bind(callback, SERVICE_WORKER_ERROR_START_WORKER_FAILED));
     return;
   }
   if (is_redundant()) {
     RecordStartWorkerResult(SERVICE_WORKER_ERROR_REDUNDANT);
     RunSoon(base::Bind(callback, SERVICE_WORKER_ERROR_REDUNDANT));
     return;
   }
 
+  MarkIfStale();
+
   switch (running_status()) {
     case RUNNING:
       RunSoon(base::Bind(callback, SERVICE_WORKER_OK));
       return;
     case STOPPING:
     case STOPPED:
     case STARTING:
       if (start_callbacks_.empty()) {
         start_callbacks_.push_back(
             base::Bind(&ServiceWorkerVersion::RecordStartWorkerResult,
@@ skipping 92 matching lines @@
   // Ping will be activated in OnScriptLoaded.
   ping_controller_->Deactivate();
 
   timeout_timer_.Start(FROM_HERE,
                        base::TimeDelta::FromSeconds(kTimeoutTimerDelaySeconds),
                        this, &ServiceWorkerVersion::OnTimeoutTimer);
 }
 
 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() {
   DCHECK(running_status() == STARTING || running_status() == RUNNING ||
          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)) {
     DCHECK(running_status() == STARTING || running_status() == STOPPING)
         << running_status();
     scoped_refptr<ServiceWorkerVersion> protect(this);
     RunCallbacks(this, &start_callbacks_, SERVICE_WORKER_ERROR_TIMEOUT);
     if (running_status() == STARTING)
       embedded_worker_->Stop();
     return;
@@ skipping 192 matching lines @@
       case net::ERR_ABORTED:
         return SERVICE_WORKER_ERROR_ABORT;
       default:
         return SERVICE_WORKER_ERROR_NETWORK;
     }
   }
 
   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