Closed Bug 1241931 Opened 6 years ago Closed 5 years ago

Shutdownhang with active GDIFontInfo::Load()


(Core :: Graphics: Text, defect)

Not set



Tracking Status
firefox47 --- fixed


(Reporter: jonathan, Assigned: jonathan)



Crash Data


(1 file, 1 obsolete file)

I have seen several of these cases while looking at reports for the signature.

Bugs of this type will contain
somewhere in the crash report threads.


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]

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.
Attachment #8714770 - Attachment is obsolete: true
Attachment #8715328 - Flags: review?(jd.bugzilla)
Comment on attachment 8715328 [details] [diff] [review]

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.
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Blocks: 1248837
You need to log in before you can comment on or make changes to this bug.