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 42 matching lines @@
 
 // Delay between the timeout timer firing.
 const int kTimeoutTimerDelaySeconds = 30;
 
 // Time to wait until stopping an idle worker.
 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;
 
 // If the SW was destructed while starting up, how many seconds it
 // had to start up for this to be considered a timeout occurrence.
 const int kDestructedStartingWorkerTimeoutThresholdSeconds = 5;
 
 const char kClaimClientsStateErrorMesage[] =
     "Only the active worker can claim clients.";
 
@@ skipping 335 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));
 }
 
 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));

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

[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)) {
     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))

[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