814086 - Limit the size of the pending queue of crash reports

Status: RESOLVED FIXED

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

Description

[Robert Kaiser]

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.

Comment 1

[Hubert Figuiere [:hub]]

How long do you want to keep them queued?

Comment 2

[Robert Kaiser]

(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.

Comment 3

[Andrew Overholt [:overholt]]

Setting priority based on triage discussions.  Feel free to decrease priority if you disagree.

Comment 4

[(not currently active) Ted Mielczarek]

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

Comment 5

[Robert Kaiser]

(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.

Comment 6

[(not currently active) Ted Mielczarek]

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.

Comment 7

[Robert Kaiser]

Good, I agree with that, thanks. Hub, see Ted's pointer for what we need to do there.

Comment 8

[Damon Sicore (:damons)]

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.

Comment 9

[(not currently active) Ted Mielczarek]

It varies, but they're usually 50-100kb in size each.

Comment 10

[Hubert Figuiere [:hub]]

Attachment: Bug 814086 - Limit the size of the pending queue of crash reports to 10.

Comment 11

[Benjamin Smedberg]

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.

Comment 12

[Hubert Figuiere [:hub]]

(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.

Comment 13

[Benjamin Smedberg]

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.

Comment 14

[Hubert Figuiere [:hub]]

Ah right, I should move it above.

Comment 15

[Hubert Figuiere [:hub]]

Attachment: Bug 814086 - Limit the size of the pending queue of crash reports to 10.

Updated patch. We now trigger when a crash needs to be submitted.

Comment 16

[Benjamin Smedberg]

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?

Comment 17

[Hubert Figuiere [:hub]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a3f97ee997d

Comment 18

[David Bolter [:davidb] (NeedInfo me for attention)]

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).

Comment 19

[Hubert Figuiere [:hub]]

See bug 820817 for the follow up.

Comment 20

[Hubert Figuiere [:hub]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d8bb1f84a4a5

Comment 21

[Hubert Figuiere [:hub]]

https://hg.mozilla.org/releases/mozilla-beta/rev/36aa0c7486e1

Comment 22

[Alex Keybl [:akeybl]]

(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.

Comment 23

[Alex Keybl [:akeybl]]

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.

Comment 24

[Hubert Figuiere [:hub]]

We have a branch now?

*sigh*

sorry. I sounds like I didn't get the memo.

Comment 25

[Hubert Figuiere [:hub]]

(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

Comment 26

[Hubert Figuiere [:hub]]

http://hg.mozilla.org/releases/mozilla-b2g18/rev/7f03b3d7948b

Comment 27

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6a3f97ee997d