Bug 26963 – Reproducible crash at FontCache::getFontData() when a custom font is used in a pseudo-style

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

Created attachment 32254 [details]
Test case (will crash)

<rdar://problem/7030998>
The page at the URL frequently causes a crash at FontCache::getFontData(), especially if repainting or relayout is forced repeatedly while the page is loading.

The problem is that when a custom font is used only in a cached pseudo-style, the invalidation mechanism for when the font is loaded, which is based on forcing a style recalc on the document, does not reach the cached pseudo-style, so they it is left pointing at stale FontData.

The attached test case demonstrates the problem: first of all, even when it is done loading the font, the first letter isn’t updated to render with the loaded font. Then if you force layout by resizing the window, it crashes.

I believe that the problem is, at least to some extent, with Node::diff() not looking deep enough into pseudo-styles. For example, it also fails to respond to a style change that only changes some property of the first-letter style.

Created attachment 32262 [details]
Example of stale cached pseudo-style

Created attachment 32283 [details]
Example of stale cached pseudo-style

Extended test case. Note how for first-line, there has to be an existing rule, because existence tracked in the pseudoBits member of of NonInheritedFlags, whereas for -webkit-input-placeholder it is unnecessary.

In order to fix the crash, it is sufficient to address the cases where a cached pseudo-style exists, and not any of the other cases of incorrectness. This can be done by validating the cache in the current style, that is, for every cached pseudo-style, re-resolve and compare the new result to the cached style. If they differ, stop and use the new style. Otherwise, keep the current style. I have a patch in progress that does this, but it still needs work.

Created attachment 32328 [details]
Validate the pseudo-style cache on style recalc

[Dave Hyatt]

Comment on attachment 32328 [details]
Validate the pseudo-style cache on style recalc

I'm concerned about the addition of the new firstLineStyle method to RenderObject.  It seems like that is going to be confusing next to the other first line style method.

Would it be cleaner to just build this pseudo style comparison into the diff method?

Comment on attachment 32328 [details]
Validate the pseudo-style cache on style recalc

(In reply to comment #6)
> (From update of attachment 32328 [details])
> I'm concerned about the addition of the new firstLineStyle method to
> RenderObject.  It seems like that is going to be confusing next to the other
> first line style method.

I will try to come up with a more descriptive/discouraging name.

> Would it be cleaner to just build this pseudo style comparison into the diff
> method?

I’ll try that and see where I get stuck :-)

Thanks for the review!

Comment on attachment 32328 [details]
Validate the pseudo-style cache on style recalc

Node::diff() is now a static function (because of a callsite in updateBeforeAfterContent(). I could make it take a renderer, but it seems unnatural to supply a renderer at that stage, because the renderer depends on style.

I can rename firstLineStyle() to uncachedFirstLineStyle() to discourage use and add a comment.

r? again.

[Darin Adler]

Comment on attachment 32328 [details]
Validate the pseudo-style cache on style recalc

> +bool Element::validatePseudoStyleCache(const RenderStyle* currentStyle, RenderStyle* newStyle)

I don't understand why this function is named "validate" yet returns a boolean. Typically if functions return a value they are named after the value. I think the value means "needs to dump the style cache" or something like that, so validate seems an unhelpful name.

> +    Vector<RenderStyle*, 10> pseudoStyleCache;

I'd prefer a typedef so the magic number 10 isn't repeated here.

> +        if (*newPseudoStyle.get() != *pseudoStyleCache[i]) {

Do you really need the call to get() here? If so, I'm surprised you do.

Otherwise, based on Hyatt's review and my reading of the code, r=me

Fixed in <http://trac.webkit.org/projects/webkit/changeset/45612>.

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