Closed Bug 1241931 Opened 4 years ago Closed 4 years ago
Shutdownhang with active GDIFont
I have seen several of these cases while looking at reports for the signature. Bugs of this type will contain GDIFontInfo::Load() somewhere in the crash report threads. e.g. bp-2bd52255-84f1-4e3e-b31a-b03902160118 bp-a02a0fd3-f5c9-4d42-b9b0-caa2a2160119 0 ntdll.dll KiFastSystemCallRet 1 gdi32.dll NtGdiGetFontData 2 gdi32.dll GetFontData 3 xul.dll GDIFontInfo::EnumerateFontsForFamily(tagENUMLOGFONTEXW const*, tagNEWTEXTMETRICEXW const*, unsigned long, long) gfx/thebes/gfxGDIFontList.cpp 4 gdi32.dll EnumFontsInternalW 5 gdi32.dll EnumFontFamiliesExW 6 xul.dll GDIFontInfo::LoadFontFamilyData(nsAString_internal const&) gfx/thebes/gfxGDIFontList.cpp 7 xul.dll FontInfoData::Load() gfx/thebes/gfxFontInfoLoader.cpp 8 xul.dll GDIFontInfo::Load() gfx/thebes/gfxGDIFontList.cpp 9 xul.dll AsyncFontInfoLoader::Run() gfx/thebes/gfxFontInfoLoader.cpp 10 xul.dll nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp I don't think there is a complete hang on GetFontData causing the problem overall. (just a slow step repeated.) The problem is likely the loading iterates over many fonts and there is no check for shutdown while this is ongoing. Even when not a shutdownhang crashing here, this code will likely contribute to a slow shutdown. (If done soon after startup.)
Crash Signature: [@ shutdownhang | WaitForSingleObjectEx | WaitForSingleObject | PR_Wait | nsThread::ProcessNextEvent | NS_ProcessNextEvent | nsThread::Shutdown [@ shutdownhang | WaitForSingleObjectEx | WaitForSingleObject | PR_WaitCondVar | nsThread::ProcessNextEvent | … → [@ shutdownhang | WaitForSingleObjectEx | WaitForSingleObject | PR_Wait | nsThread::ProcessNextEvent | NS_ProcessNextEvent | nsThread::Shutdown] [@ shutdownhang | WaitForSingleObjectEx | WaitForSingleObject | PR_WaitCondVar | nsThread::ProcessNextEvent |…
I don't have commit access. If patch is good please run on try server.
Attachment #8714770 - Flags: review?(jd.bugzilla)
Assignee: nobody → jonathan
Component: General → Graphics: Text
Product: Firefox → Core
Version: unspecified → Trunk
Comment on attachment 8714770 [details] [diff] [review] bug-1241931-shutdown-font-load.patch Overall, looks good. But I don't think we need to treat the flag as "shutdown", we should use the same logic for any other situation where the loader is canceled. This happens when a change to system fonts occurs, as when a font is added or deleted. > + mozilla::Atomic<bool> mLoadShutdown; > + Rename this to mCanceled here and elsewhere in the patch. > -gfxFontInfoLoader::CancelLoader() > +gfxFontInfoLoader::CancelLoader(bool shutdown /*= false*/) ... > + if (shutdown && mFontInfo) > + mFontInfo->mLoadShutdown = true; No need for the parameter, any call to CancelLoader() should set mCanceled without the need to check for shotdown or mFontInfo.
Attachment #8714770 - Flags: review?(jd.bugzilla) → review-
Made basic change, less confident due to not having tested other cancel paths.
Try server build https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9cfa826faad
Comment on attachment 8715328 [details] [diff] [review] bug-1241931-shutdown-font-load.patch Looks good, let's see what happens with the tryserver build.
Attachment #8715328 - Flags: review?(jd.bugzilla) → review+
Jonathan, thanks for working on this. I'll land the patch once inbound is open.
You need to log in before you can comment on or make changes to this bug.