Make sure 'font-display: optional' doesn't cause relayout

This patch ensures that an 'optional' web font never causes relayout:

- When document rendering starts, any 'optional' web font that hasn't
  been loaded enters the failure period directly.

- When an @font-face rule is added, if document rendering has started
  and the font is not available from memory cache, then it enters the
  failure period directly.

In this way, any element attempting to use such 'optional' fonts will
always use a font that's further down in the 'font-family' list during
the document's lifetime.

Bug: 1040632
Change-Id: Idde675a8dddadf15d22c13b95db649410fd9cd0e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2057502Reviewed-by: Kunihiko Sakamoto <ksakamoto@chromium.org>
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
Reviewed-by: Rune Lillesveen <futhark@chromium.org>
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#749080}

third_party/blink/renderer/core/css/remote_font_face_source.cc

@@ -5,6 +5,7 @@
 #include "third_party/blink/renderer/core/css/remote_font_face_source.h"
 
 #include "base/metrics/histogram_functions.h"
+#include "third_party/blink/public/common/features.h"
 #include "third_party/blink/public/mojom/feature_policy/feature_policy_feature.mojom-blink.h"
 #include "third_party/blink/public/platform/task_type.h"
 #include "third_party/blink/public/platform/web_effective_connection_type.h"
@@ -27,58 +28,71 @@
 
 namespace blink {
 
-namespace {
-
-RemoteFontFaceSource::DisplayPeriod ComputePeriod(
-    FontDisplay displayValue,
-    RemoteFontFaceSource::Phase phase,
-    bool is_intervention_triggered) {
-  switch (displayValue) {
+RemoteFontFaceSource::DisplayPeriod RemoteFontFaceSource::ComputePeriod()
+    const {
+  switch (display_) {
     case kFontDisplayAuto:
-      if (is_intervention_triggered)
-        return RemoteFontFaceSource::kSwapPeriod;
+      if (is_intervention_triggered_)
+        return kSwapPeriod;
       FALLTHROUGH;
     case kFontDisplayBlock:
-      switch (phase) {
-        case RemoteFontFaceSource::kNoLimitExceeded:
-        case RemoteFontFaceSource::kShortLimitExceeded:
-          return RemoteFontFaceSource::kBlockPeriod;
-        case RemoteFontFaceSource::kLongLimitExceeded:
-          return RemoteFontFaceSource::kSwapPeriod;
+      switch (phase_) {
+        case kNoLimitExceeded:
+        case kShortLimitExceeded:
+          return kBlockPeriod;
+        case kLongLimitExceeded:
+          return kSwapPeriod;
       }
 
     case kFontDisplaySwap:
-      return RemoteFontFaceSource::kSwapPeriod;
+      return kSwapPeriod;
 
     case kFontDisplayFallback:
-      switch (phase) {
-        case RemoteFontFaceSource::kNoLimitExceeded:
-          return RemoteFontFaceSource::kBlockPeriod;
-        case RemoteFontFaceSource::kShortLimitExceeded:
-          return RemoteFontFaceSource::kSwapPeriod;
-        case RemoteFontFaceSource::kLongLimitExceeded:
-          return RemoteFontFaceSource::kFailurePeriod;
+      switch (phase_) {
+        case kNoLimitExceeded:
+          return kBlockPeriod;
+        case kShortLimitExceeded:
+          return kSwapPeriod;
+        case kLongLimitExceeded:
+          return kFailurePeriod;
+      }
+
+    case kFontDisplayOptional: {
+      const bool use_phase_value =
+          !base::FeatureList::IsEnabled(
+              features::kFontPreloadingDelaysRendering) ||
+          !GetDocument();
+
+      if (use_phase_value) {
+        switch (phase_) {
+          case kNoLimitExceeded:
+            return kBlockPeriod;
+          case kShortLimitExceeded:
+          case kLongLimitExceeded:
+            return kFailurePeriod;
+        }
       }
 
-    case kFontDisplayOptional:
-      switch (phase) {
-        case RemoteFontFaceSource::kNoLimitExceeded:
-          return RemoteFontFaceSource::kBlockPeriod;
-        case RemoteFontFaceSource::kShortLimitExceeded:
-        case RemoteFontFaceSource::kLongLimitExceeded:
-          return RemoteFontFaceSource::kFailurePeriod;
+      // We simply skip the block period, as we should never render invisible
+      // fallback for 'font-display: optional'.
+
+      if (GetDocument()->GetFontPreloadManager().RenderingHasBegun()) {
+        if (FinishedFromMemoryCache() ||
+            finished_before_document_rendering_begin_)
+          return kSwapPeriod;
+        return kFailurePeriod;
       }
 
+      return kSwapPeriod;
+    }
     case kFontDisplayEnumMax:
       NOTREACHED();
       break;
   }
   NOTREACHED();
-  return RemoteFontFaceSource::kSwapPeriod;
+  return kSwapPeriod;
 }
 
-}  // namespace
-
 RemoteFontFaceSource::RemoteFontFaceSource(CSSFontFace* css_font_face,
                                            FontSelector* font_selector,
                                            FontDisplay display)
@@ -90,13 +104,18 @@ RemoteFontFaceSource::RemoteFontFaceSource(CSSFontFace* css_font_face,
                                                font_selector,
                                                ReportOptions::kDoNotReport)),
       phase_(kNoLimitExceeded),
-      is_intervention_triggered_(ShouldTriggerWebFontsIntervention()) {
+      is_intervention_triggered_(ShouldTriggerWebFontsIntervention()),
+      finished_before_document_rendering_begin_(false) {
   DCHECK(face_);
-  period_ = ComputePeriod(display_, phase_, is_intervention_triggered_);
+  period_ = ComputePeriod();
 }
 
 RemoteFontFaceSource::~RemoteFontFaceSource() = default;
 
+Document* RemoteFontFaceSource::GetDocument() const {
+  return Document::DynamicFrom(font_selector_->GetExecutionContext());
+}
+
 void RemoteFontFaceSource::Dispose() {
   ClearResource();
   PruneTable();
@@ -147,6 +166,17 @@ void RemoteFontFaceSource::NotifyFinished(Resource* resource) {
   ClearResource();
 
   PruneTable();
+
+  if (GetDocument() &&
+      !GetDocument()->GetFontPreloadManager().RenderingHasBegun()) {
+    finished_before_document_rendering_begin_ = true;
+  }
+
+  if (FinishedFromMemoryCache())
+    period_ = kNotApplicablePeriod;
+  else
+    UpdatePeriod();
+
   if (face_->FontLoaded(this)) {
     font_selector_->FontFaceInvalidated();
 
@@ -187,8 +217,7 @@ void RemoteFontFaceSource::SetDisplay(FontDisplay display) {
 }
 
 void RemoteFontFaceSource::UpdatePeriod() {
-  DisplayPeriod new_period =
-      ComputePeriod(display_, phase_, is_intervention_triggered_);
+  DisplayPeriod new_period = ComputePeriod();
 
   // Fallback font is invisible iff the font is loading and in the block period.
   // Invalidate the font if its fallback visibility has changed.

third_party/blink/renderer/core/css/remote_font_face_source.h

@@ -13,6 +13,7 @@
 namespace blink {
 
 class CSSFontFace;
+class Document;
 class FontSelector;
 class FontCustomPlatformData;
 
@@ -23,9 +24,6 @@ class RemoteFontFaceSource final : public CSSFontFaceSource,
 
  public:
   enum Phase { kNoLimitExceeded, kShortLimitExceeded, kLongLimitExceeded };
-  // Periods of the Font Display Timeline.
-  // https://drafts.csswg.org/css-fonts-4/#font-display-timeline
-  enum DisplayPeriod { kBlockPeriod, kSwapPeriod, kFailurePeriod };
 
   RemoteFontFaceSource(CSSFontFace*, FontSelector*, FontDisplay);
   ~RemoteFontFaceSource() override;
@@ -60,6 +58,19 @@ class RemoteFontFaceSource final : public CSSFontFaceSource,
       const FontDescription&);
 
  private:
+  // Periods of the Font Display Timeline.
+  // https://drafts.csswg.org/css-fonts-4/#font-display-timeline
+  // Note that kNotApplicablePeriod is an implementation detail indicating that
+  // the font is loaded from memory cache synchronously, and hence, made
+  // immediately available. As we never need to use a fallback for it, using
+  // other DisplayPeriod values seem artificial. So we use a special value.
+  enum DisplayPeriod {
+    kBlockPeriod,
+    kSwapPeriod,
+    kFailurePeriod,
+    kNotApplicablePeriod
+  };
+
   class FontLoadHistograms {
     DISALLOW_NEW();
 
@@ -113,6 +124,9 @@ class RemoteFontFaceSource final : public CSSFontFaceSource,
     DataSource data_source_;
   };
 
+  Document* GetDocument() const;
+
+  DisplayPeriod ComputePeriod() const;
   void UpdatePeriod();
   bool ShouldTriggerWebFontsIntervention();
   bool IsLowPriorityLoadingAllowedForRemoteFont() const override;
@@ -132,6 +146,7 @@ class RemoteFontFaceSource final : public CSSFontFaceSource,
   DisplayPeriod period_;
   FontLoadHistograms histograms_;
   bool is_intervention_triggered_;
+  bool finished_before_document_rendering_begin_;
 };
 
 }  // namespace blink

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

@@ -111,9 +111,6 @@ void FontPreloadManager::WillBeginRendering() {
 
   state_ = State::kUnblocked;
   finish_observers_.clear();
-
-  // TODO(xiaochengh): Mark all 'font-display: optional' fonts that are still
-  // loading as failure.
 }
 
 void FontPreloadManager::FontPreloadingDelaysRenderingTimerFired(TimerBase*) {

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

@@ -31,6 +31,7 @@ class CORE_EXPORT FontPreloadManager final {
 
   bool HasPendingRenderBlockingFonts() const;
   void WillBeginRendering();
+  bool RenderingHasBegun() const { return state_ == State::kUnblocked; }
 
   void FontPreloadingStarted(FontResource*);
   void FontPreloadingFinished(FontResource*, ResourceFinishObserver*);

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

@@ -6,7 +6,11 @@
 
 #include "base/test/scoped_feature_list.h"
 #include "third_party/blink/public/common/features.h"
+#include "third_party/blink/renderer/core/dom/element.h"
 #include "third_party/blink/renderer/core/frame/web_local_frame_impl.h"
+#include "third_party/blink/renderer/core/html/html_head_element.h"
+#include "third_party/blink/renderer/core/layout/layout_object.h"
+#include "third_party/blink/renderer/core/style/computed_style.h"
 #include "third_party/blink/renderer/core/testing/sim/sim_request.h"
 #include "third_party/blink/renderer/core/testing/sim/sim_test.h"
 #include "third_party/blink/renderer/platform/testing/unit_test_helpers.h"
@@ -21,6 +25,11 @@ class FontPreloadManagerTest : public SimTest {
     SimTest::SetUp();
   }
 
+  static Vector<char> ReadAhemWoff2() {
+    return test::ReadFromFile(test::CoreTestDataPath("Ahem.woff2"))
+        ->CopyAs<Vector<char>>();
+  }
+
  protected:
   FontPreloadManager& GetFontPreloadManager() {
     return GetDocument().GetFontPreloadManager();
@@ -29,6 +38,12 @@ class FontPreloadManagerTest : public SimTest {
   using State = FontPreloadManager::State;
   State GetState() { return GetFontPreloadManager().state_; }
 
+  Element* GetTarget() { return GetDocument().getElementById("target"); }
+
+  const Font& GetTargetFont() {
+    return GetTarget()->GetLayoutObject()->Style()->GetFont();
+  }
+
  private:
   base::test::ScopedFeatureList scoped_feature_list_;
 };
@@ -170,4 +185,202 @@ TEST_F(FontPreloadManagerTest, SlowFontTimeoutAfterBody) {
   font_resource.Finish();
 }
 
+// A trivial test case to verify test setup
+TEST_F(FontPreloadManagerTest, RegularWebFont) {
+  SimRequest main_resource("https://example.com", "text/html");
+  SimRequest font_resource("https://example.com/Ahem.woff2", "font/woff2");
+
+  LoadURL("https://example.com");
+  main_resource.Complete(R"HTML(
+    <!doctype html>
+    <style>
+      @font-face {
+        font-family: custom-font;
+        src: url(https://example.com/Ahem.woff2) format("woff2");
+      }
+      #target {
+        font: 25px/1 custom-font, monospace;
+      }
+    </style>
+    <span id=target style="position:relative">0123456789</span>
+  )HTML");
+
+  // Now rendering has started, as there's no blocking resources.
+  EXPECT_FALSE(Compositor().DeferMainFrameUpdate());
+  EXPECT_EQ(State::kUnblocked, GetState());
+
+  font_resource.Complete(ReadAhemWoff2());
+
+  // Now everything is loaded. The web font should be used in rendering.
+  Compositor().BeginFrame().DrawCount();
+  EXPECT_EQ(250, GetTarget()->OffsetWidth());
+  EXPECT_FALSE(GetTargetFont().ShouldSkipDrawing());
+}
+
+TEST_F(FontPreloadManagerTest, OptionalFontWithoutPreloading) {
+  SimRequest main_resource("https://example.com", "text/html");
+  SimRequest font_resource("https://example.com/Ahem.woff2", "font/woff2");
+
+  LoadURL("https://example.com");
+  main_resource.Complete(R"HTML(
+    <!doctype html>
+    <style>
+      @font-face {
+        font-family: custom-font;
+        src: url(https://example.com/Ahem.woff2) format("woff2");
+        font-display: optional;
+      }
+      #target {
+        font: 25px/1 custom-font, monospace;
+      }
+    </style>
+    <span id=target>0123456789</span>
+  )HTML");
+
+  // Now rendering has started, as there's no blocking resources.
+  EXPECT_FALSE(Compositor().DeferMainFrameUpdate());
+  EXPECT_EQ(State::kUnblocked, GetState());
+
+  font_resource.Complete(ReadAhemWoff2());
+
+  // The 'optional' web font isn't used, as it didn't finish loading before
+  // rendering started. Text is rendered in visible fallback.
+  Compositor().BeginFrame().Contains(SimCanvas::kText);
+  EXPECT_GT(250, GetTarget()->OffsetWidth());
+  EXPECT_FALSE(GetTargetFont().ShouldSkipDrawing());
+}
+
+TEST_F(FontPreloadManagerTest, OptionalFontRemoveAndReadd) {
+  SimRequest main_resource("https://example.com", "text/html");
+  SimRequest font_resource("https://example.com/Ahem.woff2", "font/woff2");
+
+  LoadURL("https://example.com");
+  main_resource.Complete(R"HTML(
+    <!doctype html>
+    <style>
+      @font-face {
+        font-family: custom-font;
+        src: url(https://example.com/Ahem.woff2) format("woff2");
+        font-display: optional;
+      }
+      #target {
+        font: 25px/1 custom-font, monospace;
+      }
+    </style>
+    <span id=target>0123456789</span>
+  )HTML");
+
+  // Now rendering has started, as there's no blocking resources.
+  EXPECT_FALSE(Compositor().DeferMainFrameUpdate());
+  EXPECT_EQ(State::kUnblocked, GetState());
+
+  // The 'optional' web font isn't used, as it didn't finish loading before
+  // rendering started. Text is rendered in visible fallback.
+  Compositor().BeginFrame().Contains(SimCanvas::kText);
+  EXPECT_GT(250, GetTarget()->OffsetWidth());
+  EXPECT_FALSE(GetTargetFont().ShouldSkipDrawing());
+
+  font_resource.Complete(ReadAhemWoff2());
+
+  Element* style = GetDocument().QuerySelector("style");
+  style->remove();
+  GetDocument().head()->appendChild(style);
+
+  // After removing and readding the style sheet, we've created a new font face
+  // that got loaded immediately from the memory cache. So it can be used.
+  Compositor().BeginFrame().Contains(SimCanvas::kText);
+  EXPECT_EQ(250, GetTarget()->OffsetWidth());
+  EXPECT_FALSE(GetTargetFont().ShouldSkipDrawing());
+}
+
+TEST_F(FontPreloadManagerTest, OptionalFontSlowPreloading) {
+  SimRequest main_resource("https://example.com", "text/html");
+  SimRequest font_resource("https://example.com/Ahem.woff2", "font/woff2");
+
+  LoadURL("https://example.com");
+  main_resource.Complete(R"HTML(
+    <!doctype html>
+    <link rel="preload" as="font" type="font/woff2"
+          href="https://example.com/Ahem.woff2" crossorigin>
+    <style>
+      @font-face {
+        font-family: custom-font;
+        src: url(https://example.com/Ahem.woff2) format("woff2");
+        font-display: optional;
+      }
+      #target {
+        font: 25px/1 custom-font, monospace;
+      }
+    </style>
+    <span id=target>0123456789</span>
+  )HTML");
+
+  // Rendering is blocked due to font being preloaded.
+  EXPECT_TRUE(Compositor().DeferMainFrameUpdate());
+  EXPECT_TRUE(GetFontPreloadManager().HasPendingRenderBlockingFonts());
+  EXPECT_EQ(State::kLoading, GetState());
+
+  GetFontPreloadManager().FontPreloadingDelaysRenderingTimerFired(nullptr);
+
+  // Rendering is unblocked after the font preloading has timed out.
+  EXPECT_FALSE(Compositor().DeferMainFrameUpdate());
+  EXPECT_FALSE(GetFontPreloadManager().HasPendingRenderBlockingFonts());
+  EXPECT_EQ(State::kUnblocked, GetState());
+
+  // First frame renders text with visible fallback, as the 'optional' web font
+  // isn't loaded yet, and should be treated as in the failure period.
+  Compositor().BeginFrame();
+  EXPECT_GT(250, GetTarget()->OffsetWidth());
+  EXPECT_FALSE(GetTargetFont().ShouldSkipDrawing());
+
+  font_resource.Complete(ReadAhemWoff2());
+
+  // The 'optional' web font should not cause relayout even if it finishes
+  // loading now.
+  Compositor().BeginFrame();
+  EXPECT_GT(250, GetTarget()->OffsetWidth());
+  EXPECT_FALSE(GetTargetFont().ShouldSkipDrawing());
+}
+
+TEST_F(FontPreloadManagerTest, OptionalFontFastPreloading) {
+  SimRequest main_resource("https://example.com", "text/html");
+  SimRequest font_resource("https://example.com/Ahem.woff2", "font/woff2");
+
+  LoadURL("https://example.com");
+  main_resource.Complete(R"HTML(
+    <!doctype html>
+    <link rel="preload" as="font" type="font/woff2"
+          href="https://example.com/Ahem.woff2" crossorigin>
+    <style>
+      @font-face {
+        font-family: custom-font;
+        src: url(https://example.com/Ahem.woff2) format("woff2");
+        font-display: optional;
+      }
+      #target {
+        font: 25px/1 custom-font, monospace;
+      }
+    </style>
+    <span id=target>0123456789</span>
+  )HTML");
+
+  // Rendering is blocked due to font being preloaded.
+  EXPECT_TRUE(Compositor().DeferMainFrameUpdate());
+  EXPECT_TRUE(GetFontPreloadManager().HasPendingRenderBlockingFonts());
+  EXPECT_EQ(State::kLoading, GetState());
+
+  font_resource.Complete(ReadAhemWoff2());
+  test::RunPendingTasks();
+
+  // Rendering is unblocked after the font is preloaded.
+  EXPECT_FALSE(Compositor().DeferMainFrameUpdate());
+  EXPECT_FALSE(GetFontPreloadManager().HasPendingRenderBlockingFonts());
+  EXPECT_EQ(State::kUnblocked, GetState());
+
+  // The 'optional' web font should be used in the first paint.
+  Compositor().BeginFrame();
+  EXPECT_EQ(250, GetTarget()->OffsetWidth());
+  EXPECT_FALSE(GetTargetFont().ShouldSkipDrawing());
+}
+
 }  // namespace blink

third_party/blink/renderer/platform/loader/fetch/resource.cc

@@ -567,22 +567,23 @@ String Resource::ReasonNotDeletable() const {
   return builder.ToString();
 }
 
-void Resource::DidAddClient(ResourceClient* c) {
+void Resource::DidAddClient(ResourceClient* client) {
   if (scoped_refptr<SharedBuffer> data = Data()) {
     for (const auto& span : *data) {
-      c->DataReceived(this, span.data(), span.size());
+      client->DataReceived(this, span.data(), span.size());
       // Stop pushing data if the client removed itself.
-      if (!HasClient(c))
+      if (!HasClient(client))
         break;
     }
   }
-  if (!HasClient(c))
+  if (!HasClient(client))
     return;
   if (IsFinishedInternal()) {
-    c->NotifyFinished(this);
-    if (clients_.Contains(c)) {
-      finished_clients_.insert(c);
-      clients_.erase(c);
+    client->SetHasFinishedFromMemoryCache();
+    client->NotifyFinished(this);
+    if (clients_.Contains(client)) {
+      finished_clients_.insert(client);
+      clients_.erase(client);
     }
   }
 }

third_party/blink/renderer/platform/loader/fetch/resource_client.h

@@ -64,6 +64,9 @@ class PLATFORM_EXPORT ResourceClient : public GarbageCollectedMixin {
 
   Resource* GetResource() const { return resource_; }
 
+  bool FinishedFromMemoryCache() const { return finished_from_memory_cache_; }
+  void SetHasFinishedFromMemoryCache() { finished_from_memory_cache_ = true; }
+
   // Name for debugging, e.g. shown in memory-infra.
   virtual String DebugName() const = 0;
 
@@ -86,6 +89,10 @@ class PLATFORM_EXPORT ResourceClient : public GarbageCollectedMixin {
                    base::SingleThreadTaskRunner* task_runner);
 
   Member<Resource> resource_;
+
+  // If true, the Resource was already available from the memory cache when this
+  // ResourceClient was setup, so that the request finished immediately.
+  bool finished_from_memory_cache_ = false;
 };
 
 }  // namespace blink