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

content/browser/service_worker/service_worker_register_job.cc

Index: content/browser/service_worker/service_worker_register_job.cc
diff --git a/content/browser/service_worker/service_worker_register_job.cc b/content/browser/service_worker/service_worker_register_job.cc
index 69f5acdcb9c995aaa4c2b258356001fe1f6fb181..d5fa1949fd5d7704949801bd9f075fed7de17819 100644
--- a/content/browser/service_worker/service_worker_register_job.cc
+++ b/content/browser/service_worker/service_worker_register_job.cc
@@ -352,6 +352,19 @@ void ServiceWorkerRegisterJob::UpdateAndContinue() {
 
 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()->set_last_update_check(base::Time::Now());
+
+    if (registration()->waiting_version() || registration()->active_version())

[michaeln, 2016/01/19 19:20:28]

if job_type_ == UPDATE_JOB works as the condition here, that might be a little
clearer

[jungkees, 2016/01/20 05:38:11]

I discussed with falken@ and tried so, but later he found that the condition (if
job_type_ == UPDATE_JOB) doesn't seem to cover all the cases:
https://codereview.chromium.org/1381153004/#msg30

[falken, 2016/01/20 09:31:54]

Right. I was then worried about whether waiting_version || active_version really
holds iff there is a stored version. For example, SWRegistration::DeleteVersion
does some sketchy unsetting of the versions and might not cleanup the stored
registration properly. But I think in that case, we'd update the check time when
the installing version reaches OnInstallFinished.
(I'm meaning to move DeleteVersion into a job to avoid race conditions with
ongoing jobs).

+      context_->storage()->UpdateLastUpdateCheckTime(registration());
+  }

[michaeln, 2016/01/21 20:55:31]

else { there's an assumption here that last_update_check() is set to a non-empty
value }

how about add that test to the main condition on line 360?

if (network_accessed_for_script() ||
    force_bypass_cache_for_scripts() ||
    reg->last_update_check().is_null())

[jungkees, 2016/01/22 05:22:27]

Done.

+
   if (status == SERVICE_WORKER_OK) {
     InstallAndContinue();
     return;
@@ -359,16 +372,6 @@ void ServiceWorkerRegisterJob::OnStartWorkerFinished(
 
   // 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;
@@ -433,7 +436,6 @@ void ServiceWorkerRegisterJob::OnInstallFinished(
   }
 
   SetPhase(STORE);
-  registration()->set_last_update_check(base::Time::Now());

[michaeln, 2016/01/19 19:20:28]

Is there any chance we can be here w/o having set the datamember in
OnStartWorkerFinished? Would it hurt to set the value to a slightly newer value
here?

[jungkees, 2016/01/20 05:38:11]

No, I don't think so.
But yes because there's a case where the data member is updated in
OnStartWorkerFinished but doesn't call InstallAndContinue. (That is, the update
job resolves immediately without advancing to install even though the network
cache was validated.)

[falken, 2016/01/20 09:31:54]

I guess it'd only be possible if network_accessed_for_script or
force_bypass_cache_for_scripts was wrong. Adding it in both places wouldn't
really hurt except for being a bit redundant. How about doing:

if (new_version()->embedded_worker()->network_accessed_for_script() ||
    new_version()->force_bypass_cache_for_scripts()) {
  ...
} else {
  DCHECK_NE(status, SERVICE_WORKER_OK);
}

if (status == SERVICE_WORKER_OK)
  InstallAndContinue();

This asserts you couldn't get to OnInstallFinished without setting the update
check time.

[michaeln, 2016/01/21 20:55:31]

Right, an assurance of some kind about having a meaningfully set value is the
gist of my question.

DCHECK(!reg->last_update_check().is_null());

[jungkees, 2016/01/22 05:22:26]

Okay. To assure that, I added this assertion as well as
(reg->last_update_check().is_null()) check in the main condition on line 360.

   context_->storage()->StoreRegistration(
       registration(),
       new_version(),