Closed Bug 1572244 Opened 5 years ago Closed 4 years ago

[mozcrash] Report assertion failures when processing crashes

Categories

(Testing :: Mozbase, enhancement, P3)

Version 3
enhancement

Tracking

(firefox74 fixed)

RESOLVED FIXED
mozilla74
Tracking Status
firefox74 --- fixed

People

(Reporter: erahm, Assigned: KrisWright)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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)

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.

Priority: -- → P3

(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']

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?

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.

Assignee: nobody → kwright
Status: NEW → ASSIGNED
Blocks: 1603499

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?

Flags: needinfo?(hskupin)
Attachment #9120531 - Attachment description: Bug 1572244 - Print the MozCrashReason as crash_reason → Bug 1572244 - Print the MozCrashReason as 'Mozilla crash reason'

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.

Pushed by kwright@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0a0423b62715
Print the MozCrashReason as 'Mozilla crash reason' r=ahal,gbrown
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla74

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

Flags: needinfo?(hskupin) → needinfo?(kwright)

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

Flags: needinfo?(kwright)

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

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: