Bug 95866

I believe Hyatt wrote most of this and likely could explain his intent.

I should note that all of this code was written before the existence of OwnPtr or PassOwnPtr. :)

Here's my analysis of FontData ownership and other class that hold pointers to it: FontData allocated in CSSFontFaceSource either comes from the cache or is only custom. All SegmentedFontData are custom fonts. FontCache.cpp has a hash table with SimpleFontData in the value field. Note that this rules out SegmentedFontData from storage in the cache. If the font is retained, it adds it to the cache and will not remove it unless it's use count goes to zero. Otherwise, it puts it in the cache but also adds it to the inactive font list. The inactive font list is not cleared until purgeInactiveFontDataIfNeeded or purgeInactiveFontData, at which point the SimpleFontData is deleted. The FontCachePurgeProtector triggers font cache purging upon destruction. However, FontCache::getFontData only puts FontData in the cache when the FontSelector::getFontData method returns 0. But, the CSSFontSelector code uses FontCache::getCachedFontData for all non-custom fonts. FontCache::getCachedFontData always adds the FontData to the cache and it's never custom data. From this we infer that FontCache owns non-custom FontData, and FontFallbackList owns custom FontData that DOES NOT COME FROM FontSelector. The "retains" flag seems consistent in this context. CSSSegmentedFontFace has a HashMap where the values are SegmentedFontData*. These are not deleted by CSSSegmentedFontFace, which is good, but it is not clear that this table is always cleared when FontFallbackList prunes its tree. That would require calling CSSSegmentedFontFace::~CSSSegmentedFontFace(), CSSSegmentedFontFace::fontLoaded(CSSFontFace*) or CSSSegmentedFontFace::appendFontFace(PassRefPtr<CSSFontFace> fontFace). CSSFontFaceSource has identical issues to CSSSegmentedFontFace. Who clears the HashMap in CSSSegmentedFontFace and CSSFontFaceSource when FontData is deleted? It must be the case that these objects must always destructed when the FontData is cleared. Otherwise we would see problems all over tha place that we do not see now. GlyphPage holds an array of SimpleFontData*. This is cleared when fonts are purged from the GlyphPageTreeNode. GlyphPageTreeNode holds a HashMap keyed on FontData*. Cleared in destructor and when font is purged. SimpleFontData owns its derived font data and deletes it. Are the SimpleFontData DerivedData members ever cached? No, allocated directly in platform layers, held in RefPtrs, and never held by anything else. InlineFlowBox has a hash map of SimpleFontData (rendering). But this is only ever stack allocated.

It sounds like FontCache owns all non-custom FontData, which is easy, because that FontData is global (shared between pages), correct? custom FontData is more tricky, because theoretically it is not shared between pages, but rather per-document, except in the case of seamless, where it can be inherited across document boundaries. From your description, it sounds like some of the custom FontData are owned by FontFallbackList, but it's not clear to me from your description who deletes the rest.

Some FontData is owned by SimpleFontData, but that is all correctly accounted for so we can ignore it for the purposes of this discussion. All other FontData is requested via FontFallbackList::getFontData: FontFallbackList::getFontData calls FontCache::getFontData calls CSSFontSelector::getFontData calls CSSFontFace::getFontData calls CSSFontFaceSelector::getFontData which allocates FontData or CSSSegmentedFontFace::getFontData which allocates FontData On the return path, FontCache caches and owns non-custom FontData, while FontFallbackList owns custom FontData. There is no way, as far as I can tell, to get a custom FontData object without going through FontFallbackList and FontFallbackList always caches that custom font for removal when no longer used.

When modifying the code to use RefPtr we should modify the m_fontList member in FontFallbackList to use a custom class for the pair in mutable Vector<pair<const FontData*, IsCustomFontOrNot>, 1> m_fontList;

Created attachment 164403 [details] Patch

Comment on attachment 164403 [details] Patch This patch is not for submission because more testing and performance testing is required. I think the code is right, however, so review pending test results would be very welcome. Because many platform files are touched by this patch, and I can't build most of them, I promise to be available for an extended period when it lands.

Comment on attachment 164403 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164403&action=review I think this great. But we need to be very careful not to ref-churn FontData's too much. It's not clear to me that all these cases need PRP. I'm happy to talk more about this via VC or chat if that's helpful. I'm technically on vacation this week, but since in many ways you're doing me a favor here by cleaning up my seamless mess, I'm happy to set up a time to chat if that's helpful. :) > Source/WebCore/css/CSSFontFaceSource.cpp:119 > + return fontData; .release() presumably. > Source/WebCore/css/CSSFontFaceSource.cpp:191 > + fontData = 0; .clear() is fine too. > Source/WebCore/css/CSSFontFaceSource.cpp:193 > + return fontData; .release() > Source/WebCore/css/CSSFontFaceSource.h:62 > + PassRefPtr<SimpleFontData> getFontData(const FontDescription&, bool syntheticBold, bool syntheticItalic, CSSFontSelector*); Does CSSFontFaceSource own the FontData? Can it just return a raw pointer here? Do any clients need to hold the FontData longer than local? Or it could return RefPtr<FontData>& to indicate it requires a ref, but local callers don't have to bother taking one. That's kinda an advanced pattern reserved for perf-sensitive places (and really should be documented better). > Source/WebCore/css/CSSFontSelector.cpp:468 > +PassRefPtr<FontData> CSSFontSelector::getFontData(const FontDescription& fontDescription, const AtomicString& familyName) Unclear to me if this is creating a FontData or not? Do callers need to ref it? > Source/WebCore/platform/graphics/FontCache.cpp:353 > - return getLastResortFallbackFont(fontDescription, DoNotRetain); > + return getLastResortFallbackFont(fontDescription, DoNotRetain).leakRef(); Does DoNotRetain even make sense anymore? That was in a model where the Cache kept its own separate refcount, right? > Source/WebCore/platform/graphics/FontCache.cpp:390 > + RefPtr<SimpleFontData> fontData = *it.get(); Might want to use const RefPtr<SimpleFontData>& fontData here. > Source/WebCore/platform/graphics/FontFallbackList.h:87 > - mutable Vector<pair<const FontData*, bool>, 1> m_fontList; > + mutable Vector<pair<RefPtr<FontData>, bool>, 1> m_fontList; Why is the bool still needed if we always take a ref to the FontData now?

I don't know if we have any microbenchmarks in PerformanceTests which cover font lookup very well. We obviously could write one. Otherwise the PLT is probably our best proxy. I'm not sure the process for testing with the PLT these days, or if it's even possible pre-commit. Ryosuke, James and Tony G have been following the Chromium performance story more closely and would know.

Comment on attachment 164403 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164403&action=review I also want to try to do something about the horrible code duplication in all of the platform-specific methods. The Skia code for SVG filters manages to avoid all this duplication, and I think we can apply the same principles here. It will have to be another bug. >> Source/WebCore/css/CSSFontFaceSource.cpp:119 >> + return fontData; > > .release() presumably. No release I think. We have a reference to the RefPtr, so the ref-count was not incremented in the =. We want to increment the ref count for the return, because the RefPtr is still in the m_fontDataTable. So we construct a return value from the RefPtr& and get the behavior we want. I believe, and I'll verify, that if we release we will zero out the m_ptr in the m_fontDataTable. >> Source/WebCore/css/CSSFontFaceSource.cpp:191 >> + fontData = 0; > > .clear() is fine too. OK. Didn't know about that. >> Source/WebCore/css/CSSFontFaceSource.cpp:193 >> + return fontData; > > .release() Same argument as above. We have a RefPtr& and we do not want to release that ref. >> Source/WebCore/css/CSSFontFaceSource.h:62 >> + PassRefPtr<SimpleFontData> getFontData(const FontDescription&, bool syntheticBold, bool syntheticItalic, CSSFontSelector*); > > Does CSSFontFaceSource own the FontData? Can it just return a raw pointer here? Do any clients need to hold the FontData longer than local? Or it could return RefPtr<FontData>& to indicate it requires a ref, but local callers don't have to bother taking one. That's kinda an advanced pattern reserved for perf-sensitive places (and really should be documented better). CSSFontFaceSource maintains a list of FontData that it know it has created previously. I should look into whether or not this is double caching, but I am pretty sure that it saves work. In particular, for some reason the FontCache code checks for the FontData in the CSSFontSelector, and hence here, before it looks in it's own cache. I think there are places where a reference to a RefPtr would work fine. That is, any of these cases where we are returning something that is definitely in a cache. I'll look into it. >> Source/WebCore/css/CSSFontSelector.cpp:468 >> +PassRefPtr<FontData> CSSFontSelector::getFontData(const FontDescription& fontDescription, const AtomicString& familyName) > > Unclear to me if this is creating a FontData or not? Do callers need to ref it? I think callers do need a ref, because FontCache definitely needs a ref and it calls this method to find FontData. I built this patch up by started at the cache code, adding RefPtr in those locations, and then through search, compilation and testing finding all of the places where the FontData came from to fill those caches. >> Source/WebCore/platform/graphics/FontCache.cpp:353 >> + return getLastResortFallbackFont(fontDescription, DoNotRetain).leakRef(); > > Does DoNotRetain even make sense anymore? That was in a model where the Cache kept its own separate refcount, right? DoNotRetain prevents custom fonts from ending up in this cache. I don't know for sure why that's the case, but I believe it is because this cache is not cleared on document destruction, which is when we really want any custom fonts for the page to be purged. Hence the separate cache, effectively in FontFallbackList, for custom fonts, with purging managed by the document (for now). I am loath to mess with this right now, particularly given the method name seems to imply that this must be non-retained FontData. >> Source/WebCore/platform/graphics/FontCache.cpp:390 >> + RefPtr<SimpleFontData> fontData = *it.get(); > > Might want to use const RefPtr<SimpleFontData>& fontData here. Yep. >> Source/WebCore/platform/graphics/FontFallbackList.h:87 >> + mutable Vector<pair<RefPtr<FontData>, bool>, 1> m_fontList; > > Why is the bool still needed if we always take a ref to the FontData now? I don't think it is. I was leaving it until this afternoon.

Created attachment 164419 [details] Patch This patch should apply.

Comment on attachment 164403 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164403&action=review >>> Source/WebCore/css/CSSFontFaceSource.cpp:119 >>> + return fontData; >> >> .release() presumably. > > No release I think. We have a reference to the RefPtr, so the ref-count was not incremented in the =. We want to increment the ref count for the return, because the RefPtr is still in the m_fontDataTable. So we construct a return value from the RefPtr& and get the behavior we want. I believe, and I'll verify, that if we release we will zero out the m_ptr in the m_fontDataTable. I see, tricky. Would .release() be a compile error? if so, then no comment is probably necessary, as anyone who tried to add .release() woudl hit it. (I think it would be, since I expect .release() must not be const.) >>> Source/WebCore/platform/graphics/FontCache.cpp:353 >>> + return getLastResortFallbackFont(fontDescription, DoNotRetain).leakRef(); >> >> Does DoNotRetain even make sense anymore? That was in a model where the Cache kept its own separate refcount, right? > > DoNotRetain prevents custom fonts from ending up in this cache. I don't know for sure why that's the case, but I believe it is because this cache is not cleared on document destruction, which is when we really want any custom fonts for the page to be purged. Hence the separate cache, effectively in FontFallbackList, for custom fonts, with purging managed by the document (for now). I am loath to mess with this right now, particularly given the method name seems to imply that this must be non-retained FontData. I see. So it's basically DoNotCache now. :)

Comment on attachment 164419 [details] Patch Attachment 164419 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13870525

Created attachment 164475 [details] Patch This should fix the mac build.

Comment on attachment 164475 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164475&action=review > Source/WebCore/ChangeLog:-41 > -<<<<<<< .mine Do you know about resolve-ChangeLogs? It's a nifty little script for dealing with this sort of update problem. Similarly if you're a git user, check out http://trac.webkit.org/wiki/UsingGitWithWebKit, which shows you how to setup resolve-ChangeLogs as a merge-driver.

Comment on attachment 164475 [details] Patch Attachment 164475 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13879387

Comment on attachment 164475 [details] Patch Attachment 164475 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/13890028

Created attachment 164557 [details] Patch This should fix the builds, and it addresses the coding issues. Still need performance numbers and some sanity checking.

(In reply to comment #16) > (From update of attachment 164475 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=164475&action=review > > > Source/WebCore/ChangeLog:-41 > > -<<<<<<< .mine > > Do you know about resolve-ChangeLogs? It's a nifty little script for dealing with this sort of update problem. Similarly if you're a git user, check out http://trac.webkit.org/wiki/UsingGitWithWebKit, which shows you how to setup resolve-ChangeLogs as a merge-driver. Yes, I now about it. But this wasn't me. :-) Someone has fixed the issue now.

Comment on attachment 164557 [details] Patch Attachment 164557 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/13901216

Created attachment 164567 [details] Patch Fix the GTK build.

Comment on attachment 164567 [details] Patch Attachment 164567 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13905125

Created attachment 164600 [details] Patch Fix the Debug build, and perf info.

I sanity checked with a debug chrome for an afternoon. FontData does get deleted, but only when it's intended to. Once the cache warms up, almost every call avoids the long chain of PassRefPtr, so performance really is not much of an issue. Angry Birds still crashes, but that's expected.

(In reply to comment #25) > I sanity checked with a debug chrome for an afternoon. FontData does get deleted, but only when it's intended to. Once the cache warms up, almost every call avoids the long chain of PassRefPtr, so performance really is not much of an issue. > > Angry Birds still crashes, but that's expected. Why is that expected? Don't the custom fonts still get held in the FontFallbackList with a ref now instead of a raw pointer?

(In reply to comment #26) > (In reply to comment #25) > > I sanity checked with a debug chrome for an afternoon. FontData does get deleted, but only when it's intended to. Once the cache warms up, almost every call avoids the long chain of PassRefPtr, so performance really is not much of an issue. > > > > Angry Birds still crashes, but that's expected. > > Why is that expected? Don't the custom fonts still get held in the FontFallbackList with a ref now instead of a raw pointer? The FontFallbackList is still asked to purge data by the Document, which means it removes the FontData from its hash map and all of the GlyphPageTreeNodes associated with it. It's the deleted GlyphPageTreeNode object that other code gets hold of and tries to access, pulling bad pointers from that. So while the FontData lives on, the FontFallbackList that was using it is a source of crashes. We started down the road of this patch so that we could be confident that, if we held FontData in FontFallbackList as a RefPtr, we do not need to be concerned with explicitly deleting it when we were not sure who else might be holding on (such as the FontCache, CSSFontFaceSource and SegmentedFontFace objects that also hold non-transient pointers to FontData objects). With the patch that will follow this one we'll move control of the data in FontFallbackList to the FontFallbackList itself. That's the original patch that lit up the debug tests.

Comment on attachment 164600 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=164600&action=review > Source/WebCore/css/CSSFontSelector.h:58 > + virtual PassRefPtr<FontData> getFontData(const FontDescription&, const AtomicString&); I suspect familyName is still useful as an arg name. :)

(In reply to comment #27) > (In reply to comment #26) > > (In reply to comment #25) > > > I sanity checked with a debug chrome for an afternoon. FontData does get deleted, but only when it's intended to. Once the cache warms up, almost every call avoids the long chain of PassRefPtr, so performance really is not much of an issue. > > > > > > Angry Birds still crashes, but that's expected. > > > > Why is that expected? Don't the custom fonts still get held in the FontFallbackList with a ref now instead of a raw pointer? > > The FontFallbackList is still asked to purge data by the Document, which means it removes the FontData from its hash map and all of the GlyphPageTreeNodes associated with it. It's the deleted GlyphPageTreeNode object that other code gets hold of and tries to access, pulling bad pointers from that. So while the FontData lives on, the FontFallbackList that was using it is a source of crashes. I'm curious how the code is getting hold of the GlyphPageTreeNodes except through lookup via the FontFallbackList. > We started down the road of this patch so that we could be confident that, if we held FontData in FontFallbackList as a RefPtr, we do not need to be concerned with explicitly deleting it when we were not sure who else might be holding on (such as the FontCache, CSSFontFaceSource and SegmentedFontFace objects that also hold non-transient pointers to FontData objects). > > With the patch that will follow this one we'll move control of the data in FontFallbackList to the FontFallbackList itself. That's the original patch that lit up the debug tests. OK. This patch itself looks fine. My only concern is understanding the grand plan. One way to fix the seamless issue is to make the inner document never clear the fonts and make the outer document always mange them. You've started down a path of allowing cleaner sharing of FontData, which is great. My concern would be if we've learned from this that FontData can't ever really be shared and we're just making a half-solution here. Regardless, you know what's up here, I'm sure, and I'll get it from the horse's mouth tomorrow, and we can move forward with this then. Thanks again, sorry for the slowness of my replies.

(In reply to comment #29) > I'm curious how the code is getting hold of the GlyphPageTreeNodes except through lookup via the FontFallbackList. The primary means is through lookup via FontFallbackList. Some FontData subclasses get GlyphPageTreeNodes directly from GlyphPageTreeNode::getRootChild(this, ...), where "this" is FontData. In that case the object doing the getting (the FontData) is still in existence and the GlyphPageTreeNode is created if it does not already exist. When we fix the other bug, FontFalbackList will not have caches of bad data as it does now. > This patch itself looks fine. My only concern is understanding the grand plan. One way to fix the seamless issue is to make the inner document never clear the fonts and make the outer document always mange them. You've started down a path of allowing cleaner sharing of FontData, which is great. My concern would be if we've learned from this that FontData can't ever really be shared and we're just making a half-solution here. > > Regardless, you know what's up here, I'm sure, and I'll get it from the horse's mouth tomorrow, and we can move forward with this then. FontData can definitely be shared between FontFallbackLists. It is encouraged, in fact, by the FontCache. FontFallbackLists can also be shared by Font objects. It is designed that way. The only issue is whether Font objects can be copy-constructed from one document to another. What really matters is whether Font contains per-document information. It shouldn't, but it does due to the custom font data caching behavior in Document. If we move that code to FontFallbackList, which is owned by Font, and not tied to Document, then I think it becomes safe to share Font across documents. There is a FIXME in there somewhere about how Font should be ref counted. A precondition to doing so is that we deal with the custom font caching. Even than, I am not entirely sure that it is safe to do. I would be willing to give it a shot however, as it would dramatically reduce allocations of Font objects. > Thanks again, sorry for the slowness of my replies. No problem at all.

Comment on attachment 164600 [details] Patch Thanks.

Created attachment 166533 [details] Patch Revised Changelog and update to accomodate recent patches.

r=eric. Eric R+'d the previous patch. The current one that I am about to commit just fixes merge conflicts and updates the Changelog with some additional info, once it clears EWS.

Committed r130079: <http://trac.webkit.org/changeset/130079>

Reverted r130079 for reason: Broke the chomium windows compile. Committed r130105: <http://trac.webkit.org/changeset/130105>

Broke the chromium windows compile: http://build.chromium.org/p/chromium.webkit/builders/Win%20Builder/builds/28909/steps/compile/logs/stdio 553>..\platform\graphics\chromium\SimpleFontDataChromiumWin.cpp(123): error C2556: 'WTF::PassOwnPtr<T> WebCore::SimpleFontData::smallCapsFontData(const WebCore::FontDescription &) const' : overloaded function differs only by return type from 'WTF::PassRefPtr<T> WebCore::SimpleFontData::smallCapsFontData(const WebCore::FontDescription &) const' 553> with 553> [ 553> T=WebCore::SimpleFontData 553> ] 553> c:\b\build\slave\win-latest-rel\build\src\third_party\webkit\source\webcore\platform\graphics\SimpleFontData.h(100) : see declaration of 'WebCore::SimpleFontData::smallCapsFontData' 553>..\platform\graphics\chromium\SimpleFontDataChromiumWin.cpp(123): error C2371: 'WebCore::SimpleFontData::smallCapsFontData' : redefinition; different basic types 553> c:\b\build\slave\win-latest-rel\build\src\third_party\webkit\source\webcore\platform\graphics\SimpleFontData.h(100) : see declaration of 'WebCore::SimpleFontData::smallCapsFontData' 553>..\platform\graphics\chromium\SimpleFontDataChromiumWin.cpp(133): error C2556: 'WTF::PassOwnPtr<T> WebCore::SimpleFontData::emphasisMarkFontData(const WebCore::FontDescription &) const' : overloaded function differs only by return type from 'WTF::PassRefPtr<T> WebCore::SimpleFontData::emphasisMarkFontData(const WebCore::FontDescription &) const' 553> with 553> [ 553> T=WebCore::SimpleFontData 553> ] 553> c:\b\build\slave\win-latest-rel\build\src\third_party\webkit\source\webcore\platform\graphics\SimpleFontData.h(101) : see declaration of 'WebCore::SimpleFontData::emphasisMarkFontData' 553>..\platform\graphics\chromium\SimpleFontDataChromiumWin.cpp(133): error C2371: 'WebCore::SimpleFontData::emphasisMarkFontData' : redefinition; different basic types 553> c:\b\build\slave\win-latest-rel\build\src\third_party\webkit\source\webcore\platform\graphics\SimpleFontData.h(101) : see declaration of 'WebCore::SimpleFontData::emphasisMarkFontData'

Committed r130160: <http://trac.webkit.org/changeset/130160>

(In reply to comment #37) > Committed r130160: <http://trac.webkit.org/changeset/130160> It made 2 tests crash on Qt - https://bugs.webkit.org/show_bug.cgi?id=98158