service worker: Refactoring in controllee request handler for main resource.

There was very similar code, make it the same function.

This patch tries to maintain the same behavior as previous to minimize
changes.

Detailed changelog:
  - Move |need_to_update|. The point of this upfront was to choose whether
    to "unblock" association on early return. In the need_to_update case,
    we know we call an async function, so didn't want to unblock.
    It's easier to always to unblock upfront, and block before the async
    functions. RAII would improve this, but avoided that to minimize the
    diff.
  - Remove |status| from TRACE when it's already guaranteed to be kOk.
  - Previously we avoided calling AssociateRegistration until after
    trying to make the active worker transition ACTIVATING -> ACTIVATED.
    There shouldn't be a need for that: once there's an active worker,
    the provider host should "use" the registration. But to minimize the
    diff, I've maintained this old behavior. When the callback fires,
    we unassociate there if something failed.

Change-Id: I0d8a9bea5f516a42002ecb7e236a8f1e9eced678
Reviewed-on: https://chromium-review.googlesource.com/1143092
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Makoto Shimazu <shimazu@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577464}

content/browser/service_worker/service_worker_controllee_request_handler.cc

@@ -321,6 +321,8 @@ void ServiceWorkerControlleeRequestHandler::PrepareForMainResource(
 
   // Also prevent a registration from claiming this host while it's not
   // yet execution ready.
+  // TODO(falken): Make an RAII helper instead of all these explcit allow and
+  // disallow calls.
   provider_host_->SetAllowAssociation(false);
 
   stripped_url_ = net::SimplifyUrlForRequest(url);
@@ -339,11 +341,9 @@ void ServiceWorkerControlleeRequestHandler::
   if (JobWasCanceled())
     return;
 
-  const bool need_to_update = !force_update_started_ && registration &&
-                              context_->force_update_on_page_load();
-
-  if (provider_host_ && !need_to_update)
+  if (provider_host_)
     provider_host_->SetAllowAssociation(true);
+
   if (status != blink::ServiceWorkerStatusCode::kOk) {
     url_job_->FallbackToNetwork();
     TRACE_EVENT_ASYNC_END1(
@@ -376,11 +376,10 @@ void ServiceWorkerControlleeRequestHandler::
           registration->pattern(), provider_host_->topmost_frame_url(),
           resource_context_, provider_host_->web_contents_getter())) {
     url_job_->FallbackToNetwork();
-    TRACE_EVENT_ASYNC_END2(
+    TRACE_EVENT_ASYNC_END1(
         "ServiceWorker",
         "ServiceWorkerControlleeRequestHandler::PrepareForMainResource",
-        url_job_.get(), "Status", blink::ServiceWorkerStatusToString(status),
-        "Info", "ServiceWorker is blocked");
+        url_job_.get(), "Info", "ServiceWorker is blocked");
     return;
   }
 
@@ -395,8 +394,11 @@ void ServiceWorkerControlleeRequestHandler::
     return;
   }
 
+  const bool need_to_update = !force_update_started_ && registration &&
+                              context_->force_update_on_page_load();
   if (need_to_update) {
     force_update_started_ = true;
+    provider_host_->SetAllowAssociation(false);
     context_->UpdateServiceWorker(
         registration.get(), true /* force_bypass_cache */,
         true /* skip_script_comparison */,
@@ -413,43 +415,112 @@ void ServiceWorkerControlleeRequestHandler::
 
   scoped_refptr<ServiceWorkerVersion> active_version =
       registration->active_version();
+  if (!active_version) {
+    // Although there is no active version, a registration exists, so associate
+    // it, so it can be used for .ready.
+    // TODO(falken): There's no need to associate the registration just for
+    // .ready. Change this to AddMatchingRegistration instead, and call it
+    // unconditionally if there is a |provider_host_|.
+    provider_host_->AssociateRegistration(registration.get(),
+                                          false /* notify_controllerchange */);
+
+    url_job_->FallbackToNetwork();
+    TRACE_EVENT_ASYNC_END1(
+        "ServiceWorker",
+        "ServiceWorkerControlleeRequestHandler::PrepareForMainResource",
+        url_job_.get(), "Info",
+        "No active version, so falling back to network");
+    return;
+  }
+
+  // TODO(falken): Change these to DCHECK if it holds.
+  CHECK(active_version->status() == ServiceWorkerVersion::ACTIVATING ||
+        active_version->status() == ServiceWorkerVersion::ACTIVATED);
 
   // Wait until it's activated before firing fetch events.
-  if (active_version.get() &&
-      active_version->status() == ServiceWorkerVersion::ACTIVATING) {
+  if (active_version->status() == ServiceWorkerVersion::ACTIVATING) {
     provider_host_->SetAllowAssociation(false);
-    active_version->RegisterStatusChangeCallback(base::BindOnce(
-        &self::OnVersionStatusChanged, weak_factory_.GetWeakPtr(), registration,
-        active_version));
-    TRACE_EVENT_ASYNC_END2(
+    registration->active_version()->RegisterStatusChangeCallback(base::BindOnce(
+        &ServiceWorkerControlleeRequestHandler::
+            ContinueWithInScopeMainResourceRequest,
+        weak_factory_.GetWeakPtr(), registration, active_version));
+    TRACE_EVENT_ASYNC_END1(
         "ServiceWorker",
         "ServiceWorkerControlleeRequestHandler::PrepareForMainResource",
-        url_job_.get(), "Status", blink::ServiceWorkerStatusToString(status),
-        "Info", "Wait until finished SW activation");
+        url_job_.get(), "Info", "Wait until finished SW activation");
     return;
   }
 
-  // TODO(falken): Factor out the rest of this function and
-  // OnVersionStatusChanged into the same function.
+  ContinueWithInScopeMainResourceRequest(std::move(registration),
+                                         std::move(active_version));
+}
 
-  // A registration exists, so associate it. Note that the controller is only
-  // set if there's an active version. If there's no active version, we should
-  // still associate so the provider host can use .ready.
-  provider_host_->AssociateRegistration(registration.get(),
-                                        false /* notify_controllerchange */);
+void ServiceWorkerControlleeRequestHandler::
+    ContinueWithInScopeMainResourceRequest(
+        scoped_refptr<ServiceWorkerRegistration> registration,
+        scoped_refptr<ServiceWorkerVersion> active_version) {
+  if (provider_host_)
+    provider_host_->SetAllowAssociation(true);
 
-  if (!active_version.get() ||
-      active_version->status() != ServiceWorkerVersion::ACTIVATED) {
+  // The job may have been canceled before this was invoked. In that
+  // case, |url_job_| can't be used, so return.
+  if (JobWasCanceled()) {
+    TRACE_EVENT_ASYNC_END1(
+        "ServiceWorker",
+        "ServiceWorkerControlleeRequestHandler::PrepareForMainResource",
+        url_job_.get(), "Info", "The job was canceled");
+    return;
+  }
+
+  if (!provider_host_) {
+    url_job_->FallbackToNetwork();
+    TRACE_EVENT_ASYNC_END1(
+        "ServiceWorker",
+        "ServiceWorkerControlleeRequestHandler::PrepareForMainResource",
+        url_job_.get(), "Info",
+        "The provider host is gone, so falling back to network");
+    return;
+  }
+
+  if (active_version->status() != ServiceWorkerVersion::ACTIVATED) {
+    // TODO(falken): Clean this up and clarify in what cases we come here. I
+    // guess it's:
+    // - strange system error cases where promoting from ACTIVATING to ACTIVATED
+    //   failed (shouldn't happen)
+    // - something calling Doom(), etc, making the active_version REDUNDANT
+    // - a version called skipWaiting() during activation so the expected
+    //   version is no longer the active one (shouldn't happen: skipWaiting()
+    //   waits for the active version to finish activating).
+    // In most cases, it sounds like falling back to network would not be right,
+    // since it's still in-scope. We probably should do:
+    //   1) If the provider host has an active version that is ACTIVATED, just
+    //      use that, even if it wasn't the expected one.
+    //   2) If the provider host has an active version that is not ACTIVATED,
+    //      just fail the load. The correct thing is probably to re-try
+    //      activating that version, but there's a risk of an infinite loop of
+    //      retries.
+    //   3) If the provider host does not have an active version, just fail the
+    //      load.
     url_job_->FallbackToNetwork();
     TRACE_EVENT_ASYNC_END2(
         "ServiceWorker",
         "ServiceWorkerControlleeRequestHandler::PrepareForMainResource",
-        url_job_.get(), "Status", blink::ServiceWorkerStatusToString(status),
-        "Info",
-        "ServiceWorkerVersion is not available, so falling back to network");
+        url_job_.get(), "Info",
+        "The expected active version is not ACTIVATED, so falling back to "
+        "network",
+        "Status",
+        ServiceWorkerVersion::VersionStatusToString(active_version->status()));
     return;
   }
 
+  provider_host_->AssociateRegistration(registration.get(),
+                                        false /* notify_controllerchange */);
+
+  // TODO(falken): Change these to DCHECK if it holds, or else figure out
+  // how this happens.
+  CHECK_EQ(active_version, registration->active_version());
+  CHECK_EQ(active_version, provider_host_->controller());
+
   DCHECK_NE(active_version->fetch_handler_existence(),
             ServiceWorkerVersion::FetchHandlerExistence::UNKNOWN);
   ServiceWorkerMetrics::CountControlledPageLoad(
@@ -461,47 +532,14 @@ void ServiceWorkerControlleeRequestHandler::
   }
   bool is_forwarded =
       MaybeForwardToServiceWorker(url_job_.get(), active_version.get());
-
-  TRACE_EVENT_ASYNC_END2(
+  TRACE_EVENT_ASYNC_END1(
       "ServiceWorker",
       "ServiceWorkerControlleeRequestHandler::PrepareForMainResource",
-      url_job_.get(), "Status", blink::ServiceWorkerStatusToString(status),
-      "Info",
+      url_job_.get(), "Info",
       (is_forwarded) ? "Forwarded to the ServiceWorker"
                      : "Skipped the ServiceWorker which has no fetch handler");
 }
 
-void ServiceWorkerControlleeRequestHandler::OnVersionStatusChanged(
-    scoped_refptr<ServiceWorkerRegistration> registration,
-    scoped_refptr<ServiceWorkerVersion> version) {
-  // The job may have been canceled before this was invoked.
-  if (JobWasCanceled())
-    return;
-
-  if (provider_host_)
-    provider_host_->SetAllowAssociation(true);
-  if (version != registration->active_version() ||
-      version->status() != ServiceWorkerVersion::ACTIVATED ||
-      !provider_host_) {
-    url_job_->FallbackToNetwork();
-    return;
-  }
-
-  DCHECK_NE(version->fetch_handler_existence(),
-            ServiceWorkerVersion::FetchHandlerExistence::UNKNOWN);
-  ServiceWorkerMetrics::CountControlledPageLoad(
-      version->site_for_uma(), stripped_url_, is_main_frame_load_);
-
-  provider_host_->AssociateRegistration(registration.get(),
-                                        false /* notify_controllerchange */);
-
-  if (blink::ServiceWorkerUtils::IsServicificationEnabled() &&
-      IsResourceTypeFrame(resource_type_)) {
-    provider_host_->AddServiceWorkerToUpdate(version);
-  }
-  MaybeForwardToServiceWorker(url_job_.get(), version.get());
-}
-
 void ServiceWorkerControlleeRequestHandler::DidUpdateRegistration(
     const scoped_refptr<ServiceWorkerRegistration>& original_registration,
     blink::ServiceWorkerStatusCode status,

content/browser/service_worker/service_worker_controllee_request_handler.h

@@ -96,7 +96,7 @@ class CONTENT_EXPORT ServiceWorkerControlleeRequestHandler
   void DidLookupRegistrationForMainResource(
       blink::ServiceWorkerStatusCode status,
       scoped_refptr<ServiceWorkerRegistration> registration);
-  void OnVersionStatusChanged(
+  void ContinueWithInScopeMainResourceRequest(
       scoped_refptr<ServiceWorkerRegistration> registration,
       scoped_refptr<ServiceWorkerVersion> version);
 

content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc

@@ -139,6 +139,11 @@ class ServiceWorkerControlleeRequestHandlerTest : public testing::Test {
 
   ServiceWorkerContextCore* context() const { return helper_->context(); }
 
+  void SetProviderHostIsSecure(ServiceWorkerProviderHost* host,
+                               bool is_secure) {
+    host->info_->is_parent_frame_secure = is_secure;
+  }
+
  protected:
   TestBrowserThreadBundle browser_thread_bundle_;
   std::unique_ptr<EmbeddedWorkerTestHelper> helper_;
@@ -197,6 +202,34 @@ TEST_F(ServiceWorkerControlleeRequestHandlerTest, DisallowServiceWorker) {
   SetBrowserClientForTesting(old_browser_client);
 }
 
+TEST_F(ServiceWorkerControlleeRequestHandlerTest, InsecureContext) {
+  // Store an activated worker.
+  version_->set_fetch_handler_existence(
+      ServiceWorkerVersion::FetchHandlerExistence::EXISTS);
+  version_->SetStatus(ServiceWorkerVersion::ACTIVATED);
+  registration_->SetActiveVersion(version_);
+  context()->storage()->StoreRegistration(registration_.get(), version_.get(),
+                                          base::DoNothing());
+  base::RunLoop().RunUntilIdle();
+
+  SetProviderHostIsSecure(provider_host_.get(), false);
+
+  // Conduct a main resource load.
+  ServiceWorkerRequestTestResources test_resources(
+      this, GURL("https://host/scope/doc"), RESOURCE_TYPE_MAIN_FRAME);
+  ServiceWorkerURLRequestJob* sw_job = test_resources.MaybeCreateJob();
+
+  EXPECT_FALSE(sw_job->ShouldFallbackToNetwork());
+  EXPECT_FALSE(sw_job->ShouldForwardToServiceWorker());
+  EXPECT_FALSE(version_->HasControllee());
+  base::RunLoop().RunUntilIdle();
+
+  // Verify we did not use the worker.
+  EXPECT_TRUE(sw_job->ShouldFallbackToNetwork());
+  EXPECT_FALSE(sw_job->ShouldForwardToServiceWorker());
+  EXPECT_FALSE(version_->HasControllee());
+}
+
 TEST_F(ServiceWorkerControlleeRequestHandlerTest, ActivateWaitingVersion) {
   // Store a registration that is installed but not activated yet.
   version_->set_fetch_handler_existence(

content/browser/service_worker/service_worker_provider_host.h

@@ -467,6 +467,8 @@ class CONTENT_EXPORT ServiceWorkerProviderHost
   friend class ServiceWorkerProviderHostTest;
   friend class ServiceWorkerWriteToCacheJobTest;
   friend class ServiceWorkerContextRequestHandlerTest;
+  friend class service_worker_controllee_request_handler_unittest::
+      ServiceWorkerControlleeRequestHandlerTest;
   friend class service_worker_object_host_unittest::ServiceWorkerObjectHostTest;
   FRIEND_TEST_ALL_PREFIXES(ServiceWorkerWriteToCacheJobTest, Update_SameScript);
   FRIEND_TEST_ALL_PREFIXES(ServiceWorkerWriteToCacheJobTest,

content/browser/service_worker/service_worker_version.cc

@@ -113,25 +113,6 @@ void ClearTick(base::TimeTicks* time) {
   *time = base::TimeTicks();
 }
 
-std::string VersionStatusToString(ServiceWorkerVersion::Status status) {
-  switch (status) {
-    case ServiceWorkerVersion::NEW:
-      return "new";
-    case ServiceWorkerVersion::INSTALLING:
-      return "installing";
-    case ServiceWorkerVersion::INSTALLED:
-      return "installed";
-    case ServiceWorkerVersion::ACTIVATING:
-      return "activating";
-    case ServiceWorkerVersion::ACTIVATED:
-      return "activated";
-    case ServiceWorkerVersion::REDUNDANT:
-      return "redundant";
-  }
-  NOTREACHED() << status;
-  return std::string();
-}
-
 const int kInvalidTraceId = -1;
 
 int NextTraceId() {
@@ -1347,6 +1328,7 @@ void ServiceWorkerVersion::CountFeature(blink::mojom::WebFeature feature) {
     provider_host_by_uuid.second->CountFeature(feature);
 }
 
+// static
 bool ServiceWorkerVersion::IsInstalled(ServiceWorkerVersion::Status status) {
   switch (status) {
     case ServiceWorkerVersion::NEW:
@@ -1362,6 +1344,27 @@ bool ServiceWorkerVersion::IsInstalled(ServiceWorkerVersion::Status status) {
   return false;
 }
 
+// static
+std::string ServiceWorkerVersion::VersionStatusToString(
+    ServiceWorkerVersion::Status status) {
+  switch (status) {
+    case ServiceWorkerVersion::NEW:
+      return "new";
+    case ServiceWorkerVersion::INSTALLING:
+      return "installing";
+    case ServiceWorkerVersion::INSTALLED:
+      return "installed";
+    case ServiceWorkerVersion::ACTIVATING:
+      return "activating";
+    case ServiceWorkerVersion::ACTIVATED:
+      return "activated";
+    case ServiceWorkerVersion::REDUNDANT:
+      return "redundant";
+  }
+  NOTREACHED() << status;
+  return std::string();
+}
+
 void ServiceWorkerVersion::IncrementPendingUpdateHintCount() {
   DCHECK(blink::ServiceWorkerUtils::IsServicificationEnabled());
   pending_update_hint_count_++;

content/browser/service_worker/service_worker_version.h

@@ -484,6 +484,7 @@ class CONTENT_EXPORT ServiceWorkerVersion
   }
 
   static bool IsInstalled(ServiceWorkerVersion::Status status);
+  static std::string VersionStatusToString(ServiceWorkerVersion::Status status);
 
   // For scheduling Soft Update after main resource requests. We schedule
   // a Soft Update to happen "soon" after each main resource request, attempting