Closed
Bug 1274395
Opened 5 years ago
Closed 5 years ago
Cleanup pending crash reports between test runs
Categories
(Testing :: General, defect)
Testing
General
Tracking
(firefox49 fixed)
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: gbrown, Assigned: gbrown)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
11.55 KB,
patch
|
jmaher
:
review+
|
Details | Diff | Splinter Review |
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•5 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?
Comment 2•5 years ago
|
||
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•5 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•5 years ago
|
||
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 5•5 years ago
|
||
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•5 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.
Comment 7•5 years ago
|
||
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•5 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?
Comment 9•5 years ago
|
||
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•5 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•5 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•5 years ago
|
||
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 13•5 years ago
|
||
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+
Comment 14•5 years ago
|
||
hmm, do we need to bump versions of mozcrash?
![]() |
Assignee | |
Comment 16•5 years ago
|
||
Pardon the rush here. I'm happy to re-open or follow-up if there is additional feedback.
Comment 17•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6251c1e010a1
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment 18•5 years ago
|
||
(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•5 years 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•5 years ago
|
Attachment #8756496 -
Flags: feedback?(ted)
You need to log in
before you can comment on or make changes to this bug.
Description
•