Bug 89658 – FontFallbackList should release cached font data when the FontCache is invalidated

• Component: Layout and Rendering
• Status: RESOLVED INVALID

[Balazs Kelemen]

This has shown in #76534, i.e. the assertion |fontCache()->generation() == m_generation| in FontFallbackList::fontDataAt is failing. This means we hurt the invariant that the FontData* stored in FontFallbackList also exists in the FontCache with it's associated FontPlatformData. I believe the reason why it did not shown up earlier is that FontCache::invalidate is not well tested.

[Balazs Kelemen]

*** Bug 76534 has been marked as a duplicate of this bug. ***

[Balazs Kelemen]

Created attachment 148804 [details]
Patch

[Csaba Osztrogonác]

It causes assertion on Qt, so it should block bug79668, please don't remove it.

[Build Bot]

Comment on attachment 148804 [details]
Patch

Attachment 148804 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13034156

[Early Warning System Bot]

Comment on attachment 148804 [details]
Patch

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

Comment on attachment 148804 [details]
Patch

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

> Source/WebCore/ChangeLog:14
> +        but it is not reliable. Fix this by making FontFallBackList a client of

What does this mean? Please describe the circumstances under which the problem occurs.

[Balazs Kelemen]

(In reply to comment #6)
> (From update of attachment 148804 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148804&action=review
> 
> > Source/WebCore/ChangeLog:14
> > +        but it is not reliable. Fix this by making FontFallBackList a client of
> 
> What does this mean? Please describe the circumstances under which the problem occurs.

Ok, this has not been formulated well, but I think it's quite simple. If there is some style change that affects a font of an element, we invalidate the FontFallbackList, so it will acquire the fonts from the FontCache again. But if we invalidate the FontCache itself, we should invalidate all FontFallbackList immediately. As I see FontFallbackList is nothing more than a simple cache that saves a hashmap lookup in the general case (when we lookup the font data for a non-extraordinary character). In the case of the Qt DRT, it invalidates the FontCache after every test. But than it loads a new url, so we will do a full relayout anyway, and it hides this issue, except in that particular test that has been skipped for the failed assert. Actually I don't know how can that test trigger this, maybe that's a result of some other relayouting bug. I can investigate more to understand, but basically I still think that the logic of FontCache::invalidate and FontFallbackList's is plain broken, just hard to trigger.

I think the appropriate test would be to force a font cache invalidation from the test and do an incremental relayout. For being able to do that we have to add FontCache::invalidate to internals. Do you think that makes sense?

Note that a font cache invalidation could happen any time. For example the Mac port installs a listener for font settings change and call it from there (in FontCacheMac.mm)

[Balazs Kelemen]

I tried further to trigger this bug with the approach of invalidating the font cache during page load, with no success. In the general case there is a CSSFontSelector of the document and it schedules a full style update upon font cache invalidation and it will update all fonts. However, this is not the case with the problematic test case (svg/carto.net/combobox.svg). With this test, the CSSFontSelector schedules the style update, and we assert with this particular subtree:
<text x="240" y="333">This comboBox gives<tspan x="240" dy="15">feedback on changes</tspan><tspan x="240" dy="15">in the selection</tspan></text>

The relevant call stack is the following:
#0  WebCore::FontFallbackList::fontDataAt
#1  WebCore::FontFallbackList::primaryFontData
#2  WebCore::FontFallbackList::primarySimpleFontData
#3  WebCore::Font::primaryFont
#4  WebCore::textRunNeedsRenderingContext
#5  WebCore::SVGTextMetrics::constructTextRun
#6  WebCore::SVGTextMetricsBuilder::initializeMeasurementWithTextRenderer
#7  WebCore::SVGTextMetricsBuilder::measureTextRenderer
#8  WebCore::SVGTextMetricsBuilder::walkTree
#9  WebCore::SVGTextMetricsBuilder::walkTree
#10 WebCore::SVGTextMetricsBuilder::measureTextRenderer
#11 WebCore::SVGTextLayoutAttributesBuilder::rebuildMetricsForTextRenderer
#12 WebCore::RenderSVGText::subtreeStyleDidChange
#13 WebCore::RenderSVGInlineText::styleDidChange
#14 WebCore::RenderObject::setStyle
#15 WebCore::Text::recalcTextStyle
#16 WebCore::Element::recalcStyle

As I see basically the problem is that: when we start walking the tree of svg text element's from measureTextRenderer, we reach element's with a renderer that still have the old style (RenderObject::setStyle did not reach them yet), and this old style refers to a font that has invalid font data in it's FontFallbackList.
Do you think I understand the situation correctly? Furthermore, do you see a better way to fix this than making FontFallbackList's invalidate their data immediately after FontCache::invalidate?

(In reply to comment #8)

Thanks for looking into this.

> I tried further to trigger this bug with the approach of invalidating the font cache during page load, with no success. In the general case there is a CSSFontSelector of the document and it schedules a full style update upon font cache invalidation and it will update all fonts.

Indeed. That’s how things should work.

> However, this is not the case with the problematic test case (svg/carto.net/combobox.svg). With this test, the CSSFontSelector schedules the style update, and we assert with this particular subtree:
> <text x="240" y="333">This comboBox gives<tspan x="240" dy="15">feedback on changes</tspan><tspan x="240" dy="15">in the selection</tspan></text>
> 
> The relevant call stack is the following:
> #0  WebCore::FontFallbackList::fontDataAt
> #1  WebCore::FontFallbackList::primaryFontData
> #2  WebCore::FontFallbackList::primarySimpleFontData
> #3  WebCore::Font::primaryFont
> #4  WebCore::textRunNeedsRenderingContext
> #5  WebCore::SVGTextMetrics::constructTextRun
> #6  WebCore::SVGTextMetricsBuilder::initializeMeasurementWithTextRenderer
> #7  WebCore::SVGTextMetricsBuilder::measureTextRenderer
> #8  WebCore::SVGTextMetricsBuilder::walkTree
> #9  WebCore::SVGTextMetricsBuilder::walkTree
> #10 WebCore::SVGTextMetricsBuilder::measureTextRenderer
> #11 WebCore::SVGTextLayoutAttributesBuilder::rebuildMetricsForTextRenderer
> #12 WebCore::RenderSVGText::subtreeStyleDidChange
> #13 WebCore::RenderSVGInlineText::styleDidChange
> #14 WebCore::RenderObject::setStyle
> #15 WebCore::Text::recalcTextStyle
> #16 WebCore::Element::recalcStyle
> 
> As I see basically the problem is that: when we start walking the tree of svg text element's from measureTextRenderer, we reach element's with a renderer that still have the old style (RenderObject::setStyle did not reach them yet), and this old style refers to a font that has invalid font data in it's FontFallbackList.

This looks like a serious bug in that piece of SVG code. Using stale style could lead to incorrect layout or rendering, even if the particular way we noticed the issue (through the stale font data) were fixed.

> Do you think I understand the situation correctly? Furthermore, do you see a better way to fix this than making FontFallbackList's invalidate their data immediately after FontCache::invalidate?

I think we should fix the SVG code. It would be neat to come up with a test case that shows the correctness problem here even when the font cache is not getting invalidated.

[Balazs Kelemen]

Nico, do you agree that this is a bug in SVG code? If so, I'm going to invalidate this entry or at least change the title.

[Balazs Kelemen]

This is not a problem of the font system. Let's continue in bug 76534.