unify MozCrashReason and AbortMessage crash annotations (and use __func__ in them and for assertions)

ASSIGNED
Assigned to

Status

()

Toolkit
Crash Reporting
ASSIGNED
a year ago
6 months ago

People

(Reporter: dbaron, Assigned: dbaron, NeedInfo)

Tracking

(Blocks: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(6 attachments)

(Assignee)

Description

a year ago
Different types of aborts send different types of crash annotations (AbortMessage from bug 918389, and MozCrashReason from bug 1183355).  We should unify these into a single crash annotation.

Right now the AbortMessage includes the PID, the file, and the line number, and the string given to NS_RUNTIMEABORT(); the MozCrashReason includes only the string given.  In the unified form, we should include the file and the string given, should not include the line number, and should maybe include the function if __func__ as suggested by ekr on dev-platform (?) works.

We should probably unify these to MozCrashReason if only because Socorro handles it better (bug 1276991, which I could swear was not only already filed, but which I'd filed a dupe of... yet searching turns up nothing).
(Assignee)

Updated

a year ago
Component: XPCOM → Breakpad Integration
Product: Core → Toolkit
(Assignee)

Comment 1

a year ago
(Also, the MozCrashReason mechanism is probably safer.)
Yes, please! I'd also be happy if we just removed NS_RUNTIMEABORT in favour of MOZ_CRASH.
(Assignee)

Updated

a year ago
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
(Assignee)

Comment 3

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=178ed3ca1119
(Assignee)

Comment 4

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=51c596fef67e shows that I wasn't completely crazy, but it seems like __func__ isn't as portable as I thought... although probably I'm doing something wrong since there are other uses in the tree.  Will poke around once I'm less jetlagged...
(Assignee)

Comment 5

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3251c28213d9 is a bit better so far.

(__func__ is a macro only on some compilers, like the one I use.  Per C++11 it should behave like a local variable, and thus can't be used in concatenation of adjacent string literals.)
(Assignee)

Comment 6

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f65251da4ad5&group_state=expanded

I think this is a good try run, but these days...
(Assignee)

Updated

a year ago
Summary: unify MozCrashReason and AbortMessage crash annotations → unify MozCrashReason and AbortMessage crash annotations (and use __func__ in them and for assertions)
(Assignee)

Comment 7

a year ago
Created attachment 8764075 [details]
Bug 1277448 patch 1 - Move __func__ polyfill for older MSVC versions from xpcom/base/Logging.h to mfbt/Assertions.h

Note that xpcom/base/Logging.h currently includes mfbt/Assertions.h (as
mozilla/Assertions.h).

Review commit: https://reviewboard.mozilla.org/r/60120/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/60120/
Attachment #8764075 - Flags: review?(jwalden+bmo)
Attachment #8764076 - Flags: review?(nfroyd)
Attachment #8764077 - Flags: review?(nfroyd)
Attachment #8764078 - Flags: review?(nfroyd)
Attachment #8764079 - Flags: review?(jwalden+bmo)
Attachment #8764080 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 8

a year ago
Created attachment 8764076 [details]
Bug 1277448 patch 2 - Pass __func__ to NS_DebugBreak and friends.

This will allow them to print the function name in patches 3 and 4.

Note that this does not modify the nsIDebug interface, since that
requires finding relevant JS callers, some of which may be in
extensions.

Review commit: https://reviewboard.mozilla.org/r/60122/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/60122/
(Assignee)

Comment 9

a year ago
Created attachment 8764077 [details]
Bug 1277448 patch 3 - Print the function name in NS_DebugBreak assertion messages.

Review commit: https://reviewboard.mozilla.org/r/60124/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/60124/
(Assignee)

Comment 10

a year ago
Created attachment 8764078 [details]
Bug 1277448 patch 4 - Send a MozCrashReason annotation for NS_RUNTIMEABORT instead of AbortMessage.

This excludes the PID and the line number that are present in the
AbortMessage annotation, and formats the annotation somewhat
consistently with other MozCrashReason annotations.  (It adds file and
function name information that is not present in them; the file
information will added to them in patch 5, but I don't see how to add
the function information to them.)

This uses the same printing mechanism already used in this function so
that we don't add any additional crashing risk by using the heap.

This removes the AbortMessage annotation and the xpcom_runtime_abort()
note.  However, these were only present in crashes in the parent
process; MozCrashReason shows up for child process crashes as well.  It
therefore moves to a more reliable and single way to find and search for
the abort messages in crash-stats.

Review commit: https://reviewboard.mozilla.org/r/60126/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/60126/
(Assignee)

Comment 11

a year ago
Created attachment 8764079 [details]
Bug 1277448 patch 5 - Add file to crash annotations for MOZ_CRASH and MOZ_RELEASE_ASSERT.

This doesn't add function name (like I did for NS_RUNTIMEABORT) because
__func__ is (per C++ 2011, [dcl.fct.def.general], clause 8) a variable
rather than a macro, and thus can't be portably used as part of the
concatenation of adjacent string literals.

Review commit: https://reviewboard.mozilla.org/r/60128/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/60128/
(Assignee)

Comment 12

a year ago
Created attachment 8764080 [details]
Bug 1277448 patch 6 - Make MFBT assertion messages print the function name.

Review commit: https://reviewboard.mozilla.org/r/60130/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/60130/
(Assignee)

Updated

a year ago
Attachment #8764078 - Flags: review?(nfroyd)
(Assignee)

Updated

a year ago
Attachment #8764079 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 13

a year ago
Comment on attachment 8764078 [details]
Bug 1277448 patch 4 - Send a MozCrashReason annotation for NS_RUNTIMEABORT instead of AbortMessage.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60126/diff/1-2/
Attachment #8764078 - Flags: review?(nfroyd)
Attachment #8764079 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 14

a year ago
Comment on attachment 8764079 [details]
Bug 1277448 patch 5 - Add file to crash annotations for MOZ_CRASH and MOZ_RELEASE_ASSERT.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60128/diff/1-2/
(Assignee)

Comment 15

a year ago
Comment on attachment 8764080 [details]
Bug 1277448 patch 6 - Make MFBT assertion messages print the function name.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/60130/diff/1-2/
Comment on attachment 8764076 [details]
Bug 1277448 patch 2 - Pass __func__ to NS_DebugBreak and friends.

https://reviewboard.mozilla.org/r/60122/#review57066

Tenative r=me, but I would like to have some statistics on what this (and/or this whole patch set) does to binary size.  ISTR somebody looking at this and finding that we had a decent chunk of space devoted to just filenames, and that's after the linker was able to merge duplicate filenames together. ( I know I've gotten wins from the IPC code by coalescing generated NS_RUNTIMEABORT calls into a single function)  With `__func__`, we're much less likely to benefit from string merging.

Would you please investigate what the size changes are here, and report back?
Attachment #8764076 - Flags: review?(nfroyd) → review+
Attachment #8764077 - Flags: review?(nfroyd) → review+
Comment on attachment 8764077 [details]
Bug 1277448 patch 3 - Print the function name in NS_DebugBreak assertion messages.

https://reviewboard.mozilla.org/r/60124/#review57070
Comment on attachment 8764078 [details]
Bug 1277448 patch 4 - Send a MozCrashReason annotation for NS_RUNTIMEABORT instead of AbortMessage.

https://reviewboard.mozilla.org/r/60126/#review57074

r=me assuming my understanding below is correct.

::: xpcom/base/nsDebugImpl.cpp:413
(Diff revision 2)
> +      if (aFunction) {
> +        PrintToBuffer(", function %s", aFunction);
> +      }
> +#undef PrintToBuffer
> +
> +      CrashReporter::AnnotateMozCrashReason(crashReasonBuffer.buffer);

Passing the allocated-on-the-stack buffer here, which AnnotateMozCrashReason stores into a global variable, is safe because at this point, this function is effectively noreturn, and the stack will not be unwound before the crash reporter takes over?

I think this is consistent with other uses of AnnotateMozCrashReason, but I believe this is the only place where we're passing a non-static string to AnnotateMozCrashReason.  So I'd like to confirm that my understanding here is correct.
Attachment #8764078 - Flags: review?(nfroyd) → review+
Comment on attachment 8764075 [details]
Bug 1277448 patch 1 - Move __func__ polyfill for older MSVC versions from xpcom/base/Logging.h to mfbt/Assertions.h

https://reviewboard.mozilla.org/r/60120/#review59262
Attachment #8764075 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8764080 [details]
Bug 1277448 patch 6 - Make MFBT assertion messages print the function name.

https://reviewboard.mozilla.org/r/60130/#review59264

It's worth noting that last I recall, the syntax of __func__ was unspecified.  So if anyone ever decides to bucket crashes by __func__-string across platforms or compilers, it probably isn't going to work.
Attachment #8764080 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8764079 [details]
Bug 1277448 patch 5 - Add file to crash annotations for MOZ_CRASH and MOZ_RELEASE_ASSERT.

https://reviewboard.mozilla.org/r/60128/#review59276

__FILE__ is also not going to be usable across platforms.  Wonder if compilers have ever considered a command-line flag to control what portion of the file path appears in __FILE__ -- seems like it'd be useful to get nice paths in output.
Attachment #8764079 - Flags: review?(jwalden+bmo) → review+
Blocks: 1289663
(In reply to Nicholas Nethercote [:njn] from comment #2)
> Yes, please! I'd also be happy if we just removed NS_RUNTIMEABORT in favour
> of MOZ_CRASH.

btw, bug 1296189 will replace 271 calls to NS_RUNTIMEABORT("some string literal message") with MOZ_CRASH(), leaving just 23 calls to NS_RUNTIMEABORT() using variables as arguments.
(Assignee)

Comment 23

11 months ago
Comparative try runs to compare binary size:

Without patches:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2459fa98e4f6

With patches:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ec31ae32067
(Assignee)

Comment 24

11 months ago
Linux opt .bz2: 0.12% increase
Linux opt libxul.so: 0.5% increase
Mac dmg: 0.15% increase
win32 zip: 0.06% increase

So I should probably back off on passing these things in non-DEBUG builds.
(Assignee)

Comment 25

10 months ago
Comparative try runs to compare binary size:

without patches:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=653c4d0caf81c77e0e267db95013c506cf0373b9

with patches other than patch 5:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa237dd9baecc31374d225a17e51b54a8b411554
See Also: → bug 1296189
Blocks: 1259255
Do you think this is good to land?
Flags: needinfo?(dbaron)
(Assignee)

Comment 27

6 months ago
The approach used in bug 1338574 should probably be good for fixing the codesize issue here too, so I should probably take a look at actually landing the relevant parts of this.
You need to log in before you can comment on or make changes to this bug.