1274395 - Cleanup pending crash reports between test runs

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Geoff Brown [:gbrown]]

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

Comment 1

[Geoff Brown [:gbrown]]

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?

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

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

Comment 3

[Geoff Brown [:gbrown]]

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

Comment 4

[Geoff Brown [:gbrown]]

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

Comment 5

[Joel Maher ( :jmaher ) (UTC-4) (on leave, back October 1st)]

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.

Comment 6

[Geoff Brown [:gbrown]]

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

Comment 7

[(not currently active) Ted Mielczarek]

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.

Comment 8

[Geoff Brown [:gbrown]]

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

Comment 9

[(not currently active) Ted Mielczarek]

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?

Comment 10

[Geoff Brown [:gbrown]]

(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

Comment 11

[Geoff Brown [:gbrown]]

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

Comment 12

[Geoff Brown [:gbrown]]

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

Comment 13

[Joel Maher ( :jmaher ) (UTC-4) (on leave, back October 1st)]

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.

Comment 14

[Joel Maher ( :jmaher ) (UTC-4) (on leave, back October 1st)]

hmm, do we need to bump versions of mozcrash?

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6251c1e010a1

Comment 16

[Geoff Brown [:gbrown]]

Pardon the rush here. I'm happy to re-open or follow-up if there is additional feedback.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6251c1e010a1

Comment 18

[(not currently active) Ted Mielczarek]

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

Comment 19

[Geoff Brown [:gbrown]]

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.