Closed Bug 1399796 Opened 4 years ago Closed 4 years ago

Unsubmitted crash reports get sent off automatically despite settings to the contrary

Categories

(Toolkit :: Crash Reporting, defect)

56 Branch
Unspecified
All
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 blocking fixed
firefox57 --- fixed

People

(Reporter: philipp, Assigned: mconley)

References

Details

(Keywords: regression)

Attachments

(1 file)

[Tracking Requested - why for this release]:
in case this behaviour filters through to release builds, by a ballpark estimate that could lead to a 10-fold increase of crash reports that are sent off to the crash stats servers. not sure how well our infrastructure would cope with that...


there is a mechanism in pre-release builds that is prompting users to send off unsubmitted crash reports to mozilla through an infobar. in bug 1303067 this was enabled for beta builds as long as EARLY_BETA_OR_EARLIER is set.

though as usual EARLY_BETA_OR_EARLIER got unset after 56.0b5 this cycle (and i see that browser.crashReports.unsubmittedCheck.enabled & browser.crashReports.unsubmittedCheck.autoSubmit are indeed set to false by default in about:config on current late 56.0 betas) the incoming flow of infobar crashes wasn't affected by this at all as opposed to prior cycles. this is visualised in this graph showing crash reports coming through the infobar for the most common signature:
https://crash-stats.mozilla.com/signature/?product=Firefox&submitted_from_infobar=__true__&release_channel=beta&signature=IPCError-browser%20%7C%20ShutDownKill&date=%3E%3D2017-03-14T09%3A06%3A59.000Z&date=%3C2017-09-14T09%3A06%3A59.000Z#graphs (in past cycles the that has stopped somewhere mid-beta - in 56.0b it's going on for the whole time).

in the last week 125k crashes per day on 56.0b are coming through the infobar - by a rough estimate (multiplying by 50) this would translate to something like 6 million additional daily reports being sent to socorro on release if the same issue was present there as well.
I looked at recent changes to ContentCrashHandlers.jsm and this stood out in one of the diffs:
https://hg.mozilla.org/mozilla-central/rev/67205d8212f1#l2.60

I think this is a regression from bug 1355492--this new code is unconditionally calling `UnsubmittedCrashHandler.checkForUnsubmittedCrashReports`, where the old code did that in an observer service callback that only got registered if the enabled pref was set.
Blocks: 1355492
Flags: needinfo?(mconley)
Summary: Infobar submitted crashes didn't stop after EARLY_BETA_OR_EARLIER was unset on 56.0b → Unsubmitted crash reports get sent off automatically despite settings to the contrary
We can't ship 56 with this bug, or we'll likely DoS our own infra.  Liz FYI.
Flags: needinfo?(lhenry)
Looking at this now.
Assignee: nobody → mconley
Flags: needinfo?(mconley)
Yeah, this was caused by 1355492. I can craft a low-risk patch. Give me a few moments.
Comment on attachment 8908692 [details]
Bug 1399796 - UnsubmittedCrashHandler should never check for unsubmitted crash reports if disabled or suppressed.

https://reviewboard.mozilla.org/r/180338/#review185498
Attachment #8908692 - Flags: review?(felipc) → review+
Comment on attachment 8908692 [details]
Bug 1399796 - UnsubmittedCrashHandler should never check for unsubmitted crash reports if disabled or suppressed.

Approval Request Comment
[Feature/Bug causing the regression]:

Bug 1355492.

[User impact if declined]:

Users on release will see the "You have unsubmitted crash reports" notification if they have unsubmitted crash reports. We're only supposed to show that for EARLY_BETA_OR_EARLIER.

[Is this code covered by automated tests?]:

Yes, and I've added a regression test for this case.

[Has the fix been verified in Nightly?]:

Not yet.

[Needs manual test from QE? If yes, steps to reproduce]: 

None - the automated test covers it.

[List of other uplifts needed for the feature/fix]:

None.

[Is the change risky?]:

No.

[Why is the change risky/not risky?]:

Adding some extra logic to bail out of the checking function if we're disabled.

[String changes made/needed]:

None.
Attachment #8908692 - Flags: approval-mozilla-release?
Attachment #8908692 - Flags: approval-mozilla-beta?
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1256cadba71b
UnsubmittedCrashHandler should never check for unsubmitted crash reports if disabled or suppressed. r=Felipe
Backed out for eslint failure at browser/modules/ContentCrashHandlers.jsm:675: Async method 'checkForUnsubmittedCrashReports' expected no return value:

https://hg.mozilla.org/integration/autoland/rev/20d129c15a9990da780859ca97fff236a1052e05

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=1256cadba71bc9e0677e4f92174bc76ad8952eb5&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=131319767&repo=autoland

 TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/modules/ContentCrashHandlers.jsm:675:7 | Async method 'checkForUnsubmittedCrashReports' expected no return value. (consistent-return)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/modules/ContentCrashHandlers.jsm:682:9 | Async method 'checkForUnsubmittedCrashReports' expected no return value. (consistent-return)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/modules/ContentCrashHandlers.jsm:685:5 | Async method 'checkForUnsubmittedCrashReports' expected no return value. (consistent-return)
Flags: needinfo?(mconley)
Please check that bc's browser/modules/test/browser/browser_UnsubmittedCrashHandler.js also passes: https://treeherder.mozilla.org/logviewer.html#?job_id=131323646&repo=autoland
Bah, linting, plus the fact that I needed to flip the enabled pref to false before the test I added. *sigh*.

I've run eslint and the browser_UnsubmittedCrashHandler.js as well[1], and both pass for me.

[1]: I swear I ran this before I posted my initial patch, but I must have failed to ./mach build faster or something - the original patch should not have passed for me locally.
Flags: needinfo?(mconley)
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c3c6406d94b5
UnsubmittedCrashHandler should never check for unsubmitted crash reports if disabled or suppressed. r=Felipe
Duplicate of this bug: 1400388
https://hg.mozilla.org/mozilla-central/rev/c3c6406d94b5
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment on attachment 8908692 [details]
Bug 1399796 - UnsubmittedCrashHandler should never check for unsubmitted crash reports if disabled or suppressed.

Please uplift to m-r so that we don't DDoS our crash reporting system.
56 is on m-r now but this should still make it to beta for 57 when we re-sync m-c to m-b next week.
Flags: needinfo?(lhenry)
Attachment #8908692 - Flags: approval-mozilla-release?
Attachment #8908692 - Flags: approval-mozilla-release+
Attachment #8908692 - Flags: approval-mozilla-beta?
Attachment #8908692 - Flags: approval-mozilla-beta-
Have we seen the expected drop-off in crash reports coming from beta yet?
Flags: needinfo?(madperson)
This hasn't shipped to beta yet.  We should know after 56.0 rc1 goes out tomorrow.
yes, the problem looks solved after the rollout of rc1 earlier today - the share of infobar to non-infobar crashes in the first release candidate build is 1:20 now, while it was 1:1 in the first hours of 56.0b12.
Flags: needinfo?(madperson)
Excellent, thanks!
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.