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.
Created attachment 103796 [details] [PATCH] Proposed Fix
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.
> > 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.
> > 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.
> 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.
Created attachment 103843 [details] [PATCH] Take 2
Comment on attachment 103843 [details] [PATCH] Take 2 Clearing flags on attachment: 103843 Committed r93011: <http://trac.webkit.org/changeset/93011>
All reviewed patches have been landed. Closing bug.
> > 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.
Created attachment 104004 [details] [PATCH] Add FontCache::getNonRetainedLastResortFallbackFont(...) This goes with mitz's suggestion. Its messy though =(.
Reopening because the original problem, although improved, was not entirely fixed.
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 on attachment 104004 [details] [PATCH] Add FontCache::getNonRetainedLastResortFallbackFont(...) Attachment 104004 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9401083
Created attachment 104006 [details] [PATCH] Fix Builds
Comment on attachment 104006 [details] [PATCH] Fix Builds Clearing flags on attachment: 104006 Committed r93140: <http://trac.webkit.org/changeset/93140>
Created attachment 104079 [details] [PATCH] Attempt at Windows Build Fix
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.
Attempted a Windows Build Fix in: <http://trac.webkit.org/changeset/93147>
Attempted a Chromium Windows Build Fix in: <http://trac.webkit.org/changeset/93149>
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.
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.