1241931 - Shutdownhang with active GDIFontInfo::Load()

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Jonathan Howard]

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

Comment 1

[Jonathan Howard]

Attachment: bug-1241931-shutdown-font-load.patch

I don't have commit access. If patch is good please run on try server.

Comment 2

[John Daggett (:jtd)]

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.

Comment 3

[Jonathan Howard]

Attachment: bug-1241931-shutdown-font-load.patch

Made basic change, less confident due to not having tested other cancel paths.

Comment 4

[John Daggett (:jtd)]

Try server build
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9cfa826faad

Comment 5

[John Daggett (:jtd)]

Comment on attachment 8715328 [details] [diff] [review]
bug-1241931-shutdown-font-load.patch

Looks good, let's see what happens with the tryserver build.

Comment 6

[John Daggett (:jtd)]

Jonathan, thanks for working on this. I'll land the patch once inbound is open.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0382e8ba269

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d0382e8ba269