Closed Bug 462197 Opened 12 years ago Closed 7 years ago

The "Remove All Reports" button in about:crashes should remove old InstallTime files

Categories

(Toolkit :: Crash Reporting, enhancement)

x86
Linux
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: fahlmanc_ca, Assigned: jwatt)

References

Details

Attachments

(2 files, 1 obsolete file)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b2pre) Gecko/20081029 Minefield/3.1b2pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b2pre) Gecko/20081029 Minefield/3.1b2pre

Could the option to Remove submitted crash reports in about:crashes also remove InstallTime files?

If they are actually needed, could it remove all files except the one for the build you are using?

(These files pile up if you are using daily builds. Do they accomplish anything useful?)

Reproducible: Always

Steps to Reproduce:
1.
2.
3.
Component: General → Breakpad Integration
Product: Firefox → Toolkit
QA Contact: general → breakpad.integration
We only really need the one corresponding to the BuildID of the build you're currently running. We use them so we can send that data along with your crash reports (the time you installed the build you're currently using). I didn't want to put cleanup code in the startup path, since that could slow down startup considerably. We could make the "remove reports" button clean up old InstallTime files, I suppose, as long as it leaves the current one in place.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Remove submitted crash reports option in about:crashes could remove InstallTime files → Remove submitted crash reports option in about:crashes could remove old InstallTime files
Is there a better way to detect "the current one" than in some way looping over all files and checking last modified times?
Uh, ignore that question.
Summary: Remove submitted crash reports option in about:crashes could remove old InstallTime files → The "Remove All Reports" button in about:crashes should remove old InstallTime files
Attachment #543673 - Flags: review?(ted.mielczarek)
Attachment #543672 - Attachment is patch: true
Part 2 is NOT YET TESTED.

I'm also wondering about part 2, whether we don't want to support giving correct install time data for users running more than one version of Firefox. Maybe there should be a last modified check in there too, and the InstallTime files should only be deleted if they're not for the current build, AND over a year old? Or is that not worth it?
At any rate, I'd like to commit part 1 soonish, since patches I have for other bugs build on that.
Yeah, probably best to only delete InstallDate files older than a certain date. This patch only deletes them if they're over a year old.
Assignee: nobody → jwatt
Attachment #543673 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #543680 - Flags: review?(ted.mielczarek)
Attachment #543673 - Flags: review?(ted.mielczarek)
Now tested BTW.
Comment on attachment 543672 [details] [diff] [review]
part 1 - change variable name in separete patch, to keep real patch cleaner

Review of attachment 543672 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #543672 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 543680 [details] [diff] [review]
part 2 - remove the InstallTime files as suggested in comment 1

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

This looks fine, but can you add a test? The tests in http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/test/browser/ should be good templates. In particular, http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/test/browser/browser_bug471404.js tests that the "Remove all Reports" button is only visible if there are reports present, you could probably reuse that.

Note that those tests override the AppData directory so you can write arbitrary data in and not screw up the user's system.
Attachment #543680 - Flags: review?(ted.mielczarek) → review+
It would probably be good to have a try run on this before landing.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/be9dfaa088b2
https://hg.mozilla.org/mozilla-central/rev/fbe92fdbabc7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.