NS_DebugBreak should not use mozalloc_abort for crashing

NEW
Unassigned

Status

()

3 years ago
3 years ago

People

(Reporter: fitzgen, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

mozalloc_abort implies unhandleable OOM but this is actually an assertion failure. That is a bit confusing and could send people down the wrong rabbit hole.

Stack for an example crash I was seeing:

> 06:11:58 INFO - 0 xpcshell!mozalloc_abort(char const*) [mozalloc_abort.cpp:463ddf9d456a : 33 + 0x0]
> 06:11:58 INFO - rax = 0x0000000000000000 rdx = 0x0000000000000000
> 06:11:58 INFO - rcx = 0x00007fd7efaa4f4d rbx = 0x00007fd7efd77828
> 06:11:58 INFO - rsi = 0x00007fd7efd78a80 rdi = 0x00007fd7efd77180
> 06:11:58 INFO - rbp = 0x00007fffe356be10 rsp = 0x00007fffe356be00
> 06:11:58 INFO - r8 = 0x00007fd7f8fbe9c0 r9 = 0x79726f6d656d2f63
> 06:11:58 INFO - r10 = 0x00007fffe356bb90 r11 = 0x00007fd7efa2c940
> 06:11:58 INFO - r12 = 0x00007fd7efd77828 r13 = 0x00007fd7f5eb52a0
> 06:11:58 INFO - r14 = 0x00007fd7f6351c93 r15 = 0x00007fd7f5ecce89
> 06:11:58 INFO - rip = 0x0000000000405256
> 06:11:58 INFO - Found by: given as instruction pointer in context
> 06:11:58 INFO - 1 libxul.so!NS_DebugBreak [nsDebugImpl.cpp:463ddf9d456a : 472 + 0xc]
> 06:11:58 INFO - rbx = 0x00007fffe356de5b rbp = 0x00007fffe356c280
> 06:11:58 INFO - rsp = 0x00007fffe356be20 r12 = 0x00007fd7efd77828
> 06:11:58 INFO - r13 = 0x00007fd7f5eb52a0 r14 = 0x00007fd7f6351c93
> 06:11:58 INFO - r15 = 0x00007fd7f5ecce89 rip = 0x00007fd7f327c6f4
> 06:11:58 INFO - Found by: call frame info
> 06:11:58 INFO - 2 libxul.so!mozilla::ShutdownXPCOM(nsIServiceManager*) [XPCOMInit.cpp:463ddf9d456a : 1018 + 0x25]
> 06:11:58 INFO - rbx = 0x00007fd7e9ab4000 rbp = 0x00007fffe356c2d0
> 06:11:58 INFO - rsp = 0x00007fffe356c290 r12 = 0x00007fd7f32aa1a0
> 06:11:58 INFO - r13 = 0x00007fd7e5002000 r14 = 0x00007fffe356c8c0
> 06:11:58 INFO - r15 = 0x00007fd7e9a0c2a8 rip = 0x00007fd7f32fc4b7
> 06:11:58 INFO - Found by: call frame info 
> ... etc ...

Comment 1

3 years ago
I don't understand the problem exactly. We previously had three different ways of "crashing", two of which were buggy on various platforms. Having the single mozalloc_abort method of crashing reliably is really nice.

Are you just worried about the naming of the stack frames?
Flags: needinfo?(nfitzgerald)
Yeah the naming is confusing to me, especialy when I asked on #developers if mozalloc_abort meant I had an unhandlable OOM as the name implies (originally missed the assertion failure log) and was told that yes I did have an unhandleable OOM.

I think `moz_abort` would be a better name in this light.

(Aside: Why not just use MOZ_ASSERT for assertions, though? Those are always really clear to me.)
Flags: needinfo?(nfitzgerald)

Comment 3

3 years ago
MOZ_ASSERT is debug-only.

MOZ_CRASH is the release-mode version of that, but it doesn't take any message parameter. crash-stats uses the mozalloc_abort frame and its message (which is annotated in the crash report) to help distinguish between intentional crashes and "real" crashes in a method.
How do you feels about s/mozalloc_abort/moz_abort/ so that it doesn't imply OOM?

I can no longer find any xpcomFunctions.debugBreak implementations on dxr, but I had thought that NS_DebugBreak only caused assertion failures on debug mode. That is certainly the behavior I was seeing in my try pushes.

Comment 5

3 years ago
I don't much care about the name. I don't know if glandium or somebody else does.

NS_DebugBreak(NS_DEBUG_ABORT or NS_DEBUG_BREAK) is fatal in all builds. I'm not sure what NS_DEBUG_ASSERTION does in a release build. Mostly it is hidden behind macros which are debug-only.
You need to log in before you can comment on or make changes to this bug.