Closed
Bug 471054
Opened 14 years ago
Closed 13 years ago
Prune old crash reports in pending/
Categories
(Toolkit :: Crash Reporting, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a1
People
(Reporter: benjamin, Assigned: benjamin)
Details
Attachments
(1 file, 3 obsolete files)
9.92 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•14 years ago
|
||
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)
Assignee | ||
Comment 2•14 years ago
|
||
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)
Assignee | ||
Comment 3•14 years ago
|
||
This one actually works on Windows, instead of just compiling...
Attachment #356530 -
Attachment is obsolete: true
Attachment #356549 -
Flags: review?(ted.mielczarek)
Comment 4•14 years ago
|
||
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-
Assignee | ||
Comment 5•14 years ago
|
||
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)
Assignee | ||
Comment 6•14 years ago
|
||
ok, I've verified correct behavior from tryserver builds
Comment 7•14 years ago
|
||
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+
Assignee | ||
Comment 8•14 years ago
|
||
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?
Assignee | ||
Updated•13 years ago
|
Attachment #359043 -
Flags: approval1.9.1?
Comment 9•13 years ago
|
||
Pushed to m-c: http://hg.mozilla.org/mozilla-central/rev/34c447754f1b
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
Target Milestone: --- → mozilla1.9.3a1
Comment 10•13 years ago
|
||
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.
Description
•