471054 - Prune old crash reports in pending/

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Benjamin Smedberg]

If submitting crash reports fails, users can end up with many old crash reports in the pending/ directory. This will happen even more if we start server-side throttling. I'm working on a patch which will prune reports so that we only keep the newest 10.

Comment 1

[Benjamin Smedberg]

Attachment: Sweep old crashes in pending/, rev. 1

Boy, this seems like a lot of code to do something relatively simple, but I can't think of a simpler way.

Comment 2

[Benjamin Smedberg]

Comment on attachment 356530 [details] [diff] [review]
Sweep old crashes in pending/, rev. 1

The Windows bit isn't right.

Comment 3

[Benjamin Smedberg]

Attachment: Sweep old crashes in pending/, rev. 1.1

This one actually works on Windows, instead of just compiling...

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 356549 [details] [diff] [review]
Sweep old crashes in pending/, rev. 1.1

That's an awful lot of platform-specific code mixed into that file. I had been trying to hide as much of it as possible in the crashreporter_platform.cpp files. Couldn't we just add a UIFindFiles function or something, that took a PruneVector& and filled it?

Using FindFilesA is wrong. The rest of the Win32 code uses std:string, but it always uses W functions and explicitly converts from Wide to UTF8 (there are UTF8ToWide and WideToUTF8 helpers in crashreporter_win.cpp). For example:
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/client/crashreporter_win.cpp#1339

RemoveOldDumps seems incorrectly named, since it only deletes one dump. Also, seems like it'd do the wrong thing if you actually passed it a vector whose size was > kSaveCount + 1, but maybe you're ok with that.

+    BOOL ok = true;
+    for (; ok; ok = FindNextFileA(dirlist, &fdata)) {

Any reason you didn't stick the variable decl in the for loop?

Comment 5

[Benjamin Smedberg]

Attachment: Sweep old crashes in pending/, rev. 2

It builds on linux/windows. I've verified correct behavior on Windows, and doing tryserver runs to test elsewhere.

Comment 6

[Benjamin Smedberg]

ok, I've verified correct behavior from tryserver builds

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 357394 [details] [diff] [review]
Sweep old crashes in pending/, rev. 2

+void PruneSavedDumps(const std::string& directory);

UIPruneSavedDumps, just for consistency? We should just wrap all that stuff in a namespace or something...

+++ b/toolkit/crashreporter/client/crashreporter_unix.cpp

Needs a tri-license block.

+    string path = (--dumpfiles.end())->path;

This feels a little too clever to me, I had to think about it for a minute to figure out what it was doing, but the only alternative I could offer would be:
dumpfiles[dumpfiles.size() - 1].path()

Looks good otherwise.

Comment 8

[Benjamin Smedberg]

Attachment: Sweep old crashes in pending/, rev. 2.1

Carrying over review with nits picked, requesting approval 1.9.1

Comment 9

[(not currently active) Ted Mielczarek]

Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/34c447754f1b

Comment 10

[(not currently active) Ted Mielczarek]

I made a typo while fixing up conflicts, so I pushed a followup:
http://hg.mozilla.org/mozilla-central/rev/cbe9e7669db3

Of course I failed to notice it because I only built on Linux. :-/