Bug 1301722 - Unsubmitted crash report notification should go away if not interacted with for some period of time. r?florian draft
authorMike Conley <mconley@mozilla.com>
Tue, 13 Sep 2016 14:35:08 -0400
changeset 414109 39c6bf904396fef85a3ab622cd467e148b38d574
parent 413960 29af101880db7ce7f5f87f58e1ff20988c1c5fc3
child 531367 21a60d0781808adb874b8576d66102f5ab685c6a
push id29588
push usermconley@mozilla.com
push dateThu, 15 Sep 2016 15:43:21 +0000
reviewersflorian
bugs1301722
milestone51.0a1
Bug 1301722 - Unsubmitted crash report notification should go away if not interacted with for some period of time. r?florian MozReview-Commit-ID: JxTmmesyTYd
browser/app/profile/firefox.js
browser/modules/ContentCrashHandlers.jsm
browser/modules/test/browser_UnsubmittedCrashHandler.js
--- a/browser/app/profile/firefox.js
+++ b/browser/app/profile/firefox.js
@@ -1512,9 +1512,14 @@ pref("webchannel.allowObject.urlWhitelis
 // crash reports, and then show a notification for submitting
 // those reports.
 #ifdef RELEASE_BUILD
 pref("browser.crashReports.unsubmittedCheck.enabled", false);
 #else
 pref("browser.crashReports.unsubmittedCheck.enabled", true);
 #endif
 
+// chancesUntilSuppress is how many times we'll show the unsubmitted
+// crash report notification across different days and shutdown
+// without a user choice before we suppress the notification for
+// some number of days.
+pref("browser.crashReports.unsubmittedCheck.chancesUntilSuppress", 4);
 pref("browser.crashReports.unsubmittedCheck.autoSubmit", false);
\ No newline at end of file
--- a/browser/modules/ContentCrashHandlers.jsm
+++ b/browser/modules/ContentCrashHandlers.jsm
@@ -2,16 +2,17 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 "use strict";
 
 var Cc = Components.classes;
 var Ci = Components.interfaces;
 var Cu = Components.utils;
+var Cr = Components.results;
 
 this.EXPORTED_SYMBOLS = [ "TabCrashHandler",
                           "PluginCrashReporter",
                           "UnsubmittedCrashHandler" ];
 
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 Cu.import("resource://gre/modules/Services.jsm");
 
@@ -33,16 +34,18 @@ XPCOMUtils.defineLazyModuleGetter(this, 
 XPCOMUtils.defineLazyGetter(this, "gNavigatorBundle", function() {
   const url = "chrome://browser/locale/browser.properties";
   return Services.strings.createBundle(url);
 });
 
 // We don't process crash reports older than 28 days, so don't bother
 // submitting them
 const PENDING_CRASH_REPORT_DAYS = 28;
+const DAY = 24 * 60 * 60 * 1000; // milliseconds
+const DAYS_TO_SUPPRESS = 30;
 
 this.TabCrashHandler = {
   _crashedTabCount: 0,
 
   get prefs() {
     delete this.prefs;
     return this.prefs = Services.prefs.getBranch("browser.tabs.crashReporting.");
   },
@@ -339,39 +342,105 @@ this.TabCrashHandler = {
 /**
  * This component is responsible for scanning the pending
  * crash report directory for reports, and (if enabled), to
  * prompt the user to submit those reports. It might also
  * submit those reports automatically without prompting if
  * the user has opted in.
  */
 this.UnsubmittedCrashHandler = {
+  get prefs() {
+    delete this.prefs;
+    return this.prefs =
+      Services.prefs.getBranch("browser.crashReports.unsubmittedCheck.");
+  },
+
+  // showingNotification is set to true once a notification
+  // is successfully shown, and then set back to false if
+  // the notification is dismissed by an action by the user.
+  showingNotification: false,
+  // suppressed is true if we've determined that we've shown
+  // the notification too many times across too many days without
+  // user interaction, so we're suppressing the notification for
+  // some number of days. See the documentation for
+  // shouldShowPendingSubmissionsNotification().
+  suppressed: false,
+
   init() {
     if (this.initialized) {
       return;
     }
 
     this.initialized = true;
 
-    let pref = "browser.crashReports.unsubmittedCheck.enabled";
-    let shouldCheck = Services.prefs.getBoolPref(pref);
+    let shouldCheck = this.prefs.getBoolPref("enabled");
 
     if (shouldCheck) {
+      if (this.prefs.prefHasUserValue("suppressUntilDate")) {
+        if (this.prefs.getCharPref("suppressUntilDate") > this.dateString()) {
+          // We'll be suppressing any notifications until after suppressedDate,
+          // so there's no need to do anything more.
+          this.suppressed = true;
+          return;
+        }
+
+        // We're done suppressing, so we don't need this pref anymore.
+        this.prefs.clearUserPref("suppressUntilDate");
+      }
+
       Services.obs.addObserver(this, "browser-delayed-startup-finished",
                                false);
+      Services.obs.addObserver(this, "profile-before-change",
+                               false);
     }
   },
 
-  observe(subject, topic, data) {
-    if (topic != "browser-delayed-startup-finished") {
+  uninit() {
+    if (!this.initialized) {
+      return;
+    }
+
+    this.initialized = false;
+
+    if (this.suppressed) {
+      this.suppressed = false;
+      // No need to do any more clean-up, since we were suppressed.
       return;
     }
 
-    Services.obs.removeObserver(this, topic);
-    this.checkForUnsubmittedCrashReports();
+    if (this.showingNotification) {
+      this.prefs.setBoolPref("shutdownWhileShowing", true);
+      this.showingNotification = false;
+    }
+
+    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, it's okay.
+      if (e.result != Cr.NS_ERROR_FAILURE) {
+        throw e;
+      }
+    }
+
+    Services.obs.removeObserver(this, "profile-before-change");
+  },
+
+  observe(subject, topic, data) {
+    switch (topic) {
+      case "browser-delayed-startup-finished": {
+        Services.obs.removeObserver(this, topic);
+        this.checkForUnsubmittedCrashReports();
+        break;
+      }
+      case "profile-before-change": {
+        this.uninit();
+        break;
+      }
+    }
   },
 
   /**
    * Scans the profile directory for unsubmitted crash reports
    * within the past PENDING_CRASH_REPORT_DAYS days. If it
    * finds any, it will, if necessary, attempt to open a notification
    * bar to prompt the user to submit them.
    *
@@ -390,24 +459,72 @@ this.UnsubmittedCrashHandler = {
     } catch (e) {
       Cu.reportError(e);
       return null;
     }
 
     if (reportIDs.length) {
       if (CrashNotificationBar.autoSubmit) {
         CrashNotificationBar.submitReports(reportIDs);
-      } else {
+      } else if (this.shouldShowPendingSubmissionsNotification()) {
         return this.showPendingSubmissionsNotification(reportIDs);
       }
     }
     return null;
   }),
 
   /**
+   * Returns true if the notification should be shown.
+   * shouldShowPendingSubmissionsNotification makes this decision
+   * by looking at whether or not the user has seen the notification
+   * over several days without ever interacting with it. If this occurs
+   * too many times, we suppress the notification for DAYS_TO_SUPPRESS
+   * days.
+   *
+   * @returns bool
+   */
+  shouldShowPendingSubmissionsNotification() {
+    if (!this.prefs.prefHasUserValue("shutdownWhileShowing")) {
+      return true;
+    }
+
+    let shutdownWhileShowing = this.prefs.getBoolPref("shutdownWhileShowing");
+    this.prefs.clearUserPref("shutdownWhileShowing");
+
+    if (!this.prefs.prefHasUserValue("lastShownDate")) {
+      // This isn't expected, but we're being defensive here. We'll
+      // opt for showing the notification in this case.
+      return true;
+    }
+
+    let lastShownDate = this.prefs.getCharPref("lastShownDate");
+    if (this.dateString() > lastShownDate && shutdownWhileShowing) {
+      // We're on a newer day then when we last showed the
+      // notification without closing it. We don't want to do
+      // this too many times, so we'll decrement a counter for
+      // 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");
+      if (--chances < 0) {
+        // We're out of chances!
+        this.prefs.clearUserPref("chancesUntilSuppress");
+        // 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;
+      }
+      this.prefs.setIntPref("chancesUntilSuppress", chances);
+    }
+
+    return true;
+  },
+
+  /**
    * Given an array of unsubmitted crash report IDs, try to open
    * up a notification asking the user to submit them.
    *
    * @param reportIDs (Array<string>)
    *        The Array of report IDs to offer the user to send.
    * @returns The <xul:notification> if one is shown. null otherwise.
    */
   showPendingSubmissionsNotification(reportIDs) {
@@ -416,21 +533,44 @@ this.UnsubmittedCrashHandler = {
       return null;
     }
 
     let messageTemplate =
       gNavigatorBundle.GetStringFromName("pendingCrashReports2.label");
 
     let message = PluralForm.get(count, messageTemplate).replace("#1", count);
 
-    return CrashNotificationBar.show({
+    let notification = CrashNotificationBar.show({
       notificationID: "pending-crash-reports",
       message,
       reportIDs,
+      onAction: () => {
+        this.showingNotification = false;
+      },
     });
+
+    if (notification) {
+      this.showingNotification = true;
+      this.prefs.setCharPref("lastShownDate", this.dateString());
+    }
+
+    return notification;
+  },
+
+  /**
+   * Returns a string representation of a Date in the format
+   * YYYYMMDD.
+   *
+   * @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()) {
+    return someDate.toLocaleFormat("%Y%m%d");
   },
 };
 
 this.CrashNotificationBar = {
   /**
    * Attempts to show a notification bar to the user in the most
    * recent browser window asking them to submit some crash report
    * IDs. If a notification cannot be shown (for example, there
@@ -447,19 +587,25 @@ this.CrashNotificationBar = {
    *        notificationID (string)
    *          The ID for the notification to be opened.
    *
    *        message (string)
    *          The message to be displayed in the notification.
    *
    *        reportIDs (Array<string>)
    *          The array of report IDs to offer to the user.
+   *
+   *        onAction (function, optional)
+   *          A callback to fire once the user performs an
+   *          action on the notification bar (this includes
+   *          dismissing the notification).
+   *
    * @returns The <xul:notification> if one is shown. null otherwise.
    */
-  show({ notificationID, message, reportIDs }) {
+  show({ notificationID, message, reportIDs, onAction }) {
     let chromeWin = RecentWindow.getMostRecentBrowserWindow();
     if (!chromeWin) {
       // Can't show a notification in this case. We'll hopefully
       // get another opportunity to have the user submit their
       // crash reports later.
       return null;
     }
 
@@ -468,23 +614,29 @@ this.CrashNotificationBar = {
     if (notification) {
       return null;
     }
 
     let buttons = [{
       label: gNavigatorBundle.GetStringFromName("pendingCrashReports.send"),
       callback: () => {
         this.submitReports(reportIDs);
+        if (onAction) {
+          onAction();
+        }
       },
     },
     {
       label: gNavigatorBundle.GetStringFromName("pendingCrashReports.alwaysSend"),
       callback: () => {
         this.autoSubmit = true;
         this.submitReports(reportIDs);
+        if (onAction) {
+          onAction();
+        }
       },
     },
     {
       label: gNavigatorBundle.GetStringFromName("pendingCrashReports.viewAll"),
       callback: function() {
         chromeWin.openUILinkIn("about:crashes", "tab");
         return true;
       },
@@ -494,16 +646,19 @@ this.CrashNotificationBar = {
       if (eventType == "dismissed") {
         // The user intentionally dismissed the notification,
         // which we interpret as meaning that they don't care
         // to submit the reports. We'll ignore these particular
         // reports going forward.
         reportIDs.forEach(function(reportID) {
           CrashSubmit.ignore(reportID);
         });
+        if (onAction) {
+          onAction();
+        }
       }
     };
 
     return nb.appendNotification(message, notificationID,
                                  "chrome://browser/skin/tab-crashed.svg",
                                  nb.PRIORITY_INFO_HIGH, buttons,
                                  eventCallback);
   },
--- a/browser/modules/test/browser_UnsubmittedCrashHandler.js
+++ b/browser/modules/test/browser_UnsubmittedCrashHandler.js
@@ -193,16 +193,22 @@ add_task(function* setup() {
     notification.close();
   }
 
   let env = Cc["@mozilla.org/process/environment;1"]
               .getService(Components.interfaces.nsIEnvironment);
   let oldServerURL = env.get("MOZ_CRASHREPORTER_URL");
   env.set("MOZ_CRASHREPORTER_URL", SERVER_URL);
 
+  yield SpecialPowers.pushPrefEnv({
+    set: [
+      ["browser.crashReports.unsubmittedCheck.enabled", true],
+    ],
+  });
+
   registerCleanupFunction(function() {
     gNotificationBox = null;
     clearPendingCrashReports();
     env.set("MOZ_CRASHREPORTER_URL", oldServerURL);
   });
 });
 
 /**
@@ -412,8 +418,249 @@ add_task(function* test_can_ignore() {
   yield waitForIgnoredReports(reportIDs);
 
   notification =
     yield UnsubmittedCrashHandler.checkForUnsubmittedCrashReports();
   Assert.equal(notification, null, "There should be no notification");
 
   clearPendingCrashReports();
 });
+
+/**
+ * Tests that if the notification is shown, then the
+ * lastShownDate is set for today.
+ */
+add_task(function* test_last_shown_date() {
+  yield createPendingCrashReports(1);
+  let notification =
+    yield UnsubmittedCrashHandler.checkForUnsubmittedCrashReports();
+  Assert.ok(notification, "There should be a notification");
+
+  let today = new Date().toLocaleFormat("%Y%m%d");
+  let lastShownDate =
+    UnsubmittedCrashHandler.prefs.getCharPref("lastShownDate");
+  Assert.equal(today, lastShownDate,
+               "Last shown date should be today.");
+
+  UnsubmittedCrashHandler.prefs.clearUserPref("lastShownDate");
+  gNotificationBox.removeNotification(notification, true);
+  clearPendingCrashReports();
+});
+
+/**
+ * Tests that if UnsubmittedCrashHandler is uninit with a
+ * notification still being shown, that
+ * browser.crashReports.unsubmittedCheck.shutdownWhileShowing is
+ * set to true.
+ */
+add_task(function* test_shutdown_while_showing() {
+  yield createPendingCrashReports(1);
+  let notification =
+    yield UnsubmittedCrashHandler.checkForUnsubmittedCrashReports();
+  Assert.ok(notification, "There should be a notification");
+
+  UnsubmittedCrashHandler.uninit();
+  let shutdownWhileShowing =
+    UnsubmittedCrashHandler.prefs.getBoolPref("shutdownWhileShowing");
+  Assert.ok(shutdownWhileShowing,
+            "We should have noticed that we uninitted while showing " +
+            "the notification.");
+  UnsubmittedCrashHandler.prefs.clearUserPref("shutdownWhileShowing");
+  UnsubmittedCrashHandler.init();
+
+  gNotificationBox.removeNotification(notification, true);
+  clearPendingCrashReports();
+});
+
+/**
+ * Tests that if UnsubmittedCrashHandler is uninit after
+ * the notification has been closed, that
+ * browser.crashReports.unsubmittedCheck.shutdownWhileShowing is
+ * not set in prefs.
+ */
+add_task(function* test_shutdown_while_not_showing() {
+  let reportIDs = yield createPendingCrashReports(1);
+  let notification =
+    yield UnsubmittedCrashHandler.checkForUnsubmittedCrashReports();
+  Assert.ok(notification, "There should be a notification");
+
+  // Dismiss the notification by clicking on the "X" button.
+  let anonyNodes = document.getAnonymousNodes(notification)[0];
+  let closeButton = anonyNodes.querySelector(".close-icon");
+  closeButton.click();
+  yield waitForIgnoredReports(reportIDs);
+
+  UnsubmittedCrashHandler.uninit();
+  Assert.throws(() => {
+    let shutdownWhileShowing =
+      UnsubmittedCrashHandler.prefs.getBoolPref("shutdownWhileShowing");
+  }, "We should have noticed that the notification had closed before " +
+     "uninitting.");
+  UnsubmittedCrashHandler.init();
+
+  gNotificationBox.removeNotification(notification, true);
+  clearPendingCrashReports();
+});
+
+/**
+ * Tests that if
+ * browser.crashReports.unsubmittedCheck.shutdownWhileShowing is
+ * set and the lastShownDate is today, then we don't decrement
+ * browser.crashReports.unsubmittedCheck.chancesUntilSuppress.
+ */
+add_task(function* test_dont_decrement_chances_on_same_day() {
+  let initChances =
+    UnsubmittedCrashHandler.prefs.getIntPref("chancesUntilSuppress");
+  Assert.ok(initChances > 1, "We should start with at least 1 chance.");
+
+  yield createPendingCrashReports(1);
+  let notification =
+    yield UnsubmittedCrashHandler.checkForUnsubmittedCrashReports();
+  Assert.ok(notification, "There should be a notification");
+
+  UnsubmittedCrashHandler.uninit();
+
+  gNotificationBox.removeNotification(notification, true);
+
+  let shutdownWhileShowing =
+    UnsubmittedCrashHandler.prefs.getBoolPref("shutdownWhileShowing");
+  Assert.ok(shutdownWhileShowing,
+            "We should have noticed that we uninitted while showing " +
+            "the notification.");
+
+  let today = new Date().toLocaleFormat("%Y%m%d");
+  let lastShownDate =
+    UnsubmittedCrashHandler.prefs.getCharPref("lastShownDate");
+  Assert.equal(today, lastShownDate,
+               "Last shown date should be today.");
+
+  UnsubmittedCrashHandler.init();
+
+  notification =
+      yield UnsubmittedCrashHandler.checkForUnsubmittedCrashReports();
+  Assert.ok(notification, "There should still be a notification");
+
+  let chances =
+    UnsubmittedCrashHandler.prefs.getIntPref("chancesUntilSuppress");
+
+  Assert.equal(initChances, chances,
+               "We should not have decremented chances.");
+
+  gNotificationBox.removeNotification(notification, true);
+  clearPendingCrashReports();
+});
+
+/**
+ * Tests that if
+ * browser.crashReports.unsubmittedCheck.shutdownWhileShowing is
+ * set and the lastShownDate is before today, then we decrement
+ * browser.crashReports.unsubmittedCheck.chancesUntilSuppress.
+ */
+add_task(function* test_decrement_chances_on_other_day() {
+  let initChances =
+    UnsubmittedCrashHandler.prefs.getIntPref("chancesUntilSuppress");
+  Assert.ok(initChances > 1, "We should start with at least 1 chance.");
+
+  yield createPendingCrashReports(1);
+  let notification =
+    yield UnsubmittedCrashHandler.checkForUnsubmittedCrashReports();
+  Assert.ok(notification, "There should be a notification");
+
+  UnsubmittedCrashHandler.uninit();
+
+  gNotificationBox.removeNotification(notification, true);
+
+  let shutdownWhileShowing =
+    UnsubmittedCrashHandler.prefs.getBoolPref("shutdownWhileShowing");
+  Assert.ok(shutdownWhileShowing,
+            "We should have noticed that we uninitted while showing " +
+            "the notification.");
+
+  // Now pretend that the notification was shown yesterday.
+  let yesterday = new Date(Date.now() - DAY).toLocaleFormat("%Y%m%d");
+  UnsubmittedCrashHandler.prefs.setCharPref("lastShownDate", yesterday);
+
+  UnsubmittedCrashHandler.init();
+
+  notification =
+    yield UnsubmittedCrashHandler.checkForUnsubmittedCrashReports();
+  Assert.ok(notification, "There should still be a notification");
+
+  let chances =
+    UnsubmittedCrashHandler.prefs.getIntPref("chancesUntilSuppress");
+
+  Assert.equal(initChances - 1, chances,
+               "We should have decremented our chances.");
+  UnsubmittedCrashHandler.prefs.clearUserPref("chancesUntilSuppress");
+
+  gNotificationBox.removeNotification(notification, true);
+  clearPendingCrashReports();
+});
+
+/**
+ * Tests that if we've shutdown too many times showing the
+ * notification, and we've run out of chances, then
+ * browser.crashReports.unsubmittedCheck.suppressUntilDate is
+ * set for some days into the future.
+ */
+add_task(function* test_can_suppress_after_chances() {
+  // Pretend that a notification was shown yesterday.
+  let yesterday = new Date(Date.now() - DAY).toLocaleFormat("%Y%m%d");
+  UnsubmittedCrashHandler.prefs.setCharPref("lastShownDate", yesterday);
+  UnsubmittedCrashHandler.prefs.setBoolPref("shutdownWhileShowing", true);
+  UnsubmittedCrashHandler.prefs.setIntPref("chancesUntilSuppress", 0);
+
+  yield createPendingCrashReports(1);
+  let notification =
+    yield UnsubmittedCrashHandler.checkForUnsubmittedCrashReports();
+  Assert.equal(notification, null,
+               "There should be no notification if we've run out of chances");
+
+  // We should have set suppressUntilDate into the future
+  let suppressUntilDate =
+    UnsubmittedCrashHandler.prefs.getCharPref("suppressUntilDate");
+
+  let today = new Date().toLocaleFormat("%Y%m%d");
+  Assert.ok(suppressUntilDate > today,
+            "We should be suppressing until some days into the future.");
+
+  UnsubmittedCrashHandler.prefs.clearUserPref("chancesUntilSuppress");
+  UnsubmittedCrashHandler.prefs.clearUserPref("suppressUntilDate");
+  UnsubmittedCrashHandler.prefs.clearUserPref("lastShownDate");
+  clearPendingCrashReports();
+});
+
+/**
+ * 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 future = new Date(Date.now() + (DAY * 5)).toLocaleFormat("%Y%m%d");
+  UnsubmittedCrashHandler.prefs.setCharPref("suppressUntilDate", future);
+  UnsubmittedCrashHandler.uninit();
+  UnsubmittedCrashHandler.init();
+
+  Assert.ok(UnsubmittedCrashHandler.suppressed,
+            "The UnsubmittedCrashHandler should be suppressed.");
+  UnsubmittedCrashHandler.prefs.clearUserPref("suppressUntilDate");
+
+  UnsubmittedCrashHandler.uninit();
+  UnsubmittedCrashHandler.init();
+});
+
+/**
+ * Tests that if there's a suppression date set, but we've exceeded
+ * it, then we can show the notification again.
+ */
+add_task(function* test_end_suppression() {
+  let yesterday = new Date(Date.now() - DAY).toLocaleFormat("%Y%m%d");
+  UnsubmittedCrashHandler.prefs.setCharPref("suppressUntilDate", yesterday);
+  UnsubmittedCrashHandler.uninit();
+  UnsubmittedCrashHandler.init();
+
+  Assert.ok(!UnsubmittedCrashHandler.suppressed,
+            "The UnsubmittedCrashHandler should not be suppressed.");
+  Assert.ok(!UnsubmittedCrashHandler.prefs.prefHasUserValue("suppressUntilDate"),
+            "The suppression date should been cleared from preferences.");
+
+  UnsubmittedCrashHandler.uninit();
+  UnsubmittedCrashHandler.init();
+});