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

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, enhancement)

Description

[crf]

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.

Comment 1

[(not currently active) Ted Mielczarek]

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.

Comment 2

[Jonathan Watt [:jwatt]]

Is there a better way to detect "the current one" than in some way looping over all files and checking last modified times?

Comment 3

[Jonathan Watt [:jwatt]]

Uh, ignore that question.

Comment 4

[Jonathan Watt [:jwatt]]

Attachment: part 1 - change variable name in separete patch, to keep real patch cleaner

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: part 2 - remove the InstallTime files as suggested in comment 1

Comment 6

[Jonathan Watt [:jwatt]]

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?

Comment 7

[Jonathan Watt [:jwatt]]

At any rate, I'd like to commit part 1 soonish, since patches I have for other bugs build on that.

Comment 8

[Jonathan Watt [:jwatt]]

Attachment: part 2 - remove the InstallTime files as suggested in comment 1

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.

Comment 9

[Jonathan Watt [:jwatt]]

Now tested BTW.

Comment 10

[(not currently active) Ted Mielczarek]

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

Comment 11

[(not currently active) Ted Mielczarek]

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.

Comment 12

[(not currently active) Ted Mielczarek]

It would probably be good to have a try run on this before landing.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/be9dfaa088b2
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbe92fdbabc7

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/be9dfaa088b2
https://hg.mozilla.org/mozilla-central/rev/fbe92fdbabc7