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

content/browser/service_worker/service_worker_job_unittest.cc

Index: content/browser/service_worker/service_worker_job_unittest.cc
diff --git a/content/browser/service_worker/service_worker_job_unittest.cc b/content/browser/service_worker/service_worker_job_unittest.cc
index d8cd7ef3a18f79d0c78957e3b82ce49843c761b9..9138398aa68f4b8f09e1de5b40ed3d5e321feccc 100644
--- a/content/browser/service_worker/service_worker_job_unittest.cc
+++ b/content/browser/service_worker/service_worker_job_unittest.cc
@@ -773,7 +773,9 @@ class UpdateJobTestHelper
     ServiceWorkerVersion::Status status;
   };
 
-  UpdateJobTestHelper() : EmbeddedWorkerTestHelper(base::FilePath()) {}
+  UpdateJobTestHelper()
+      : EmbeddedWorkerTestHelper(base::FilePath()),
+        force_bypass_cache_for_scripts_(false) {}
   ~UpdateJobTestHelper() override {
     if (registration_.get())
       registration_->RemoveListener(this);
@@ -784,6 +786,13 @@ 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;
+  }
+
   scoped_refptr<ServiceWorkerRegistration> SetupInitialRegistration(
       const GURL& test_origin) {
     scoped_refptr<ServiceWorkerRegistration> registration;
@@ -819,6 +828,10 @@ class UpdateJobTestHelper
     ASSERT_TRUE(version);
     version->AddListener(this);
 
+    // If |force_bypass_cache_for_scripts_| is set, simulate bypassing the
+    // network cache.

[falken, 2016/01/05 04:21:16]

rm comment that repeats the code

[jungkees, 2016/01/06 02:57:54]

Done.

+    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 resource_id = storage()->NewResourceId();
@@ -878,6 +891,78 @@ class UpdateJobTestHelper
   std::vector<AttributeChangeLogEntry> attribute_change_log_;
   std::vector<StateChangeLogEntry> state_change_log_;
   bool update_found_ = false;
+  bool force_bypass_cache_for_scripts_;

[falken, 2016/01/05 04:21:16]

init to false here instead of in the ctor for consistency with update_found_

[jungkees, 2016/01/06 02:57:54]

Done.

+};
+
+class UpdateJobFailToStartWorkerTestHelper : public UpdateJobTestHelper {
+ public:
+  UpdateJobFailToStartWorkerTestHelper()
+      : UpdateJobTestHelper(), force_start_worker_failure_(false) {}
+  ~UpdateJobFailToStartWorkerTestHelper() override {}
+
+  void set_force_start_worker_failure(bool force_start_worker_failure) {
+    force_start_worker_failure_ = force_start_worker_failure;
+  }
+
+  // UpdateJobTestHelper overrides
+  void OnStartWorker(int embedded_worker_id,
+                     int64 version_id,
+                     const GURL& scope,
+                     const GURL& script) override {
+    const std::string kMockScriptBody = "mock_script";
+    const uint64 kMockScriptSize = 19284;

[falken, 2016/01/05 04:21:16]

"int64" -> "int64_t" and "uint64" -> "uint64_t" throughout this file.

[jungkees, 2016/01/06 02:57:55]

Done.

+    ServiceWorkerVersion* version = context()->GetLiveVersion(version_id);
+    ServiceWorkerRegistration* registration =
+        context()->GetLiveRegistration(version->registration_id());
+    bool is_update = registration->active_version() &&
+                     version != registration->active_version();
+
+    ASSERT_TRUE(version);
+    version->AddListener(this);
+
+    // If |force_bypass_cache_for_scripts_| is set, simulate bypassing the
+    // network cache.
+    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 resource_id = storage()->NewResourceId();
+      version->script_cache_map()->NotifyStartedCaching(script, resource_id);
+      WriteStringResponse(storage(), resource_id, kMockScriptBody);
+      version->script_cache_map()->NotifyFinishedCaching(
+          script, kMockScriptSize, net::URLRequestStatus(), std::string());
+    } else {
+      if (script.GetOrigin() == kNoChangeOrigin) {
+        version->SetStartWorkerStatusCode(SERVICE_WORKER_ERROR_EXISTS);
+        EmbeddedWorkerTestHelper::OnStopWorker(embedded_worker_id);
+        return;
+      }
+
+      // Spoof caching the script for the new version.
+      int64 resource_id = storage()->NewResourceId();
+      version->script_cache_map()->NotifyStartedCaching(script, resource_id);
+      WriteStringResponse(storage(), resource_id, "mock_different_script");
+      version->script_cache_map()->NotifyFinishedCaching(
+          script, kMockScriptSize, net::URLRequestStatus(), std::string());
+    }

[falken, 2016/01/05 04:21:16]

I'd rather not repeat all the code in OnStartWorker up to here. How about
UpdateJobTestHelper just has a force_start_worker_failure_ member instead making
a subclass for it?

[jungkees, 2016/01/06 02:57:55]

Agreed. Done.

+    // If |force_start_worker_failure_| is set, simulate worker script
+    // evaluation failure.
+    (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);
+    int thread_id = next_thread_id();
+    SimulateWorkerThreadStarted(thread_id, embedded_worker_id);
+    set_next_thread_id(++thread_id);
+    SimulateWorkerScriptEvaluated(embedded_worker_id,
+                                  !force_start_worker_failure_);
+    if (!force_start_worker_failure_)
+      SimulateWorkerStarted(embedded_worker_id);
+  }
+
+ private:
+  bool force_start_worker_failure_;
 };
 
 // Helper class for update tests that evicts the active version when the update
@@ -962,18 +1047,42 @@ TEST_F(ServiceWorkerJobTest, Update_BumpLastUpdateCheckTime) {
   scoped_refptr<ServiceWorkerRegistration> registration =
       update_helper->SetupInitialRegistration(kNoChangeOrigin);
 
-  // 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.
+  registration->AddListener(update_helper);
+
+  // 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());
 
-  // 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 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 to a worker that loads successfully but fails to start up
+  // (script evaluation failure). The check time should be updated.
+  UpdateJobFailToStartWorkerTestHelper* update_fail_to_start_worker_helper =
+      new UpdateJobFailToStartWorkerTestHelper;
+  helper_.reset(update_fail_to_start_worker_helper);
+
+  registration = update_fail_to_start_worker_helper->SetupInitialRegistration(
+      kNoChangeOrigin);
+
+  registration->AddListener(update_fail_to_start_worker_helper);
+
+  update_fail_to_start_worker_helper->state_change_log_.clear();
+  update_fail_to_start_worker_helper->set_force_bypass_cache_for_scripts(true);
+  update_fail_to_start_worker_helper->set_force_start_worker_failure(true);
   registration->set_last_update_check(kYesterday);
   registration->active_version()->StartUpdate();
   base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(ServiceWorkerVersion::REDUNDANT,
+            update_fail_to_start_worker_helper->state_change_log_[0].status);

[falken, 2016/01/05 04:21:16]

This doesn't actually test the new behavior. The helper is using kNoChangeOrigin
which means it bails out with ERROR_EXISTS in OnStartWorker.
set_force_start_worker_failure makes no difference.

[jungkees, 2016/01/06 02:57:55]

Sorry for having missed that. I updated the second and the third cases using
kNewVersionOrigin. Note that I removed ASSERT_FALSE(update_found_); in
OnUpdateFound to run StartUpdate() multiple times. This change ensures other
Update_* tests do not break.

   EXPECT_LT(kYesterday, registration->last_update_check());
 }