1201994 - NS_DebugBreak should not use mozalloc_abort for crashing

Status: RESOLVED FIXED

(Core :: XPCOM, task, P5)

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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

[Benjamin Smedberg]

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?

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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

Comment 3

[Benjamin Smedberg]

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.

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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

[Benjamin Smedberg]

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.

Comment 6

[Chris Peterson [:cpeterson]]

NS_DebugBreak() still calls mozalloc_abort(aMsg) (via Abort()) to crash on assertion failure:

https://searchfox.org/mozilla-central/rev/b24799980a929597dcc553cb0854aa6c960c82b5/xpcom/base/nsDebugImpl.cpp#452

It should probably call MOZ_CRASH_UNSAFE(aMsg).

Comment 7

[Chris Peterson [:cpeterson]]

Attachment: Bug 1201994 - Replace mozalloc_abort() with MOZ_CRASH_UNSAFE() in NS_DebugBreak(). r?#xpcom-reviewers

mozalloc_abort() implies OOM. NS_DebugBreak() is used for other debug breaks and aborts. Calling MOZ_CRASH_UNSAFE(aMsg) instead of MOZ_CRASH() is unlikely to cause new OOM crashes because NS_DebugBreak() already did a lot of work to format aMsg and walk the stack before calling Abort() and MOZ_CRASH_UNSAFE(aMsg).

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/888bdef269f4
Replace mozalloc_abort() with MOZ_CRASH_UNSAFE() in NS_DebugBreak(). r=xpcom-reviewers,mccr8

Comment 9

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/888bdef269f4