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

Status: VERIFIED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[[:philipp]]

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

Comment 1

[(not currently active) Ted Mielczarek]

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.

Comment 2

[Julien Cristau [:jcristau]]

We can't ship 56 with this bug, or we'll likely DoS our own infra.  Liz FYI.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Looking at this now.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Yeah, this was caused by 1355492. I can craft a low-risk patch. Give me a few moments.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1399796 - UnsubmittedCrashHandler should never check for unsubmitted crash reports if disabled or suppressed.

Bug 1355492 moved the logic for scanning for unsubmitted crash reports out of the initialization
of UnsubmittedCrashHandler, and the initialization is what decided whether or not it was
appropriate to scan in the first place. This was done so that scanning could be deferred
until idle after first paint.

This patch makes it so that the scanning logic first ensures that the UnsubmittedCrashHandler
is actually enabled and not suppressed (which is calculated earlier).

I've also taken the liberty of adding a regression test.

Review commit: https://reviewboard.mozilla.org/r/180338/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/180338/

Comment 6

[:Felipe Gomes (needinfo for replies!)]

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

Comment 7

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1256cadba71b
UnsubmittedCrashHandler should never check for unsubmitted crash reports if disabled or suppressed. r=Felipe

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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)

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 11

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8908692 [details]
Bug 1399796 - UnsubmittedCrashHandler should never check for unsubmitted crash reports if disabled or suppressed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180338/diff/1-2/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c3c6406d94b5
UnsubmittedCrashHandler should never check for unsubmitted crash reports if disabled or suppressed. r=Felipe

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/c3c6406d94b5

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a841920fd8cb (FIREFOX_56b13_RELBRANCH)
https://hg.mozilla.org/releases/mozilla-release/rev/09f4708df24c

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Have we seen the expected drop-off in crash reports coming from beta yet?

Comment 19

[Julien Cristau [:jcristau]]

This hasn't shipped to beta yet.  We should know after 56.0 rc1 goes out tomorrow.

Comment 20

[[:philipp]]

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.

Comment 21

[Mike Conley (:mconley) (:⚙️)]

Excellent, thanks!