Bug 54758

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.

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.

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.

Comment on attachment 82984 [details] proposed patch This is really really really scary. No changelog = r-.

Created attachment 83678 [details] proposed patch Added missing ChangeLogs

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.

Created attachment 83684 [details] proposed patch cleaned up rogue tab

Pinging for review

Added oliver (original author) to cc list

Did you mean a different oliver?

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.

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.

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

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

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

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).

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