Closed Bug 1301722 Opened 8 years ago Closed 8 years ago

Unsubmitted crash report notification should go away if not interacted with for some period of time

Categories

(Toolkit :: Crash Reporting, defect)

Product:
Toolkit
Component:
Crash Reporting
Version:
49 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox51 --- fixed

People

(Reporter: mconley, Unassigned)

References

Details

Attachments

(1 file)

Bug 1301722 - Unsubmitted crash report notification should go away if not interacted with for some period of time.
58 bytes, text/x-review-board-request
florian
: review+
Details 
Once bug 1287178 lands, the unsubmitted crash report notification gives the user some options:

1) Submit the crash reports
2) Always submit crash reports and never show me this again
3) Dismiss (which marks those unsubmitted crash reports as ignorable for future notifications)
4) Do nothing, which will keep the notification open in that user's window until the window is closed.

If the user chooses #4, we'll have this notification sticking around for every session until either the user makes a decision or there are no more unsubmitted crash reports that are 28 days old or less.

That's not great. If we detect that the user has not interacted with the notification after some period of time, we should hide it, and not show it again for a while.

It's still unclear whether or not this auto-hiding should be channel specific or not, but I guess we can figure that out here.
The approach to this is somewhat similar to what was done in bug 1280381 for the autocomplete opt-in in the AwesomeBar, but is also slightly different since the notification can be dismissed and come back again.

Here's how I think I'm going to do it:

Have the following pref initialized:
browser.crashReports.unsubmittedCheck.chancesUntilSuppress = 4

1) When UnsubmittedCrashReports initializes, have it check browser.crashReports.unsubmittedCheck.suppressUntilDate* to see if it has a value in it. If it does, check to see if the current date exceeds the one stored in prefs. If not, exit. If so, clear the pref. Have it then check browser.crashReports.unsubmittedCheck.shutdownWhileShowing. If that value exists and is true, clear it, and then check browser.crashReports.unsubmittedCheck.lastShownDate. If a value there exists, and the notification is being shown on a new day, then take browser.crashReports.unsubmittedCheck.chancesUntilSuppress, and decrement it by 1. If the value of browser.crashReports.unsubmittedCheck.chancesUntilSuppress is 0, reset the pref, and then set browser.crashReports.unsubmittedCheck.suppressUntilDate to suppress for 30 days and exit. Otherwise, continue to the next step.
2) If the notification is going to be shown, set browser.crashReports.unsubmittedCheck.lastShownDate with the current date*, have UnsubmittedCrashReports hold an internal bool saying that the notification is being shown, and then set up event handlers for the Send, Always Send and dismiss button that will clear that bool if they're fired.
3) At profile-before-change, check to see if the notification bool is still true. If so, then the user never actually dismissed the notification. Set browser.crashReports.unsubmittedCheck.shutdownWhileShowing to true.

What this algorithm does is make sure that if the user sees the notification 4 times across 4 different days and never does anything about it, then we hide future notifications for 30 days before starting the process again.

* For the "date" values stored in prefs, round down to just the initial hour of the day. We really only care about days here - not hours, minutes or seconds.
Depends on: 1301723
Hey florian,

I thought you'd be a good reviewer for this patch due to your work in bug 1280381, which was similar. If you don't think you're the right person for this, or have any questions about UnsubmittedCrashHandler, let me know!
Comment on attachment 8790887 [details]
Bug 1301722 - Unsubmitted crash report notification should go away if not interacted with for some period of time.

https://reviewboard.mozilla.org/r/78500/#review77222

Looks good overall (would be f+ if I could set it from reviewboard), all the comments are trivial.

::: browser/modules/ContentCrashHandlers.jsm:379
(Diff revision 1)
> -    let shouldCheck = Services.prefs.getBoolPref(pref);
>  
>      if (shouldCheck) {
> +      try {
> +        let suppressedDate = this.prefs.getCharPref("suppressUntilDate");
> +        if (suppressedDate > this.dateString()) {

I think I would prefer the try block to be replaced by:
if (this.prefs.prefHasUserValue("suppressUntilDate"))


The suppressedDate variable is used only once, can it be inlined?

::: browser/modules/ContentCrashHandlers.jsm:384
(Diff revision 1)
> +        if (suppressedDate > this.dateString()) {
> +          // We'll be suppressing any notifications until after suppressedDate,
> +          // so there's no need to do anything more.
> +          this.suppressed = true;
> +          return;
> +        } else {

nit: No else after a return.

::: browser/modules/ContentCrashHandlers.jsm:407
(Diff revision 1)
> -      return;
> +    this.suppressed = false;
> +
> +    if (this.showingNotification) {
> +      this.prefs.setBoolPref("shutdownWhileShowing", true);
> +    }
> +    this.showingNotification = false;

nit: this could/should go instead if above if block

::: browser/modules/ContentCrashHandlers.jsm:420
(Diff revision 1)
> +    }
> +    try {
> +      Services.obs.removeObserver(this, "profile-before-change");
> +    } catch (e if e.result == Cr.NS_ERROR_FAILURE) {
> +      // The profile-before-change observer might not have been set
> +      // if we're suppressed.

Can't we simplify with an early return at the beginning of the method if (this.suppressed) ? (It seems to me that this try/catch can be removed, and the comment in the previous catch can be simplified.

::: browser/modules/ContentCrashHandlers.jsm:465
(Diff revision 1)
>  
>      if (reportIDs.length) {
>        if (CrashNotificationBar.autoSubmit) {
>          CrashNotificationBar.submitReports(reportIDs);
>        } else {
> +        if (this.shouldShowPendingSubmissionsNotification()) {

nit: } else if {
I thought we had a linter rule to error on this, but I'm not sure.

::: browser/modules/ContentCrashHandlers.jsm:498
(Diff revision 1)
> +        // this situation. Too many of these, and we'll assume the
> +        // user doesn't know or care about unsubmitted notifications,
> +        // and we'll suppress the notification for a while.
> +        let chances = this.prefs.getIntPref("chancesUntilSuppress");
> +        chances--;
> +        if (chances < 0) {

nit: these 2 lines can be merged to:
if (--chances < 0)
I don't think it affects readability.

::: browser/modules/ContentCrashHandlers.jsm:506
(Diff revision 1)
> +          // We'll suppress for DAYS_TO_SUPPRESS days.
> +          let suppressUntil =
> +            this.dateString(new Date(Date.now() + (DAY * DAYS_TO_SUPPRESS)));
> +          this.prefs.setCharPref("suppressUntilDate", suppressUntil);
> +          return false;
> +        } else {

nit: no else after a return

::: browser/modules/ContentCrashHandlers.jsm:514
(Diff revision 1)
> +      }
> +    } catch (e if e.result == Cr.NS_ERROR_UNEXPECTED) {
> +      // We only set shutdownWhileShowing if we shutdown without ever
> +      // dismissing the unsubmitted notification bar. It's fine if
> +      // this value doesn't exist. If getting lastShownDate threw,
> +      // that's less expected, but we'll opt for just showing

Mixing expected and unexpected exceptions inside a catch block doesn't smell good to me. Is it possible to simplify with a prefHasUserValue call?

::: browser/modules/ContentCrashHandlers.jsm:565
(Diff revision 1)
> +   * @param someDate (Date, optional)
> +   *        The Date to convert to the string. If not provided,
> +   *        defaults to today's date.
> +   * @returns String
> +   */
> +  dateString(someDate=new Date()) {

nit: spaces around '='

::: browser/modules/test/browser_UnsubmittedCrashHandler.js:578
(Diff revision 1)
> +  UnsubmittedCrashHandler.prefs.setCharPref("lastShownDate", yesterday);
> +
> +  UnsubmittedCrashHandler.init();
> +
> +  notification =
> +      yield UnsubmittedCrashHandler.checkForUnsubmittedCrashReports();

nit: indent

::: browser/modules/test/browser_UnsubmittedCrashHandler.js:630
(Diff revision 1)
> +/**
> + * Tests that if there's a suppression date set, then no notification
> + * will be shown even if there are pending crash reports.
> + */
> +add_task(function* test_suppression() {
> +  let tomorrow = new Date(Date.now() + DAY).toLocaleFormat("%Y%m%d");

I'm wondering if there's a slight chance that this may fail intermittently when the test is running just a split second before midnight... but given that everything insidethis task is running synchronously I think we are fine.

::: browser/modules/test/browser_UnsubmittedCrashHandler.js:655
(Diff revision 1)
> +  UnsubmittedCrashHandler.uninit();
> +  UnsubmittedCrashHandler.init();
> +
> +  Assert.ok(!UnsubmittedCrashHandler.suppressed,
> +            "The UnsubmittedCrashHandler should not be suppressed.");
> +  UnsubmittedCrashHandler.prefs.clearUserPref("suppressUntilDate");

Don't you want to test that UnsubmittedCrashHandler cleared the pref, rather than clearing it from the test?
Attachment #8790887 - Flags: review?(florian) → review-
Comment on attachment 8790887 [details]
Bug 1301722 - Unsubmitted crash report notification should go away if not interacted with for some period of time.

https://reviewboard.mozilla.org/r/78500/#review77222

> nit: this could/should go instead if above if block

Not sure what you mean - I need to read the values before I reset them... I've opted to move them into a `_reset()` function instead. Does that work?

> I'm wondering if there's a slight chance that this may fail intermittently when the test is running just a split second before midnight... but given that everything insidethis task is running synchronously I think we are fine.

Yeah, it's synchronous in here, so I think we're okay - but I'll add a comment to call it out explicitly. Good eye.
ni'ing for the _reset() question from comment 5.
Flags: needinfo?(florian)
Also needinfo'ing shorlander to make sure I've got the suppression logic right:

Basically, if the user has seen the notification but not interacted with it over 4 different browser sessions across 4 different days, we suppress the notification for 30 days. Does that work for you?
Flags: needinfo?(shorlander)
(In reply to Mike Conley (:mconley) - (needinfo me!) from comment #5)

> > nit: this could/should go instead if above if block
> 
> Not sure what you mean - I need to read the values before I reset them...
> I've opted to move them into a `_reset()` function instead. Does that work?

I meant "this could/should go inside the above if block", ie.

+    if (this.showingNotification) {
+      this.prefs.setBoolPref("shutdownWhileShowing", true);
+      this.showingNotification = false;
+    }

Sorry for writing garbage in my review comment :(.

> > I'm wondering if there's a slight chance that this may fail intermittently when the test is running just a split second before midnight... but given that everything insidethis task is running synchronously I think we are fine.
> 
> Yeah, it's synchronous in here, so I think we're okay - but I'll add a
> comment to call it out explicitly. Good eye.

If we have some doubt you can add 2 days instead of one and it'll never break, even if async.
Flags: needinfo?(florian)
Comment on attachment 8790887 [details]
Bug 1301722 - Unsubmitted crash report notification should go away if not interacted with for some period of time.

https://reviewboard.mozilla.org/r/78500/#review77568

Looks good, just a few trivial nits left.

::: browser/modules/ContentCrashHandlers.jsm:416
(Diff revision 3)
> +      this.showingNotification = false;
> +    }
> +
> +    try {
> +      Services.obs.removeObserver(this, "browser-delayed-startup-finished");
> +    } catch (e) {

Was there a reason for not keeping the catch (e if e.result == Cr.NS_ERROR_FAILURE) syntax here?

::: browser/modules/ContentCrashHandlers.jsm:418
(Diff revision 3)
> +
> +    try {
> +      Services.obs.removeObserver(this, "browser-delayed-startup-finished");
> +    } catch (e) {
> +      // The browser-delayed-startup-finished observer might have already
> +      // fired and removed itself, so if this fails, its okay.

nit: its okay -> it's okay

::: browser/modules/ContentCrashHandlers.jsm:485
(Diff revision 3)
> +   * days.
> +   *
> +   * @returns bool
> +   */
> +  shouldShowPendingSubmissionsNotification() {
> +    if (this.prefs.prefHasUserValue("shutdownWhileShowing")) {

nit: use an early return here to reduce the indent of the whole method?
Attachment #8790887 - Flags: review?(florian) → review+
Comment on attachment 8790887 [details]
Bug 1301722 - Unsubmitted crash report notification should go away if not interacted with for some period of time.

https://reviewboard.mozilla.org/r/78500/#review77568

> Was there a reason for not keeping the catch (e if e.result == Cr.NS_ERROR_FAILURE) syntax here?

Yeah - eslint was rapping my knuckles for it. This sort of thing stopped being supported in browser/modules since bug 1233470.
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/30eda0df6d1d
Unsubmitted crash report notification should go away if not interacted with for some period of time. r=florian
https://hg.mozilla.org/mozilla-central/rev/30eda0df6d1d
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Blocks: 1303067
(In reply to Mike Conley (:mconley) - (needinfo me!) from comment #8)
> Also needinfo'ing shorlander to make sure I've got the suppression logic
> right:
> 
> Basically, if the user has seen the notification but not interacted with it
> over 4 different browser sessions across 4 different days, we suppress the
> notification for 30 days. Does that work for you?

WFM, thanks!
Flags: needinfo?(shorlander)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: