Bug 66153 - Abandoned Memory: Temporary CSS Fonts May Never Be Purged
Summary: Abandoned Memory: Temporary CSS Fonts May Never Be Purged
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-08-12 12:19 PDT by Joseph Pecoraro
Modified: 2011-08-16 15:03 PDT (History)
6 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (3.29 KB, patch)
2011-08-12 12:41 PDT, Joseph Pecoraro
mitz: review-
Details | Formatted Diff | Diff
[PATCH] Take 2 (1.76 KB, patch)
2011-08-12 16:50 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Add FontCache::getNonRetainedLastResortFallbackFont(...) (25.50 KB, patch)
2011-08-15 22:13 PDT, Joseph Pecoraro
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
[PATCH] Fix Builds (24.91 KB, patch)
2011-08-15 22:28 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Attempt at Windows Build Fix (9.54 KB, patch)
2011-08-16 13:03 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2011-08-12 12:19:18 PDT
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.
Comment 1 Joseph Pecoraro 2011-08-12 12:41:33 PDT
Created attachment 103796 [details]
[PATCH] Proposed Fix
Comment 2 Joseph Pecoraro 2011-08-12 12:43:50 PDT
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 3 mitz 2011-08-12 13:25:36 PDT
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.
Comment 4 Joseph Pecoraro 2011-08-12 14:09:49 PDT
> > 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.
Comment 5 Joseph Pecoraro 2011-08-12 14:14:35 PDT
> > 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.
Comment 6 Joseph Pecoraro 2011-08-12 16:01:55 PDT
> 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.
Comment 7 mitz 2011-08-12 16:38:47 PDT
(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.
Comment 8 Joseph Pecoraro 2011-08-12 16:50:39 PDT
Created attachment 103843 [details]
[PATCH] Take 2
Comment 9 WebKit Review Bot 2011-08-12 18:44:15 PDT
Comment on attachment 103843 [details]
[PATCH] Take 2

Clearing flags on attachment: 103843

Committed r93011: <http://trac.webkit.org/changeset/93011>
Comment 10 WebKit Review Bot 2011-08-12 18:44:20 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Joseph Pecoraro 2011-08-15 21:48:04 PDT
> > 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.
Comment 12 Joseph Pecoraro 2011-08-15 22:13:47 PDT
Created attachment 104004 [details]
[PATCH] Add FontCache::getNonRetainedLastResortFallbackFont(...)

This goes with mitz's suggestion. Its messy though =(.
Comment 13 Joseph Pecoraro 2011-08-15 22:14:24 PDT
Reopening because the original problem, although improved, was not entirely fixed.
Comment 14 WebKit Review Bot 2011-08-15 22:17:59 PDT
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
Comment 15 Early Warning System Bot 2011-08-15 22:21:18 PDT
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
Comment 16 Joseph Pecoraro 2011-08-15 22:28:10 PDT
Created attachment 104006 [details]
[PATCH] Fix Builds
Comment 17 WebKit Review Bot 2011-08-16 12:18:25 PDT
Comment on attachment 104006 [details]
[PATCH] Fix Builds

Clearing flags on attachment: 104006

Committed r93140: <http://trac.webkit.org/changeset/93140>
Comment 18 WebKit Review Bot 2011-08-16 12:18:31 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Joseph Pecoraro 2011-08-16 13:03:19 PDT
Created attachment 104079 [details]
[PATCH] Attempt at Windows Build Fix
Comment 20 WebKit Review Bot 2011-08-16 13:06:34 PDT
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.
Comment 21 Joseph Pecoraro 2011-08-16 13:12:10 PDT
Attempted a Windows Build Fix in: <http://trac.webkit.org/changeset/93147>
Comment 22 Joseph Pecoraro 2011-08-16 13:29:27 PDT
Attempted a Chromium Windows Build Fix in: <http://trac.webkit.org/changeset/93149>
Comment 23 Joseph Pecoraro 2011-08-16 13:41:02 PDT
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.
Comment 24 Joseph Pecoraro 2011-08-16 15:03:18 PDT
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.