Cleanup pending crash reports between test runs

RESOLVED FIXED in Firefox 49

Status

Testing
General
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: gbrown, Assigned: gbrown)

Tracking

(Blocks: 1 bug)

unspecified
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
See https://bugzilla.mozilla.org/show_bug.cgi?id=1269998#c43 .. https://bugzilla.mozilla.org/show_bug.cgi?id=1269998#c48.

Also:

<blassey> gbrown: we have pending crash reports on our test machines
<jmaher> gbrown: it seems that upon cleanup we need to delete all files from [userdir]/Crashes instead of [userdir]/profile/*
<blassey> and we want to delete them between runs
<blassey> gbrown, jmaher we need to do both
<jmaher> blassey: yes, thanks for clarifying
<jmaher> s/instead of/in addition to/
<blassey> and its [AppDir]/Crash\ Reports
<gbrown> so, just as we delete the profile between runs, we want to delete the crash reports directory
<jmaher> gbrown: yes
<gbrown> and that's on all platforms, desktop + android?
<jmaher> there is a cleanup() routine, and I believe a couple of try/except clauses as well
<jmaher> gbrown: my understanding is all platforms, although I am not sure if android puts crashreports in the same spot
(Assignee)

Comment 1

2 years ago
I would like to simply mozfile.remove(crashDir) in places like https://hg.mozilla.org/mozilla-central/annotate/c67dc1f9fab8/layout/tools/reftest/runreftest.py#l399 and https://hg.mozilla.org/mozilla-central/annotate/c67dc1f9fab8/testing/mochitest/runtests.py#l1771. But if crashDir = <uappdir>/"Crash Reports", I can't think of a reliable way to determine <uappdir> in python.

If we run in browser, then determining <uappdir> is easy -- https://hg.mozilla.org/mozilla-central/annotate/c67dc1f9fab8/toolkit/crashreporter/CrashSubmit.jsm#l82. But then how/when would we cleanup?
I mistyped on irc, it is [UAppData] and that is well specified. See https://mxr.mozilla.org/mozilla-central/source/dom/system/OSFileConstants.cpp#107
(Assignee)

Comment 3

2 years ago
I verified the value of "[UAppData]/Crash Reports" on try, in a mochitest:

Linux
/home/cltbld/.mozilla/firefox/Crash Reports

Windows (7)
C:\Users\cltbld\AppData\Roaming\Mozilla\Firefox\Crash Reports

OS X (10.10)
/Users/cltbld/Library/Application Support/Firefox/Crash Reports
(Assignee)

Comment 4

2 years ago
Created attachment 8756071 [details] [diff] [review]
remove pending crash reports before running tests

https://treeherder.mozilla.org/#/jobs?repo=try&revision=69066a17fbe8 shows the pending crash reports being found and deleted on most runs. I am seeing some clipboard failures on Windows - any chance that's related?

This is still a no-op on Android. Android emulator tests should be okay without any cleanup: they get a clean image on every run. But local device tests and autophone might need crash report cleanups -- I'll handle that in a separate patch.
Attachment #8756071 - Flags: review?(jmaher)
Comment on attachment 8756071 [details] [diff] [review]
remove pending crash reports before running tests

Review of attachment 8756071 [details] [diff] [review]:
-----------------------------------------------------------------

a thought for a followup patch/bug- I think this code is pretty sound.

::: testing/mozbase/mozcrash/mozcrash/mozcrash.py
@@ +514,5 @@
> +        location = os.path.expanduser("~/.mozilla/firefox/Crash Reports")
> +    logger = get_logger()
> +    if os.path.exists(location):
> +        logger.info("Removing pending crash reports at '%s'" % location)
> +        mozfile.remove(location)

sometimes there are issues with files on windows not able to be removed as something else has control of it, should we try/except this?  Possibly we just deal with this rare case when it comes up.
Attachment #8756071 - Flags: review?(jmaher) → review+
(Assignee)

Comment 6

2 years ago
(In reply to Joel Maher (:jmaher) from comment #5)
> sometimes there are issues with files on windows not able to be removed as
> something else has control of it, should we try/except this?  

Yes, let's add a try/except. I wasn't going to, because mozfile already catches some errors, and most existing mozfile.remove clients do not try/except. But not all exceptions are caught, and some clients have added try/except; let's catch and ignore.
Um, I don't want to do this unconditionally in our test harnesses. This would remove pending crash reports from developers' machines, which is not something we should do.
(Assignee)

Comment 8

2 years ago
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #7)
> Um, I don't want to do this unconditionally in our test harnesses. This
> would remove pending crash reports from developers' machines, which is not
> something we should do.

Agreed. So, perhaps add an option to remove pending crash reports and set that option when called from mozharness?
That seems fine, yes. I would also like to figure out *why* we have pending crash reports there. Do we have test harnesses that aren't properly configuring the crash reporter, or are these leftovers from someone manually running Firefox and crashing it?
(Assignee)

Comment 10

2 years ago
(In reply to Geoff Brown [:gbrown] (pto May 28-June 13) from comment #4)
> I am seeing
> some clipboard failures on Windows - any chance that's related?

Results seem no worse than a base push without my changes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a39caf431794
(Assignee)

Comment 11

2 years ago
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #9)
> That seems fine, yes. I would also like to figure out *why* we have pending
> crash reports there. Do we have test harnesses that aren't properly
> configuring the crash reporter, or are these leftovers from someone manually
> running Firefox and crashing it?

Try pushes are finding pending crash reports on most runs. I don't think we would see that much frequency from people manually running Firefox on test machines. I don't have any other insight into the origin of the pending crash reports.
(Assignee)

Comment 12

2 years ago
Created attachment 8756496 [details] [diff] [review]
remove pending crash reports before running tests

Now with --cleanup-crashes option for mochitest and reftest harnesses.

I verified that 'mach mochitest|reftest' does not cleanup pending crash reports.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d966d7dd10f1
Attachment #8756071 - Attachment is obsolete: true
Attachment #8756496 - Flags: review?(jmaher)
Attachment #8756496 - Flags: feedback?(ted)
Comment on attachment 8756496 [details] [diff] [review]
remove pending crash reports before running tests

Review of attachment 8756496 [details] [diff] [review]:
-----------------------------------------------------------------

oh, this is nice.  I will want to do this for talos.  Especially as we share similar machines.
Attachment #8756496 - Flags: review?(jmaher) → review+
hmm, do we need to bump versions of mozcrash?
Blocks: 1275692
(Assignee)

Updated

2 years ago
Blocks: 1275702

Comment 15

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6251c1e010a1
(Assignee)

Comment 16

2 years ago
Pardon the rush here. I'm happy to re-open or follow-up if there is additional feedback.
https://hg.mozilla.org/mozilla-central/rev/6251c1e010a1
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox49: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
(In reply to Geoff Brown [:gbrown] (pto May 28-June 13) from comment #11)
> Try pushes are finding pending crash reports on most runs. I don't think we
> would see that much frequency from people manually running Firefox on test
> machines. I don't have any other insight into the origin of the pending
> crash reports.

We should probably figure out where these are coming from :-/
(Assignee)

Comment 19

a year ago
I still see most buildbot-originated Linux x64 opt (for example) mochitest and reftest jobs finding pending crash reports. However, I can find no such taskcluster-originated jobs. I don't have an explanation.
(Assignee)

Updated

a year ago
Attachment #8756496 - Flags: feedback?(ted)
You need to log in before you can comment on or make changes to this bug.