Closed Bug 471054 Opened 12 years ago Closed 11 years ago

Prune old crash reports in pending/

Categories

(Toolkit :: Crash Reporting, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1

People

(Reporter: benjamin, Assigned: benjamin)

Details

Attachments

(1 file, 3 obsolete files)

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.
Boy, this seems like a lot of code to do something relatively simple, but I can't think of a simpler way.
Attachment #356530 - Flags: review?(ted.mielczarek)
Comment on attachment 356530 [details] [diff] [review]
Sweep old crashes in pending/, rev. 1

The Windows bit isn't right.
Attachment #356530 - Flags: review?(ted.mielczarek)
This one actually works on Windows, instead of just compiling...
Attachment #356530 - Attachment is obsolete: true
Attachment #356549 - Flags: review?(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?
Attachment #356549 - Flags: review?(ted.mielczarek) → review-
It builds on linux/windows. I've verified correct behavior on Windows, and doing tryserver runs to test elsewhere.
Attachment #356549 - Attachment is obsolete: true
Attachment #357394 - Flags: review?(ted.mielczarek)
ok, I've verified correct behavior from tryserver builds
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.
Attachment #357394 - Flags: review?(ted.mielczarek) → review+
Carrying over review with nits picked, requesting approval 1.9.1
Attachment #357394 - Attachment is obsolete: true
Attachment #359043 - Flags: review+
Attachment #359043 - Flags: approval1.9.1?
Attachment #359043 - Flags: approval1.9.1?
Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/34c447754f1b
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
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. :-/
You need to log in before you can comment on or make changes to this bug.