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 <stdint.h>
 
 #include <vector>
 
@@ skipping 334 matching lines @@
                                            version_id, 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) {
+  // Bump the last update check time only when the register/update job fetched
+  // the version having bypassed the network cache. We assume that the
+  // BYPASS_CACHE flag evicts an existing cache entry, so even if the install
+  // ultimately failed for whatever reason, we know the version in the HTTP
+  // cache is not stale, so it's OK to bump the update check time.
+  if (new_version()->embedded_worker()->network_accessed_for_script() ||
+      new_version()->force_bypass_cache_for_scripts() ||
+      registration()->last_update_check().is_null()) {
+    registration()->set_last_update_check(base::Time::Now());
+
+    if (registration()->waiting_version() || registration()->active_version())
+      context_->storage()->UpdateLastUpdateCheckTime(registration());
+  } else {
+    DCHECK(!registration()->last_update_check().is_null());

[michaeln, 2016/01/22 19:48:13]

This dcheck is very useless here. If the time is null the if condition will be
met and the else clause can't possibly be entered.

[jungkees, 2016/01/25 20:03:00]

I moved the dcheck as suggested.

+  }
+
   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 43 matching lines @@
 void ServiceWorkerRegisterJob::OnInstallFinished(
     ServiceWorkerStatusCode status) {
   ServiceWorkerMetrics::RecordInstallEventStatus(status);
 
   if (status != SERVICE_WORKER_OK) {
     // "8. If installFailed is true, then:..."
     Complete(status);
     return;
   }
 
   SetPhase(STORE);

[michaeln, 2016/01/22 19:48:13]

If you're going to add the dcheck, please put it here.

It might be helpful here, if somehow this method gets entered w/o having that
time set (so we skipped OnStartWorkerFinished somehow), it would trip and we'd
find the bug faster.

In current code, the time is undoubtedly set upon return. With your new code,
that's probably not true in all cases (some tests might need to be updated to
set the time). I'd vote to make sure the time is set in all cases so we don't
have the possibility of installed registrations with null time values.

[jungkees, 2016/01/25 20:03:00]

Yes, I moved the dcheck here.
I agree we have to make sure the time value for an installed registration won't
be null here. But I doubt we have a path here without getting through
OnStartWorkerFinished. If we detect such an error case, the condition
(new_version()->embedded_worker()->network_accessed_for_script() ||
new_version()->force_bypass_cache_for_scripts()) has to be rechecked I believe.
Also the very reason that this change suggests is aligning the behavior to the
spec change which says the time shouldn't be updated depending on conditions.

-  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