Make the sub resource load also skip the fetch handler if skippable.

In the previous implementation, we did not skip service worker for
fetching sub resource load.  Since we do not start the service worker
when the fetch handler is decided to be skipped, sub resource load may
get stuck.

This CL make the renderer behave as if the fetch handler does not
exist if the fetch handler is skipped in the main resource load.
To let the renderer know the fetch handler should be skipped, we
introduced the effective fetch handler type in
ControllerServiceWorkerInfo.  If the effective fetch handler type
is skippable, the renderer returns nullptr when the controller is
requested.

You may wonder why we need to introduce the effective fetch handler
type in addition to the existing fetch_handler_type.  That is because
we cannot change the fetch_handler_type behavior for the service
worker registration and the metrics.  Since the service worker
registration is persistent data, I do not think it is good idea to
change its contents by the flag.  For metrics, we want to compare the
same fetch handler case with the different flags.  The
fetch_handler_type should also need to be persistent here.
Note that FCP/LCP with skippable fetch handler type is taken in this
way.

Bug: 1364999
Bug: 1347319
Change-Id: I29f5af4589d5a675bf033dd74a17c975dd1a7722
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3905774
Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org>
Reviewed-by: Shunya Shishido <sisidovski@chromium.org>
Reviewed-by: Minoru Chikamune <chikamune@chromium.org>
Commit-Queue: Yoshisato Yanagisawa <yyanagisawa@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Sam McNally <sammc@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1052295}

content/browser/service_worker/service_worker_container_host.cc

@@ -609,6 +609,8 @@ void ServiceWorkerContainerHost::SendSetControllerServiceWorker(
 
   controller_info->mode = GetControllerMode();
   controller_info->fetch_handler_type = controller()->fetch_handler_type();
+  controller_info->effective_fetch_handler_type =
+      controller()->EffectiveFetchHandlerType();
 
   // Pass an endpoint for the client to talk to this controller.
   mojo::Remote<blink::mojom::ControllerServiceWorker> remote =

content/browser/service_worker/service_worker_controllee_request_handler.cc

@@ -433,50 +433,50 @@ void ServiceWorkerControlleeRequestHandler::ContinueWithActivatedVersion(
   if (blink::IsRequestDestinationFrame(destination_))
     container_host_->AddServiceWorkerToUpdate(active_version);
 
-  if (active_version->fetch_handler_existence() !=
-      ServiceWorkerVersion::FetchHandlerExistence::EXISTS) {
-    RecordSkipReason(FetchHandlerSkipReason::kNoFetchHandler);
-
-    TRACE_EVENT_WITH_FLOW1(
-        "ServiceWorker",
-        "ServiceWorkerControlleeRequestHandler::ContinueWithActivatedVersion",
-        TRACE_ID_LOCAL(this),
-        TRACE_EVENT_FLAG_FLOW_IN | TRACE_EVENT_FLAG_FLOW_OUT, "Info",
-        "Skipped the ServiceWorker which has no fetch handler");
-    CompleteWithoutLoader();
-    return;
+  switch (active_version->EffectiveFetchHandlerType()) {
+    case ServiceWorkerVersion::FetchHandlerType::kNoHandler: {
+      RecordSkipReason(FetchHandlerSkipReason::kNoFetchHandler);
+      TRACE_EVENT_WITH_FLOW1(
+          "ServiceWorker",
+          "ServiceWorkerControlleeRequestHandler::ContinueWithActivatedVersion",
+          TRACE_ID_LOCAL(this),
+          TRACE_EVENT_FLAG_FLOW_IN | TRACE_EVENT_FLAG_FLOW_OUT, "Info",
+          "Skipping the ServiceWorker which has no fetch handler");
+      CompleteWithoutLoader();
+      return;
+    }
+    case ServiceWorkerVersion::FetchHandlerType::kEmptyFetchHandler: {
+      RecordSkipReason(FetchHandlerSkipReason::kSkippedForEmptyFetchHandler);
+      TRACE_EVENT_WITH_FLOW2(
+          "ServiceWorker",
+          "ServiceWorkerControlleeRequestHandler::ContinueWithActivatedVersion",
+          TRACE_ID_LOCAL(this),
+          TRACE_EVENT_FLAG_FLOW_IN | TRACE_EVENT_FLAG_FLOW_OUT, "Info",
+          "The fetch handler is skippable. Falling back to network",
+          "FetchHandlerType",
+          FetchHandlerTypeToString(
+              active_version->EffectiveFetchHandlerType()));
+      CompleteWithoutLoader();
+      return;
+    }
+    case ServiceWorkerVersion::FetchHandlerType::kNotSkippable: {
+      RecordSkipReason(FetchHandlerSkipReason::kNotSkipped);
+      TRACE_EVENT_WITH_FLOW1(
+          "ServiceWorker",
+          "ServiceWorkerControlleeRequestHandler::ContinueWithActivatedVersion",
+          TRACE_ID_LOCAL(this),
+          TRACE_EVENT_FLAG_FLOW_IN | TRACE_EVENT_FLAG_FLOW_OUT, "Info",
+          "Forwarding to the ServiceWorker");
+      break;
+    }
   }
 
-  if (features::kSkipEmptyFetchHandler.Get() &&
-      active_version->fetch_handler_type() ==
-          ServiceWorkerVersion::FetchHandlerType::kEmptyFetchHandler) {
-    RecordSkipReason(FetchHandlerSkipReason::kSkippedForEmptyFetchHandler);
-
-    TRACE_EVENT_WITH_FLOW2(
-        "ServiceWorker",
-        "ServiceWorkerControlleeRequestHandler::ContinueWithActivatedVersion",
-        TRACE_ID_LOCAL(this),
-        TRACE_EVENT_FLAG_FLOW_IN | TRACE_EVENT_FLAG_FLOW_OUT, "Info",
-        "The fetch handler is skippable. Falling back to network",
-        "FetchHandlerType",
-        FetchHandlerTypeToString(active_version->fetch_handler_type()));
-    CompleteWithoutLoader();
-    return;
-  }
-  RecordSkipReason(FetchHandlerSkipReason::kNotSkipped);
-
   // Finally, we want to forward to the service worker! Make a
   // ServiceWorkerMainResourceLoader which does that work.
   loader_wrapper_ = std::make_unique<ServiceWorkerMainResourceLoaderWrapper>(
       std::make_unique<ServiceWorkerMainResourceLoader>(
           std::move(fallback_callback_), container_host_, frame_tree_node_id_));
 
-  TRACE_EVENT_WITH_FLOW1(
-      "ServiceWorker",
-      "ServiceWorkerControlleeRequestHandler::ContinueWithActivatedVersion",
-      TRACE_ID_LOCAL(this),
-      TRACE_EVENT_FLAG_FLOW_IN | TRACE_EVENT_FLAG_FLOW_OUT, "Info",
-      "Forwarded to the ServiceWorker");
   std::move(loader_callback_)
       .Run(base::MakeRefCounted<SingleRequestURLLoaderFactory>(
           base::BindOnce(&ServiceWorkerMainResourceLoader::StartRequest,

content/browser/service_worker/service_worker_main_resource_loader_interceptor.cc

@@ -276,6 +276,8 @@ ServiceWorkerMainResourceLoaderInterceptor::
   controller_info->mode = container_host->GetControllerMode();
   controller_info->fetch_handler_type =
       container_host->controller()->fetch_handler_type();
+  controller_info->effective_fetch_handler_type =
+      container_host->controller()->EffectiveFetchHandlerType();
   // Note that |controller_info->remote_controller| is null if the controller
   // has no fetch event handler. In that case the renderer frame won't get the
   // controller pointer upon the navigation commit, and subresource loading will

content/browser/service_worker/service_worker_version.cc

@@ -431,6 +431,23 @@ void ServiceWorkerVersion::set_fetch_handler_type(
   fetch_handler_type_ = fetch_handler_type;
 }
 
+ServiceWorkerVersion::FetchHandlerType
+ServiceWorkerVersion::EffectiveFetchHandlerType() const {
+  switch (fetch_handler_type()) {
+    case FetchHandlerType::kNoHandler:
+      return FetchHandlerType::kNoHandler;
+    case FetchHandlerType::kNotSkippable:
+      return FetchHandlerType::kNotSkippable;
+    case FetchHandlerType::kEmptyFetchHandler: {
+      if (features::kSkipEmptyFetchHandler.Get()) {
+        return FetchHandlerType::kEmptyFetchHandler;
+      } else {
+        return FetchHandlerType::kNotSkippable;
+      }
+    }
+  }
+}
+
 void ServiceWorkerVersion::StartWorker(ServiceWorkerMetrics::EventType purpose,
                                        StatusCallback callback) {
   TRACE_EVENT_INSTANT2(

content/browser/service_worker/service_worker_version.h

@@ -235,6 +235,24 @@ class CONTENT_EXPORT ServiceWorkerVersion
   FetchHandlerType fetch_handler_type() const;
   void set_fetch_handler_type(FetchHandlerType fetch_handler_type);
 
+  // If the feature flag for `fetch_handler_type_` is enabled,
+  // the function returns `fetch_handler_type_`.
+  // Otherwise, kNotSkippable would be returned if `fetch_handler_type_`
+  // is not kNoHandler.  Note that kNoHandler will be returned if
+  // `fetch_handler_type_` is kNoHandler.
+  //
+  // You may wonder why we need to introduce the effective fetch handler
+  // type in addition to the existing fetch_handler_type.  That is because
+  // we cannot change the fetch_handler_type behavior for the service
+  // worker registration and the metrics.  Since the service worker
+  // registration is persistent data, I do not think it is good idea to
+  // change its contents by the flag.  For metrics, we want to compare the
+  // same fetch handler case with the different flags.  The
+  // fetch_handler_type should also need to be persistent here.
+  // Note that FCP/LCP with skippable fetch handler type is taken in this
+  // way.
+  FetchHandlerType EffectiveFetchHandlerType() const;
+
   base::TimeDelta TimeSinceNoControllees() const {
     return GetTickDuration(no_controllees_time_);
   }

content/renderer/service_worker/service_worker_provider_context.cc

@@ -106,6 +106,15 @@ ServiceWorkerProviderContext::GetSubresourceLoaderFactoryInternal() {
     return nullptr;
   }
 
+  if (effective_fetch_handler_type_ !=
+      blink::mojom::ServiceWorkerFetchHandlerType::kNotSkippable) {
+    // The fetch handler can be skipped.  The service worker process should
+    // not be ready for this case.
+    CountFeature(
+        blink::mojom::WebFeature::kServiceWorkerSkippedForSubresourceLoad);
+    return nullptr;
+  }
+
   if (!subresource_loader_factory_) {
     DCHECK(!controller_connector_);
     DCHECK(remote_controller_);
@@ -314,6 +323,7 @@ void ServiceWorkerProviderContext::SetController(
           controller_));
   controller_mode_ = controller_info->mode;
   fetch_handler_type_ = controller_info->fetch_handler_type;
+  effective_fetch_handler_type_ = controller_info->effective_fetch_handler_type;
   remote_controller_ = std::move(controller_info->remote_controller);
 
   // Propagate the controller to workers related to this provider.

content/renderer/service_worker/service_worker_provider_context.h

@@ -281,6 +281,8 @@ class CONTENT_EXPORT ServiceWorkerProviderContext
 
   blink::mojom::ServiceWorkerFetchHandlerType fetch_handler_type_ =
       blink::mojom::ServiceWorkerFetchHandlerType::kNoHandler;
+  blink::mojom::ServiceWorkerFetchHandlerType effective_fetch_handler_type_ =
+      blink::mojom::ServiceWorkerFetchHandlerType::kNoHandler;
 
   // Tracks feature usage for UseCounter.
   std::set<blink::mojom::WebFeature> used_features_;

content/renderer/service_worker/service_worker_provider_context_unittest.cc

@@ -361,6 +361,8 @@ TEST_F(ServiceWorkerProviderContextTest, SetController) {
 
     auto info = blink::mojom::ControllerServiceWorkerInfo::New();
     info->mode = blink::mojom::ControllerServiceWorkerMode::kControlled;
+    info->fetch_handler_type = info->effective_fetch_handler_type =
+        blink::mojom::ServiceWorkerFetchHandlerType::kNotSkippable;
     info->object_info = std::move(object_info);
     container_remote->SetController(std::move(info), true);
     base::RunLoop().RunUntilIdle();
@@ -407,6 +409,8 @@ TEST_F(ServiceWorkerProviderContextTest, SetController) {
 
     auto info = blink::mojom::ControllerServiceWorkerInfo::New();
     info->mode = blink::mojom::ControllerServiceWorkerMode::kControlled;
+    info->fetch_handler_type = info->effective_fetch_handler_type =
+        blink::mojom::ServiceWorkerFetchHandlerType::kNotSkippable;
     info->object_info = std::move(object_info);
     container_remote->SetController(std::move(info), true);
     base::RunLoop().RunUntilIdle();
@@ -477,6 +481,9 @@ TEST_F(ServiceWorkerProviderContextTest, SetControllerServiceWorker) {
                          mojo::NullRemote());
   controller_info1->mode =
       blink::mojom::ControllerServiceWorkerMode::kControlled;
+  controller_info1->fetch_handler_type =
+      controller_info1->effective_fetch_handler_type =
+          blink::mojom::ServiceWorkerFetchHandlerType::kNotSkippable;
   controller_info1->object_info = std::move(object_info1);
   controller_info1->remote_controller = remote_controller1.Unbind();
 
@@ -522,6 +529,9 @@ TEST_F(ServiceWorkerProviderContextTest, SetControllerServiceWorker) {
                          mojo::NullRemote());
   controller_info2->mode =
       blink::mojom::ControllerServiceWorkerMode::kControlled;
+  controller_info2->fetch_handler_type =
+      controller_info2->effective_fetch_handler_type =
+          blink::mojom::ServiceWorkerFetchHandlerType::kNotSkippable;
   controller_info2->object_info = std::move(object_info2);
   controller_info2->remote_controller = remote_controller2.Unbind();
 
@@ -613,6 +623,9 @@ TEST_F(ServiceWorkerProviderContextTest, SetControllerServiceWorker) {
                          mojo::NullRemote());
   controller_info4->mode =
       blink::mojom::ControllerServiceWorkerMode::kControlled;
+  controller_info4->fetch_handler_type =
+      controller_info4->effective_fetch_handler_type =
+          blink::mojom::ServiceWorkerFetchHandlerType::kNotSkippable;
   controller_info4->object_info = std::move(object_info4);
   controller_info4->remote_controller = remote_controller4.Unbind();
   container_remote->SetController(std::move(controller_info4), true);
@@ -665,6 +678,9 @@ TEST_F(ServiceWorkerProviderContextTest, ControllerWithoutFetchHandler) {
   auto controller_info = blink::mojom::ControllerServiceWorkerInfo::New();
   controller_info->mode =
       blink::mojom::ControllerServiceWorkerMode::kNoFetchEventHandler;
+  controller_info->fetch_handler_type =
+      controller_info->effective_fetch_handler_type =
+          blink::mojom::ServiceWorkerFetchHandlerType::kNoHandler;
   controller_info->object_info = std::move(object_info);
 
   mojo::AssociatedRemote<blink::mojom::ServiceWorkerContainer> container_remote;
@@ -768,6 +784,9 @@ TEST_F(ServiceWorkerProviderContextTest, OnNetworkProviderDestroyed) {
                         mojo::NullRemote());
   controller_info->mode =
       blink::mojom::ControllerServiceWorkerMode::kControlled;
+  controller_info->fetch_handler_type =
+      controller_info->effective_fetch_handler_type =
+          blink::mojom::ServiceWorkerFetchHandlerType::kNotSkippable;
   controller_info->object_info = std::move(object_info);
   controller_info->remote_controller = remote_controller.Unbind();
 
@@ -816,6 +835,9 @@ TEST_F(ServiceWorkerProviderContextTest,
                         mojo::NullRemote());
   controller_info->mode =
       blink::mojom::ControllerServiceWorkerMode::kControlled;
+  controller_info->fetch_handler_type =
+      controller_info->effective_fetch_handler_type =
+          blink::mojom::ServiceWorkerFetchHandlerType::kNotSkippable;
   controller_info->object_info = std::move(object_info);
   controller_info->remote_controller = remote_controller.Unbind();
 

third_party/blink/public/mojom/service_worker/controller_service_worker.mojom

@@ -70,6 +70,12 @@ struct ControllerServiceWorkerInfo {
   // Type of the service worker fetch handler.
   ServiceWorkerFetchHandlerType fetch_handler_type =
       ServiceWorkerFetchHandlerType.kNoHandler;
+  // Effective type of the service worker fetch handler.
+  // This value will be kNotSkippable if the feature flag does not allow
+  // to skip the fetch handler type. The value will be different from
+  // `fetch_handler_type` above in that case.
+  ServiceWorkerFetchHandlerType effective_fetch_handler_type =
+      ServiceWorkerFetchHandlerType.kNoHandler;
 
 
   // Non-null iff there is a controller and it has a fetch event handler.

third_party/blink/public/mojom/service_worker/controller_service_worker_mode.mojom

@@ -6,6 +6,7 @@ module blink.mojom;
 
 // Describes whether a controller service worker exists and if it has a fetch
 // handler.
+// TODO(crbug.com/1368415): clean up ControllerServiceWorkerMode.
 enum ControllerServiceWorkerMode {
   // No controller exists.
   kNoController,

third_party/blink/public/mojom/use_counter/metrics/web_feature.mojom

@@ -3681,6 +3681,7 @@ enum WebFeature {
   kDestructiveDocumentWriteAfterModuleScript = 4360,
   kCSSAtSupportsDropInvalidWhileForgivingParsing = 4361,
   kPermissionsPolicyUnload = 4362,
+  kServiceWorkerSkippedForSubresourceLoad = 4363,
 
   // Add new features immediately above this line. Don't change assigned
   // numbers of any item, and don't reuse removed slots.

third_party/blink/web_tests/http/tests/serviceworker/resources/empty-fetch-handler.js

@@ -0,0 +1,2 @@
+// An empty fetch handler.
+self.addEventListener('fetch', event => {});

third_party/blink/web_tests/http/tests/serviceworker/resources/network-fallback-worker.js

@@ -1,3 +1,7 @@
 self.addEventListener('fetch', event => {
-  // Do nothing.
+  // Actually does nothing, but trick to make this not recognized as
+  // an empty fetch handler. Otherwise, the test timeout waiting
+  // kServiceWorkerInterceptedRequestFromOriginDirtyStyleSheet in
+  // usecounter-request-from-no-cors-style-sheet.html
+  let a = 0;
 });

third_party/blink/web_tests/http/tests/serviceworker/subresource-load-with-skip-handler.html

@@ -0,0 +1,52 @@
+<!DOCTYPE html>
+<meta charset="utf-8">
+<title>
+  The subresource load should not be blocked by skipping the fetch handler.
+</title>
+<!-- This cannot be upstreamed to WPT because it test uses Chrome's UseCounter
+     mechanism. -->
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="resources/test-helpers.js"></script>
+<script>
+function isUseCounted(win, feature) {
+  return win.internals.isUseCounted(win.document, feature);
+}
+
+function observeUseCounter(win, feature) {
+  return win.internals.observeUseCounter(win.document, feature);
+}
+
+function addStyleSheet(win, href) {
+  const link = win.document.createElement('link');
+  link.rel = 'stylesheet';
+  link.href = href;
+  win.document.head.appendChild(link);
+}
+
+const scope = 'resources/simple.html';
+// |kServiceWorkerNotHandledSubresourceLoad| in web_feature.mojom.
+const kFeature = 4363;
+
+promise_test(async t => {
+  const registration = await service_worker_unregister_and_register(
+      t, 'resources/empty-fetch-handler.js', scope);
+  t.add_cleanup(() => { registration.unregister(); });
+  await wait_for_state(t, registration.installing, 'activated');
+
+  // Set up the iframe in the scope.
+  const frame = await with_iframe('resources/simple.html');
+  t.add_cleanup(() => { frame.remove(); });
+  const win = frame.contentWindow;
+
+  // Ensure that the window is controlled by the service worker.
+  assert_not_equals(win.navigator.serviceWorker.controller, null);
+
+  // The stylesheet loaded at the page controlled by the service worker.
+  addStyleSheet(win, 'font-face.css');
+
+  // Ensure the service worker has been skipped.
+  await observeUseCounter(win, kFeature);
+  assert_true(isUseCounted(win, kFeature));
+}, 'An empty service worker handler should be skipped for loading subresource');
+</script>

tools/metrics/histograms/enums.xml

@@ -41449,6 +41449,7 @@ Called by update_use_counter_feature_enum.py.-->
   <int value="4360" label="DestructiveDocumentWriteAfterModuleScript"/>
   <int value="4361" label="CSSAtSupportsDropInvalidWhileForgivingParsing"/>
   <int value="4362" label="PermissionsPolicyUnload"/>
+  <int value="4363" label="ServiceWorkerSkippedForSubresourceLoad"/>
 </enum>
 
 <enum name="FeaturePolicyAllowlistType">