Bug 66153 – Abandoned Memory: Temporary CSS Fonts May Never Be Purged

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Joseph Pecoraro]

For CSS Fonts I'm seeing a null FontPlatformData entry inside of gFontPlatformDataCache,
which can never be purged. In this case, the hash key contains the dataURI for the
CSS Font, so this can be a rather large leak depending on the size of the dataURI.

Also, mitz pointed out that the temporary FontData itself is not being "released"
after a previous change. Better to not put the "temporary" font into the cache at all.

Patch to follow.

[Joseph Pecoraro]

Created attachment 103796 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

I'm open to splitting this up to separate patches. Also, instead of exposing the enum maybe
I should create a new FontCache function getNonRetainedCachedFont which internally send
the DoNotRetain.

Comment on attachment 103796 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=103796&action=review

> Source/WebCore/css/CSSFontFaceSource.cpp:191
> -        SimpleFontData* tempData = fontCache()->getCachedFontData(fontDescription, m_string);
> +        SimpleFontData* tempData = fontCache()->getCachedFontData(fontDescription, m_string, false, FontCache::DoNotRetain);
>          if (!tempData)
>              tempData = fontCache()->getLastResortFallbackFont(fontDescription);

This is not an ideal fix, since it still creates a cache entry. If we can’t (why?) go back to what this code looked like before, i.e. just getting a FontPlatformData from the cache and creating a SimpleFontData here, maybe we should add a FontCache function that takes a fontDescription and a family name (or not, given the FIXME) and returns a newly-created, non-cached SimpleFontData.

> Source/WebCore/platform/graphics/FontCache.cpp:200
>          result = createFontPlatformData(fontDescription, familyName);
> -        gFontPlatformDataCache->set(key, result);
> +        if (result)
> +            gFontPlatformDataCache->set(key, result);
>          foundResult = result;

This looks wrong. We want to cache the fact that there is no platform font for the description and family name we were passed, so that we don’t have to call createFontPlatformData() over and over.

[Joseph Pecoraro]

> > Source/WebCore/css/CSSFontFaceSource.cpp:191
> > -        SimpleFontData* tempData = fontCache()->getCachedFontData(fontDescription, m_string);
> > +        SimpleFontData* tempData = fontCache()->getCachedFontData(fontDescription, m_string, false, FontCache::DoNotRetain);
> >          if (!tempData)
> >              tempData = fontCache()->getLastResortFallbackFont(fontDescription);
> 
> This is not an ideal fix, since it still creates a cache entry. If we can’t (why?) go back to what this code looked
> like before, i.e. just getting a FontPlatformData from the cache and creating a SimpleFontData here, maybe
> we should add a FontCache function that takes a fontDescription and a family name (or not, given the FIXME)
> and returns a newly-created, non-cached SimpleFontData.

This sounds like what getLastResortFallbackFont does. Can we just remove the getCachedFontData()
completely here? It looks to me like this temporary font case only ever happens:

  • for remote fonts (have a CachedFont that !isLoaded)
  • when m_string is a complete URL

So would we ever expect getCachedFontPlatformData to return something valid for a URI?
Maybe I'm missing where this cache entry could be overwritten later on.

[Joseph Pecoraro]

> > Source/WebCore/platform/graphics/FontCache.cpp:200
> >          result = createFontPlatformData(fontDescription, familyName);
> > -        gFontPlatformDataCache->set(key, result);
> > +        if (result)
> > +            gFontPlatformDataCache->set(key, result);
> >          foundResult = result;
> 
> This looks wrong. We want to cache the fact that there is no platform font for the
> description and family name we were passed, so that we don’t have to call
> createFontPlatformData() over and over.

Gotcha that makes sense. In this case it was bad because the HashKey for
gFontPlatformDataCache contained a very large string which we would never
release. This sounds like a case of unbounded growth but if we can avoid
creating such entries entirely we should be fine for the most part. We can
revisit possibly pruning the "0x0" platform font data's in this list for
another time.

[Joseph Pecoraro]

> Can we just remove the getCachedFontData() completely here?
> So would we ever expect getCachedFontPlatformData to return something valid for a URI?

I guess we can try passing the lastPathComponent of this URL.

(In reply to comment #4)
> > > Source/WebCore/css/CSSFontFaceSource.cpp:191
> > > -        SimpleFontData* tempData = fontCache()->getCachedFontData(fontDescription, m_string);
> > > +        SimpleFontData* tempData = fontCache()->getCachedFontData(fontDescription, m_string, false, FontCache::DoNotRetain);
> > >          if (!tempData)
> > >              tempData = fontCache()->getLastResortFallbackFont(fontDescription);
> > 
> > This is not an ideal fix, since it still creates a cache entry. If we can’t (why?) go back to what this code looked
> > like before, i.e. just getting a FontPlatformData from the cache and creating a SimpleFontData here, maybe
> > we should add a FontCache function that takes a fontDescription and a family name (or not, given the FIXME)
> > and returns a newly-created, non-cached SimpleFontData.
> 
> This sounds like what getLastResortFallbackFont does. Can we just remove the getCachedFontData()
> completely here? It looks to me like this temporary font case only ever happens:
> 
>   • for remote fonts (have a CachedFont that !isLoaded)
>   • when m_string is a complete URL
> 
> So would we ever expect getCachedFontPlatformData to return something valid for a URI?
> Maybe I'm missing where this cache entry could be overwritten later on.

I think you’re right and that first call to the font cache practically always returns 0 (unless you happen to have a local font whose name is equal to the URI), but at the cost of creating an uninteresting negative cache entry. So the right thing to do would be to just remove it.

[Joseph Pecoraro]

Created attachment 103843 [details]
[PATCH] Take 2

[WebKit Review Bot]

Comment on attachment 103843 [details]
[PATCH] Take 2

Clearing flags on attachment: 103843

Committed r93011: <http://trac.webkit.org/changeset/93011>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

> > we should add a FontCache function that takes a fontDescription and a family name (or not, given the FIXME)
> > and returns a newly-created, non-cached SimpleFontData.
> 
> This sounds like what getLastResortFallbackFont does.

Actually I was incorrect. getLastResortFallbackFont does not return an uncached font.
Threading a ShouldRetain parameter through all the ports is ugly too. Although this
patch was an improvement, because this retains it actually didn't entirely fix the
original problem.

[Joseph Pecoraro]

Created attachment 104004 [details]
[PATCH] Add FontCache::getNonRetainedLastResortFallbackFont(...)

This goes with mitz's suggestion. Its messy though =(.

[Joseph Pecoraro]

Reopening because the original problem, although improved, was not entirely fixed.

[WebKit Review Bot]

Comment on attachment 104004 [details]
[PATCH] Add FontCache::getNonRetainedLastResortFallbackFont(...)

Attachment 104004 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9379968

[Early Warning System Bot]

Comment on attachment 104004 [details]
[PATCH] Add FontCache::getNonRetainedLastResortFallbackFont(...)

Attachment 104004 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9401083

[Joseph Pecoraro]

Created attachment 104006 [details]
[PATCH] Fix Builds

[WebKit Review Bot]

Comment on attachment 104006 [details]
[PATCH] Fix Builds

Clearing flags on attachment: 104006

Committed r93140: <http://trac.webkit.org/changeset/93140>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

Created attachment 104079 [details]
[PATCH] Attempt at Windows Build Fix

[WebKit Review Bot]

Attachment 104079 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/platform/graphics/FontCache.h:102:  The parameter name "font" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/FontCache.h:104:  The parameter name "font" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Attempted a Windows Build Fix in: <http://trac.webkit.org/changeset/93147>

[Joseph Pecoraro]

Attempted a Chromium Windows Build Fix in: <http://trac.webkit.org/changeset/93149>

[Joseph Pecoraro]

Grr. I'm going to try just making ShouldRetain public and if that fixes the
issue file a follow-up bug to make it private again or mark the Chromium
GetLastResortFallbackFontProcData struct a friend so that it can use the enum.

[Joseph Pecoraro]

Took a while, but I was able to fix chromium-win:
<http://trac.webkit.org/changeset/93151>
<http://trac.webkit.org/changeset/93156>
<http://trac.webkit.org/changeset/93160>

Filing a bug to cleanup after these changes.