WebFonts cache-aware timeout adaptation

third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp

Index: third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp
diff --git a/third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp b/third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp
index e6f9a8c7845dd7d7ec5e0fb09577a04afb6d853a..dc8bd927db8da26489066f8ae8820e353d71089f 100644
--- a/third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp
+++ b/third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp
@@ -9,6 +9,7 @@
 #include "core/css/CSSFontSelector.h"
 #include "core/dom/Document.h"
 #include "core/fetch/ResourceFetcher.h"
+#include "core/fetch/ResourceLoader.h"
 #include "core/inspector/ConsoleMessage.h"
 #include "core/loader/FrameLoaderClient.h"
 #include "core/page/NetworkStateNotifier.h"
@@ -56,7 +57,7 @@ RemoteFontFaceSource::RemoteFontFaceSource(FontResource* font,
     : m_font(font),
       m_fontSelector(fontSelector),
       m_display(display),
-      m_period(display == FontDisplaySwap ? SwapPeriod : BlockPeriod),
+      m_period(BlockPeriod),
       m_histograms(font->url().protocolIsData()
                        ? FontLoadHistograms::FromDataURL
                        : font->isLoaded() ? FontLoadHistograms::FromMemoryCache
@@ -161,8 +162,15 @@ void RemoteFontFaceSource::fontLoadLongLimitExceeded(FontResource*) {
   m_histograms.longLimitExceeded(m_isInterventionTriggered);
 }
 
+void RemoteFontFaceSource::willReloadAfterDiskCacheMiss(const FontResource*) {
+  DCHECK(m_font->isLoading());
+  DCHECK(m_font->loader()->isCacheAwareLoadingActivated());
+  if (m_display == FontDisplaySwap)
+    switchToSwapPeriod();
+}
+
 void RemoteFontFaceSource::switchToSwapPeriod() {
-  ASSERT(m_period == BlockPeriod);
+  DCHECK_EQ(m_period, BlockPeriod);
   m_period = SwapPeriod;
 
   pruneTable();
@@ -177,7 +185,7 @@ void RemoteFontFaceSource::switchToSwapPeriod() {
 void RemoteFontFaceSource::switchToFailurePeriod() {
   if (m_period == BlockPeriod)
     switchToSwapPeriod();
-  ASSERT(m_period == SwapPeriod);
+  DCHECK_EQ(m_period, SwapPeriod);
   m_period = FailurePeriod;
 }
 
@@ -243,6 +251,15 @@ void RemoteFontFaceSource::beginLoadIfNeeded() {
     m_histograms.loadStarted();
   }
 
+  if (m_display == FontDisplaySwap && m_font->isLoading() &&
+      !m_font->loader()->isCacheAwareLoadingActivated()) {
+    // |m_period| might be modified if font load timeout callbacks are called
+    // during |m_font->addClient()| in constructor. Currently this should not
+    // happen with |m_display == FontDisplaySwap|.
+    DCHECK_EQ(m_period, BlockPeriod);
+    m_period = SwapPeriod;

[Kunihiko Sakamoto, 2016/11/02 03:46:36]

Does this mean the text is displayed with fallback font, even before the disk
cache miss fallback?

In general, changing m_period without calling switchTo*Period() is a bad idea.
RemoteFontFaceSource::createLoadingFallbackFontData() returns visible fallback
after this point (because m_period is SwapPeriod), but the layout engine may
have references to invisible fallback FontData created before. It would result
in a bug where font is sometimes displayed, sometimes not. switchTo*Period()
invalidates existing FontData to prevent that.

[Shao-Chuan Lee, 2016/11/02 04:22:40]

In this case CAL is not activated and disk cache miss callback will not be
called, so we fallback to original behavior. Activation is done in startLoad()
so we have to check here.

The problem of calling switchToSwapPeriod() here is this code is reached during
style recalc, and by invalidating font face the style recalc flag is set again
and causes DCHECK fail in Document::updateStyle() at
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Docum...
Though I'm indeed assuming create*FontData() is always called after this...

[Kunihiko Sakamoto, 2016/11/02 05:49:16]

Got it, but...
I'm afraid that this code implicitly relies on current callback order etc. too
much, and hard to maintain in long term.

Could you explain why your approach in PS4 (set m_period to SwapPeriod in the
constructor) doesn't work?

[Shao-Chuan Lee, 2016/11/02 06:24:10]

The original design of cache-aware loading is to call disk cache miss callback
immediately in startLoad() if CAL is enabled but cannot be activated. Now the
callback is only called on real cache misses.

Original design leads to the same problem I mentioned by calling startLoad() ->
willReloadAfterDiskCacheMiss() -> switchToSwapPeriod(). Having checks in the
callback to not invalidate font face in such cases will be equivalent to current
approach.

Is it possible to defer switchToSwapPeriod() until style recalc is finished,
e.g. posting to TaskRunner?

[Kunihiko Sakamoto, 2016/11/02 07:46:42]

Let me check if my understanding is correct...

- We cannot set m_period to Swap in the constructor because
isCacheAwareLoadingActivate() can only be used after startLoad(), and
- We cannot call switchToSwapPeriod() here, because the font invalidation may
cause nested style recalc.

are these correct?
I feel this is a fragile assumption. Can you add DCHECK to ensure that?
That's not ideal. We should avoid running expensive style recalc twice.

+  }
+
   if (m_face)
     m_face->didBeginLoad();
 }