Bump fetch priority to at least kHigh if resource is render-blocking

This patch follows from the fetch spec revision: whatwg/fetch#1432 Bug: 1271296 Change-Id: I3f56e921e238a7e28774d5407a78d4df4f4145d5 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3627294 Reviewed-by: Yoav Weiss <yoavweiss@chromium.org> Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org> Reviewed-by: Patrick Meenan <pmeenan@chromium.org> Cr-Commit-Position: refs/heads/main@{#1007082} NOKEYCHECK=True GitOrigin-RevId: 5d88188b6b09aa1bb29b96177ab49a0d66a560c5
mjfroman · May 24, 2022 · e14d72a · e14d72a
1 parent 93f1473
commit e14d72a
Show file tree

Hide file tree

Showing 8 changed files with 78 additions and 20 deletions.
diff --git a/blink/renderer/core/frame/web_frame_test.cc b/blink/renderer/core/frame/web_frame_test.cc
@@ -14305,4 +14305,36 @@ TEST_F(WebFrameTest, FrameOwnerColorScheme) {
             mojom::blink::ColorScheme::kDark);
 }
 
+TEST_F(WebFrameSimTest, RenderBlockingPromotesResource) {
+  ScopedBlockingAttributeForTest enabled_scope(true);
+
+  SimRequest main_request("https://example.com/", "text/html");
+  SimSubresourceRequest script_request("https://example.com/script.js",
+                                       "text/javascript");
+
+  LoadURL("https://example.com/");
+  main_request.Write(R"HTML(
+    <!doctype html>
+    <script defer fetchpriority="low" src="script.js"></script>
+  )HTML");
+
+  Resource* script = GetDocument().Fetcher()->AllResources().at(
+      ToKURL("https://example.com/script.js"));
+
+  // Script is fetched at the low priority due to `fetchpriority="low"`.
+  ASSERT_TRUE(script);
+  EXPECT_EQ(ResourceLoadPriority::kLow,
+            script->GetResourceRequest().Priority());
+
+  main_request.Complete(R"HTML(
+    <script defer fetchpriority="low" blocking="render" src="script.js"></script>
+  )HTML");
+
+  // `blocking=render` promotes the priority to high.
+  EXPECT_EQ(ResourceLoadPriority::kHigh,
+            script->GetResourceRequest().Priority());
+
+  script_request.Complete();
+}
+
 }  // namespace blink
diff --git a/blink/renderer/core/script/document_write_intervention.cc b/blink/renderer/core/script/document_write_intervention.cc
@@ -210,6 +210,7 @@ void PossiblyFetchBlockedDocWriteScript(
   FetchParameters params(options.CreateFetchParameters(
       resource->Url(), context->GetSecurityOrigin(), context->GetCurrentWorld(),
       cross_origin, resource->Encoding(), FetchParameters::kIdleLoad));
+  params.SetRenderBlockingBehavior(RenderBlockingBehavior::kNonBlocking);
   AddHeader(&params);
   ScriptResource::Fetch(params, element_document.Fetcher(), nullptr,
                         ScriptResource::kNoStreaming);

diff --git a/blink/renderer/platform/loader/fetch/resource_fetcher.cc b/blink/renderer/platform/loader/fetch/resource_fetcher.cc
@@ -194,11 +194,12 @@ static ThreadSpecific<PriorityObserverMap>& PriorityObservers() {
   return map;
 }
 
-ResourceLoadPriority AdjustPriorityWithPriorityHint(
+ResourceLoadPriority AdjustPriorityWithPriorityHintAndRenderBlocking(
     ResourceLoadPriority priority_so_far,
     ResourceType type,
     const ResourceRequestHead& resource_request,
     FetchParameters::DeferOption defer_option,
+    RenderBlockingBehavior render_blocking_behavior,
     bool is_link_preload) {
   mojom::blink::FetchPriorityHint fetch_priority_hint =
       resource_request.GetFetchPriorityHint();
@@ -233,6 +234,13 @@ ResourceLoadPriority AdjustPriorityWithPriorityHint(
       break;
   }
 
+  // Render-blocking is a signal that the resource is important, so we bump it
+  // to at least kHigh.
+  if (render_blocking_behavior == RenderBlockingBehavior::kBlocking &&
+      new_priority < ResourceLoadPriority::kHigh) {
+    new_priority = ResourceLoadPriority::kHigh;
+  }
+
   return new_priority;
 }
 
@@ -400,6 +408,7 @@ ResourceLoadPriority ResourceFetcher::ComputeLoadPriority(
     ResourcePriority::VisibilityStatus visibility,
     FetchParameters::DeferOption defer_option,
     FetchParameters::SpeculativePreloadType speculative_preload_type,
+    RenderBlockingBehavior render_blocking_behavior,
     bool is_link_preload) {
   DCHECK(!resource_request.PriorityHasBeenSet() ||
          type == ResourceType::kImage);
@@ -470,8 +479,9 @@ ResourceLoadPriority ResourceFetcher::ComputeLoadPriority(
     }
   }
 
-  priority = AdjustPriorityWithPriorityHint(priority, type, resource_request,
-                                            defer_option, is_link_preload);
+  priority = AdjustPriorityWithPriorityHintAndRenderBlocking(
+      priority, type, resource_request, defer_option, render_blocking_behavior,
+      is_link_preload);
 
   if (properties_->IsSubframeDeprioritizationEnabled()) {
     if (properties_->IsOutermostMainFrame()) {
@@ -873,7 +883,8 @@ absl::optional<ResourceRequestBlockedReason> ResourceFetcher::PrepareRequest(
     computed_load_priority = ComputeLoadPriority(
         resource_type, params.GetResourceRequest(),
         ResourcePriority::kNotVisible, params.Defer(),
-        params.GetSpeculativePreloadType(), params.IsLinkPreload());
+        params.GetSpeculativePreloadType(), params.GetRenderBlockingBehavior(),
+        params.IsLinkPreload());
   }
 
   DCHECK_NE(computed_load_priority, ResourceLoadPriority::kUnresolved);
@@ -2229,10 +2240,7 @@ void ResourceFetcher::EmulateLoadStartedForInspector(
   resource_request.SetRequestDestination(request_destination);
   if (!resource_request.PriorityHasBeenSet()) {
     resource_request.SetPriority(ComputeLoadPriority(
-        resource->GetType(), resource_request, ResourcePriority::kNotVisible,
-        FetchParameters::DeferOption::kNoDefer,
-        FetchParameters::SpeculativePreloadType::kNotSpeculative,
-        false /* is_link_preload */));
+        resource->GetType(), resource_request, ResourcePriority::kNotVisible));
   }
   resource_request.SetReferrerString(Referrer::NoReferrer());
   resource_request.SetReferrerPolicy(network::mojom::ReferrerPolicy::kNever);

diff --git a/blink/renderer/platform/loader/fetch/resource_fetcher.h b/blink/renderer/platform/loader/fetch/resource_fetcher.h
@@ -294,9 +294,11 @@ class PLATFORM_EXPORT ResourceFetcher
       ResourcePriority::VisibilityStatus visibility_statue,
       FetchParameters::DeferOption defer_option,
       FetchParameters::SpeculativePreloadType speculative_preload_type,
+      RenderBlockingBehavior render_blocking_behavior,
       bool is_link_preload) {
     return ComputeLoadPriority(type, request, visibility_statue, defer_option,
-                               speculative_preload_type, is_link_preload);
+                               speculative_preload_type,
+                               render_blocking_behavior, is_link_preload);
   }
 
   void SetThrottleOptionOverride(
@@ -339,6 +341,7 @@ class PLATFORM_EXPORT ResourceFetcher
       FetchParameters::DeferOption = FetchParameters::DeferOption::kNoDefer,
       FetchParameters::SpeculativePreloadType =
           FetchParameters::SpeculativePreloadType::kNotSpeculative,
+      RenderBlockingBehavior = RenderBlockingBehavior::kNonBlocking,
       bool is_link_preload = false);
 
   // |virtual_time_pauser| is an output parameter. PrepareRequest may

diff --git a/blink/renderer/platform/loader/fetch/resource_fetcher_test.cc b/blink/renderer/platform/loader/fetch/resource_fetcher_test.cc
@@ -1289,7 +1289,7 @@ TEST_F(ResourceFetcherTest, DeprioritizeSubframe) {
         ResourceType::kScript, request, ResourcePriority::kNotVisible,
         FetchParameters::DeferOption::kNoDefer,
         FetchParameters::SpeculativePreloadType::kNotSpeculative,
-        false /* is_link_preload */);
+        RenderBlockingBehavior::kNonBlocking, false /* is_link_preload */);
     EXPECT_EQ(priority, ResourceLoadPriority::kHigh);
   }
 
@@ -1301,7 +1301,7 @@ TEST_F(ResourceFetcherTest, DeprioritizeSubframe) {
         ResourceType::kScript, request, ResourcePriority::kNotVisible,
         FetchParameters::DeferOption::kNoDefer,
         FetchParameters::SpeculativePreloadType::kNotSpeculative,
-        false /* is_link_preload */);
+        RenderBlockingBehavior::kNonBlocking, false /* is_link_preload */);
     EXPECT_EQ(priority, ResourceLoadPriority::kHigh);
   }
 
@@ -1313,7 +1313,7 @@ TEST_F(ResourceFetcherTest, DeprioritizeSubframe) {
         ResourceType::kScript, request, ResourcePriority::kNotVisible,
         FetchParameters::DeferOption::kNoDefer,
         FetchParameters::SpeculativePreloadType::kNotSpeculative,
-        false /* is_link_preload */);
+        RenderBlockingBehavior::kNonBlocking, false /* is_link_preload */);
     EXPECT_EQ(priority, ResourceLoadPriority::kHigh);
   }
 
@@ -1325,7 +1325,7 @@ TEST_F(ResourceFetcherTest, DeprioritizeSubframe) {
         ResourceType::kScript, request, ResourcePriority::kNotVisible,
         FetchParameters::DeferOption::kNoDefer,
         FetchParameters::SpeculativePreloadType::kNotSpeculative,
-        false /* is_link_preload */);
+        RenderBlockingBehavior::kNonBlocking, false /* is_link_preload */);
     EXPECT_EQ(priority, ResourceLoadPriority::kLow);
   }
   {
@@ -1336,7 +1336,7 @@ TEST_F(ResourceFetcherTest, DeprioritizeSubframe) {
         ResourceType::kMock, request, ResourcePriority::kNotVisible,
         FetchParameters::DeferOption::kNoDefer,
         FetchParameters::SpeculativePreloadType::kNotSpeculative,
-        false /* is_link_preload */);
+        RenderBlockingBehavior::kNonBlocking, false /* is_link_preload */);
     EXPECT_EQ(priority, ResourceLoadPriority::kMedium);
   }
 
@@ -1349,7 +1349,7 @@ TEST_F(ResourceFetcherTest, DeprioritizeSubframe) {
         ResourceType::kMock, request, ResourcePriority::kNotVisible,
         FetchParameters::DeferOption::kNoDefer,
         FetchParameters::SpeculativePreloadType::kNotSpeculative,
-        false /* is_link_preload */);
+        RenderBlockingBehavior::kNonBlocking, false /* is_link_preload */);
     EXPECT_EQ(priority, ResourceLoadPriority::kLowest);
   }
 }

diff --git a/blink/web_tests/http/tests/priority-hints/script-async-initial-load.html b/blink/web_tests/http/tests/priority-hints/script-async-initial-load.html
@@ -28,7 +28,12 @@
     promise: internals.getInitialResourcePriority(new URL('../resources/dummy.js?4', location), document),
     expected_priority: kLow,
     description: 'missing fetchpriority on async <script> has no effect'
-  }
+  },
+  {
+    promise: internals.getInitialResourcePriority(new URL('../resources/dummy.js?5', location), document),
+    expected_priority: kHigh,
+    description: 'blocking=render on async <script> raises the priority to kHigh'
+  },
 ];
 
 </script>
@@ -38,6 +43,7 @@
 <script id=script3 async fetchpriority=auto src=../resources/dummy.js?2></script>
 <script id=script4 async fetchpriority=xyz src=../resources/dummy.js?3></script>
 <script id=script5 async src=../resources/dummy.js?4></script>
+<script id=script6 async fetchpriority=low blocking=render src=../resources/dummy.js?5></script>
 
 <script>
   promise_test(async (t) => {
@@ -51,6 +57,7 @@
     assert_true(internals.isPreloaded(script3.src), script3.src + base_msg);
     assert_true(internals.isPreloaded(script4.src), script4.src + base_msg);
     assert_true(internals.isPreloaded(script5.src), script5.src + base_msg);
+    assert_true(internals.isPreloaded(script6.src), script6.src + base_msg);
   }, 'all scripts were fetched by the preload scanner');
 
   // Setup the tests described by |priority_tests|.

diff --git a/blink/web_tests/http/tests/priority-hints/script-initial-load.html b/blink/web_tests/http/tests/priority-hints/script-initial-load.html
@@ -14,8 +14,8 @@
   },
   {
     promise: internals.getInitialResourcePriority(new URL('../resources/dummy.js?2', location), document),
-    expected_priority: kLow,
-    description: 'low fetchpriority on <script> lowers priority to kLow'
+    expected_priority: kHigh,
+    description: 'low fetchpriority on <script> has no effect'
   },
   {
     promise: internals.getInitialResourcePriority(new URL('../resources/dummy.js?3', location), document),
@@ -101,4 +101,4 @@
 <script id=script10 src=../resources/dummy.js?10></script>
 
 </body>
-</html>
+</html>
diff --git a/blink/web_tests/http/tests/priority-hints/script-module-initial-load.html b/blink/web_tests/http/tests/priority-hints/script-module-initial-load.html
@@ -28,7 +28,12 @@
     promise: internals.getInitialResourcePriority(new URL('../resources/dummy.js?4', location), document),
     expected_priority: kHigh,
     description: 'missing fetchpriority on module <script> has no effect'
-  }
+  },
+  {
+    promise: internals.getInitialResourcePriority(new URL('../resources/dummy.js?5', location), document),
+    expected_priority: kHigh,
+    description: 'blocking=render on module <script> raises the priority to kHigh'
+  },
 ];
 
 </script>
@@ -38,6 +43,7 @@
 <script id=script3 type=module fetchpriority=auto src=../resources/dummy.js?2></script>
 <script id=script4 type=module fetchpriority=xyz src=../resources/dummy.js?3></script>
 <script id=script5 type=module src=../resources/dummy.js?4></script>
+<script id=script6 type=module fetchpriority=low blocking=render src=../resources/dummy.js?5></script>
 
 <script>
   promise_test(async (t) => {
@@ -51,6 +57,7 @@
     assert_true(internals.isPreloaded(script3.src), script3.src + base_msg);
     assert_true(internals.isPreloaded(script4.src), script4.src + base_msg);
     assert_true(internals.isPreloaded(script5.src), script5.src + base_msg);
+    assert_true(internals.isPreloaded(script6.src), script6.src + base_msg);
   }, 'all scripts were fetched by the preload scanner');
 
   // Setup the tests described by |priority_tests|.