service worker: Update ServiceWorkerControlleeRequestHandlerTest

Update tests for ServiceWorkerControlleeRequestHandler to have
better coverage with S13nServiceWorker enabled/disabled.

Bug: 860361
Change-Id: Ie4b915fd2aea96afc9880c759d4bcdb82d320892
Reviewed-on: https://chromium-review.googlesource.com/1151070
Commit-Queue: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578977}

content/browser/service_worker/service_worker_controllee_request_handler.h

@@ -86,6 +86,9 @@ class CONTENT_EXPORT ServiceWorkerControlleeRequestHandler
   base::Optional<SubresourceLoaderParams> MaybeCreateSubresourceLoaderParams()
       override;
 
+  // Exposed for testing.
+  ServiceWorkerURLJobWrapper* url_job() const { return url_job_.get(); }
+
  private:
   FRIEND_TEST_ALL_PREFIXES(ServiceWorkerControlleeRequestHandlerTest,
                            ActivateWaitingVersion);

content/browser/service_worker/service_worker_controllee_request_handler_unittest.cc

@@ -11,6 +11,7 @@
 #include "base/files/scoped_temp_dir.h"
 #include "base/logging.h"
 #include "base/run_loop.h"
+#include "base/test/scoped_feature_list.h"
 #include "components/offline_pages/buildflags/buildflags.h"
 #include "content/browser/service_worker/embedded_worker_test_helper.h"
 #include "content/browser/service_worker/service_worker_context_core.h"
@@ -33,6 +34,7 @@
 #include "services/network/public/cpp/resource_request_body.h"
 #include "services/network/public/mojom/request_context_frame_type.mojom.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/blink/public/common/features.h"
 #include "third_party/blink/public/mojom/service_worker/service_worker_registration.mojom.h"
 
 namespace content {
@@ -40,7 +42,8 @@ namespace service_worker_controllee_request_handler_unittest {
 
 int kMockProviderId = 1;
 
-class ServiceWorkerControlleeRequestHandlerTest : public testing::Test {
+class ServiceWorkerControlleeRequestHandlerTest
+    : public testing::TestWithParam<bool> {
  public:
   class ServiceWorkerRequestTestResources {
    public:
@@ -51,6 +54,7 @@ class ServiceWorkerControlleeRequestHandlerTest : public testing::Test {
         network::mojom::FetchRequestMode fetch_type =
             network::mojom::FetchRequestMode::kNoCORS)
         : test_(test),
+          resource_type_(type),
           request_(test->url_request_context_.CreateRequest(
               url,
               net::DEFAULT_PRIORITY,
@@ -77,21 +81,57 @@ class ServiceWorkerControlleeRequestHandlerTest : public testing::Test {
       return static_cast<ServiceWorkerURLRequestJob*>(job_.get());
     }
 
+    void MaybeCreateLoader() {
+      network::ResourceRequest resource_request;
+      resource_request.url = request_->url();
+      resource_request.resource_type = resource_type_;
+      resource_request.headers = request()->extra_request_headers();
+      handler_->MaybeCreateLoader(
+          resource_request, &test_->mock_resource_context_,
+          base::DoNothing());
+    }
+
+    ServiceWorkerURLJobWrapper* MaybeCreateJobWrapper() {
+      if (test_->IsServiceWorkerServicificationEnabled()) {
+        MaybeCreateLoader();
+        return handler_->url_job();
+      } else {
+        ServiceWorkerURLRequestJob* job = MaybeCreateJob();
+        if (job) {
+          job_wrapper_ =
+              std::make_unique<ServiceWorkerURLJobWrapper>(job->GetWeakPtr());
+        }
+        return job_wrapper_.get();
+      }
+    }
+
     void ResetHandler() { handler_.reset(nullptr); }
 
     net::URLRequest* request() const { return request_.get(); }
 
    private:
     ServiceWorkerControlleeRequestHandlerTest* test_;
+    const ResourceType resource_type_;
     std::unique_ptr<net::URLRequest> request_;
     std::unique_ptr<ServiceWorkerControlleeRequestHandler> handler_;
+    // |job_| and |job_wrapper_| are only for non-S13nServiceWorker cases.
+    // When S13nServiceWorker is enabled we use a job wrapper which is owned by
+    // |handler_|.
     std::unique_ptr<net::URLRequestJob> job_;
+    std::unique_ptr<ServiceWorkerURLJobWrapper> job_wrapper_;
   };
 
   ServiceWorkerControlleeRequestHandlerTest()
       : browser_thread_bundle_(TestBrowserThreadBundle::IO_MAINLOOP) {}
 
   void SetUp() override {
+    if (IsServiceWorkerServicificationEnabled()) {
+      scoped_feature_list_.InitAndEnableFeature(
+          blink::features::kServiceWorkerServicification);
+    } else {
+      scoped_feature_list_.InitAndDisableFeature(
+          blink::features::kServiceWorkerServicification);
+    }
     SetUpWithHelper(new EmbeddedWorkerTestHelper(base::FilePath()));
   }
 
@@ -144,7 +184,14 @@ class ServiceWorkerControlleeRequestHandlerTest : public testing::Test {
     host->info_->is_parent_frame_secure = is_secure;
   }
 
+  bool IsServiceWorkerServicificationEnabled() { return GetParam(); }
+
  protected:
+  // |scoped_feature_list_| must be before |thread_bundle_|, since
+  // the thread bundle's destruction causes service worker-related
+  // objects to destruct, whose destructors need to know whether servicification
+  // is enabled.
+  base::test::ScopedFeatureList scoped_feature_list_;
   TestBrowserThreadBundle browser_thread_bundle_;
   std::unique_ptr<EmbeddedWorkerTestHelper> helper_;
   scoped_refptr<ServiceWorkerRegistration> registration_;
@@ -170,7 +217,7 @@ class ServiceWorkerTestContentBrowserClient : public TestContentBrowserClient {
   }
 };
 
-TEST_F(ServiceWorkerControlleeRequestHandlerTest, DisallowServiceWorker) {
+TEST_P(ServiceWorkerControlleeRequestHandlerTest, DisallowServiceWorker) {
   ServiceWorkerTestContentBrowserClient test_browser_client;
   ContentBrowserClient* old_browser_client =
       SetBrowserClientForTesting(&test_browser_client);
@@ -187,7 +234,8 @@ TEST_F(ServiceWorkerControlleeRequestHandlerTest, DisallowServiceWorker) {
   // Conduct a main resource load.
   ServiceWorkerRequestTestResources test_resources(
       this, GURL("https://host/scope/doc"), RESOURCE_TYPE_MAIN_FRAME);
-  ServiceWorkerURLRequestJob* sw_job = test_resources.MaybeCreateJob();
+  ServiceWorkerURLJobWrapper* sw_job = test_resources.MaybeCreateJobWrapper();
+  ASSERT_TRUE(sw_job);
 
   EXPECT_FALSE(sw_job->ShouldFallbackToNetwork());
   EXPECT_FALSE(sw_job->ShouldForwardToServiceWorker());
@@ -202,7 +250,7 @@ TEST_F(ServiceWorkerControlleeRequestHandlerTest, DisallowServiceWorker) {
   SetBrowserClientForTesting(old_browser_client);
 }
 
-TEST_F(ServiceWorkerControlleeRequestHandlerTest, InsecureContext) {
+TEST_P(ServiceWorkerControlleeRequestHandlerTest, InsecureContext) {
   // Store an activated worker.
   version_->set_fetch_handler_existence(
       ServiceWorkerVersion::FetchHandlerExistence::EXISTS);
@@ -217,7 +265,8 @@ TEST_F(ServiceWorkerControlleeRequestHandlerTest, InsecureContext) {
   // Conduct a main resource load.
   ServiceWorkerRequestTestResources test_resources(
       this, GURL("https://host/scope/doc"), RESOURCE_TYPE_MAIN_FRAME);
-  ServiceWorkerURLRequestJob* sw_job = test_resources.MaybeCreateJob();
+  ServiceWorkerURLJobWrapper* sw_job = test_resources.MaybeCreateJobWrapper();
+  ASSERT_TRUE(sw_job);
 
   EXPECT_FALSE(sw_job->ShouldFallbackToNetwork());
   EXPECT_FALSE(sw_job->ShouldForwardToServiceWorker());
@@ -230,7 +279,7 @@ TEST_F(ServiceWorkerControlleeRequestHandlerTest, InsecureContext) {
   EXPECT_FALSE(version_->HasControllee());
 }
 
-TEST_F(ServiceWorkerControlleeRequestHandlerTest, ActivateWaitingVersion) {
+TEST_P(ServiceWorkerControlleeRequestHandlerTest, ActivateWaitingVersion) {
   // Store a registration that is installed but not activated yet.
   version_->set_fetch_handler_existence(
       ServiceWorkerVersion::FetchHandlerExistence::EXISTS);
@@ -243,7 +292,8 @@ TEST_F(ServiceWorkerControlleeRequestHandlerTest, ActivateWaitingVersion) {
   // Conduct a main resource load.
   ServiceWorkerRequestTestResources test_resources(
       this, GURL("https://host/scope/doc"), RESOURCE_TYPE_MAIN_FRAME);
-  ServiceWorkerURLRequestJob* sw_job = test_resources.MaybeCreateJob();
+  ServiceWorkerURLJobWrapper* sw_job = test_resources.MaybeCreateJobWrapper();
+  ASSERT_TRUE(sw_job);
 
   EXPECT_FALSE(sw_job->ShouldFallbackToNetwork());
   EXPECT_FALSE(sw_job->ShouldForwardToServiceWorker());
@@ -251,19 +301,22 @@ TEST_F(ServiceWorkerControlleeRequestHandlerTest, ActivateWaitingVersion) {
 
   base::RunLoop().RunUntilIdle();
 
-  EXPECT_EQ(ServiceWorkerVersion::ACTIVATED,
-            version_->status());
+  EXPECT_EQ(ServiceWorkerVersion::ACTIVATED, version_->status());
   EXPECT_FALSE(sw_job->ShouldFallbackToNetwork());
   EXPECT_TRUE(sw_job->ShouldForwardToServiceWorker());
   EXPECT_TRUE(version_->HasControllee());
 
-  // Navigations should trigger an update too.
   test_resources.ResetHandler();
-  EXPECT_TRUE(version_->update_timer_.IsRunning());
+  // Navigations should trigger an update too when S13nServiceWorker is
+  // disabled. Note that when S13nServiceWorker is enabled we defer scheduling
+  // update. See the comment on
+  // ServiceWorkerProviderHost::AddServiceWorkerToUpdate() for details.
+  if (!IsServiceWorkerServicificationEnabled())
+    EXPECT_TRUE(version_->update_timer_.IsRunning());
 }
 
 // Test that an installing registration is associated with a provider host.
-TEST_F(ServiceWorkerControlleeRequestHandlerTest, InstallingRegistration) {
+TEST_P(ServiceWorkerControlleeRequestHandlerTest, InstallingRegistration) {
   // Create an installing registration.
   version_->SetStatus(ServiceWorkerVersion::INSTALLING);
   version_->set_fetch_handler_existence(
@@ -274,7 +327,7 @@ TEST_F(ServiceWorkerControlleeRequestHandlerTest, InstallingRegistration) {
   // Conduct a main resource load.
   ServiceWorkerRequestTestResources test_resources(
       this, GURL("https://host/scope/doc"), RESOURCE_TYPE_MAIN_FRAME);
-  ServiceWorkerURLRequestJob* job = test_resources.MaybeCreateJob();
+  ServiceWorkerURLJobWrapper* job = test_resources.MaybeCreateJobWrapper();
 
   base::RunLoop().RunUntilIdle();
 
@@ -289,7 +342,7 @@ TEST_F(ServiceWorkerControlleeRequestHandlerTest, InstallingRegistration) {
 }
 
 // Test to not regress crbug/414118.
-TEST_F(ServiceWorkerControlleeRequestHandlerTest, DeletedProviderHost) {
+TEST_P(ServiceWorkerControlleeRequestHandlerTest, DeletedProviderHost) {
   // Store a registration so the call to FindRegistrationForDocument will read
   // from the database.
   version_->set_fetch_handler_existence(
@@ -305,7 +358,8 @@ TEST_F(ServiceWorkerControlleeRequestHandlerTest, DeletedProviderHost) {
   // Conduct a main resource load.
   ServiceWorkerRequestTestResources test_resources(
       this, GURL("https://host/scope/doc"), RESOURCE_TYPE_MAIN_FRAME);
-  ServiceWorkerURLRequestJob* sw_job = test_resources.MaybeCreateJob();
+  ServiceWorkerURLJobWrapper* sw_job = test_resources.MaybeCreateJobWrapper();
+  ASSERT_TRUE(sw_job);
 
   EXPECT_FALSE(sw_job->ShouldFallbackToNetwork());
   EXPECT_FALSE(sw_job->ShouldForwardToServiceWorker());
@@ -323,7 +377,14 @@ TEST_F(ServiceWorkerControlleeRequestHandlerTest, DeletedProviderHost) {
 // Tests the scenario where a controllee request handler was created
 // for a subresource request, but before MaybeCreateJob() is run, the
 // controller/active version becomes null.
-TEST_F(ServiceWorkerControlleeRequestHandlerTest, LostActiveVersion) {
+TEST_P(ServiceWorkerControlleeRequestHandlerTest, LostActiveVersion) {
+  // Skip this test when S13nServiceWorker is enabled because we don't use
+  // ServiceWorkerControlleeRequestHandler for subresource loading.
+  // TODO(bashi): Add a test in ServiceWorkerProviderHost to cover this
+  // scenario when S13nServiceWorker is enabled.
+  if (IsServiceWorkerServicificationEnabled())
+    return;
+
   // Store an activated worker.
   version_->set_fetch_handler_existence(
       ServiceWorkerVersion::FetchHandlerExistence::EXISTS);
@@ -358,7 +419,14 @@ TEST_F(ServiceWorkerControlleeRequestHandlerTest, LostActiveVersion) {
             sub_job->response_type_);
 }
 
-TEST_F(ServiceWorkerControlleeRequestHandlerTest, FallbackWithNoFetchHandler) {
+TEST_P(ServiceWorkerControlleeRequestHandlerTest, FallbackWithNoFetchHandler) {
+  // Skip this test when S13nServiceWorker is enabled because we don't use
+  // ServiceWorkerControlleeRequestHandler for subresource loading.
+  // TODO(bashi): Add a test in ServiceWorkerProviderHost to cover this
+  // scenario when S13nServiceWorker is enabled.
+  if (IsServiceWorkerServicificationEnabled())
+    return;
+
   version_->set_fetch_handler_existence(
       ServiceWorkerVersion::FetchHandlerExistence::DOES_NOT_EXIST);
   version_->SetStatus(ServiceWorkerVersion::ACTIVATED);
@@ -408,7 +476,7 @@ TEST_F(ServiceWorkerControlleeRequestHandlerTest, FallbackWithNoFetchHandler) {
 }
 
 #if BUILDFLAG(ENABLE_OFFLINE_PAGES)
-TEST_F(ServiceWorkerControlleeRequestHandlerTest, FallbackWithOfflineHeader) {
+TEST_P(ServiceWorkerControlleeRequestHandlerTest, FallbackWithOfflineHeader) {
   version_->set_fetch_handler_existence(
       ServiceWorkerVersion::FetchHandlerExistence::EXISTS);
   version_->SetStatus(ServiceWorkerVersion::ACTIVATED);
@@ -424,12 +492,12 @@ TEST_F(ServiceWorkerControlleeRequestHandlerTest, FallbackWithOfflineHeader) {
   // Sets an offline header to indicate force loading offline page.
   test_resources.request()->SetExtraRequestHeaderByName(
       "X-Chrome-offline", "reason=download", true);
-  ServiceWorkerURLRequestJob* sw_job = test_resources.MaybeCreateJob();
+  ServiceWorkerURLJobWrapper* sw_job = test_resources.MaybeCreateJobWrapper();
 
   EXPECT_FALSE(sw_job);
 }
 
-TEST_F(ServiceWorkerControlleeRequestHandlerTest, FallbackWithNoOfflineHeader) {
+TEST_P(ServiceWorkerControlleeRequestHandlerTest, FallbackWithNoOfflineHeader) {
   version_->set_fetch_handler_existence(
       ServiceWorkerVersion::FetchHandlerExistence::EXISTS);
   version_->SetStatus(ServiceWorkerVersion::ACTIVATED);
@@ -445,11 +513,15 @@ TEST_F(ServiceWorkerControlleeRequestHandlerTest, FallbackWithNoOfflineHeader) {
   // Empty offline header value should not cause fallback.
   test_resources.request()->SetExtraRequestHeaderByName("X-Chrome-offline", "",
                                                         true);
-  ServiceWorkerURLRequestJob* sw_job = test_resources.MaybeCreateJob();
+  ServiceWorkerURLJobWrapper* sw_job = test_resources.MaybeCreateJobWrapper();
 
   EXPECT_TRUE(sw_job);
 }
-#endif  // BUILDFLAG(ENABLE_OFFLINE_PAGE
+#endif  // BUILDFLAG(ENABLE_OFFLINE_PAGE)
+
+INSTANTIATE_TEST_CASE_P(IsServiceWorkerServicificationEnabled,
+                        ServiceWorkerControlleeRequestHandlerTest,
+                        ::testing::Bool(););
 
 }  // namespace service_worker_controllee_request_handler_unittest
 }  // namespace content

content/browser/service_worker/service_worker_navigation_loader.cc

@@ -141,6 +141,10 @@ bool ServiceWorkerNavigationLoader::ShouldFallbackToNetwork() {
   return response_type_ == ResponseType::FALLBACK_TO_NETWORK;
 }
 
+bool ServiceWorkerNavigationLoader::ShouldForwardToServiceWorker() {
+  return response_type_ == ResponseType::FORWARD_TO_SERVICE_WORKER;
+}
+
 bool ServiceWorkerNavigationLoader::WasCanceled() const {
   return status_ == Status::kCancelled;
 }

content/browser/service_worker/service_worker_navigation_loader.h

@@ -87,6 +87,7 @@ class CONTENT_EXPORT ServiceWorkerNavigationLoader
   void FallbackToNetwork();
   void ForwardToServiceWorker();
   bool ShouldFallbackToNetwork();
+  bool ShouldForwardToServiceWorker();
   bool WasCanceled() const;
 
   // The navigation request that was holding this job is

content/browser/service_worker/service_worker_url_job_wrapper.cc

@@ -64,6 +64,14 @@ bool ServiceWorkerURLJobWrapper::ShouldFallbackToNetwork() {
   }
 }
 
+bool ServiceWorkerURLJobWrapper::ShouldForwardToServiceWorker() {
+  if (url_loader_job_) {
+    return url_loader_job_->ShouldForwardToServiceWorker();
+  } else {
+    return url_request_job_->ShouldForwardToServiceWorker();
+  }
+}
+
 void ServiceWorkerURLJobWrapper::FailDueToLostController() {
   // This function is only called for subresource requests, so it can't
   // be called for |url_loader_job_|, which is for navigations.

content/browser/service_worker/service_worker_url_job_wrapper.h

@@ -22,7 +22,7 @@ class ServiceWorkerVersion;
 // non-S13nServiceWorker). It wraps either a
 // ServiceWorkerURLRequestJob or a callback for URLLoader and forwards to the
 // underlying implementation.
-class ServiceWorkerURLJobWrapper {
+class CONTENT_EXPORT ServiceWorkerURLJobWrapper {
  public:
   // A helper used by the ServiceWorkerNavigationLoader or
   // ServiceWorkerURLRequestJob.
@@ -72,6 +72,9 @@ class ServiceWorkerURLJobWrapper {
   // instead should fallback to the network.
   bool ShouldFallbackToNetwork();
 
+  // Returns true if this job should be forwarded to a service worker.
+  bool ShouldForwardToServiceWorker();
+
   // Tells the job to abort with a start error. Currently this is only called
   // because the controller was lost. This function could be made more generic
   // if needed later.