Bug 54758 – Crash when laying out page with loaded fonts (intermittent)

• Status: RESOLVED
• Resolution: INVALID
• Component: Layout and Rendering

[Russell Brenner]

When laying out a page that uses a downloaded font, preliminary layout is performed using the appropriate fallback font (default serif, sans, or monotype). The download does not start until the first use of the font.

When the download completes, CSSFontSelector::fontLoaded() calls Document::scheduleForcedStyleRecalc() to notify of the switch from the fallback to the actual font. Also upon completion of the download, CSSFontFaceSource::fontLoaded() and CSSSegmentedFontFace::fontLoaded() have been calling pruneTable(), which immediately wipes out associated GlyphPageTreeNodes.

The problem is that those GlyphPageTreeNodes, the ones associated with the fallback font, may still be in use on the UI thread, frequently by Font::glyphDataForCharacter(), causing a segfault.

[Russell Brenner]

Created attachment 82984 [details]
proposed patch

This patch simply removes the calls to pruneTable() at the moment that the font download is completed, deferring pruning until destruction of the owner.

Element::recalcStyle triggers ~Font, which leads to FontCache::removeClient, which leads to ~CSSSegmentedFontFace and ~CSSFontFaceSource, each of which calls pruneTable at a time when it appears safe to do so.

This patch is #ifdef'ed for Android, but I believe it to be safe for all platforms.

[Alexey Proskuryakov]

Could you please attach a crash log, <http://www.webkit.org/quality/crashlogs.html>?

I don't understand why this bug says "Platform: Android", but the patch targets every other platform except for Android.

[Russell Brenner]

Created attachment 83347 [details]
stack trace

Attaching stack trace of a typical failure. Here's an excerpt showing the fallout of a freed node pointer:

(gdb) frame 1
#1  0x822530a6 in WebCore::Font::glyphDataForCharacter (this=0x78d7f4, c=69, mirror=false, forceSmallCaps=false)
    at external/webkit/WebCore/platform/graphics/FontFastPath.cpp:76
76	                GlyphData data = page->glyphDataForCharacter(c);
(gdb) l
71	    if (!useSmallCapsFont) {
72	        // Fastest loop, for the common case (not small caps).
73	        while (true) {
74	            page = node->page();
75	            if (page) {
76	                GlyphData data = page->glyphDataForCharacter(c);
77	                if (data.fontData) {
78	                    if (data.fontData->platformData().orientation() == Vertical && data.fontData->orientation() == Horizontal && Font::isCJKIdeograph(c)) {
79	                        const SimpleFontData* ideographFontData = data.fontData->brokenIdeographFontData();
80	                        GlyphPageTreeNode* ideographNode = GlyphPageTreeNode::getRootChild(ideographFontData, pageNumber);
(gdb) p page
$1 = (WebCore::GlyphPage *) 0x9


With the proposed patch, these two calls pruneTable() remain in place for all platforms except android (#if ! PLATFORM(ANDROID)). This may be overly conservative, as I believe the pruning will still take place in a timely fashion when the page is updated in response to Document::scheduleForcedStyleRecalc(), called by CSSFontSelector::fontLoaded(). The more aggressive patch would be to remove these two calls from all platforms.

[Eric Seidel (no email)]

Comment on attachment 82984 [details]
proposed patch

This is really really really scary.  No changelog = r-.

[Russell Brenner]

Created attachment 83678 [details]
proposed patch

Added missing ChangeLogs

[WebKit Review Bot]

Attachment 83678 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'Source/WebCore/ChangeLog', u..." exit_code: 1

ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Russell Brenner]

Created attachment 83684 [details]
proposed patch

cleaned up rogue tab

[Russell Brenner]

Pinging for review

[Russell Brenner]

Added oliver (original author) to cc list

[Oliver Hunt]

Did you mean a different oliver?

[Russell Brenner]

Must be, if this code doesn't look familiar. I pulled the name oliver from http://trac.webkit.org/changeset/26484 and didn't catch that it mapped to :olliej. Sorry 'bout that. AFAICT, it looks like :oliver is no longer active.

[Oliver Hunt]

After some digging it looks like svn history has been borken somehow -- it's definitely referring to me but it's all hyatt's fault.

[Oliver Hunt]

it == the revision that introduced css fontface.  According to the changelog hyatt did it (look changelog being more helpful/correct than revision history!!! hah!)

[Russell Brenner]

Thanks for sleuthing that. Haven't connected with hyatt yet via irc; will try smtp.

[Russell Brenner]

Underlying issue was traced to skia font matching for custom fonts with bold or italic style. Resolution was found in platform code.

[Eric Seidel (no email)]

Comment on attachment 83684 [details]
proposed patch

Cleared review? from attachment 83684 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).

[Naveen Bobbili]

Is there any update on this? I have recently come accross the same issue when running webcrawler test on motorola devices.