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,
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
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;
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
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
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
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
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
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.
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
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.
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
Comment on attachment 80174 [details] A carpet-bomb approach 3 ok
Comment on attachment 80174 [details] A carpet-bomb approach 3 Clearing flags on attachment: 80174 Committed r76679: <http://trac.webkit.org/changeset/76679>
All reviewed patches have been landed. Closing bug.
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