Bug 52422 – [chromium] More crash in FontFallbackList::determinePitch(const Font* font)

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Hironori Bono]

Unfortunately, my previous change for <https://bugs.webkit.org/show_bug.cgi?id=25770> cannot fix the crash in Bug 25770.

This is a crash log for Chrome 10.0.634.0 stored in our crash server.

Product, Version	 Chrome ,  10.0.634.0
Stack Signature	 WebCore::FontFallbackList::determinePitch(WebCore::Font const *)-1EA68F4
New Stack Signature	 WebCore::FontFallbackList::determinePitch(WebCore::Font const *) baeba093_841e5de7_6067284b_f63eebe8_a3d8ae41
Files	 Download minidump
Report Time (UTC)	 2011/01/13 10:29:47, Thu
Uptime	 10453 ms
OS Name, Version	 Windows NT ,  5.1.2600 Service Pack 3
CPU Architecture, Info	 x86 ,  GenuineIntel family 6 model 23 stepping 6
extension-1	 hehijbfgiekmjfkfjpbkbammjbdenadd
num-extensions	 1
num-views	 1
plat	 Win32
ptype	 renderer
url-chunk-1	 http://sdf.kr/xe/buwak
Comments	
Add Comment

0x023323ce	 [chrome.dll	 - fontfallbacklist.cpp:77]	WebCore::FontFallbackList::determinePitch(WebCore::Font const *)
0x022d6227	 [chrome.dll	 - renderblocklinelayout.cpp:1641]	WebCore::RenderBlock::findNextLineBreak(WebCore::BidiResolver<WebCore::InlineIterator,WebCore::BidiRun> &,bool,bool &,bool &,bool &,WebCore::EClear *,WebCore::RenderBlock::FloatingObject *)
0x022d413a	 [chrome.dll	 - renderblocklinelayout.cpp:667]	WebCore::RenderBlock::layoutInlineChildren(bool,int &,int &)
0x0221b470	 [chrome.dll	 - renderblock.cpp:1230]	WebCore::RenderBlock::layoutBlock(bool,int)
0x0221b16c	 [chrome.dll	 - renderblock.cpp:1128]	WebCore::RenderBlock::layout()
0x0221caee	 [chrome.dll	 - renderblock.cpp:1959]	WebCore::RenderBlock::layoutBlockChild(WebCore::RenderBox *,WebCore::RenderBlock::MarginInfo &,int &,int &)
0x0221c91a	 [chrome.dll	 - renderblock.cpp:1897]	WebCore::RenderBlock::layoutBlockChildren(bool,int &)
0x0221b480	 [chrome.dll	 - renderblock.cpp:1232]	WebCore::RenderBlock::layoutBlock(bool,int)
0x0221b16c	 [chrome.dll	 - renderblock.cpp:1128]	WebCore::RenderBlock::layout()
0x0221f160	 [chrome.dll	 - renderblock.cpp:3066]	WebCore::RenderBlock::insertFloatingObject(WebCore::RenderBox *)
0x022d5999	 [chrome.dll	 - renderblocklinelayout.cpp:1309]	WebCore::RenderBlock::skipLeadingWhitespace(WebCore::BidiResolver<WebCore::InlineIterator,WebCore::BidiRun> &,bool,bool,bool,WebCore::RenderBlock::FloatingObject *)
0x022d5c86	 [chrome.dll	 - renderblocklinelayout.cpp:1428]	WebCore::RenderBlock::findNextLineBreak(WebCore::BidiResolver<WebCore::InlineIterator,WebCore::BidiRun> &,bool,bool &,bool &,bool &,WebCore::EClear *,WebCore::RenderBlock::FloatingObject *)
0x022d413a	 [chrome.dll	 - renderblocklinelayout.cpp:667]	WebCore::RenderBlock::layoutInlineChildren(bool,int &,int &)
0x0221b470	 [chrome.dll	 - renderblock.cpp:1230]	WebCore::RenderBlock::layoutBlock(bool,int)
0x0221b16c	 [chrome.dll	 - renderblock.cpp:1128]	WebCore::RenderBlock::layout()
0x0221caee	 [chrome.dll	 - renderblock.cpp:1959]	WebCore::RenderBlock::layoutBlockChild(WebCore::RenderBox *,WebCore::RenderBlock::MarginInfo &,int &,int &)
0x0221c91a	 [chrome.dll	 - renderblock.cpp:1897]	WebCore::RenderBlock::layoutBlockChildren(bool,int &)
0x0221b480	 [chrome.dll	 - renderblock.cpp:1232]	WebCore::RenderBlock::layoutBlock(bool,int)
0x0221b16c	 [chrome.dll	 - renderblock.cpp:1128]	WebCore::RenderBlock::layout()
0x0221caee	 [chrome.dll	 - renderblock.cpp:1959]	WebCore::RenderBlock::layoutBlockChild(WebCore::RenderBox *,WebCore::RenderBlock::MarginInfo &,int &,int &)
0x0221c91a	 [chrome.dll	 - renderblock.cpp:1897]	WebCore::RenderBlock::layoutBlockChildren(bool,int &)
0x0221b480	 [chrome.dll	 - renderblock.cpp:1232]	WebCore::RenderBlock::layoutBlock(bool,int)
0x0221b16c	 [chrome.dll	 - renderblock.cpp:1128]	WebCore::RenderBlock::layout()
0x0221caee	 [chrome.dll	 - renderblock.cpp:1959]	WebCore::RenderBlock::layoutBlockChild(WebCore::RenderBox *,WebCore::RenderBlock::MarginInfo &,int &,int &)
0x0221c91a	 [chrome.dll	 - renderblock.cpp:1897]	WebCore::RenderBlock::layoutBlockChildren(bool,int &)
0x0221b480	 [chrome.dll	 - renderblock.cpp:1232]	WebCore::RenderBlock::layoutBlock(bool,int)
0x0221b16c	 [chrome.dll	 - renderblock.cpp:1128]	WebCore::RenderBlock::layout()
0x0221caee	 [chrome.dll	 - renderblock.cpp:1959]	WebCore::RenderBlock::layoutBlockChild(WebCore::RenderBox *,WebCore::RenderBlock::MarginInfo &,int &,int &)
0x0221c91a	 [chrome.dll	 - renderblock.cpp:1897]	WebCore::RenderBlock::layoutBlockChildren(bool,int &)
0x0221b480	 [chrome.dll	 - renderblock.cpp:1232]	WebCore::RenderBlock::layoutBlock(bool,int)
0x0221b16c	 [chrome.dll	 - renderblock.cpp:1128]	WebCore::RenderBlock::layout()
0x021739c9	 [chrome.dll	 - renderview.cpp:130]	WebCore::RenderView::layout()
0x0219bfef	 [chrome.dll	 - frameview.cpp:872]	WebCore::FrameView::layout(bool)
0x0219e192	 [chrome.dll	 - frameview.cpp:2247]	WebCore::FrameView::updateLayoutAndStyleIfNeededRecursive()
0x01d721ee	 [chrome.dll	 - render_widget.cc:504]	RenderWidget::DoDeferredUpdate()
0x01d7216c	 [chrome.dll	 - render_widget.cc:483]	RenderWidget::CallDoDeferredUpdate()
0x01d73cbb	 [chrome.dll	 - task.h:331]	RunnableMethod<RenderWidget,void ( RenderWidget::*)(void),Tuple0>::Run()
0x01cff63f	 [chrome.dll	 - message_loop.cc:356]	MessageLoop::RunTask(Task *)
0x01cff6c6	 [chrome.dll	 - message_loop.cc:365]	MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const &)
0x01cffa78	 [chrome.dll	 - message_loop.cc:558]	MessageLoop::DoWork()
0x01d16d3a	 [chrome.dll	 - message_pump_default.cc:50]	base::MessagePumpDefault::Run(base::MessagePump::Delegate *)
0x01cff5c0	 [chrome.dll	 - message_loop.cc:331]	MessageLoop::RunInternal()
0x01cff545	 [chrome.dll	 - message_loop.cc:304]	MessageLoop::RunHandler()
0x01cff439	 [chrome.dll	 - message_loop.cc:234]	MessageLoop::Run()
0x01d2d874	 [chrome.dll	 - renderer_main.cc:297]	RendererMain(MainFunctionParams const &)
0x01c33f99	 [chrome.dll	 - chrome_main.cc:914]	ChromeMain
0x00403e7a	 [chrome.exe	 - client_util.cc:280]	MainDllLoader::Launch(HINSTANCE__ *,sandbox::SandboxInterfaceInfo *)
0x00404243	 [chrome.exe	 - chrome_exe_main_win.cc:46]	wWinMain
0x00449cc0	 [chrome.exe	 - crt0.c:263]	__tmainCRTStartup
0x7c7e7076	 [kernel32.dll	 + 0x00017076]	BaseProcessStart

Even though I still cannot reproduce this crash on my PCs, it may be a good idea to try using all fonts installed to Windows XP by default, i.e. adopting carpet-bombing. (I would note this crash mostly happens when rendering Korean pages.)

Regards,

[Hironori Bono]

Created attachment 78889 [details]
A blind and speculative fix

Greetings,

This is just a blind and speculative fix (inspired by "FontCacheWin.cpp") for this crash. Even though I do not have any clear ideas for writing layout tests for this issue, it may be a good idea to add a function (such as layoutTestController.disableFont("Arial")) that prevents DumpRenderTree from loading the given fonts?

Regards,

Hironori Bono

[David Levin]

Comment on attachment 78889 [details]
A blind and speculative fix

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

> Source/WebCore/ChangeLog:14
> +        No new tests. (OOPS!)

oops. :)

This line should be removed.  Ideally there would be a test.

What about doing what you mentioned in the bug or something like that? Would that cause a crash that is fixed by this change?

> Source/WebCore/platform/graphics/chromium/FontCacheChromiumWin.cpp:593
> +        if (simpleFont = getCachedFontData(description, fallbackFonts[i])) {

Consider a fail fast approach.

if (!(simpleFont = getCachedFontData(description, fallbackFonts[i])))
    continue;

[Hironori Bono]

Greetings,

Thank you for your comments. I would like to describe some more details about this issue before updating my change.

(In reply to comment #2)
> (From update of attachment 78889 [details])
> oops. :)
> This line should be removed.  Ideally there would be a test.

Unfortunately, I do not have any idea why this crash still happens, i.e. I do not have any ideas how to reproduce this crash with Chromium with my r72116 <http://trac.webkit.org/changeset/73116>. This change just tries emulating Safari/Win and sees it makes any differences. If it does not make any differences, I need another change.

> What about doing what you mentioned in the bug or something like that?

When I tried enumerating all the fonts with a EnumFontFamiliesEx() call, it took long time (>1s) on my virtual machine and blocks WebKit during the period. Even though WebKit needs to enumerate all the installed fonts only once (and it is pretty rare to fall back to my code), I'm wondering if it is OK to add such huge blocking code.

> Would that cause a crash that is fixed by this change?

In brief, I do not have any clear ideas about this crash except that my r72116 did not fix the crash. As shown in the crash dump, this crash happens in FontFallbackList::determinePitch(), some time after FontCache::getLastResortFallbackFont() returns 0. So, this crash dump does not include any information about why FontCache::getLastResortFallbackFont() returns 0. (I noticed ASSERT_NOT_REACHED() did not cause crashes on Release builds.) If we add code that intentionally crashes as ASSERT_NOT_REACHED() does, we can investigate why this function returns 0. Nevertheless, I'm not sure it is acceptable. (I assume this is a reason why this crash has not been fixed for a long time.)

It is definitely welcome if there are any suggestions about how to identify why this crash still happens and fix it.

Regards,

Hironori Bono

[Hironori Bono]

Greetings,

Tamura-san has reported a case that causes this crash while integrating vertical-text support to Chromium at Bug 51450: <http://webkit.org/b/51450>. I will investigate his case and update the change.

Regards,

Hironori Bono

[Hironori Bono]

Created attachment 79721 [details]
A speculative fix 2

(In reply to comment #4)
> Tamura-san has reported a case that causes this crash while integrating vertical-text support to Chromium at Bug 51450: <http://webkit.org/b/51450>. I will investigate his case and update the change.

To investigate the above case, I noticed FontCache::createFontPlatformData() of Chromium might return 0 when users changed the system fonts to a font which has localized names on Windows XP, such as "HGGothicM". When a font has localized names, localized Windows returns its localized name for GetObject() call. That is, when we create a font with its English name ("HGGothicM"), the returned font name becomes its localized name "HGゴシックM". This caused a font-name mismatch in FontCache::createFontPlatformData() and this function returns 0 when my previous fix gets system fonts for a last-resort font. I have updated my change so we can bypass the said font-name check when finding the last-resort font. It would be definitely helpful to give me feedbacks.

Regards,

Hironori Bono

[Hironori Bono]

Comment on attachment 79721 [details]
A speculative fix 2

Greetings,

I would like to cancel this review request since it has a stupid mistake.

Regards,

Hironori Bono

[Hironori Bono]

Created attachment 80033 [details]
A carpet-bomb approach

Greetings,

After investigating this issue on Korean WinXP, it is better to use the original idea written in my first description (trying all the installed fonts). Is it possible to review the updated one?

Regards,

Hironori Bono

[WebKit Review Bot]

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

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

Source/WebCore/platform/graphics/chromium/FontCacheChromiumWin.cpp:579:  Use 0 instead of NULL.  [readability/null] [5]
Source/WebCore/platform/graphics/chromium/FontCacheChromiumWin.cpp:583:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 2 in 2 files


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

[Hironori Bono]

Created attachment 80034 [details]
A carpet-bomb approach 2

Greetings,

(In reply to comment #8)
> Source/WebCore/platform/graphics/chromium/FontCacheChromiumWin.cpp:579:  Use 0 instead of NULL.  [readability/null] [5]
> Source/WebCore/platform/graphics/chromium/FontCacheChromiumWin.cpp:583:  Use 0 instead of NULL.  [readability/null] [5]
> Total errors found: 2 in 2 files

Oops, I have uploaded a change to fix these style errors.

Regards,

Hironori Bono

[Kent Tamura]

Comment on attachment 80034 [details]
A carpet-bomb approach 2

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

I think the change is reasonable.
r- because of a style error.

> Source/WebCore/platform/graphics/chromium/FontCacheChromiumWin.cpp:419
> +    OutputDebugString(logFont->lfFaceName);

Is this needed?

> Source/WebCore/platform/graphics/chromium/FontCacheChromiumWin.cpp:587
> +    if (dc) {
> +      GetLastResortFallbackFontProcData procData(this, &description, fallbackFontName);
> +      EnumFontFamilies(dc, 0, getLastResortFallbackFontProc, reinterpret_cast<LPARAM>(&procData));
> +      ReleaseDC(0, dc);
> +
> +      if (procData.m_fontData)
> +        return procData.m_fontData;
> +    }

Wrong indentation.

[Hironori Bono]

Created attachment 80174 [details]
A carpet-bomb approach 3

Greetings,

Thank you for your review and comments.

(In reply to comment #10)
> (From update of attachment 80034 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80034&action=review
> > Source/WebCore/platform/graphics/chromium/FontCacheChromiumWin.cpp:419
> > +    OutputDebugString(logFont->lfFaceName);
> 
> Is this needed?

Oops, this API call is just for debugging and I should have removed it. I have removed this call.

> > Source/WebCore/platform/graphics/chromium/FontCacheChromiumWin.cpp:587
> > +    if (dc) {
> > +      GetLastResortFallbackFontProcData procData(this, &description, fallbackFontName);
> > +      EnumFontFamilies(dc, 0, getLastResortFallbackFontProc, reinterpret_cast<LPARAM>(&procData));
> > +      ReleaseDC(0, dc);
> > +
> > +      if (procData.m_fontData)
> > +        return procData.m_fontData;
> > +    }
> 
> Wrong indentation.

Ah, thank you for noticing it. I have fixed the indentation.

Regards,

Hironori Bono

[Kent Tamura]

Comment on attachment 80174 [details]
A carpet-bomb approach 3

ok

[WebKit Commit Bot]

Comment on attachment 80174 [details]
A carpet-bomb approach 3

Clearing flags on attachment: 80174

Committed r76679: <http://trac.webkit.org/changeset/76679>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Hironori Bono]

Greetings Jungshik,

Unfortunately, we still see this crash on 11.0.658.0. Is it OK to bypass the font-name check in FontCache::createFontPlatformData() when we are finding a last-resort font?

Regards,

Hironori Bono