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

Status: NEW

(Toolkit :: Crash Reporting, defect)

Description

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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

Comment 1

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(Also, the MozCrashReason mechanism is probably safer.)

Comment 2

[Nicholas Nethercote [inactive]]

Yes, please! I'd also be happy if we just removed NS_RUNTIMEABORT in favour of MOZ_CRASH.

Comment 3

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=178ed3ca1119

Comment 4

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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

Comment 5

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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

Comment 6

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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

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

Comment 7

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: 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/

Comment 8

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: 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/

Comment 9

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: 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/

Comment 10

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: 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/

Comment 11

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: 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/

Comment 12

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: 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/

Comment 13

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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/

Comment 14

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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/

Comment 15

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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 16

[Nathan Froyd [:froydnj]]

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?

Comment 17

[Nathan Froyd [:froydnj]]

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 18

[Nathan Froyd [:froydnj]]

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.

Comment 19

[Jeff Walden [:Waldo]]

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

Comment 20

[Jeff Walden [:Waldo]]

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.

Comment 21

[Jeff Walden [:Waldo]]

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.

Comment 22

[Chris Peterson [:cpeterson]]

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

Comment 23

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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

Comment 24

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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.

Comment 25

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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

Comment 26

[Kan-Ru Chen [:kanru] (UTC+9)]

Do you think this is good to land?

Comment 27

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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.

Comment 28

[BugBot [:suhaib / :marco]]

Clear a needinfo that is pending on an inactive user.

For more information, please visit auto_nag documentation.