[renderblocking] Add a new class to get link preload finish callbacks

In the existing code, LinkLoader is the only class that receives
callbacks when a link preload/modulepreload finishes. This patch moves
the logic into a new class PendingLinkPreload. The purposes are:

1. The new class will allow link header preloads to also receive
   callbacks when finishing, so that we can enable render-blocking on
   link headers. This will be implemented in crrev.com/c/3551557.

2. Moves the place where render-blocking starts from
   LinkLoader::LoadLink() (which may or may not actually start a fetch)
   to PreloadHelper::Preload/ModulePreloadIfNeeded(), right before
   where we actually start the preload. This fixes a bug that an invalid
   preload link with `blocking="render"` infinitely blocks rendering,
   for which we don't start a fetch so their finish callbacks will
   never be fired, which means the old implementation never unblocks
   rendering on them. This is verified by the new test
   invalid-render-blocking-preload-link.html.

The most important code changes to review are in:
- link_loader.h/cc
- pending_link_preload.h/cc
- preload_helper.h/cc

Bug: 1271296
Change-Id: I8f183b2b0f26c550e76319cda70d890b1d52f247
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3547303
Reviewed-by: Mason Freed <masonf@chromium.org>
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/main@{#987234}

third_party/blink/renderer/core/html/blocking_attribute.cc

@@ -6,6 +6,7 @@
 
 #include "third_party/blink/renderer/core/dom/document.h"
 #include "third_party/blink/renderer/core/dom/element.h"
+#include "third_party/blink/renderer/core/dom/space_split_string.h"
 #include "third_party/blink/renderer/core/frame/web_feature.h"
 #include "third_party/blink/renderer/platform/runtime_enabled_features.h"
 #include "third_party/blink/renderer/platform/wtf/std_lib_extras.h"
@@ -26,6 +27,15 @@ HashSet<AtomicString>& BlockingAttribute::SupportedTokens() {
   return tokens;
 }
 
+// static
+bool BlockingAttribute::IsRenderBlocking(const String& attribute_value) {
+  if (!RuntimeEnabledFeatures::BlockingAttributeEnabled())
+    return false;
+  if (attribute_value.IsEmpty())
+    return false;
+  return SpaceSplitString(AtomicString(attribute_value)).Contains(kRenderToken);
+}
+
 bool BlockingAttribute::ValidateTokenValue(const AtomicString& token_value,
                                            ExceptionState&) const {
   DCHECK(RuntimeEnabledFeatures::BlockingAttributeEnabled());

third_party/blink/renderer/core/html/blocking_attribute.h

@@ -20,6 +20,7 @@ class BlockingAttribute final : public DOMTokenList {
   explicit BlockingAttribute(Element* element)
       : DOMTokenList(*element, html_names::kBlockingAttr) {}
 
+  static bool IsRenderBlocking(const String& attribute_value);
   bool IsRenderBlocking() const { return contains(kRenderToken); }
 
   void CountTokenUsage();

third_party/blink/renderer/core/html/html_link_element.cc

@@ -324,21 +324,13 @@ void HTMLLinkElement::LinkLoaded() {
   if (rel_attribute_.IsLinkPrefetch()) {
     UseCounter::Count(GetDocument(), WebFeature::kLinkPrefetchLoadEvent);
   }
-  if (RenderBlockingResourceManager* manager =
-          GetDocument().GetRenderBlockingResourceManager()) {
-    manager->RemovePendingPreload(*this);
-  }
   DispatchEvent(*Event::Create(event_type_names::kLoad));
 }
 
 void HTMLLinkElement::LinkLoadingErrored() {
   if (rel_attribute_.IsLinkPrefetch()) {
     UseCounter::Count(GetDocument(), WebFeature::kLinkPrefetchErrorEvent);
   }
-  if (RenderBlockingResourceManager* manager =
-          GetDocument().GetRenderBlockingResourceManager()) {
-    manager->RemovePendingPreload(*this);
-  }
   DispatchEvent(*Event::Create(event_type_names::kError));
 }
 
@@ -446,17 +438,6 @@ DOMTokenList* HTMLLinkElement::scopes() const {
   return scopes_.Get();
 }
 
-bool HTMLLinkElement::IsRenderBlockingPreload() const {
-  if (!RuntimeEnabledFeatures::BlockingAttributeEnabled())
-    return false;
-  return (rel_attribute_.IsLinkPreload() || rel_attribute_.IsModulePreload()) &&
-         blocking_attribute_->IsRenderBlocking();
-}
-
-bool HTMLLinkElement::IsFontPreload() const {
-  return rel_attribute_.IsLinkPreload() && EqualIgnoringASCIICase(as_, "font");
-}
-
 void HTMLLinkElement::Trace(Visitor* visitor) const {
   visitor->Trace(link_);
   visitor->Trace(sizes_);

third_party/blink/renderer/core/html/html_link_element.h

@@ -110,8 +110,6 @@ class CORE_EXPORT HTMLLinkElement final : public HTMLElement,
   // From LinkLoaderClient
   bool ShouldLoadLink() override;
   bool IsLinkCreatedByParser() override;
-  bool IsRenderBlockingPreload() const override;
-  bool IsFontPreload() const override;
 
   // For LinkStyle
   bool LoadLink(const LinkLoadParameters&);

third_party/blink/renderer/core/html/link_style.cc

@@ -331,7 +331,8 @@ void LinkStyle::Process() {
       owner_->GetReferrerPolicy(),
       owner_->GetNonEmptyURLAttribute(html_names::kHrefAttr),
       owner_->FastGetAttribute(html_names::kImagesrcsetAttr),
-      owner_->FastGetAttribute(html_names::kImagesizesAttr));
+      owner_->FastGetAttribute(html_names::kImagesizesAttr),
+      owner_->FastGetAttribute(html_names::kBlockingAttr));
 
   WTF::TextEncoding charset = GetCharset();
 

third_party/blink/renderer/core/loader/build.gni

@@ -59,6 +59,8 @@ blink_core_sources_loader = [
   "link_loader.cc",
   "link_loader.h",
   "link_loader_client.h",
+  "pending_link_preload.h",
+  "pending_link_preload.cc",
   "loader_factory_for_frame.cc",
   "loader_factory_for_frame.h",
   "loader_factory_for_worker.cc",

third_party/blink/renderer/core/loader/link_load_parameters.cc

@@ -21,7 +21,8 @@ LinkLoadParameters::LinkLoadParameters(
     network::mojom::ReferrerPolicy referrer_policy,
     const KURL& href,
     const String& image_srcset,
-    const String& image_sizes)
+    const String& image_sizes,
+    const String& blocking)
     : rel(rel),
       cross_origin(cross_origin),
       type(type),
@@ -33,7 +34,8 @@ LinkLoadParameters::LinkLoadParameters(
       referrer_policy(referrer_policy),
       href(href),
       image_srcset(image_srcset),
-      image_sizes(image_sizes) {}
+      image_sizes(image_sizes),
+      blocking(blocking) {}
 
 // TODO(domfarolino)
 // Eventually we'll want to support a |fetchpriority| value on

third_party/blink/renderer/core/loader/link_load_parameters.h

@@ -30,7 +30,10 @@ struct CORE_EXPORT LinkLoadParameters {
                      network::mojom::ReferrerPolicy,
                      const KURL& href,
                      const String& image_srcset,
-                     const String& image_sizes);
+                     const String& image_sizes,
+                     // TODO(xiaochengh): The default parameter is temporarily
+                     // introduced to reduce patch diff. Remove it later.
+                     const String& blocking = String());
   LinkLoadParameters(const LinkHeader&, const KURL& base_url);
 
   LinkRelAttribute rel;
@@ -45,6 +48,7 @@ struct CORE_EXPORT LinkLoadParameters {
   KURL href;
   String image_srcset;
   String image_sizes;
+  String blocking;
   absl::optional<base::UnguessableToken> recursive_prefetch_token;
 };
 

third_party/blink/renderer/core/loader/link_loader.cc

@@ -38,6 +38,7 @@
 #include "third_party/blink/renderer/core/loader/fetch_priority_attribute.h"
 #include "third_party/blink/renderer/core/loader/link_load_parameters.h"
 #include "third_party/blink/renderer/core/loader/link_loader_client.h"
+#include "third_party/blink/renderer/core/loader/pending_link_preload.h"
 #include "third_party/blink/renderer/core/loader/preload_helper.h"
 #include "third_party/blink/renderer/core/loader/prerender_handle.h"
 #include "third_party/blink/renderer/core/loader/resource/css_style_sheet_resource.h"
@@ -78,55 +79,11 @@ PrerenderTriggerTypeFromRelAttribute(const LinkRelAttribute& rel_attribute,
 
 }  // namespace
 
-class LinkLoader::FinishObserver final : public ResourceFinishObserver {
-  USING_PRE_FINALIZER(FinishObserver, ClearResource);
-
- public:
-  FinishObserver(LinkLoader* loader, Resource* resource)
-      : loader_(loader), resource_(resource) {
-    resource_->AddFinishObserver(
-        this, loader_->client_->GetLoadingTaskRunner().get());
-  }
-
-  // ResourceFinishObserver implementation
-  void NotifyFinished() override {
-    if (!resource_)
-      return;
-    loader_->NotifyFinished();
-    ClearResource();
-  }
-  String DebugName() const override {
-    return "LinkLoader::ResourceFinishObserver";
-  }
-
-  Resource* GetResource() { return resource_; }
-  void ClearResource() {
-    if (!resource_)
-      return;
-    resource_->RemoveFinishObserver(this);
-    resource_ = nullptr;
-  }
-
-  void Trace(Visitor* visitor) const override {
-    visitor->Trace(loader_);
-    visitor->Trace(resource_);
-    blink::ResourceFinishObserver::Trace(visitor);
-  }
-
- private:
-  Member<LinkLoader> loader_;
-  Member<Resource> resource_;
-};
-
 LinkLoader::LinkLoader(LinkLoaderClient* client) : client_(client) {
   DCHECK(client_);
 }
 
-LinkLoader::~LinkLoader() = default;
-
-void LinkLoader::NotifyFinished() {
-  DCHECK(finish_observer_);
-  Resource* resource = finish_observer_->GetResource();
+void LinkLoader::NotifyFinished(Resource* resource) {
   if (resource->ErrorOccurred() ||
       (resource->IsLinkPreload() &&
        resource->IntegrityDisposition() ==
@@ -139,17 +96,17 @@ void LinkLoader::NotifyFinished() {
 
 // https://html.spec.whatwg.org/C/#link-type-modulepreload
 void LinkLoader::NotifyModuleLoadFinished(ModuleScript* module) {
-  // Step 11. "If result is null, fire an event named error at the link element,
+  // Step 14. "If result is null, fire an event named error at the link element,
   // and return." [spec text]
-  // Step 12. "Fire an event named load at the link element." [spec text]
+  // Step 15. "Fire an event named load at the link element." [spec text]
   if (!module)
     client_->LinkLoadingErrored();
   else
     client_->LinkLoaded();
 }
 
 Resource* LinkLoader::GetResourceForTesting() {
-  return finish_observer_ ? finish_observer_->GetResource() : nullptr;
+  return pending_preload_ ? pending_preload_->GetResourceForTesting() : nullptr;
 }
 
 bool LinkLoader::LoadLink(const LinkLoadParameters& params,
@@ -160,36 +117,23 @@ bool LinkLoader::LoadLink(const LinkLoadParameters& params,
   if (!client_->ShouldLoadLink())
     return false;
 
-  if (RenderBlockingResourceManager* manager =
-          document.GetRenderBlockingResourceManager()) {
-    if (client_->IsRenderBlockingPreload()) {
-      manager->AddPendingPreload(
-          *client_, RenderBlockingResourceManager::PreloadType::kRegular);
-    } else if (client_->IsFontPreload()) {
-      manager->AddPendingPreload(
-          *client_,
-          RenderBlockingResourceManager::PreloadType::kShortBlockingFont);
-    }
-  }
-
   PreloadHelper::DnsPrefetchIfNeeded(params, &document, document.GetFrame(),
                                      PreloadHelper::kLinkCalledFromMarkup);
 
   PreloadHelper::PreconnectIfNeeded(params, &document, document.GetFrame(),
                                     PreloadHelper::kLinkCalledFromMarkup);
 
-  Resource* resource = PreloadHelper::PreloadIfNeeded(
+  pending_preload_ = MakeGarbageCollected<PendingLinkPreload>(document, this);
+
+  PreloadHelper::PreloadIfNeeded(
       params, document, NullURL(), PreloadHelper::kLinkCalledFromMarkup,
       nullptr /* viewport_description */,
-      client_->IsLinkCreatedByParser() ? kParserInserted : kNotParserInserted);
-  if (!resource) {
-    resource = PreloadHelper::PrefetchIfNeeded(params, document);
-  }
-  if (resource)
-    finish_observer_ = MakeGarbageCollected<FinishObserver>(this, resource);
-
+      client_->IsLinkCreatedByParser() ? kParserInserted : kNotParserInserted,
+      pending_preload_);
+  if (!pending_preload_->HasResource())
+    PreloadHelper::PrefetchIfNeeded(params, document, pending_preload_);
   PreloadHelper::ModulePreloadIfNeeded(
-      params, document, nullptr /* viewport_description */, this);
+      params, document, nullptr /* viewport_description */, pending_preload_);
 
   absl::optional<mojom::blink::PrerenderTriggerType> trigger_type =
       PrerenderTriggerTypeFromRelAttribute(params.rel, document);
@@ -254,17 +198,16 @@ void LinkLoader::Abort() {
     prerender_->Cancel();
     prerender_.Clear();
   }
-  if (finish_observer_) {
-    finish_observer_->ClearResource();
-    finish_observer_ = nullptr;
+  if (pending_preload_) {
+    pending_preload_->Dispose();
+    pending_preload_.Clear();
   }
 }
 
 void LinkLoader::Trace(Visitor* visitor) const {
-  visitor->Trace(finish_observer_);
   visitor->Trace(client_);
+  visitor->Trace(pending_preload_);
   visitor->Trace(prerender_);
-  SingleModuleClient::Trace(visitor);
 }
 
 }  // namespace blink

third_party/blink/renderer/core/loader/link_loader.h

@@ -34,23 +34,23 @@
 
 #include "third_party/blink/renderer/core/core_export.h"
 #include "third_party/blink/renderer/core/loader/link_load_parameters.h"
-#include "third_party/blink/renderer/core/script/modulator.h"
 #include "third_party/blink/renderer/platform/loader/fetch/fetch_parameters.h"
 
 namespace blink {
 
 class Document;
 class LinkLoaderClient;
+class PendingLinkPreload;
+class ModuleScript;
 class PrerenderHandle;
 class Resource;
 class ResourceClient;
 
 // The LinkLoader can load link rel types icon, dns-prefetch, prefetch, and
 // prerender.
-class CORE_EXPORT LinkLoader final : public SingleModuleClient {
+class CORE_EXPORT LinkLoader final : public GarbageCollected<LinkLoader> {
  public:
   explicit LinkLoader(LinkLoaderClient*);
-  ~LinkLoader() override;
 
   void Abort();
   bool LoadLink(const LinkLoadParameters&, Document&);
@@ -64,17 +64,14 @@ class CORE_EXPORT LinkLoader final : public SingleModuleClient {
 
   Resource* GetResourceForTesting();
 
-  void Trace(Visitor*) const override;
+  void NotifyModuleLoadFinished(ModuleScript*);
+  void NotifyFinished(Resource*);
 
- private:
-  class FinishObserver;
-
-  void NotifyFinished();
-  // SingleModuleClient implementation
-  void NotifyModuleLoadFinished(ModuleScript*) override;
+  void Trace(Visitor*) const;
 
-  Member<FinishObserver> finish_observer_;
+ private:
   Member<LinkLoaderClient> client_;
+  Member<PendingLinkPreload> pending_preload_;
 
   Member<PrerenderHandle> prerender_;
 };

third_party/blink/renderer/core/loader/link_loader_client.h

@@ -45,18 +45,13 @@ class CORE_EXPORT LinkLoaderClient : public GarbageCollectedMixin {
 
   virtual bool ShouldLoadLink() = 0;
 
-  // Returns true if the link is a render-blocking preload or modulepreload.
-  // https://html.spec.whatwg.org/multipage/urls-and-fetching.html#render-blocking
-  virtual bool IsRenderBlockingPreload() const = 0;
-
-  virtual bool IsFontPreload() const = 0;
-
   virtual void LinkLoaded() = 0;
   virtual void LinkLoadingErrored() = 0;
   // There is no notification for cancellation.
 
   virtual bool IsLinkCreatedByParser() = 0;
 
+  // TODO(xiaochengh): Remove this unused function.
   virtual scoped_refptr<base::SingleThreadTaskRunner>
   GetLoadingTaskRunner() = 0;
 };

third_party/blink/renderer/core/loader/link_loader_test.cc

@@ -50,9 +50,6 @@ class MockLinkLoaderClient final
   bool ShouldLoadLink() override { return should_load_; }
   bool IsLinkCreatedByParser() override { return true; }
 
-  bool IsRenderBlockingPreload() const override { return false; }
-  bool IsFontPreload() const override { return false; }
-
   void LinkLoaded() override {}
   void LinkLoadingErrored() override {}