RemoteFontFaceSource should keep FontCustomPlatformData over FontResource revalidation

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 cf7ce8b81954bb9086d4bd25f1c01b05d2c89095..1ab1a6e9a30fb18a3358e04bfb9334f0f504e4cb 100644
--- a/third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp
+++ b/third_party/WebKit/Source/core/css/RemoteFontFaceSource.cpp
@@ -14,6 +14,7 @@
 #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"
@@ -79,8 +80,10 @@ RemoteFontFaceSource::RemoteFontFaceSource(FontResource* font,
 RemoteFontFaceSource::~RemoteFontFaceSource() {}
 
 void RemoteFontFaceSource::dispose() {
-  m_font->removeClient(this);
-  m_font = nullptr;
+  if (m_font) {
+    m_font->removeClient(this);
+    m_font = nullptr;
+  }
   pruneTable();
 }
 
@@ -97,18 +100,19 @@ void RemoteFontFaceSource::pruneTable() {
 }
 
 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);
@@ -117,7 +121,8 @@ void RemoteFontFaceSource::notifyFinished(Resource*) {
                           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 &&
@@ -131,6 +136,9 @@ void RemoteFontFaceSource::notifyFinished(Resource*) {
           "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();
@@ -139,7 +147,7 @@ void RemoteFontFaceSource::notifyFinished(Resource*) {
 }
 
 void RemoteFontFaceSource::fontLoadShortLimitExceeded(FontResource*) {
-  if (m_font->isLoaded())
+  if (isLoaded())
     return;
 
   if (m_display == FontDisplayFallback)
@@ -149,7 +157,7 @@ void RemoteFontFaceSource::fontLoadShortLimitExceeded(FontResource*) {
 }
 
 void RemoteFontFaceSource::fontLoadLongLimitExceeded(FontResource*) {
-  if (m_font->isLoaded())
+  if (isLoaded())
     return;
 
   if (m_display == FontDisplayBlock ||
@@ -171,7 +179,7 @@ void RemoteFontFaceSource::switchToSwapPeriod() {
     m_face->didBecomeVisibleFallback(this);
   }
 
-  m_histograms.recordFallbackTime(m_font.get());
+  m_histograms.recordFallbackTime();
 }
 
 void RemoteFontFaceSource::switchToFailurePeriod() {
@@ -206,16 +214,16 @@ bool RemoteFontFaceSource::isLowPriorityLoadingAllowedForRemoteFont() const {
 
 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(),
@@ -241,6 +249,10 @@ PassRefPtr<SimpleFontData> RemoteFontFaceSource::createLoadingFallbackFontData(
 }
 
 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 &&
@@ -297,8 +309,7 @@ void RemoteFontFaceSource::FontLoadHistograms::longLimitExceeded(
     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);