1536675 - MOZ_CRASH_UNSAFE results in blaming MOZ_Crash instead of the correct function

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Something I noticed in bug 1527502: several unrelated crashes got filed together because the PROCESS-CRASH messages in CI all blame MOZ_Crash(char const*, int, char const*) instead of something more useful. This is probably not the only affected bug.

The common feature of the crashes seems to be the use of MOZ_CRASH_UNSAFE or MOZ_CRASH_UNSAFE_PRINTF, leading to a crash inside the function MOZ_Crash instead of at the site of the macro expansion.

I've filed this under MFBT because that's the component that seems to own the crash macros, but it's possible that this could also be adequately fixed by postprocessing the crash info in the test infrastructure; I don't know much about the details there.

Comment 1

[(Away)]

I guess the compiler is ignoring the inline here (which it is in its right to do): https://searchfox.org/mozilla-central/rev/358f816f63da072145c593e9e2ac36b7250ecd25/mfbt/Assertions.h#307

If we really want to avoid a stack frame for reporting purposes, we should be more forceful with MOZ_ALWAYS_INLINE.

Comment 2

[(Away)]

Attachment: Use a stronger inline on MOZ_Crash to keep it out of Treeherder reporting

MOZ_NO_REALLY_IM_SERIOUS_INLINE_PLZ

Comment 3

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Copying dmajor's comment from phabricator:

This actually won't work because (and I should have checked this earlier) MOZ_Crash's caller, MOZ_CrashPrintf is MOZ_NEVER_INLINE: https://searchfox.org/mozilla-central/rev/a7315d78417179b151fef6108f2bce14786ba64d/mfbt/Assertions.cpp#31

Offhand I don't know if it's safe to change MOZ_CrashPrintf too. It would need more investigation.

Comment 4

[Chris Peterson [:cpeterson]]

(In reply to David Major [:dmajor] from comment #1)

If we really want to avoid a stack frame for reporting purposes, we should be more forceful with MOZ_ALWAYS_INLINE.

btw, I don't think this bug is a problem for Breakpad crash reports from the field because MOZ_Crash is on Socorro's signature skip list (bug 733235):

https://github.com/mozilla-services/socorro/blob/aff7efc24d15abe25e6dc6af9cbcf20d9d503e1d/socorro/signature/siglists/irrelevant_signature_re.txt#L56

Comment 5

[(Away)]

(In reply to Chris Peterson [:cpeterson] from comment #4)

btw, I don't think this bug is a problem for Breakpad crash reports from the
field because MOZ_Crash is on Socorro's signature skip list (bug 733235):

Sure. This bug is about making intermittents more debuggable on Treeherder.

Comment 6

[(Away)]

Noticing that MOZ_CrashPrintf currently does two jobs: "do the printf annotation reporting" and "take down the process", what if we split out those duties? We could keep an out-of-line function for all the printf goop, and return to the caller who would then do an inline MOZ_REALLY_CRASH or equivalent.

Comment 7

[(Away)]

Attachment: Take the crashing out of MOZ_CrashPrintf

It would be helpful if MOZ_CRASH_UNSAFE_PRINTF would do its crashing inline at the caller, so that CI failure logs can blame the right code.

Before this patch, MOZ_CRASH_UNSAFE_PRINTF calls MOZ_CrashPrintf, which does the printf work and crashes.

This patch pulls out the crashing piece at the end, so that MOZ_CrashPrintf only does the printf work, and returns the string to the caller, who will MOZ_Crash inline.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cc2ac1b5534f
Take the crashing out of MOZ_CrashPrintf r=froydnj

Comment 9

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/cc2ac1b5534f

Comment 10

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

There still seem to be crashes with outlined MOZ_Crash; seen on m-i and on m-c. Do you know why we'd still be seeing that?

Comment 11

[(Away)]

Gah, I always forget about MOZ_ALWAYS_INLINE_EVEN_DEBUG. In the meantime, opt builds shouldn't be exhibiting this.

Comment 12

[(Away)]

Attachment: Inline MOZ_Crash even in debug builds

We want MOZ_Crash frames to stay out of Taskcluster logs even on debug builds. Perhaps you could say, especially on debug builds.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c077a1a61629
Inline MOZ_Crash even in debug builds r=froydnj

Comment 14

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/c077a1a61629

Comment 15

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/c077a1a61629

Comment 16

[Intermittent Failures Robot]

1 failures in 2843 pushes (0.0 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• autoland: 1

Platform breakdown:

• linux64: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1536675&startday=2019-04-01&endday=2019-04-07&tree=all