1843354 - Crash in [@ OOM | large | __delayLoadHelper2 | _tailMerge_dbghelp.dll | EnsureSymInitialized]

Status: RESOLVED FIXED

(Core :: mozglue, defect, P2)

Description

[Ryan VanderMeulen [:RyanVM]]

Crash report: https://crash-stats.mozilla.org/report/index/1c0ad6db-bbe0-4229-a330-f25fb0230713

Reason: FACILITY_VISUALCPP / ERROR_MOD_NOT_FOUND

Top 10 frames of crashing thread:

0  KERNELBASE.dll  RaiseException  
1  mozglue.dll  __delayLoadHelper2  /builds/worker/workspace/obj-build/mozglue/build/D:/a/_work/1/s/src/vctools/delayimp/delayhlp.cpp:301
2  mozglue.dll  _tailMerge_dbghelp.dll  
3  mozglue.dll  EnsureSymInitialized  mozglue/misc/StackWalk.cpp:558
3  mozglue.dll  MozDescribeCodeAddress  mozglue/misc/StackWalk.cpp:579
4  mozglue.dll  PrintStackFrameBuf  mozglue/misc/StackWalk.cpp:1014
4  mozglue.dll  PrintStackFrame  mozglue/misc/StackWalk.cpp:1027
5  mozglue.dll  DoMozStackWalkThread  mozglue/misc/StackWalk.cpp:405
6  mozglue.dll  MozStackWalk  mozglue/misc/StackWalk.cpp:431
6  mozglue.dll  MozWalkTheStack  mozglue/misc/StackWalk.cpp:1043

Comment 1

[Mike Hommey [:glandium]]

Somehow I thought we didn't try to print a stack for MOZ_CRASH on non-debug builds, but apparently I'm wrong. We've been doing that since bug 1005730, over 9 years!
This is questionable in and of itself, especially on Windows, where that stack would be printed... nowhere (except if running firefox from the command line, redirecting its output to a pipe or a file...)
But the most interesting part of this bug is that walking the stack itself crashes because dbghelp.dll is not available? Should we LoadLibrary it instead of delayload-link it?

Comment 2

[Yannis Juglaret [:yannis]]

For these signatures (with the OOM marker), in the Nightly crash reports the last error value is ERROR_COMMITMENT_LIMIT. It is likely that for most of these crashes, the reason we fail to load dbghelp.dll is the low memory condition itself (failure to commit additional pages for the DLL). I think that indeed the cleanest solution here would be to make EnsureSymInitialized() return false instead of crashing, if dbghelp.dll fails to load (with LoadLibrary). This should result in dispatching the underlying crashes to their proper signatures instead of these ones.

If we additionally want to guarantee the ability to print a stack here, we would have to preload dbghelp.dll during initialization (e.g. by calling EnsureSymInitialized()), so that we are sure that it is mapped before the low memory condition occurs. But, I don't think we need this guarantee here. We could also restrict the conditions under which MOZ_DUMP_ASSERTION_STACK is defined on Windows, but that's probably not needed either if failure to load dbghelp.dll is no longer fatal.

Comment 3

[Yannis Juglaret [:yannis]]

For the non-OOM signatures, dbghelp.dll could be failing to load for other reasons, but the solution described in comment 2 should also help dispatch these crashes to the true signature they belong to. Overall this should solve bug 1783926.

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:glandium, could you have a look please?

For more information, please visit BugBot documentation.

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:glandium, could you have a look please?

For more information, please visit BugBot documentation.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

The bug is linked to a topcrash signature, which matches the following criteria:

• Top 20 desktop browser crashes on beta
• Top 10 content process crashes on beta

:yannis, could you consider increasing the severity of this top-crash bug?

For more information, please visit BugBot documentation.

Comment 7

[Yannis Juglaret [:yannis]]

Attachment: Bug 1843354 - Rewrite DbgHelp intialization in StackWalk.cpp. r=glandium

Stack walking can currently produces crashes when we fail to delay-load
DbgHelp.dll. This patch ensures that the library is already loaded in
the process before we try to call any delay-imported function from it.
The patch also improves thread-safety for our DbgHelp initialization
code.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/724deb75d52d
Rewrite DbgHelp intialization in StackWalk.cpp. r=glandium

Comment 10

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/724deb75d52d

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:yannis, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox119 to wontfix.

For more information, please visit BugBot documentation.

Comment 12

[Yannis Juglaret [:yannis]]

No need to uplift. After the fix these crashes should dispatch to OOM | small and/or bugs like bug 1472062 as far as I can tell.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

This grafts cleanly to ESR115. Is it worth taking there too for the sake of better bucketing?

Comment 14

[Yannis Juglaret [:yannis]]

Given the volume on ESR it sounds worth having proper bucketing indeed. However, the patch from bug 1860767 should result in proper bucketing as well, and it is simpler and makes more sense. It removes unnecessary use of DbgHelp in shipped builds, whereas the patch I wrote here just prevents bad things from happening when/if unnecessary use occurs. So, I would rather recommend taking the patch from bug 1860767 than this one.