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

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>
-
 #include "base/location.h"
 #include "base/single_thread_task_runner.h"
 #include "base/thread_task_runner_handle.h"
 #include "content/browser/service_worker/service_worker_context_core.h"
 #include "content/browser/service_worker/service_worker_job_coordinator.h"
 #include "content/browser/service_worker/service_worker_metrics.h"
 #include "content/browser/service_worker/service_worker_registration.h"
 #include "content/browser/service_worker/service_worker_storage.h"
 #include "content/common/service_worker/service_worker_messages.h"
 #include "content/common/service_worker/service_worker_types.h"
@@ skipping 317 matching lines @@
   if (version_id == kInvalidServiceWorkerVersionId) {
     Complete(SERVICE_WORKER_ERROR_ABORT);
     return;
   }
 
   // "Let worker be a new ServiceWorker object..." and start
   // the worker.
   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()));
 }
 
 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())
+    if (registration()->has_installed_version())
       context_->storage()->UpdateLastUpdateCheckTime(registration());
   }
 
   if (status == SERVICE_WORKER_OK) {
     InstallAndContinue();
     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) {
     Complete(status, "Timed out while trying to start the Service Worker.");
     return;
   }
 
   const net::URLRequestStatus& main_script_status =
       new_version()->script_cache_map()->main_script_status();
   std::string message;
@@ skipping 136 matching lines @@
             base::Bind(&ServiceWorkerUtils::NoOpStatusCallback));
       }
     }
     if (!is_promise_resolved_)
       ResolvePromise(status, status_message, NULL);
   }
   DCHECK(callbacks_.empty());
   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(
     ServiceWorkerStatusCode status,
     const std::string& status_message,
     ServiceWorkerRegistration* registration) {
   DCHECK(!is_promise_resolved_);
 
   is_promise_resolved_ = true;
   promise_resolved_status_ = status;
@@ skipping 16 matching lines @@
     ServiceWorkerProviderHost* host = it->GetProviderHost();
     if (host->IsHostToRunningServiceWorker())
       continue;
     if (!ServiceWorkerUtils::ScopeMatches(registration->pattern(),
                                           host->document_url()))
       continue;
     host->AddMatchingRegistration(registration);
   }
 }
 
+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