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

As per the latest spec change, when an update successfully loaded a script
resource from the network (having bypassed the HTTP cache), the registration's
last update check time should be updated regardless of whether the installation
of the fetched resource succeeds or fails.

While the existing code updates the time stamp only when the subsequent
installation of the newly received script is successfully completed (or the
loaded script is byte-for-byte match to the incumbent script resource), this CL
makes it bump the time stamp as soon as the update succeeded.

This CL added a condition for checking whether the script resource was served
from the network:
if (net_request_->response_info().network_accessed &&
    !(net_request_->response_info().was_cached)) // This line is added.

Spec discussion: https://github.com/slightlyoff/ServiceWorker/issues/514#issuecomment-135641718

BUG=539709

Review URL: https://codereview.chromium.org/1381153004

Cr-Commit-Position: refs/heads/master@{#371485}

6 files changed, 85 insertions, 25 deletions

diff --git a/content/browser/service_worker/embedded_worker_instance.h b/content/browser/service_worker/embedded_worker_instance.h
index a4ed91c..2d7c426 100644
--- a/content/browser/service_worker/embedded_worker_instance.h
+++ b/content/browser/service_worker/embedded_worker_instance.h
@@ -150,6 +150,10 @@ class CONTENT_EXPORT EmbeddedWorkerInstance {
   void set_devtools_attached(bool attached) { devtools_attached_ = attached; }
   bool devtools_attached() const { return devtools_attached_; }
 
+  bool network_accessed_for_script() const {
+    return network_accessed_for_script_;
+  }
+
   // Called when the script load request accessed the network.
   void OnNetworkAccessedForScriptLoad();
 
diff --git a/content/browser/service_worker/embedded_worker_test_helper.cc b/content/browser/service_worker/embedded_worker_test_helper.cc
index 76c218b..bfd13f3 100644
--- a/content/browser/service_worker/embedded_worker_test_helper.cc
+++ b/content/browser/service_worker/embedded_worker_test_helper.cc
@@ -160,8 +160,8 @@ void EmbeddedWorkerTestHelper::OnStartWorker(int embedded_worker_id,
   SimulateWorkerReadyForInspection(embedded_worker_id);
   SimulateWorkerScriptCached(embedded_worker_id);
   SimulateWorkerScriptLoaded(embedded_worker_id);
-  SimulateWorkerThreadStarted(next_thread_id_++, embedded_worker_id);
-  SimulateWorkerScriptEvaluated(embedded_worker_id);
+  SimulateWorkerThreadStarted(GetNextThreadId(), embedded_worker_id);
+  SimulateWorkerScriptEvaluated(embedded_worker_id, true /* success */);
   SimulateWorkerStarted(embedded_worker_id);
 }
 
@@ -271,11 +271,12 @@ void EmbeddedWorkerTestHelper::SimulateWorkerThreadStarted(
 }
 
 void EmbeddedWorkerTestHelper::SimulateWorkerScriptEvaluated(
-    int embedded_worker_id) {
+    int embedded_worker_id,
+    bool success) {
   EmbeddedWorkerInstance* worker = registry()->GetWorker(embedded_worker_id);
   ASSERT_TRUE(worker != NULL);
-  registry()->OnWorkerScriptEvaluated(
-      worker->process_id(), embedded_worker_id, true /* success */);
+  registry()->OnWorkerScriptEvaluated(worker->process_id(), embedded_worker_id,
+                                      success);
 }
 
 void EmbeddedWorkerTestHelper::SimulateWorkerStarted(
diff --git a/content/browser/service_worker/embedded_worker_test_helper.h b/content/browser/service_worker/embedded_worker_test_helper.h
index 961f163..10c7169 100644
--- a/content/browser/service_worker/embedded_worker_test_helper.h
+++ b/content/browser/service_worker/embedded_worker_test_helper.h
@@ -77,11 +77,17 @@ class EmbeddedWorkerTestHelper : public IPC::Sender,
   ServiceWorkerContextWrapper* context_wrapper() { return wrapper_.get(); }
   void ShutdownContext();
 
+  int GetNextThreadId() { return next_thread_id_++; }
+
   int mock_render_process_id() const { return mock_render_process_id_;}
   MockRenderProcessHost* mock_render_process_host() {
     return render_process_host_.get();
   }
 
+  std::map<int, int64_t> embedded_worker_id_service_worker_version_id_map() {
+    return embedded_worker_id_service_worker_version_id_map_;
+  }
+
   // Only used for tests that force creating a new render process. There is no
   // corresponding MockRenderProcessHost.
   int new_render_process_id() const { return mock_render_process_id_ + 1; }
@@ -127,7 +133,7 @@ class EmbeddedWorkerTestHelper : public IPC::Sender,
   void SimulateWorkerScriptCached(int embedded_worker_id);
   void SimulateWorkerScriptLoaded(int embedded_worker_id);
   void SimulateWorkerThreadStarted(int thread_id, int embedded_worker_id);
-  void SimulateWorkerScriptEvaluated(int embedded_worker_id);
+  void SimulateWorkerScriptEvaluated(int embedded_worker_id, bool success);
   void SimulateWorkerStarted(int embedded_worker_id);
   void SimulateWorkerStopped(int embedded_worker_id);
   void SimulateSend(IPC::Message* message);
diff --git a/content/browser/service_worker/service_worker_job_unittest.cc b/content/browser/service_worker/service_worker_job_unittest.cc
index a5572a4..c9e29b7 100644
--- a/content/browser/service_worker/service_worker_job_unittest.cc
+++ b/content/browser/service_worker/service_worker_job_unittest.cc
@@ -788,6 +788,17 @@ class UpdateJobTestHelper
     return context()->job_coordinator();
   }
 
+  bool force_bypass_cache_for_scripts() const {
+    return force_bypass_cache_for_scripts_;
+  }
+  void set_force_bypass_cache_for_scripts(bool force_bypass_cache_for_scripts) {
+    force_bypass_cache_for_scripts_ = force_bypass_cache_for_scripts;
+  }
+
+  void set_force_start_worker_failure(bool force_start_worker_failure) {
+    force_start_worker_failure_ = force_start_worker_failure;
+  }
+
   scoped_refptr<ServiceWorkerRegistration> SetupInitialRegistration(
       const GURL& test_origin) {
     scoped_refptr<ServiceWorkerRegistration> registration;
@@ -823,6 +834,8 @@ class UpdateJobTestHelper
     ASSERT_TRUE(version);
     version->AddListener(this);
 
+    if (force_bypass_cache_for_scripts())
+      version->set_force_bypass_cache_for_scripts(true);
     if (!is_update) {
       // Spoof caching the script for the initial version.
       int64_t resource_id = storage()->NewResourceId();
@@ -844,8 +857,18 @@ class UpdateJobTestHelper
       version->script_cache_map()->NotifyFinishedCaching(
           script, kMockScriptSize, net::URLRequestStatus(), std::string());
     }
-    EmbeddedWorkerTestHelper::OnStartWorker(embedded_worker_id, version_id,
-                                            scope, script);
+    if (!force_start_worker_failure_) {
+      EmbeddedWorkerTestHelper::OnStartWorker(embedded_worker_id, version_id,
+                                              scope, script);
+    } else {
+      (embedded_worker_id_service_worker_version_id_map())[embedded_worker_id] =
+          version_id;
+      SimulateWorkerReadyForInspection(embedded_worker_id);
+      SimulateWorkerScriptCached(embedded_worker_id);
+      SimulateWorkerScriptLoaded(embedded_worker_id);
+      SimulateWorkerThreadStarted(GetNextThreadId(), embedded_worker_id);
+      SimulateWorkerScriptEvaluated(embedded_worker_id, false);
+    }
   }
 
   // ServiceWorkerRegistration::Listener overrides
@@ -865,7 +888,6 @@ class UpdateJobTestHelper
   }
 
   void OnUpdateFound(ServiceWorkerRegistration* registration) override {
-    ASSERT_FALSE(update_found_);
     update_found_ = true;
   }
 
@@ -882,6 +904,8 @@ class UpdateJobTestHelper
   std::vector<AttributeChangeLogEntry> attribute_change_log_;
   std::vector<StateChangeLogEntry> state_change_log_;
   bool update_found_ = false;
+  bool force_bypass_cache_for_scripts_ = false;
+  bool force_start_worker_failure_ = false;
 };
 
 // Helper class for update tests that evicts the active version when the update
@@ -965,16 +989,35 @@ TEST_F(ServiceWorkerJobTest, Update_BumpLastUpdateCheckTime) {
   helper_.reset(update_helper);
   scoped_refptr<ServiceWorkerRegistration> registration =
       update_helper->SetupInitialRegistration(kNoChangeOrigin);
+  ASSERT_TRUE(registration.get());
+
+  registration->AddListener(update_helper);
 
-  // Run an update where the last update check was less than 24 hours ago. The
-  // check time shouldn't change, as we didn't bypass cache.
+  // Run an update that does not bypass the network cache. The check time
+  // should not be updated.
   registration->set_last_update_check(kToday);
   registration->active_version()->StartUpdate();
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(kToday, registration->last_update_check());
+  registration->RemoveListener(update_helper);
+
+  registration = update_helper->SetupInitialRegistration(kNewVersionOrigin);
+  ASSERT_TRUE(registration.get());
+
+  registration->AddListener(update_helper);
+
+  // Run an update that bypasses the network cache. The check time should be
+  // updated.
+  update_helper->set_force_bypass_cache_for_scripts(true);
+  registration->set_last_update_check(kYesterday);
+  registration->active_version()->StartUpdate();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_LT(kYesterday, registration->last_update_check());
 
-  // Run an update where the last update check was over 24 hours ago. The
-  // check time should change, as the cache was bypassed.
+  // Run an update to a worker that loads successfully but fails to start up
+  // (script evaluation failure). The check time should be updated.
+  update_helper->set_force_bypass_cache_for_scripts(true);
+  update_helper->set_force_start_worker_failure(true);
   registration->set_last_update_check(kYesterday);
   registration->active_version()->StartUpdate();
   base::RunLoop().RunUntilIdle();
diff --git a/content/browser/service_worker/service_worker_register_job.cc b/content/browser/service_worker/service_worker_register_job.cc
index 69f5acd..b079c00 100644
--- a/content/browser/service_worker/service_worker_register_job.cc
+++ b/content/browser/service_worker/service_worker_register_job.cc
@@ -352,6 +352,20 @@ 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()->last_update_check().is_null()) {
+    registration()->set_last_update_check(base::Time::Now());
+
+    if (registration()->waiting_version() || registration()->active_version())
+      context_->storage()->UpdateLastUpdateCheckTime(registration());
+  }
+
   if (status == SERVICE_WORKER_OK) {
     InstallAndContinue();
     return;
@@ -359,16 +373,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 +437,7 @@ void ServiceWorkerRegisterJob::OnInstallFinished(
   }
 
   SetPhase(STORE);
-  registration()->set_last_update_check(base::Time::Now());
+  DCHECK(!registration()->last_update_check().is_null());
   context_->storage()->StoreRegistration(
       registration(),
       new_version(),
diff --git a/content/browser/service_worker/service_worker_write_to_cache_job.cc b/content/browser/service_worker/service_worker_write_to_cache_job.cc
index 09bcb88..b5477ab 100644
--- a/content/browser/service_worker/service_worker_write_to_cache_job.cc
+++ b/content/browser/service_worker/service_worker_write_to_cache_job.cc
@@ -341,8 +341,10 @@ void ServiceWorkerWriteToCacheJob::OnResponseStarted(
     version_->SetMainScriptHttpResponseInfo(net_request_->response_info());
   }
 
-  if (net_request_->response_info().network_accessed)
+  if (net_request_->response_info().network_accessed &&
+      !(net_request_->response_info().was_cached)) {
     version_->embedded_worker()->OnNetworkAccessedForScriptLoad();
+  }
 
   http_info_.reset(new net::HttpResponseInfo(net_request_->response_info()));
   scoped_refptr<HttpResponseInfoIOBuffer> info_buffer =