Tab crash reports have empty comments

RESOLVED FIXED in Firefox 46

Status

()

Core
DOM: Content Processes
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jimm, Assigned: mconley)

Tracking

unspecified
mozilla47
Points:
---

Firefox Tracking Flags

(e10sm8+, firefox46blocking fixed, firefox47 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
This messes up crashstats in that you can filter out empty comments to find the useful data. This might be something we need to fix in socorro or it might be something in tree.

It looks like the bug is that we're adding "Email:" without a value. 

example:
https://crash-stats.mozilla.com/report/list?signature=xul.dll%400x7e254a
(Reporter)

Comment 1

2 years ago
s/can/can't filter out...

Comment 2

2 years ago
You're saying the problem is that we're showing a comment even though the user didn't type anything there?

http://hg.mozilla.org/mozilla-central/annotate/f53533d9eb77/browser/modules/ContentCrashHandlers.jsm#l168 submits the "Comments" annotation unconditionally, so we could fix it there. Doing some filtering in crash-stats also seems reasonable.
Component: IPC → DOM: Content Processes
Ack, yes - we should not supply those values if they trim empty. I can take this.
Assignee: nobody → mconley
tracking-e10s: ? → +
Priority: -- → P2
Created attachment 8716009 [details]
MozReview Request: Bug 1245833 - Don't send empty comments or email addresses in content crash reports. r?felipe

Review commit: https://reviewboard.mozilla.org/r/33675/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33675/
Attachment #8716009 - Flags: review?(felipc)
https://reviewboard.mozilla.org/r/33673/#review30389

So this kinda bums me out because it looks like in at least two places we annotate the crash report with the URL unilaterally:

https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/dom/ipc/ContentParent.cpp#2173

and

https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/dom/ipc/TabChild.cpp#1265

So in order to make sure we don't submit them automatically, I have to overwrite them with the empty string. That seems pretty hackish, but I'm not sure if there's a good reason for us to make sure the last loaded URL is in the crash report by default. In any case, we'll never submit it, even if the user says to include the URL - we'll use the URL that's passed to aboutTabCrashed.js, at least on Desktop.

So I'm overwriting the URL with the empty string if we're not wanting to include it.
Comment on attachment 8716009 [details]
MozReview Request: Bug 1245833 - Don't send empty comments or email addresses in content crash reports. r?felipe

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33675/diff/1-2/
Comment on attachment 8716009 [details]
MozReview Request: Bug 1245833 - Don't send empty comments or email addresses in content crash reports. r?felipe

https://reviewboard.mozilla.org/r/33675/#review30607
Attachment #8716009 - Flags: review?(felipc) → review+

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5728bfd4c648
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47

Comment 10

2 years ago
I think this should be uplifted to 46.
Since not M8/P1, I am posting in-case it gets overlooked.
There is a unconfirmed possibility Socorro on release channel will be taking all these empty comments bypassing its 10% throttle. Even if not the case still good to get rid sooner from results.
Flags: needinfo?(mconley)
Which 10% throttle is this? I'm not familiar with it.
Flags: needinfo?(mconley) → needinfo?(jonathan)

Comment 12

2 years ago
https://crash-stats.mozilla.com/topcrashers/?product=Firefox&version=44.0.2
"The report covers 215350 (44.83%) of all 480318 crashes during this period.
https://crash-stats.mozilla.com/topcrashers/?product=Firefox&version=45.0b
The report covers 85379 (47.76%) of all 178767 crashes during this period.
Beta population is about 2% of Release size. Release all-crashes would be much higher if not throttled.

https://socorro.readthedocs.org/en/latest/development/throttling.html
After a bit more digging finally found again what I read a few days ago about comments.
bug 1247300 comment 3

Updated

2 years ago
Flags: needinfo?(mconley)

Updated

2 years ago
Flags: needinfo?(jonathan)
Ah, gotcha. Thanks Jonathan, I think I understand.
Flags: needinfo?(mconley)
Comment on attachment 8716009 [details]
MozReview Request: Bug 1245833 - Don't send empty comments or email addresses in content crash reports. r?felipe

Approval Request Comment
[Feature/regressing bug #]:

E10s

[User impact if declined]:

No user facing impact. However, (if I understand correctly) we risk overloading our crash processing systems (see bug 1247300 comment 3) because every crash will appear to have a comment (even though that comment will be empty).

[Describe test coverage new/current, TreeHerder]:

Automated test coverage is checking that this is working properly, and it's been on mozilla-central for quite a while now.

[Risks and why]: 

Very low risk.

[String/UUID change made/needed]:

None.
Attachment #8716009 - Flags: approval-mozilla-aurora?
Marking affected for 46 so the uplift request will be visible to sheriffs
status-firefox46: --- → affected
Peter does this sound risky to you as far as overloading Socorro?
Flags: needinfo?(peterbe)
The throttling rules in Socorro checks if a crash has COMMENT. See https://github.com/mozilla/socorro/blob/23e7943b673544c38bfe4255e1976661682c7dae/socorro/collector/throttler.py#L34

It does NOT look at the EMAIL. 

And no, we are not ready to process 100% of all Release crashes at the moment. Please tread carefully :)
Flags: needinfo?(peterbe)

Comment 18

2 years ago
(In reply to Peter Bengtsson [:peterbe] from comment #17)
> The throttling rules in Socorro checks if a crash has COMMENT. See
> https://github.com/mozilla/socorro/blob/
> 23e7943b673544c38bfe4255e1976661682c7dae/socorro/collector/throttler.py#L34

Does that rule mean we process all where a comment exists or where the comment has a length? If the former, we could get into an issue here when we are going to release e10s, if I understand this bug correctly.

> And no, we are not ready to process 100% of all Release crashes at the
> moment. Please tread carefully :)

Right, that's why this question is coming up ;-)
Flags: needinfo?(peterbe)
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #18)
> (In reply to Peter Bengtsson [:peterbe] from comment #17)
> > The throttling rules in Socorro checks if a crash has COMMENT. See
> > https://github.com/mozilla/socorro/blob/
> > 23e7943b673544c38bfe4255e1976661682c7dae/socorro/collector/throttler.py#L34
> 
> Does that rule mean we process all where a comment exists or where the
> comment has a length? If the former, we could get into an issue here when we
> are going to release e10s, if I understand this bug correctly.
> 

If the raw crash JSON HAS a field called "Comments" AND the value of that field is ANYTHING OTHER than '' or null or 0 or false, it will process it. 

In other words, if you have a raw crash JSON that looks like this:

{
  "product": "Firefox",
  "channel": "Release",
  "Comments": "",
  ...
}

then it will apply the 10% throttle rule. 

Does that make sense?
Flags: needinfo?(peterbe)
OK. if you want to uplift this, can you do it behind a flag so it stays in aurora?
Peter do you think that would limit it reasonably?
Flags: needinfo?(mconley)
Or is limiting to nightly/aurora/beta ok?   Beta adds quite a few more users and crashes ...
Flags: needinfo?(peterbe)

Comment 22

2 years ago
mconley, given comment #19, does that mean the 10% throttle will apply fine? If so, we are OK on that line.
If you set the Comments field in the raw crash, but with an empty value, to Socorro it's the same as if there was no Comments field at all. As Peter said, missing, null, '', 0, false are all values that mean the crash report will go through the usual processing rules. Only when the Comments field has a value distinct from the previously listed ones will it go through a different process. 

So, unless I am missing something, this change will have no impact on Socorro.
Flags: needinfo?(peterbe)

Comment 24

2 years ago
My concern was that crash rate might give an unexpected spike, overloading equally valid also.
Just a minor nuisance on other channels when viewing comment lists.

(In reply to Adrian Gaudebert [:adrian] from comment #23)
> If you set the Comments field in the raw crash, but with an empty value, to
> Socorro it's the same as if there was no Comments field at all. As Peter
> said, missing, null, '', 0, false are all values that mean the crash report
> will go through the usual processing rules. Only when the Comments field has
> a value distinct from the previously listed ones will it go through a
> different process. 
> 
> So, unless I am missing something, this change will have no impact on
> Socorro.

Many blank comments currently on aurora/beta for content crashes, which this has fixed in nightly. Is there confidence that comments are blank not space/tab (or probably unlikely but don't want to discount being newline)?
I think we, the Socorro peeps, are struggling to understand what's going on here with the code change. 

But, if you're worried about there being a possible newline character in the comment we can change our code so that that's stripped out too. 

Mike, if in doubt, please ping me on IRC. peterbe@#breakpad
(Reporter)

Comment 26

2 years ago
https://crash-stats.mozilla.com/report/list?product=Firefox&range_unit=days&version=Firefox%3A45.0b4&signature=gfxContext%3A%3APushGroupAndCopyBackground&date=2016-02-23+13%3A20%3A46&range_value=7#tab-comments

Why do I see empty comments here followed by the text "Email:"? This was the original issue I filed this bug against.
Flags: needinfo?(mconley) → needinfo?(peterbe)
(Reporter)

Comment 27

2 years ago
side by side compare between nightly and aurora for the same signature with similar volume - 

nightly:
https://crash-stats.mozilla.com/report/list?signature=FinalizeArenas&date=2016-02-23+13%3A51%3A35&product=Firefox&version=Firefox%3A47.0a1&range_unit=hours&range_value=168#tab-sigsummary

aurora:
https://crash-stats.mozilla.com/report/list?signature=FinalizeArenas&date=2016-02-23+13%3A51%3A46&product=Firefox&version=Firefox%3A46.0a2&range_unit=hours&range_value=168#tab-sigsummary

It looks like mconley's patch fixed this problem on nightly. His patch landed 02-09, and these reports are mostly from builds after that landing. Aurora on the other hand doesn't have the patch.

Comment 28

2 years ago
(In reply to Jim Mathies [:jimm] from comment #27)
> It looks like mconley's patch fixed this problem on nightly. His patch
> landed 02-09, and these reports are mostly from builds after that landing.
> Aurora on the other hand doesn't have the patch.

OK, that means the code current on Aurora is not ready to go to release in the future without that fix.
Indeed, if all crashes have a value in the Comments field, we on the Socorro side will be in trouble. So I think we misunderstood the original issue. Please make sure this issue does not hit release, for it would make us process all the crash reports we receive, which we cannot handle with our current infra. :)
Flags: needinfo?(peterbe)
(Reporter)

Comment 30

2 years ago
Could this bug cause an over reporting on crashstats of e10s specific crashes compared to non-e10s reports?
Flags: needinfo?(adrian)
(Reporter)

Comment 31

2 years ago
Liz, for aurora approval, we have socorro sign off on this.
Flags: needinfo?(lhenry)
(Reporter)

Updated

2 years ago
tracking-e10s: + → m8+

Comment 32

2 years ago
(In reply to Jim Mathies [:jimm] from comment #30)
> Could this bug cause an over reporting on crashstats of e10s specific
> crashes compared to non-e10s reports?

Not outside of Firefox release channel. And the fixed code in Nightly apparently also will not overreport even there.
Flags: needinfo?(adrian)
I think I got this completely backwards, thanks for the clarification!
Flags: needinfo?(lhenry)
Comment on attachment 8716009 [details]
MozReview Request: Bug 1245833 - Don't send empty comments or email addresses in content crash reports. r?felipe

This should fix a critical issue with e10s that would otherwise overload crash-stats.
OK to uplift to aurora.
Attachment #8716009 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Just to make sure this lands properly and we don't lose track, marking this as a blocker for 46 release.
tracking-firefox46: --- → blocking
https://hg.mozilla.org/releases/mozilla-aurora/rev/50b1c7bdf690 by mike
status-firefox46: affected → fixed
Gah, you beat me to it. Thanks. :)
You need to log in before you can comment on or make changes to this bug.