User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:49.0) Gecko/20100101 Firefox/49.0 Build ID: 20160616004032 Steps to reproduce: 1. Disable crash reporter in options 2. Crash browser (?) 3. Open browser Actual results: See a message at the bottom saying "you have an unsubmitted crash report". Clicking that opens a tab for about:crashes with a report, and it's even listed as submitted as of today. Expected results: Nothing. Disabling crash reporter should disable crash reporter, no? This happened on Developer Edition 49.0a2

Reproducible Version 49.0a2 Build ID 20160718004012 User Agent Mozilla/5.0 (Windows NT 10.0; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0

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

Comment on attachment 8775535 [details] Bug 1287178 - Don't prompt to submit crash reports if user has disabled crash submission in preferences. https://reviewboard.mozilla.org/r/67722/#review64792 Thanks for taking this on Aryx. Just a minor issue, should be pretty straight forward. ::: browser/components/nsBrowserGlue.js:854 (Diff revision 1) > + let cr = Components.classes["@mozilla.org/toolkit/crash-reporter;1"]. > + getService(Components.interfaces.nsICrashReporter); I think I'd rather see a lazyServiceGetter be defined at the top of nsBrowserGlue.js, like here: http://searchfox.org/mozilla-central/rev/a3aa2c099970654c82999246a3c27444421f8dcd/toolkit/content/browser-child.js#19 Also in that file, I'm not seeing us checking the existence of CrashReporter, so I think it's pretty safe to assume that if MOZ_CRASHREPORTER is defined that CrashReporter will also be defined. We'll also want to check the enabled state. So to sum, I think this conditional should look like: ```JavaScript if (!CrashReporter.enabled || !CrashReporter.submitReports) { return; } ```

Comment on attachment 8775535 [details] Bug 1287178 - Don't prompt to submit crash reports if user has disabled crash submission in preferences. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67722/diff/1-2/

So I looked into this a bit more closely. It looks like "Enable Crash Reporter" is a bit erroneous itself - whether or not that checkbox is enabled, the crash reporter will still appear for a full browser process crash. That checkbox is mostly used to check whether or not the submission of the crash report is checked by default when that dialog appears. That preference is also not something that's stored in the preferences service, but in an OS-specific location (like the Windows Registry). I guess this is so the crash reporter dialog can access it without spawning a Gecko or accessing the user's profile. Feedback on how we want to proceed here, blassey?

I think shorlander is reworking how that pref works, so I'll defer to him

Comment on attachment 8775535 [details] Bug 1287178 - Don't prompt to submit crash reports if user has disabled crash submission in preferences. https://reviewboard.mozilla.org/r/67722/#review65716

From shorlander: New proposal: - Add a new button that says "Always Submit" - Remove the "Ignore" button and leave the X for dismiss since they are effectively the same - Remove the "Enable Crash Reporter" pref (since it doesn't really do anything) - Add a Pref for "Automatically Submit Un-submitted Crash Reports" -- Description: "Nightly will automatically submit un-submitted crash reports to help Mozilla make your browser more stable and secure" - Add a new behavior to the notification bar that will auto dismiss it and not ask you anymore if you don't interact with it after a given amount of time (see bug 1280381 for how we treated the search suggest opt-in prompt)

Do you mind if I take this, aryx?

Go ahead.

(In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #8) > From shorlander: > - Add a new behavior to the notification bar that will auto dismiss it and > not ask you anymore if you don't interact with it after a given amount of > time (see bug 1280381 for how we treated the search suggest opt-in prompt) can we not have this timeout behavior on pre-release builds? I understand not wanting to annoy Release users, but pre-release users have opted into testing

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #11) > (In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #8) > > From shorlander: > > - Add a new behavior to the notification bar that will auto dismiss it and > > not ask you anymore if you don't interact with it after a given amount of > > time (see bug 1280381 for how we treated the search suggest opt-in prompt) > can we not have this timeout behavior on pre-release builds? I understand > not wanting to annoy Release users, but pre-release users have opted into > testing That is true, but if you haven't interacted with this or made a choice after seeing it for a while you probably aren't going to. If you aren't going to submit crash logs then it's just UI cruft and it doesn't benefit anyone. We could investigate different criteria or perhaps show it periodically (e.g. once a month)? But leaving it up indefinitely even for pre-release users doesn't seem like the right thing to do.

I've still got some tests to write for this, but I think these current patches in isolation can probably be reviewed, and is enough to do some manual testing with. I'll upload some automated test patches soon. In the meantime, I have a question for shorlander: Assuming the above (modulo sorting out the length of time for auto-hiding the notification per release channel) holds, I assume this will be okay to ride up the trains to our release channel? If so, do we still want to keep the "View" button on the notification bar for those release users? "View" takes the users to about:crashes which is... cryptic at best, and super technical. Should we keep that View button on the pre-release channels only?

I have a question for mheubusch as well - we're replacing the "Enable Crash Reporter" preference in about:preferences (under Advanced, Data Choices) with a new preference for automatically submitting un-submitted crash reports. mheubusch, is the following language sufficient to convey that?: Label: "Automatically submit un-submitted crash reports" Description: "&brandShortName; will automatically submit un-submitted crash reports to help &vendorShortName; make your browser more stable and secure" See attachment for what this looks like in practice.

The text feels awkward to me "Automatically submit un-submitted crash reports ...". How is this different from a user's perspective than "Automatically submit crash reports ..." ?

(In reply to Mike Conley (:mconley) - (Digging through needinfos and reviews) from comment #21) > Created attachment 8789547 [details] > The preference in about:preferences > > I have a question for mheubusch as well - we're replacing the "Enable Crash > Reporter" preference in about:preferences (under Advanced, Data Choices) > with a new preference for automatically submitting un-submitted crash > reports. > > mheubusch, is the following language sufficient to convey that?: > > Label: "Automatically submit un-submitted crash reports" > Description: "&brandShortName; will automatically submit un-submitted crash > reports to help &vendorShortName; make your browser more stable and secure" > > See attachment for what this looks like in practice. I agree with :jaws that while there may be a technical nuance you are trying to express with "submit un-submitted" this will only confuse the user. If it is not incorrect, can we simply say: Label: Automatically submit crash reports Description: Crash reports help Mozilla make your browser more stable and secure Let me know if I misunderstand the nuances - I can schedule time to chat.

(In reply to mheubusch from comment #23) > I agree with :jaws that while there may be a technical nuance you are trying > to express with "submit un-submitted" this will only confuse the user. If > it is not incorrect, can we simply say: > > Label: Automatically submit crash reports > Description: Crash reports help Mozilla make your browser more stable and > secure > > Let me know if I misunderstand the nuances - I can schedule time to chat. Yeah, I agree with both of you that the original language was a bit clumsy. I have a minor concern, however: As I mentioned earlier, the original checkbox that I'm replacing here controlled whether or not the "Tell Mozilla about this crash so they can fix it" option was automatically checked in the crash dialog if the main process crashed[1]. The option to submit _unsubmitted_ crash reports doesn't actually affect the "Tell Mozilla about this crash so they can fix it" option or the dialog at all. The dialog will still appear if the main process crashes (as it should, since we need to give the user the ability to fill in more data about what went wrong). I worry that having the checkbox say "Automatically submit crash reports" is inconsistent if we still bring up the crash reporter dialog. Is that worry unfounded? [1]: See http://imgur.com/a/wzJl4

Comment on attachment 8789501 [details] Bug 1287178 - Add a dismissed event for <xul:notification> when they're dismissed via the close button. https://reviewboard.mozilla.org/r/77474/#review76324

Adding dev-doc-needed for new dismissed event for XUL notifications. We should update https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Method/appendNotification#Notification_box_events.

Comment on attachment 8789503 [details] Bug 1287178 - Replace erroneous Enable Crash Reporter pref with one that lets users opt-in to sending backlogged reports. https://reviewboard.mozilla.org/r/77478/#review76362 ::: browser/components/preferences/in-content/advanced.js (Diff revision 2) > - this._setupLearnMoreLink("toolkit.crashreporter.infoURL", > - "crashReporterLearnMore"); Just realized that I broke the Learn More link with this removal. I'll put this and the call to initSubmitCrashes back.

Comment on attachment 8789502 [details] Bug 1287178 - Move unsubmitted crash report handling into ContentCrashHandlers.jsm. https://reviewboard.mozilla.org/r/77476/#review76344 r+ with the pref name changes that we talked about ::: browser/modules/ContentCrashHandlers.jsm:470 (Diff revision 2) > + label: gNavigatorBundle.GetStringFromName("pendingCrashReports.viewAll"), > + callback: function() { > + chromeWin.openUILinkIn("about:crashes", "tab"); > + return true; > + }, I'm unsure if viewing about:crashes is at all useful for an end user, if this ever gets enabled on Release. For Nightly/Aurora that sounds ok. But still, about:crashes would need to better highlight the unsubmitted crash reports, including ignoring the previously ignored ones. That way, opening about:crashes can answer "what will get submitted if I click Yes?". Otherwise it's just sending the user to a cryptic page.

Comment on attachment 8789504 [details] Bug 1287178 - Make UnsubmittedCrashHandler pay attention to preference for automatically submitting un-submitted crash reports. https://reviewboard.mozilla.org/r/77480/#review76368 ::: browser/locales/en-US/chrome/browser/browser.properties:726 (Diff revision 2) > # See: http://developer.mozilla.org/en/docs/Localization_and_Plurals > # #1 is the number of pending crash reports > pendingCrashReports.label = You have an unsubmitted crash report;You have #1 unsubmitted crash reports > pendingCrashReports.viewAll = View > pendingCrashReports.submitAll = Submit > +pendingCrashReports.submitAlways = Submit Always I'm not a native speaker so what feels right/strange for me isn't always correct, but isn't "Always Submit" more natural than "Submit Always"?

I agree with Felipe in comment #29. I would also prefer if we didn't use the word "Submit" here (and elsewhere) but maybe that's fodder for another bug. "SUBMIT ALWAYS TO MOZILLA!" sounds like something a dictator would say. Can we use "Always Send" instead?

Comment on attachment 8789502 [details] Bug 1287178 - Move unsubmitted crash report handling into ContentCrashHandlers.jsm. https://reviewboard.mozilla.org/r/77476/#review76344 > I'm unsure if viewing about:crashes is at all useful for an end user, if this ever gets enabled on Release. For Nightly/Aurora that sounds ok. > > But still, about:crashes would need to better highlight the unsubmitted crash reports, including ignoring the previously ignored ones. That way, opening about:crashes can answer "what will get submitted if I click Yes?". Otherwise it's just sending the user to a cryptic page. Thanks, felipe. I'm going to leave the "View" button for now, but file a follow-up button to hide it for release users.

Comment on attachment 8789503 [details] Bug 1287178 - Replace erroneous Enable Crash Reporter pref with one that lets users opt-in to sending backlogged reports. https://reviewboard.mozilla.org/r/77478/#review77104

Pushed by mconley@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1599a90dcf2a Add a dismissed event for <xul:notification> when they're dismissed via the close button. r=Felipe https://hg.mozilla.org/integration/autoland/rev/aa1dbdd224f6 Move unsubmitted crash report handling into ContentCrashHandlers.jsm. r=Felipe https://hg.mozilla.org/integration/autoland/rev/c94848691f8a Replace erroneous Enable Crash Reporter pref with one that lets users opt-in to sending backlogged reports. r=jaws https://hg.mozilla.org/integration/autoland/rev/4c854d8255d2 Make UnsubmittedCrashHandler pay attention to preference for automatically submitting un-submitted crash reports. r=Felipe

https://hg.mozilla.org/mozilla-central/rev/1599a90dcf2a https://hg.mozilla.org/mozilla-central/rev/aa1dbdd224f6 https://hg.mozilla.org/mozilla-central/rev/c94848691f8a https://hg.mozilla.org/mozilla-central/rev/4c854d8255d2

Looking at the changes, I guess we want this to ride the train.

Nit: you've updated the string but forgot the associated localization comment https://hg.mozilla.org/mozilla-central/rev/4c854d8255d2#l1.9

MozReview-Commit-ID: 7pHBNtHxOQx

Comment on attachment 8791296 [details] [diff] [review] Follow-up: update a LOCALIZATION NOTE for a changed string Well spotted!

Comment on attachment 8791296 [details] [diff] [review] Follow-up: update a LOCALIZATION NOTE for a changed string Review of attachment 8791296 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the fix.

https://hg.mozilla.org/integration/mozilla-inbound/rev/75c41ccb4dc62efe38f6bdfc4123aa52afc2eeea Bug 1287178 - Follow-up: update a LOCALIZATION NOTE for a changed string. r=flod DONTBUILD

Screenshots of the change in case you're interested: https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=82d0a583a9a39bf0b0000bccbf6d5c9ec2596bcc&newProject=mozilla-central&newRev=29af101880db7ce7f5f87f58e1ff20988c1c5fc3&filter=dataChoicesTab Everything looks good to me in en-US.

(In reply to Matthew N. [:MattN] (In Taipei until Sep. 23) from comment #45) > Screenshots of the change in case you're interested: > https://screenshots.mattn.ca/compare/?oldProject=mozilla- > central&oldRev=82d0a583a9a39bf0b0000bccbf6d5c9ec2596bcc&newProject=mozilla- > central&newRev=29af101880db7ce7f5f87f58e1ff20988c1c5fc3&filter=dataChoicesTab > > Everything looks good to me in en-US. That thing is so cool.