1572244 - [mozcrash] Report assertion failures when processing crashes

Status: RESOLVED FIXED

(Testing :: Mozbase, enhancement, P3)

Description

[Eric Rahm [:erahm]]

It would be useful to note that a crash is due to an assertion when dumping a crash. In particular this would be helpful for identifying android test failures in treeherder where the test log does not include the output from Firefox.

I think we'd want to update _process_dump_file so that it parses the .extra file (we might already support that somewhere in mozcrash) and adds the annotation to StackInfo. Then we'd need to update the crash logging function to handle the crash reason.

The assertion info can be found in the MozCrashReason annotation in the .extra file, for example:

MozCrashReason=MOZ_ASSERT(aWindow || aChannel)

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

As of now we do not parse the .extra file in mozcrash. So that would have to be implemented first. I will file a separate bug given that there are also other requests which depend on that.

Comment 2

[Eric Rahm [:erahm]]

(In reply to Eric Rahm [:erahm] from comment #0)

The assertion info can be found in the MozCrashReason annotation in the .extra file, for example:

MozCrashReason=MOZ_ASSERT(aWindow || aChannel)

We're no longer using the ini format, so this will just be a field in the parsed dict, ie

reason = info['MozCrashReason']

Comment 3

[Geoff Brown [:gbrown]]

Should the MozCrashReason affect the PROCESS_CRASH log line? Should it take precedence over the top frame for purposes of bug reporting?

To use a recent concrete example, consider bug 1592769, bug title "Intermittent testing/xpcshell/example/unit/test_check_nsIException_failing.js | application crashed [@ NS_DispatchToMainThread(already_AddRefed<nsIRunnable> &&,unsigned int)]":

https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=273752457&repo=mozilla-inbound&lineNumber=2130

[task 2019-10-30T19:18:06.316Z] 19:18:06  WARNING -  PROCESS-CRASH | testing/xpcshell/example/unit/test_check_nsIException_failing.js | application crashed [@ NS_DispatchToMainThread(already_AddRefed<nsIRunnable> &&,unsigned int)]

and the .extra file has:

MozCrashReason=MOZ_CRASH()

Do we want something like:

[task 2019-10-30T19:18:06.316Z] 19:18:06  WARNING -  PROCESS-CRASH | testing/xpcshell/example/unit/test_check_nsIException_failing.js | MOZ_CRASH()
[task 2019-10-30T19:18:06.316Z] 19:18:06  INFO - Crash at testing/xpcshell/example/unit/test_check_nsIException_failing.js | application crashed [@ NS_DispatchToMainThread(already_AddRefed<nsIRunnable> &&,unsigned int)]

producing bug reports like "Intermittent testing/xpcshell/example/unit/test_check_nsIException_failing.js | MOZ_CRASH()", or

[task 2019-10-30T19:18:06.316Z] 19:18:06  WARNING -  PROCESS-CRASH | testing/xpcshell/example/unit/test_check_nsIException_failing.js | application crashed [@ NS_DispatchToMainThread(already_AddRefed<nsIRunnable> &&,unsigned int)]
[task 2019-10-30T19:18:06.316Z] 19:18:06  INFO - MozCrashReason=MOZ_CRASH()

producing bug reports just like today (but with the extra info readily available in the log)...or something else?

Comment 4

[Kris Wright :KrisWright]

Attachment: Bug 1572244 - Print the MozCrashReason as 'Mozilla crash reason'

As an INFO line under the top crash message, output MozCrashReason in the format Crash Reason: SOME_CRASH_REASON(). This doesn't affect the top line (for bug filing) but issues some extra info from the .extra file. Given this doesn't pass python unit tests, where the .extra file is not proper json, this is still WIP.

Comment 5

[Kris Wright :KrisWright]

So I mentioned in revision D59749 that I would add a test that exercises the _parse_extra_file function. I realize now while returning to this code that by calling the function during process_dump_file we're already exercising the use of the parser. I'm not super familiar with the python-tests, but from my understanding in order to add separate tests that call _parse_extra_file, since it was written as an internal function, I would need to either change the function to not be internal anymore or create a function in CrashInfo specifically for testing _parse_extra_file.

Henrik, is it worth changing the initial function or adding some accessor to add a test with, or is exercising it as a part of check_for_crashes sufficient?

Comment 6

[Kris Wright :KrisWright]

To avoid blocking other bugs, I'm going to go ahead and land this. The functionality (parsing the .extra file and checking for MozCrashReason) is currently being exercised as a part of the check_for_crashes test, since it's used in _process_dump_file. If there is additional testing that we want to use to exercise the new functionality I can file a follow-up bug with the testing.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0a0423b62715
Print the MozCrashReason as 'Mozilla crash reason' r=ahal,gbrown

Comment 8

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/0a0423b62715

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Kris Wright :KrisWright from comment #5)

Henrik, is it worth changing the initial function or adding some accessor to add a test with, or is exercising it as a part of check_for_crashes sufficient?

Sorry for the delay but due to the all hands last week I wasn't able to reply earlier. So please file a new bug to cover the new functionality in the above mentioned test. Therefore update the minidump_files fixture, so that some valid data is inserted, which then can be checked for in various tests.

Comment 10

[Kris Wright :KrisWright]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #9)

Sorry for the delay but due to the all hands last week I wasn't able to reply earlier. So please file a new bug to cover the new functionality in the above mentioned test. Therefore update the minidump_files fixture, so that some valid data is inserted, which then can be checked for in various tests.

I filed Bug 1613178 to address this.

Comment 11

[Geoff Brown [:gbrown]]

I am starting to see "Mozilla crash reason" show up in test crash reports, like bug 1613042, and it looks great. Thanks Kris!