service worker: use 200 OK for update requests even in the no update case

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 f2ecc1fd9c73d56cf0e135fa625767d129b408f9..e38eefae064ce66e67282029bb9ed94e1afb97e6 100644
--- a/content/browser/service_worker/service_worker_register_job.cc
+++ b/content/browser/service_worker/service_worker_register_job.cc
@@ -6,8 +6,6 @@
 
 #include <stdint.h>
 
-#include <vector>
-
 #include "base/location.h"
 #include "base/single_thread_task_runner.h"
 #include "base/thread_task_runner_handle.h"
@@ -345,7 +343,12 @@ void ServiceWorkerRegisterJob::UpdateAndContinue() {
   set_new_version(new ServiceWorkerVersion(registration(), script_url_,
                                            version_id, context_));
   new_version()->set_force_bypass_cache_for_scripts(force_bypass_cache_);
-  new_version()->set_skip_script_comparison(skip_script_comparison_);
+  if (registration()->has_installed_version() && !skip_script_comparison_) {
+    new_version()->set_pause_after_download(true);
+    new_version()->embedded_worker()->AddListener(this);
+  } else {
+    new_version()->set_pause_after_download(false);
+  }
   new_version()->StartWorker(
       base::Bind(&ServiceWorkerRegisterJob::OnStartWorkerFinished,
                  weak_factory_.GetWeakPtr()));
@@ -363,7 +366,7 @@ void ServiceWorkerRegisterJob::OnStartWorkerFinished(
       registration()->last_update_check().is_null()) {
     registration()->set_last_update_check(base::Time::Now());
 
-    if (registration()->waiting_version() || registration()->active_version())
+    if (registration()->has_installed_version())
       context_->storage()->UpdateLastUpdateCheckTime(registration());
   }
 
@@ -372,13 +375,6 @@ void ServiceWorkerRegisterJob::OnStartWorkerFinished(
     return;
   }
 
-  // The updated worker is identical to the incumbent.
-  if (status == SERVICE_WORKER_ERROR_EXISTS) {
-    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) {
@@ -535,9 +531,14 @@ void ServiceWorkerRegisterJob::CompleteInternal(
   if (registration()) {
     context_->storage()->NotifyDoneInstallingRegistration(
         registration(), new_version(), status);
-    if (registration()->waiting_version() || registration()->active_version())
+    if (registration()->has_installed_version())
       registration()->set_is_uninstalled(false);
   }
+
+  if (new_version()) {

[michaeln, 2016/02/06 01:13:58]

maybe put this block up top to defend against the addition of an early return at
some point in this method body

[falken, 2016/02/10 05:40:41]

Done.

+    new_version()->set_pause_after_download(false);
+    new_version()->embedded_worker()->RemoveListener(this);
+  }
 }
 
 void ServiceWorkerRegisterJob::ResolvePromise(
@@ -574,4 +575,22 @@ void ServiceWorkerRegisterJob::AddRegistrationToMatchingProviderHosts(
   }
 }
 
+void ServiceWorkerRegisterJob::OnScriptLoaded() {

[nhiroki, 2016/02/10 04:56:14]

Can you add "DCHECK(pause_after_download())" here?

[falken, 2016/02/10 05:40:40]

It's not true though because pause_after_download may have been false when
StartWorker is called, if there's no incumbent.

To really do this DCHECK I'd need to add a bool "did_pause_after_download" to
register job and I'm not sure it's worth doing that.

[nhiroki, 2016/02/10 06:38:29]

Hmmm... I'm still thinking this event handler can be called only when
|new_version()->pause_after_download()| is true. The handler is added to the
version only when the flag is set at line 347-348, and it's removed only when
the flag is unset at line 539-540.

If this assumption is not correct, an assertion in
WebEmbeddedWorkerImpl::resumeAfterDownload() seems to be failed.

 163 void WebEmbeddedWorkerImpl::resumeAfterDownload()
 164 {
 165     ASSERT(!m_askedToTerminate);
 166     ASSERT(m_pauseAfterDownloadState == IsPausedAfterDownload);
^^^ This assertion. In the case, m_pauseAfterDownloadState could be
DontPauseAfterDownload.

(sorry if I'm missing something...)

[falken, 2016/02/10 06:53:39]

Ah, of course. EmbeddedWorkerInstance::OnScriptLoaded is called but the register
job is not listening unless it was paused. Done, thanks!

+  new_version()->set_pause_after_download(false);
+  net::URLRequestStatus status =
+      new_version()->script_cache_map()->main_script_status();
+  if (!status.is_success()) {
+    // OnScriptLoaded signifies a successful network load, which translates into
+    // a script cache error only if the new script was not written because it
+    // was identical to the existing script.
+    DCHECK_EQ(status.error(), net::ERR_FILE_EXISTS);
+    ResolvePromise(SERVICE_WORKER_OK, std::string(), registration());
+    Complete(SERVICE_WORKER_ERROR_EXISTS,
+             "The updated worker is identical to the incumbent.");
+    return;
+  }
+
+  new_version()->embedded_worker()->ResumeAfterDownload();
+}
+
 }  // namespace content