RemoteFontFaceSource should keep FontCustomPlatformData over FontResource revalidation

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/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/FontCustomPlatformData.h"
 #include "platform/fonts/FontDescription.h"
 #include "platform/fonts/SimpleFontData.h"
 #include "platform/loader/fetch/ResourceFetcher.h"
 #include "platform/network/ResourceLoadPriority.h"
 #include "public/platform/WebEffectiveConnectionType.h"
 #include "wtf/CurrentTime.h"
 
 namespace blink {
 
 namespace {
@@ skipping 45 matching lines @@
     m_fontSelector->document()->addConsoleMessage(ConsoleMessage::create(
         OtherMessageSource, InfoMessageLevel,
         "Slow network is detected. Fallback font will be used while loading: " +
             m_font->url().elidedString()));
   }
 }
 
 RemoteFontFaceSource::~RemoteFontFaceSource() {}
 
 void RemoteFontFaceSource::dispose() {
-  m_font->removeClient(this);
-  m_font = nullptr;
+  if (m_font) {
+    m_font->removeClient(this);
+    m_font = nullptr;
+  }
   pruneTable();
 }
 
 void RemoteFontFaceSource::pruneTable() {
   if (m_fontDataTable.isEmpty())
     return;
 
   for (const auto& item : m_fontDataTable) {
     SimpleFontData* fontData = item.value.get();
     if (fontData && fontData->customFontData())
       fontData->customFontData()->clearFontFaceSource();
   }
   m_fontDataTable.clear();
 }
 
 bool RemoteFontFaceSource::isLoading() const {
-  return m_font->isLoading();
+  return m_font && m_font->isLoading();
 }
 
 bool RemoteFontFaceSource::isLoaded() const {
-  return m_font->isLoaded();
+  return !m_font;
 }
 
 bool RemoteFontFaceSource::isValid() const {
-  return !m_font->errorOccurred();
+  return m_font || m_customFontData;
 }
 
-void RemoteFontFaceSource::notifyFinished(Resource*) {
+void RemoteFontFaceSource::notifyFinished(Resource* unusedResource) {
+  DCHECK_EQ(unusedResource, m_font);
   m_histograms.maySetDataSource(m_font->response().wasCached()
                                     ? FontLoadHistograms::FromDiskCache
                                     : FontLoadHistograms::FromNetwork);
   m_histograms.recordRemoteFont(m_font.get(), m_isInterventionTriggered);
   m_histograms.fontLoaded(m_font->isCORSFailed(),
                           m_font->getStatus() == ResourceStatus::LoadError,
                           m_isInterventionTriggered);
 
-  m_font->ensureCustomFontData();
+  m_customFontData = m_font->getCustomFontData();
+
   // FIXME: Provide more useful message such as OTS rejection reason.
   // See crbug.com/97467
   if (m_font->getStatus() == ResourceStatus::DecodeError &&
       m_fontSelector->document()) {
     m_fontSelector->document()->addConsoleMessage(ConsoleMessage::create(
         OtherMessageSource, WarningMessageLevel,
         "Failed to decode downloaded font: " + m_font->url().elidedString()));
     if (m_font->otsParsingMessage().length() > 1)
       m_fontSelector->document()->addConsoleMessage(ConsoleMessage::create(
           OtherMessageSource, WarningMessageLevel,
           "OTS parsing error: " + m_font->otsParsingMessage()));
   }
 
+  m_font->removeClient(this);
+  m_font = nullptr;

[hiroshige, 2017/02/28 00:18:28]

I love clearing |m_font| here, but clearing |m_font| might cause FontResource to
be garbage collected (if anyone else has strong references to FontResource).
From the aspect of users,
"@font-face { font-family: ahem; src: url('foo.otf'); }
previously kept the Font 'foo.otf' alive on MemoryCache, while after this CL it
doesn't, thus calling addFontFaceRule() in the layout test after Oilpan GC might
result in a new FontResource and duplicated FontCustomPlatformData objects.

WDYT about this behavior change?

[Kunihiko Sakamoto, 2017/02/28 04:04:02]

Good point.

For @font-face fonts, CSSFontFaceSrcValue also holds a reference to
FontResource, so it won't be GC-ed as long as stylesheet is alive. But that can
happen to font faces created by scripts, using new FontFace(...).

We are monitoring memory cache hit rate for webfonts. I would like to go ahead
with this change, and see if it affects the cache hit rate. (I doubt it as usage
of new FontFace() is low)

[Takashi Toyoshima, 2017/03/01 06:44:07]

https://www.chromestatus.com/metrics/feature/popularity#CSSAtRuleFontFace
https://www.chromestatus.com/metrics/feature/popularity#FontFaceConstructor

@font-face is used in 7.7%, and FontFace is in 0.6%.

If this could affect, we may want to consider having a reference in FontFaceSet
when FontFace is added?

But, I agreed that we can go as is now and see the metrics changes.

+
   pruneTable();
   if (m_face) {
     m_fontSelector->fontFaceInvalidated();
     m_face->fontLoaded(this);
   }
 }
 
 void RemoteFontFaceSource::fontLoadShortLimitExceeded(FontResource*) {
-  if (m_font->isLoaded())
+  if (isLoaded())
     return;
 
   if (m_display == FontDisplayFallback)
     switchToSwapPeriod();
   else if (m_display == FontDisplayOptional)
     switchToFailurePeriod();
 }
 
 void RemoteFontFaceSource::fontLoadLongLimitExceeded(FontResource*) {
-  if (m_font->isLoaded())
+  if (isLoaded())
     return;
 
   if (m_display == FontDisplayBlock ||
       (!m_isInterventionTriggered && m_display == FontDisplayAuto))
     switchToSwapPeriod();
   else if (m_display == FontDisplayFallback)
     switchToFailurePeriod();
 
   m_histograms.longLimitExceeded(m_isInterventionTriggered);
 }
 
 void RemoteFontFaceSource::switchToSwapPeriod() {
   ASSERT(m_period == BlockPeriod);
   m_period = SwapPeriod;
 
   pruneTable();
   if (m_face) {
     m_fontSelector->fontFaceInvalidated();
     m_face->didBecomeVisibleFallback(this);
   }
 
-  m_histograms.recordFallbackTime(m_font.get());
+  m_histograms.recordFallbackTime();
 }
 
 void RemoteFontFaceSource::switchToFailurePeriod() {
   if (m_period == BlockPeriod)
     switchToSwapPeriod();
   ASSERT(m_period == SwapPeriod);
   m_period = FailurePeriod;
 }
 
 bool RemoteFontFaceSource::shouldTriggerWebFontsIntervention() {
@@ skipping 14 matching lines @@
 
   return networkIsSlow && m_display == FontDisplayAuto;
 }
 
 bool RemoteFontFaceSource::isLowPriorityLoadingAllowedForRemoteFont() const {
   return m_isInterventionTriggered;
 }
 
 PassRefPtr<SimpleFontData> RemoteFontFaceSource::createFontData(
     const FontDescription& fontDescription) {
+  if (m_period == FailurePeriod || !isValid())
+    return nullptr;
   if (!isLoaded())
     return createLoadingFallbackFontData(fontDescription);
+  DCHECK(m_customFontData);
 
-  if (!m_font->ensureCustomFontData() || m_period == FailurePeriod)
-    return nullptr;
-
-  m_histograms.recordFallbackTime(m_font.get());
+  m_histograms.recordFallbackTime();
 
   return SimpleFontData::create(
-      m_font->platformDataFromCustomData(fontDescription.effectiveFontSize(),
+      m_customFontData->fontPlatformData(fontDescription.effectiveFontSize(),
                                          fontDescription.isSyntheticBold(),
                                          fontDescription.isSyntheticItalic(),
                                          fontDescription.orientation(),
                                          fontDescription.variationSettings()),
       CustomFontData::create());
 }
 
 PassRefPtr<SimpleFontData> RemoteFontFaceSource::createLoadingFallbackFontData(
     const FontDescription& fontDescription) {
   // This temporary font is not retained and should not be returned.
   FontCachePurgePreventer fontCachePurgePreventer;
   SimpleFontData* temporaryFont =
       FontCache::fontCache()->getNonRetainedLastResortFallbackFont(
           fontDescription);
   if (!temporaryFont) {
     ASSERT_NOT_REACHED();
     return nullptr;
   }
   RefPtr<CSSCustomFontData> cssFontData = CSSCustomFontData::create(
       this, m_period == BlockPeriod ? CSSCustomFontData::InvisibleFallback
                                     : CSSCustomFontData::VisibleFallback);
   return SimpleFontData::create(temporaryFont->platformData(), cssFontData);
 }
 
 void RemoteFontFaceSource::beginLoadIfNeeded() {
+  if (isLoaded())

[Takashi Toyoshima, 2017/02/27 12:56:02]

Does this short-circuit affect our metrics?

[Kunihiko Sakamoto, 2017/02/28 04:04:01]

You mean m_histograms.loadStarted() at line 268? That will not be affected
because m_font->stillNeedsLoad() at line 256 cannot be true.

[Takashi Toyoshima, 2017/03/01 06:44:07]

I see. Thanks!

+    return;
+  DCHECK(m_font);
+
   if (m_fontSelector->document() && m_font->stillNeedsLoad()) {
     if (!m_font->url().protocolIsData() && !m_font->isLoaded() &&
         m_display == FontDisplayAuto &&
         m_font->isLowPriorityLoadingAllowedForRemoteFont()) {
       // Set the loading priority to VeryLow since this font is not required
       // for painting the text.
       m_font->didChangePriority(ResourceLoadPriorityVeryLow, 0);
     }
     if (m_fontSelector->document()->fetcher()->startLoad(m_font)) {
       // Start timers only when load is actually started asynchronously.
@@ skipping 36 matching lines @@
 }
 
 void RemoteFontFaceSource::FontLoadHistograms::longLimitExceeded(
     bool isInterventionTriggered) {
   m_isLongLimitExceeded = true;
   maySetDataSource(FromNetwork);
   if (m_fontDisplay == FontDisplayAuto)
     recordInterventionResult(isInterventionTriggered);
 }
 
-void RemoteFontFaceSource::FontLoadHistograms::recordFallbackTime(
-    const FontResource* font) {
+void RemoteFontFaceSource::FontLoadHistograms::recordFallbackTime() {
   if (m_blankPaintTime <= 0)
     return;
   int duration = static_cast<int>(currentTimeMS() - m_blankPaintTime);
   DEFINE_STATIC_LOCAL(CustomCountHistogram, blankTextShownTimeHistogram,
                       ("WebFont.BlankTextShownTime", 0, 10000, 50));
   blankTextShownTimeHistogram.count(duration);
   m_blankPaintTime = -1;
 }
 
 void RemoteFontFaceSource::FontLoadHistograms::recordRemoteFont(
@@ skipping 154 matching lines @@
       return Miss;
     case FromUnknown:
     // Fall through.
     default:
       NOTREACHED();
   }
   return Miss;
 }
 
 }  // namespace blink