WebFonts cache-aware timeout adaptation

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

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "core/css/RemoteFontFaceSource.h"
 
 #include "core/css/CSSCustomFontData.h"
 #include "core/css/CSSFontFace.h"
 #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"
 #include "platform/Histogram.h"
 #include "platform/RuntimeEnabledFeatures.h"
 #include "platform/fonts/FontCache.h"
 #include "platform/fonts/FontDescription.h"
 #include "platform/fonts/SimpleFontData.h"
 #include "public/platform/WebEffectiveConnectionType.h"
 #include "wtf/CurrentTime.h"
@@ skipping 27 matching lines @@
 }
 
 }  // namespace
 
 RemoteFontFaceSource::RemoteFontFaceSource(FontResource* font,
                                            CSSFontSelector* fontSelector,
                                            FontDisplay display)
     : 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
                                           : FontLoadHistograms::FromUnknown,
                    m_display),
       m_isInterventionTriggered(false) {
   ThreadState::current()->registerPreFinalizer(this);
   m_font->addClient(this);
 
   if (shouldTriggerWebFontsIntervention()) {
@@ skipping 84 matching lines @@
 
   if (m_display == FontDisplayBlock ||
       (!m_isInterventionTriggered && m_display == FontDisplayAuto))
     switchToSwapPeriod();
   else if (m_display == FontDisplayFallback)
     switchToFailurePeriod();
 
   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();
   if (m_face) {
     m_fontSelector->fontFaceInvalidated();
     m_face->didBecomeVisibleFallback(this);
   }
 
   m_histograms.recordFallbackTime(m_font.get());
 }
 
 void RemoteFontFaceSource::switchToFailurePeriod() {
   if (m_period == BlockPeriod)
     switchToSwapPeriod();
-  ASSERT(m_period == SwapPeriod);
+  DCHECK_EQ(m_period, SwapPeriod);
   m_period = FailurePeriod;
 }
 
 bool RemoteFontFaceSource::shouldTriggerWebFontsIntervention() {
   if (RuntimeEnabledFeatures::webFontsInterventionTriggerEnabled())
     return true;
   if (m_histograms.dataSource() == FontLoadHistograms::FromMemoryCache ||
       m_histograms.dataSource() == FontLoadHistograms::FromDataURL)
     return false;
 
@@ skipping 45 matching lines @@
 }
 
 void RemoteFontFaceSource::beginLoadIfNeeded() {
   if (m_fontSelector->document() && m_font->stillNeedsLoad()) {
     m_fontSelector->document()->fetcher()->startLoad(m_font);
     if (!m_font->isLoaded())
       m_font->startLoadLimitTimers();
     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();
 }
 
 DEFINE_TRACE(RemoteFontFaceSource) {
   visitor->trace(m_font);
   visitor->trace(m_fontSelector);
   CSSFontFaceSource::trace(visitor);
   FontResourceClient::trace(visitor);
 }
@@ skipping 196 matching lines @@
       return Miss;
     case FromUnknown:
     // Fall through.
     default:
       NOTREACHED();
   }
   return Miss;
 }
 
 }  // namespace blink