Closed Bug 814086 Opened 9 years ago Closed 9 years ago

Limit the size of the pending queue of crash reports

Categories

(Firefox OS Graveyard :: General, defect, P1)

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-basecamp:+, b2g18 fixed)

RESOLVED FIXED
blocking-basecamp +
Tracking Status
b2g18 --- fixed

People

(Reporter: kairo, Assigned: hub)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 811778 makes sure we only send crash reports over wifi, bug 814078 makes sure we submit queued up reports when we make a wifi connection.

As that might not be the case for a long time for some users, it would be good to make sure the queue of reports in pending/ is not filling up the device. Also, after some time, the reports probably get useless to us anyhow, so I guess it's better if we implement some mechanism to limit the size (or age) of this queue.
blocking-basecamp: --- → ?
Assignee: nobody → hub
blocking-basecamp: ? → +
Depends on: 814078
How long do you want to keep them queued?
Flags: needinfo?(kairo)
(In reply to Hub Figuiere [:hub] from comment #1)
> How long do you want to keep them queued?

I don't have a good idea what's reasonable. For now, I'd guess one week would probably be enough. In production, maybe even a month might make sense, but the real question is how much space we take up. Let's do a week for now, I guess it's probably trivial to change that at some point.
Flags: needinfo?(kairo)
Setting priority based on triage discussions.  Feel free to decrease priority if you disagree.
Priority: -- → P1
The desktop client removes the oldest reports to limit it to a maximum of 10 reports in pending. (Note that this logic doesn't currently fire for plugin crashes, so you can accumulate a huge amount of those between browser crashes.)
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/client/crashreporter_unix_common.cpp#28
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #4)
> The desktop client removes the oldest reports to limit it to a maximum of 10
> reports in pending. (Note that this logic doesn't currently fire for plugin
> crashes, so you can accumulate a huge amount of those between browser
> crashes.)

Well, in that case, we probably don't need to do anything here - though, most of our crashes on B2G are content, does that mean the logic doesn't fire there as well? If so, I guess we need to do the work in this bug to make it fire.
Correct, that logic only exists in the desktop crashreporter client. I think we should just replicate it in B2G, and run it after every content and whole-app crash.
Good, I agree with that, thanks. Hub, see Ted's pointer for what we need to do there.
What's the approximate size of a report?  i.e., what's the average size of 10 crash reports?  We're worried about space constraints all over the place, and crashes can be huge if we let them be.
It varies, but they're usually 50-100kb in size each.
Attachment #690980 - Flags: review?(fabrice)
Attachment #690980 - Flags: review?(benjamin)
Comment on attachment 690980 [details] [diff] [review]
Bug 814086 - Limit the size of the pending queue of crash reports to 10.

I don't think that "keep" should be a parameter here. It should either be a constant or read from a pref. I also wonder whether this should run in a worker thread, since it's doing quite a bit of expensive I/O. We should probably run this regularly, after startup and after a content-process crash is detected, just to keep the disk space in check.
Attachment #690980 - Flags: review?(benjamin) → review-
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #11)
> Comment on attachment 690980 [details] [diff] [review]
> Bug 814086 - Limit the size of the pending queue of crash reports to 10.
> 
> I don't think that "keep" should be a parameter here. It should either be a
> constant or read from a pref. I also wonder whether this should run in a
> worker thread, since it's doing quite a bit of expensive I/O. We should
> probably run this regularly, after startup and after a content-process crash
> is detected, just to keep the disk space in check.

According to the previous comments it was decided that the purge was done when submitting, as it is done in the desktop version - where they set the number to 10. I don't mind setting that limit in CrashSubmit.jsm instead of the shell.
It is definitely called when we get a content crash.
The fact that we only purge when submitting is a bug in the desktop version, leftover from when there weren't plugin or content processes so the crash submission UI was shown on each crash. We should fix the desktop version to purge every time a plugin crash is detected (though not in this bug).

> It is definitely called when we get a content crash.

I don't see this in the patch. I only see us purging after the crash is *submitted*, which means that if the user is never on wifi or never choose to submit crashes, that their disk can be filled up with old crashes.
Ah right, I should move it above.
Updated patch. We now trigger when a crash needs to be submitted.
Attachment #690980 - Attachment is obsolete: true
Attachment #690980 - Flags: review?(fabrice)
Attachment #691054 - Flags: review?(fabrice)
Attachment #691054 - Flags: review?(benjamin)
Comment on attachment 691054 [details] [diff] [review]
Bug 814086 - Limit the size of the pending queue of crash reports to 10.

+      if (matches) {
+        let dump = extra.clone();
+	dump.leafName = matches[1] + '.dmp';

There's funky indentation here, please fix before commit. This looks safe for branch landings. I do wonder whether this shouldn't end up on a background thread, though. File a followup?
Attachment #691054 - Flags: review?(benjamin) → review+
Hub, great to see this land. Please don't forget to file a follow up to move it off main thread as per Benjamin (or if you did already please comment the # here).
Blocks: 820817
See bug 820817 for the follow up.
(In reply to Hub Figuiere [:hub] from comment #21)
> https://hg.mozilla.org/releases/mozilla-beta/rev/36aa0c7486e1

This needs to land on mozilla-b2g18 (and shouldn't have actually landed on mozilla-beta, but not a big deal). Once landed, please mark the whiteboard with [status-b2g18:fixed], since bug 820574 hasn't been fully resolved yet.
Come to think of it, I think this change will automatically come over tonight thanks to the sheriffs. But keep in mind that we should be landing B2G-specific bugs on mozilla-aurora/mozilla-b2g18, but not mozilla-beta at this point.
We have a branch now?

*sigh*

sorry. I sounds like I didn't get the memo.
(In reply to Alex Keybl [:akeybl] from comment #22)
> (In reply to Hub Figuiere [:hub] from comment #21)
> > https://hg.mozilla.org/releases/mozilla-beta/rev/36aa0c7486e1
> 
> This needs to land on mozilla-b2g18 (and shouldn't have actually landed on
> mozilla-beta, but not a big deal). Once landed, please mark the whiteboard
> with [status-b2g18:fixed], since bug 820574 hasn't been fully resolved yet.

Backed out of beta.

https://hg.mozilla.org/releases/mozilla-beta/rev/d575fe59b2f1
https://hg.mozilla.org/mozilla-central/rev/6a3f97ee997d
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Attachment #691054 - Flags: review?(fabrice)
Whiteboard: [status-b2g18:fixed]
You need to log in before you can comment on or make changes to this bug.