1317653 - [App Verifier] Critical section already initialized in _moz_cairo_win32_printing_surface_create

Status: NEW

(Core :: Print Preview, defect, P3)

Description

[Cervantes Yu [:cyu] [:cervantes]]

Attachment: App verifier error report

App verifier reports that a CRITICAL_SECTION is initialize twice in running
./mach mochitest -f chrome layout/base/tests/chrome/test_printpreview.xul

Stack trace:
vfbasics!+7ffabecb6d61 ( @ 0)
KERNELBASE!InitializeCriticalSectionAndSpinCount+a ( @ 0)
gdi32full!LpkInitialize+13 ( @ 0)
gdi32full!GdiInitializeLanguagePack+4d ( @ 0)
xul!_moz_cairo_win32_printing_surface_create+23b (e:\hg\mozilla-central\gfx\cairo\cairo\src\cairo-win32-printing-surface.c @ 1926)
xul!mozilla::gfx::PrintTargetWindows::CreateOrNull+aa (e:\hg\mozilla-central\gfx\thebes\printtargetwindows.cpp @ 45)
xul!nsDeviceContextSpecWin::MakePrintTarget+29d (e:\hg\mozilla-central\widget\windows\nsdevicecontextspecwin.cpp @ 256)
xul!nsDeviceContext::InitForPrinting+2a (e:\hg\mozilla-central\gfx\src\nsdevicecontext.cpp @ 474)
xul!nsPrintEngine::DoCommonPrint+8d7 (e:\hg\mozilla-central\layout\printing\nsprintengine.cpp @ 669)
xul!nsPrintEngine::CommonPrint+45 (e:\hg\mozilla-central\layout\printing\nsprintengine.cpp @ 406)
xul!nsPrintEngine::PrintPreview+e8 (e:\hg\mozilla-central\layout\printing\nsprintengine.cpp @ 816)
xul!nsDocumentViewer::PrintPreview+35f (e:\hg\mozilla-central\layout\base\nsdocumentviewer.cpp @ 3929)
(frames omitted for brevity)

Comment 1

[Cervantes Yu [:cyu] [:cervantes]]

Getting the 1st initialization stack trace in windbg:
=======================================
VERIFIER STOP 0000000000000211: pid 0x3768: Critical section is already initialized. 

	00007FFAD4C217C0 : Critical section address. Run !cs -s <address> to get more information.
	00000207A5D53FD0 : Critical section debug info address.
	000002079DC04FE0 : First initialization stack trace. Use dps to dump it if non-NULL.
	0000000000000000 : Not used.


=======================================
This verifier stop is continuable.
After debugging it use `go' to continue.

=======================================

(3768.4d84): Break instruction exception - code 80000003 (first chance)
vrfcore!VerifierStopMessageEx+0x6f9:
00007ffa`be9c2149 cc              int     3
0:000> dps 000002079DC04FE0 
00000207`9dc04fe0  00000000`00000000
00000207`9dc04fe8  00200070`0000f801
00000207`9dc04ff0  00007ffa`d7a755a2 ntdll!memset+0x19e62
00000207`9dc04ff8  00007ffa`b8406d79 vfbasics!AVrfpInitializeCriticalSectionCommon+0x119
00000207`9dc05000  00007ffa`d48fcd6a KERNELBASE!InitializeCriticalSectionAndSpinCount+0xa
00000207`9dc05008  00007ffa`d4af4ea3*** ERROR: Symbol file could not be found.  Defaulted to export symbols for C:\WINDOWS\System32\gdi32full.dll - 
 gdi32full!LpkInitialize+0x13
00000207`9dc05010  00007ffa`d4af4e5d gdi32full!GdiInitializeLanguagePack+0x4d
00000207`9dc05018  00007ffa`d4af4b2e gdi32full!GdiProcessSetup+0x18e
00000207`9dc05020  00007ffa`d5ef288b USER32!ClientThreadSetup+0xbb
00000207`9dc05028  00007ffa`d5ef27a9 USER32!_ClientThreadSetup+0x9
00000207`9dc05030  00007ffa`d7a589e4 ntdll!KiUserCallbackDispatcher+0x24
00000207`9dc05038  00007ffa`d4d56ac4 win32u!NtGdiInit+0x14
00000207`9dc05040  00007ffa`d4af477d gdi32full!GdiDllInitialize+0x4d
00000207`9dc05048  00007ffa`d5efbe81 USER32!_UserClientDllInitialize+0x399
00000207`9dc05050  00007ffa`b6cb0f29 verifier!AVrfpStandardDllEntryPointRoutine+0xc9
00000207`9dc05058  00007ffa`be9ca2e5 vrfcore!VfCoreStandardDllEntryPointRoutine+0x155

Comment 2

[Cervantes Yu [:cyu] [:cervantes]]

This indicates that GdiInitializeLanguagePack is already called when GDI32.DLL is loaded. Initializing it again might result in undefined behavior.

Comment 3

[Cervantes Yu [:cyu] [:cervantes]]

Jonathan, this looks like a hack for Windows XP, but is no longer valid for Windows 10. I think we need to restrict this hack to Windows versions that need it.

Comment 4

[Jonathan Watt [:jwatt]]

Seems reasonable to me, but I'd defer to the GFX team.

I'd note that the hack is still in the upstream cairo master branch by the looks of it, although there hasn't been a new cairo snapshot for almost a year now:

https://cgit.freedesktop.org/cairo/tree/src/win32/cairo-win32-printing-surface.c#n141

Bas, can you have someone look at sticking an ifdef aronud the _cairo_win32_printing_surface_init_language_pack call?

Comment 5

[Bas Schouten (:bas.schouten)]

(In reply to Jonathan Watt [:jwatt] from comment #4)
> Seems reasonable to me, but I'd defer to the GFX team.
> 
> I'd note that the hack is still in the upstream cairo master branch by the
> looks of it, although there hasn't been a new cairo snapshot for almost a
> year now:
> 
> https://cgit.freedesktop.org/cairo/tree/src/win32/cairo-win32-printing-
> surface.c#n141
> 
> Bas, can you have someone look at sticking an ifdef aronud the
> _cairo_win32_printing_surface_init_language_pack call?

If we'd need to still run it for Windows XP we'd need to make a runtime check..

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Now that we've dropped support for XP/Vista, can we move forward with this bug?

Comment 7

[Bas Schouten (:bas.schouten)]

Hrm, interesting, the code in there seems to indicate we're already checking if LPK.DLL has already been loaded.. But regardless, I suppose it can just go away with the dropping of XP/Vista.

Comment 8

[Bas Schouten (:bas.schouten)]

Attachment: Bug 1317653: Since we don't support XP anymore remove manual language pack initialization for printing surfaces.

Review commit: https://reviewboard.mozilla.org/r/111264/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/111264/

Comment 9

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8835574 [details]
Bug 1317653: Since we don't support XP anymore remove manual language pack initialization for printing surfaces.

https://reviewboard.mozilla.org/r/111264/#review112522

Let's just #ifdef it instead of deleting it completely. I feel like that might make merging with upstream easier.

Comment 10

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

This is a P1 bug without an assignee. 

P1 are bugs which are being worked on for the current release cycle/iteration/sprint. 

If the bug is not assigned by Monday, 28 August, the bug's priority will be reset to '--'.

Comment 11

[Jonathan Watt [:jwatt]]

Land this with Jeff's suggestion?

Comment 12

[Bas Schouten (:bas.schouten)]

(In reply to Jonathan Watt [:jwatt] (needinfo? me) from comment #11)
> Land this with Jeff's suggestion?

Umm.. sure.. are we ever going to merge again? I doubt it. But it can't hurt.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 24 weeks.