MOZ_CRASH_UNSAFE results in blaming MOZ_Crash instead of the correct function

RESOLVED FIXED in Firefox 68

Status

()

defect
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: jld, Assigned: dmajor)

Tracking

unspecified
mozilla68
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox68 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.

Assignee

Comment 1

3 months ago

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.

Blocks: 1527502
See Also: 1527502
Assignee

Comment 2

3 months ago

MOZ_NO_REALLY_IM_SERIOUS_INLINE_PLZ

Attachment #9053688 - Attachment description: Use a strong inline on MOZ_Crash to keep it out of Treeherder reporting → Use a stronger inline on MOZ_Crash to keep it out of Treeherder reporting
Attachment #9053688 - Attachment is obsolete: true

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.

(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

Assignee

Comment 5

3 months ago

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

Assignee

Comment 6

3 months ago

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.

Assignee

Comment 7

3 months ago

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

3 months ago
Pushed by dmajor@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cc2ac1b5534f
Take the crashing out of MOZ_CrashPrintf r=froydnj

Comment 9

3 months ago
bugherder
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Assignee: nobody → dmajor

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?

Flags: needinfo?(dmajor)
Assignee

Comment 11

3 months ago

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

Status: RESOLVED → REOPENED
Flags: needinfo?(dmajor)
Resolution: FIXED → ---
Assignee

Comment 12

3 months ago

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

3 months ago
Pushed by dmajor@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c077a1a61629
Inline MOZ_Crash even in debug builds r=froydnj

Comment 14

3 months ago
bugherder
Status: REOPENED → RESOLVED
Closed: 3 months ago3 months ago
Resolution: --- → FIXED
Comment hidden (Intermittent Failures Robot)
You need to log in before you can comment on or make changes to this bug.