Bug 26963 - Reproducible crash at FontCache::getFontData() when a custom font is used in a pseudo-style
Summary: Reproducible crash at FontCache::getFontData() when a custom font is used in ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P1 Major
Assignee: Nobody
URL: http://craigmod.com/journal/font-face/
Keywords: HasReduction, InRadar
: 22256 (view as bug list)
Depends on:
Blocks:
 
Reported: 2009-07-04 00:12 PDT by mitz
Modified: 2009-10-28 01:34 PDT (History)
4 users (show)

See Also:


Attachments
Test case (will crash) (250 bytes, text/html)
2009-07-04 00:12 PDT, mitz
no flags Details
Example of stale cached pseudo-style (296 bytes, text/html)
2009-07-04 14:57 PDT, mitz
no flags Details
Example of stale cached pseudo-style (716 bytes, text/html)
2009-07-05 18:05 PDT, mitz
no flags Details
Validate the pseudo-style cache on style recalc (45.27 KB, patch)
2009-07-06 14:44 PDT, mitz
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2009-07-04 00:12:19 PDT
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.
Comment 1 mitz 2009-07-04 00:55:24 PDT
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.
Comment 2 mitz 2009-07-04 14:57:35 PDT
Created attachment 32262 [details]
Example of stale cached pseudo-style
Comment 3 mitz 2009-07-05 18:05:59 PDT
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.
Comment 4 mitz 2009-07-05 22:42:27 PDT
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.
Comment 5 mitz 2009-07-06 14:44:36 PDT
Created attachment 32328 [details]
Validate the pseudo-style cache on style recalc
Comment 6 Dave Hyatt 2009-07-06 15:10:53 PDT
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 7 mitz 2009-07-06 18:25:22 PDT
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 8 mitz 2009-07-06 19:33:37 PDT
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.
Comment 9 Darin Adler 2009-07-06 20:03:33 PDT
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
Comment 10 mitz 2009-07-07 16:55:13 PDT
Fixed in <http://trac.webkit.org/projects/webkit/changeset/45612>.
Comment 11 mitz 2009-10-28 01:34:23 PDT
*** Bug 22256 has been marked as a duplicate of this bug. ***