Service Worker: Change the criteria for bumping the last update check time

content/browser/service_worker/service_worker_register_job.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_register_job.h"
 
 #include <vector>
 
 #include "base/location.h"
 #include "base/single_thread_task_runner.h"
@@ skipping 322 matching lines @@
                                            context_));
   new_version()->set_force_bypass_cache_for_scripts(force_bypass_cache_);
   new_version()->set_skip_script_comparison(skip_script_comparison_);
   new_version()->StartWorker(
       base::Bind(&ServiceWorkerRegisterJob::OnStartWorkerFinished,
                  weak_factory_.GetWeakPtr()));
 }
 
 void ServiceWorkerRegisterJob::OnStartWorkerFinished(
     ServiceWorkerStatusCode status) {
+  // Only bump the last check time when we've bypassed the browser cache.
+  if (new_version()->embedded_worker()->network_accessed_for_script() ||
+      new_version()->force_bypass_cache_for_scripts()) {
+    registration()->set_last_update_check(base::Time::Now());
+    context_->storage()->UpdateLastUpdateCheckTime(registration());

[michaeln, 2015/11/09 23:19:10]

This block is reachable for first time registrations as well as updates of
existing registrations. For first time registrations, there is nothing to update
and this call will run into an error at some point.

[jungkees, 2015/11/10 02:45:17]

Executing this part of the code for the first time registration seems fine as
the first time registration also goes through an update. But as you pointed, I
think I need a guard for the first time registration here:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/se...
as the |last_update_time| shouldn't have been initialized by that time for the
first time registration.

[falken, 2015/12/02 09:27:39]

Michael is saying that for first time registrations, the registration hasn't
been written to disk yet. So UpdateLastUpdateCheckTime() will attempt to read
the registration and discover there is none. So it shouldn't be called in that
case.

[jungkees, 2015/12/04 11:46:38]

Having inspected the code sequence for a first time registration, calling
UpdateLastUpdateCheckTime ends up doing no-op in database.cc without any
side-effect:

ServiceWorkerStorage::UpdateLastUpdateCheckTime
  base::IgnoreResult(&ServiceWorkerDatabase::UpdateLastCheckTime)
    status = ServiceWorkerDatabase::ReadRegistrationData
    if (status != STATUS_OK)
    return status;

Could you check if this is just fine as-is? If it still needs a guard before the
call, will it be a good idea to add a state like
ServiceWorkerRegistration.is_stored_? I couldn't use |is_deleted_| as it's
initialized to false. Is there any existing method to check whether a
registration is stored that I'm missing?

[falken, 2015/12/07 03:53:34]

Right, it's a no-op but it requires reading disk which is slow so it's best
avoided.

You could check if job_type_ is a REGISTER or UPDATE job. A REGISTER job would
not be written to disk yet, and an UPDATE job would.

[jungkees, 2015/12/07 07:02:09]

Great. I made it only UPDATE_JOBs write to disk.

+  }
+
   if (status == SERVICE_WORKER_OK) {
     InstallAndContinue();
     return;
   }
 
   // The updated worker is identical to the incumbent.
   if (status == SERVICE_WORKER_ERROR_EXISTS) {
-    // Only bump the last check time when we've bypassed the browser cache.
-    base::TimeDelta time_since_last_check =
-        base::Time::Now() - registration()->last_update_check();
-    if (time_since_last_check > base::TimeDelta::FromHours(
-                                    kServiceWorkerScriptMaxCacheAgeInHours) ||
-        new_version()->force_bypass_cache_for_scripts()) {
-      registration()->set_last_update_check(base::Time::Now());
-      context_->storage()->UpdateLastUpdateCheckTime(registration());
-    }
-
     ResolvePromise(SERVICE_WORKER_OK, std::string(), registration());
     Complete(status, "The updated worker is identical to the incumbent.");
     return;
   }
 
   // "If serviceWorker fails to start up..." then reject the promise with an
   // error and abort.
   if (status == SERVICE_WORKER_ERROR_TIMEOUT) {
     Complete(status, "Timed out while trying to start the Service Worker.");
     return;
@@ skipping 44 matching lines @@
     ServiceWorkerStatusCode status) {
   ServiceWorkerMetrics::RecordInstallEventStatus(status);
 
   if (status != SERVICE_WORKER_OK) {
     // "8. If installFailed is true, then:..."
     Complete(status);
     return;
   }
 
   SetPhase(STORE);
-  registration()->set_last_update_check(base::Time::Now());
   context_->storage()->StoreRegistration(
       registration(),
       new_version(),
       base::Bind(&ServiceWorkerRegisterJob::OnStoreRegistrationComplete,
                  weak_factory_.GetWeakPtr()));
 }
 
 void ServiceWorkerRegisterJob::OnStoreRegistrationComplete(
     ServiceWorkerStatusCode status) {
   if (status != SERVICE_WORKER_OK) {
@@ skipping 100 matching lines @@
     if (host->IsHostToRunningServiceWorker())
       continue;
     if (!ServiceWorkerUtils::ScopeMatches(registration->pattern(),
                                           host->document_url()))
       continue;
     host->AddMatchingRegistration(registration);
   }
 }
 
 }  // namespace content