1488800 - Use PSAPI version 2+, avoiding a DLL dependency for crash reporting

Status: RESOLVED INVALID

(Toolkit :: Crash Reporting, enhancement, P2)

Description

[Carl Corcoran [:ccorcoran]]

Windows' PSAPI Version 2+, available since Windows 7, is implemented in kernelbase.dll rather than psapi.dll, which is an advantage because it would avoid a DLL load during crash handling. Currently, psapi.dll is loaded during exception handling, which could affect the quality of the resulting crash report. In the worst case, the process can hang or crash as a result.

We should ensure we're using version 2+ of psapi to avoid invoke a DLL load upon exception handling.

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

I'm going to move this to the crash reporter component since that's where the bug shows its effects.

Comment 2

[(not currently active) Ted Mielczarek]

I don't know where we use psapi in the crash reporter (I don't see anything doing it explicitly, is it just called by way of MinidumpWriteDump?) but given the fact that PSAPI_VERSION=1 defines seem to be strewn across the tree this feels like it's going to need a lot more work than fixing just the crash reporter:
https://dxr.mozilla.org/mozilla-central/search?tree=mozilla-central&q=psapi

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

Yeah, we're going to need to remove all instances of PSAPI_VERSION=1 and then set PSAPI_VERSION=2 at configure time.

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to Ted Mielczarek [:ted] [:ted.mielczarek] from comment #2)
> I don't know where we use psapi in the crash reporter (I don't see anything
> doing it explicitly, is it just called by way of MinidumpWriteDump?)

That's also a good point. Carl, could you paste a call stack from one of these PSAPI deadlocks into this bug? If PSAPI is being brought in indirectly via dbghelp.dll, PSAPI_VERSION=2 won't fix the deadlock.

(Though I think we should still fix this either way, but it becomes lower priority in that case)

Comment 5

[Carl Corcoran [:ccorcoran]]

To induce this crash I put *(int*)1 = 1; at the end of DllBlocklist_SetDllServices:

> thread 0:
> 00 ntdll!NtWaitForSingleObject+0x14
> 01 KERNELBASE!WaitForSingleObjectEx+0xa2
> 02 xul!google_breakpad::ExceptionHandler::WriteMinidumpOnHandlerThread+0x6d
> 03 xul!google_breakpad::ExceptionHandler::HandleException+0x10e
> 04 KERNELBASE!UnhandledExceptionFilter+0x190
> 05 ntdll!RtlUserThreadStart$filt$0+0x38
> 06 ntdll!_C_specific_handler+0x96
> 07 ntdll!RtlpExecuteHandlerForException+0xd
> 08 ntdll!RtlDispatchException+0x3c6
> 09 ntdll!KiUserExceptionDispatch+0x2e
> 0a mozglue!DllBlocklist_SetDllServices+0x6f
> 0b xul!mozilla::DllServices::Get+0x3d
> 0c xul!XREMain::XRE_mainRun+0x38
> ...
> 
> thread 6
> 00 ntdll!NtWaitForAlertByThreadId+0x14
> 01 ntdll!RtlAcquireSRWLockShared+0x141
> 02 mozglue!DllLoadNotification+0x1a
> 03 ntdll!LdrpSendDllNotifications+0x86
> 04 ntdll!LdrpSendPostSnapNotifications+0xc5
> 05 ntdll!LdrpNotifyLoadOfGraph+0x57
> 06 ntdll!LdrpPrepareModuleForExecution+0x79
> 07 ntdll!LdrpLoadDllInternal+0x19c
> 08 ntdll!LdrpLoadDll+0x10b
> 09 ntdll!LdrLoadDll+0xa4
> 0a mozglue!patched_LdrLoadDll+0x94
> 0b KERNELBASE!LoadLibraryExW+0x17b
> 0c KERNELBASE!LoadLibraryExA+0x31
> 0d dbgcore!Win32LiveSystemProvider::LoadPsApi+0x2e
> 0e dbgcore!Win32LiveSystemProvider::EnumModules+0x12b
> 0f dbgcore!NtWin32LiveSystemProvider::EnumModules+0xe
> 10 dbgcore!GenGetProcessInfo+0x1f5
> 11 dbgcore!MiniDumpProvideDump+0x419
> 12 dbgcore!MiniDumpWriteDump+0x267
> 13 xul!google_breakpad::ExceptionHandler::WriteMinidumpWithExceptionForProcess+0x1d2
> 14 xul!google_breakpad::ExceptionHandler::ExceptionHandlerThreadMain+0xcc
> ...

So indeed PSAPI is being loaded indirectly via MiniDumpWriteDump. During normal execution, I just confirmed we aren't loading psapi.dll; I confirmed that psapi calls are correctly being resolved to kernelbase. Even in webrtc where PSAPI_VERSION=1 is set, it's not actually linking with psapi.lib, so the linker just doesn't resolve PSAPI calls there.

So this bug as described may actually be a non-issue though we should still prevent loading DLLs during crash handling.

Comment 6

[(Away)]

> though we should still prevent loading DLLs during crash handling.

I wonder if we could do a LoadLibrary or call some benign dbghelp function up front... though it looks like a couple functions moved from dbghelp to dbgcore in newer windows, which may add a bit of complication.

Comment 7

[Carl Corcoran [:ccorcoran]]

I did confirm that just LoadLibrary("psapi.dll") at startup prevents this deadlock; that's related bug 1488181.

Comment 8

[Carl Corcoran [:ccorcoran]]

Seems that everything is working as intended; closing as invalid.