Bug 1241459 - Background tab crashes should only show about:tabcrashed for first selected tab. r?felipe, mikedeboer draft
authorMike Conley <mconley@mozilla.com>
Fri, 30 Sep 2016 15:06:49 -0400
changeset 423324 7cd908b9b8bcb3c72ab83ca6711b5d4233db0979
parent 423199 7be6b348c431d69f96f0765af3a0c0a0fe56d4bf
child 423325 f143da9b49b249a809c208ad81df1d4517be6c49
push id31878
push usermconley@mozilla.com
push dateMon, 10 Oct 2016 22:12:01 +0000
reviewersfelipe, mikedeboer
bugs1241459
milestone52.0a1
Bug 1241459 - Background tab crashes should only show about:tabcrashed for first selected tab. r?felipe, mikedeboer When a content process crashes for a tab that is not currently visible to the user (which can occur if the user is looking at only non-remote tabs, or tabs in other content processes), then we will only show the tab crash page for the first crashed tab that is selected by the user. The rest of the tabs will restore on demand. MozReview-Commit-ID: 1JBAp8diHXp
browser/base/content/tabbrowser.xml
browser/components/sessionstore/SessionStore.jsm
browser/modules/ContentCrashHandlers.jsm
--- a/browser/base/content/tabbrowser.xml
+++ b/browser/base/content/tabbrowser.xml
@@ -4968,28 +4968,21 @@
         ]]>
       </handler>
       <handler event="oop-browser-crashed">
         <![CDATA[
           if (!event.isTrusted)
             return;
 
           let browser = event.originalTarget;
-          let title = browser.contentTitle;
-          let uri = browser.currentURI;
           let icon = browser.mIconURL;
-
           let tab = this.getTabForBrowser(browser);
 
           if (this.selectedBrowser == browser) {
-            this.updateBrowserRemotenessByURL(browser, "about:tabcrashed");
-            browser.setAttribute("crashedPageTitle", title);
-            browser.docShell.displayLoadError(Cr.NS_ERROR_CONTENT_CRASHED, uri, null);
-            browser.removeAttribute("crashedPageTitle");
-            tab.setAttribute("crashed", true);
+            TabCrashHandler.onSelectedBrowserCrash(browser);
           } else {
             this.updateBrowserRemoteness(browser, false);
             SessionStore.reviveCrashedTab(tab);
           }
 
           tab.removeAttribute("soundplaying");
           this.setIcon(tab, icon, browser.contentPrincipal);
         ]]>
--- a/browser/components/sessionstore/SessionStore.jsm
+++ b/browser/components/sessionstore/SessionStore.jsm
@@ -165,16 +165,18 @@ XPCOMUtils.defineLazyModuleGetter(this, 
 XPCOMUtils.defineLazyModuleGetter(this, "SessionSaver",
   "resource:///modules/sessionstore/SessionSaver.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "SessionCookies",
   "resource:///modules/sessionstore/SessionCookies.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "SessionFile",
   "resource:///modules/sessionstore/SessionFile.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "TabAttributes",
   "resource:///modules/sessionstore/TabAttributes.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "TabCrashHandler",
+  "resource:///modules/ContentCrashHandlers.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "TabState",
   "resource:///modules/sessionstore/TabState.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "TabStateCache",
   "resource:///modules/sessionstore/TabStateCache.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "TabStateFlusher",
   "resource:///modules/sessionstore/TabStateFlusher.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "Utils",
   "resource:///modules/sessionstore/Utils.jsm");
@@ -914,17 +916,17 @@ var SessionStoreInternal = {
         this.onTabHide(win, target);
         break;
       case "TabPinned":
       case "TabUnpinned":
       case "SwapDocShells":
         this.saveStateDelayed(win);
         break;
       case "oop-browser-crashed":
-        this.onBrowserCrashed(win, target);
+        this.onBrowserCrashed(target);
         break;
       case "XULFrameLoaderCreated":
         if (target.namespaceURI == NS_XUL &&
             target.localName == "browser" &&
             target.frameLoader &&
             target.permanentKey) {
           this._lastKnownFrameLoader.set(target.permanentKey, target.frameLoader);
           this.resetEpoch(target);
@@ -1862,22 +1864,35 @@ var SessionStoreInternal = {
    * @param aWindow
    *        Window reference
    */
   onTabSelect: function ssi_onTabSelect(aWindow) {
     if (RunState.isRunning) {
       this._windows[aWindow.__SSi].selected = aWindow.gBrowser.tabContainer.selectedIndex;
 
       let tab = aWindow.gBrowser.selectedTab;
-      // If __SS_restoreState is still on the browser and it is
-      // TAB_STATE_NEEDS_RESTORE, then then we haven't restored
-      // this tab yet. Explicitly call restoreTabContent to kick off the restore.
-      if (tab.linkedBrowser.__SS_restoreState &&
-          tab.linkedBrowser.__SS_restoreState == TAB_STATE_NEEDS_RESTORE)
-        this.restoreTabContent(tab);
+      let browser = tab.linkedBrowser;
+
+      if (browser.__SS_restoreState &&
+          browser.__SS_restoreState == TAB_STATE_NEEDS_RESTORE) {
+        // If __SS_restoreState is still on the browser and it is
+        // TAB_STATE_NEEDS_RESTORE, then then we haven't restored
+        // this tab yet.
+        //
+        // It's possible that this tab was recently revived, and that
+        // we've deferred showing the tab crashed page for it (if the
+        // tab crashed in the background). If so, we need to re-enter
+        // the crashed state, since we'll be showing the tab crashed
+        // page.
+        if (TabCrashHandler.willShowCrashedTab(browser)) {
+          this.enterCrashedState(browser);
+        } else {
+          this.restoreTabContent(tab);
+        }
+      }
     }
   },
 
   onTabShow: function ssi_onTabShow(aWindow, aTab) {
     // If the tab hasn't been restored yet, move it into the right bucket
     if (aTab.linkedBrowser.__SS_restoreState &&
         aTab.linkedBrowser.__SS_restoreState == TAB_STATE_NEEDS_RESTORE) {
       TabRestoreQueue.hiddenToVisible(aTab);
@@ -1909,33 +1924,47 @@ var SessionStoreInternal = {
   /**
    * Handler for the event that is fired when a <xul:browser> crashes.
    *
    * @param aWindow
    *        The window that the crashed browser belongs to.
    * @param aBrowser
    *        The <xul:browser> that is now in the crashed state.
    */
-  onBrowserCrashed: function(aWindow, aBrowser) {
+  onBrowserCrashed: function(aBrowser) {
     NS_ASSERT(aBrowser.isRemoteBrowser,
               "Only remote browsers should be able to crash");
-    this._crashedBrowsers.add(aBrowser.permanentKey);
+
+    this.enterCrashedState(aBrowser);
+    // The browser crashed so we might never receive flush responses.
+    // Resolve all pending flush requests for the crashed browser.
+    TabStateFlusher.resolveAll(aBrowser);
+  },
+
+  /**
+   * Called when a browser is showing or is about to show the tab
+   * crashed page. This method causes SessionStore to ignore the
+   * tab until it's restored.
+   *
+   * @param browser
+   *        The <xul:browser> that is about to show the crashed page.
+   */
+  enterCrashedState(browser) {
+    this._crashedBrowsers.add(browser.permanentKey);
+
+    let win = browser.ownerGlobal;
 
     // If we hadn't yet restored, or were still in the midst of
     // restoring this browser at the time of the crash, we need
     // to reset its state so that we can try to restore it again
     // when the user revives the tab from the crash.
-    if (aBrowser.__SS_restoreState) {
-      let tab = aWindow.gBrowser.getTabForBrowser(aBrowser);
+    if (browser.__SS_restoreState) {
+      let tab = win.gBrowser.getTabForBrowser(browser);
       this._resetLocalTabRestoringState(tab);
     }
-
-    // The browser crashed so we might never receive flush responses.
-    // Resolve all pending flush requests for the crashed browser.
-    TabStateFlusher.resolveAll(aBrowser);
   },
 
   // Clean up data that has been closed a long time ago.
   // Do not reschedule a save. This will wait for the next regular
   // save.
   onIdleDaily: function() {
     // Remove old closed windows
     this._cleanupOldData([this._closedWindows]);
--- a/browser/modules/ContentCrashHandlers.jsm
+++ b/browser/modules/ContentCrashHandlers.jsm
@@ -36,69 +36,102 @@ XPCOMUtils.defineLazyGetter(this, "gNavi
   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;
+const MAX_UNSEEN_CRASHED_CHILD_IDS = 20;
 
 this.TabCrashHandler = {
   _crashedTabCount: 0,
+  childMap: new Map(),
+  browserMap: new WeakMap(),
+  unseenCrashedChildIDs: [],
+  crashedBrowserQueues: new Map(),
 
   get prefs() {
     delete this.prefs;
     return this.prefs = Services.prefs.getBranch("browser.tabs.crashReporting.");
   },
 
   init: function () {
     if (this.initialized)
       return;
     this.initialized = true;
 
-    if (AppConstants.MOZ_CRASHREPORTER) {
-      Services.obs.addObserver(this, "ipc:content-shutdown", false);
-      Services.obs.addObserver(this, "oop-frameloader-crashed", false);
-
-      this.childMap = new Map();
-      this.browserMap = new WeakMap();
-    }
+    Services.obs.addObserver(this, "ipc:content-shutdown", false);
+    Services.obs.addObserver(this, "oop-frameloader-crashed", false);
 
     this.pageListener = new RemotePages("about:tabcrashed");
     // LOAD_BACKGROUND pages don't fire load events, so the about:tabcrashed
     // content will fire up its own message when its initial scripts have
     // finished running.
     this.pageListener.addMessageListener("Load", this.receiveMessage.bind(this));
     this.pageListener.addMessageListener("RemotePage:Unload", this.receiveMessage.bind(this));
     this.pageListener.addMessageListener("closeTab", this.receiveMessage.bind(this));
     this.pageListener.addMessageListener("restoreTab", this.receiveMessage.bind(this));
     this.pageListener.addMessageListener("restoreAll", this.receiveMessage.bind(this));
   },
 
   observe: function (aSubject, aTopic, aData) {
     switch (aTopic) {
-      case "ipc:content-shutdown":
+      case "ipc:content-shutdown": {
         aSubject.QueryInterface(Ci.nsIPropertyBag2);
 
-        if (!aSubject.get("abnormal"))
+        if (!aSubject.get("abnormal")) {
           return;
+        }
+
+        let childID = aSubject.get("childID");
+        let dumpID = aSubject.get("dumpID");
+
+        if (!dumpID) {
+          Services.telemetry
+                  .getHistogramById("FX_CONTENT_CRASH_DUMP_UNAVAILABLE")
+                  .add(1);
+        } else if (AppConstants.MOZ_CRASHREPORTER) {
+          this.childMap.set(childID, dumpID);
+        }
 
-        this.childMap.set(aSubject.get("childID"), aSubject.get("dumpID"));
+        if (!this.flushCrashedBrowserQueue(childID)) {
+          this.unseenCrashedChildIDs.push(childID);
+          // The elements in unseenCrashedChildIDs will only be removed if
+          // the tab crash page is shown. However, ipc:content-shutdown might
+          // be fired for processes for which we'll never show the tab crash
+          // page - for example, the thumbnailing process. Another case to
+          // consider is if the user is configured to submit backlogged crash
+          // reports automatically, and a background tab crashes. In that case,
+          // we will never show the tab crash page, and never remove the element
+          // from the list.
+          //
+          // Instead of trying to account for all of those cases, we prevent
+          // this list from getting too large by putting a reasonable upper
+          // limit on how many childIDs we track. It's unlikely that this
+          // array would ever get so large as to be unwieldy (that'd be a lot
+          // or crashes!), but a leak is a leak.
+          if (this.unseenCrashedChildIDs.length > MAX_UNSEEN_CRASHED_CHILD_IDS) {
+            this.unseenCrashedChildIDs.shift();
+          }
+        }
         break;
-
-      case "oop-frameloader-crashed":
+      }
+      case "oop-frameloader-crashed": {
         aSubject.QueryInterface(Ci.nsIFrameLoader);
 
         let browser = aSubject.ownerElement;
-        if (!browser)
+        if (!browser) {
           return;
+        }
 
         this.browserMap.set(browser.permanentKey, aSubject.childID);
         break;
+      }
     }
   },
 
   receiveMessage: function(message) {
     let browser = message.target.browser;
     let gBrowser = browser.ownerGlobal.gBrowser;
     let tab = gBrowser.getTabForBrowser(browser);
 
@@ -129,16 +162,143 @@ this.TabCrashHandler = {
         this.maybeSendCrashReport(message);
         SessionStore.reviveAllCrashedTabs();
         break;
       }
     }
   },
 
   /**
+   * This should be called once a content process has finished
+   * shutting down abnormally. Any tabbrowser browsers that were
+   * selected at the time of the crash will then be sent to
+   * the crashed tab page.
+   *
+   * @param childID (int)
+   *        The childID of the content process that just crashed.
+   * @returns boolean
+   *        True if one or more browsers were sent to the tab crashed
+   *        page.
+   */
+  flushCrashedBrowserQueue(childID) {
+    let browserQueue = this.crashedBrowserQueues.get(childID);
+    if (!browserQueue) {
+      return false;
+    }
+
+    this.crashedBrowserQueues.delete(childID);
+
+    let sentBrowser = false;
+    for (let weakBrowser of browserQueue) {
+      let browser = weakBrowser.get();
+      if (browser) {
+        this.sendToTabCrashedPage(browser);
+        sentBrowser = true;
+      }
+    }
+
+    return sentBrowser;
+  },
+
+  /**
+   * Called by a tabbrowser when it notices that its selected browser
+   * has crashed. This will queue the browser to show the tab crash
+   * page once the content process has finished tearing down.
+   *
+   * @param browser (<xul:browser>)
+   *        The selected browser that just crashed.
+   */
+  onSelectedBrowserCrash(browser) {
+    if (!browser.isRemoteBrowser) {
+      Cu.reportError("Selected crashed browser is not remote.")
+      return;
+    }
+    if (!browser.frameLoader) {
+      Cu.reportError("Selected crashed browser has no frameloader.");
+      return;
+    }
+
+    let childID = browser.frameLoader.childID;
+    let browserQueue = this.crashedBrowserQueues.get(childID);
+    if (!browserQueue) {
+      browserQueue = [];
+      this.crashedBrowserQueues.set(childID, browserQueue);
+    }
+    // It's probably unnecessary to store this browser as a
+    // weak reference, since the content process should complete
+    // its teardown in the same tick of the event loop, and then
+    // this queue will be flushed. The weak reference is to avoid
+    // leaking browsers in case anything goes wrong during this
+    // teardown process.
+    browserQueue.push(Cu.getWeakReference(browser));
+  },
+
+  /**
+   * This method is exposed for SessionStore to call if the user selects
+   * a tab which will restore on demand. It's possible that the tab
+   * is in this state because it recently crashed. If that's the case, then
+   * it's also possible that the user has not seen the tab crash page for
+   * that particular crash, in which case, we might show it to them instead
+   * of restoring the tab.
+   *
+   * @param browser (<xul:browser>)
+   *        A browser from a browser tab that the user has just selected
+   *        to restore on demand.
+   * @returns (boolean)
+   *        True if TabCrashHandler will send the user to the tab crash
+   *        page instead.
+   */
+  willShowCrashedTab(browser) {
+    let childID = this.browserMap.get(browser.permanentKey);
+    // We will only show the tab crash page if:
+    // 1) We are aware that this browser crashed
+    // 2) We know we've never shown the tab crash page for the
+    //    crash yet
+    // 3) The user is not configured to automatically submit backlogged
+    //    crash reports. If they are, we'll send the crash report
+    //    immediately.
+    if (childID &&
+        this.unseenCrashedChildIDs.indexOf(childID) != -1) {
+      if (UnsubmittedCrashHandler.autoSubmit) {
+        let dumpID = this.childMap.get(childID);
+        if (dumpID) {
+          UnsubmittedCrashHandler.submitReports([dumpID]);
+        }
+      } else {
+        this.sendToTabCrashedPage(browser);
+        return true;
+      }
+    }
+
+    return false;
+  },
+
+  /**
+   * We show a special page to users when a normal browser tab has crashed.
+   * This method should be called to send a browser to that page once the
+   * process has completely closed.
+   *
+   * @param browser (<xul:browser>)
+   *        The browser that has recently crashed.
+   */
+  sendToTabCrashedPage(browser) {
+    let title = browser.contentTitle;
+    let uri = browser.currentURI;
+    let gBrowser = browser.ownerGlobal.gBrowser;
+    let tab = gBrowser.getTabForBrowser(browser);
+    // The tab crashed page is non-remote by default.
+    gBrowser.updateBrowserRemoteness(browser, false);
+
+    browser.setAttribute("crashedPageTitle", title);
+    browser.docShell.displayLoadError(Cr.NS_ERROR_CONTENT_CRASHED, uri, null);
+    browser.removeAttribute("crashedPageTitle");
+    tab.setAttribute("crashed", true);
+  },
+
+  /**
    * Submits a crash report from about:tabcrashed, if the crash
    * reporter is enabled and a crash report can be found.
    *
    * @param aBrowser
    *        The <xul:browser> that the report was sent from.
    * @param aFormData
    *        An Object with the following properties:
    *
@@ -260,24 +420,24 @@ this.TabCrashHandler = {
     // can decide whether or not to display the "Restore All
     // Crashed Tabs" button.
     this.pageListener.sendAsyncMessage("UpdateCount", {
       count: this._crashedTabCount,
     });
 
     let browser = message.target.browser;
 
+    let childID = this.browserMap.get(browser.permanentKey);
+    let index = this.unseenCrashedChildIDs.indexOf(childID);
+    if (index != -1) {
+      this.unseenCrashedChildIDs.splice(index, 1);
+    }
+
     let dumpID = this.getDumpID(browser);
     if (!dumpID) {
-      // Make sure to only count once even if there are multiple windows
-      // that will all show about:tabcrashed.
-      if (this._crashedTabCount == 1) {
-        Services.telemetry.getHistogramById("FX_CONTENT_CRASH_DUMP_UNAVAILABLE").add(1);
-      }
-
       message.target.sendAsyncMessage("SetCrashReportAvailable", {
         hasReport: false,
       });
       return;
     }
 
     let sendReport = this.prefs.getBoolPref("sendReport");
     let includeURL = this.prefs.getBoolPref("includeURL");
@@ -315,28 +475,28 @@ this.TabCrashHandler = {
     let browser = message.target.browser;
     let childID = this.browserMap.get(browser.permanentKey);
 
     // Make sure to only count once even if there are multiple windows
     // that will all show about:tabcrashed.
     if (this._crashedTabCount == 0 && childID) {
       Services.telemetry.getHistogramById("FX_CONTENT_CRASH_NOT_SUBMITTED").add(1);
     }
-},
+  },
 
   /**
    * For some <xul:browser>, return a crash report dump ID for that browser
    * if we have been informed of one. Otherwise, return null.
    *
    * @param browser (<xul:browser)
    *        The browser to try to get the dump ID for
    * @returns dumpID (String)
    */
   getDumpID(browser) {
-    if (!this.childMap) {
+    if (!AppConstants.MOZ_CRASHREPORTER) {
       return null;
     }
 
     return this.childMap.get(this.browserMap.get(browser.permanentKey));
   },
 }
 
 /**
@@ -468,18 +628,18 @@ this.UnsubmittedCrashHandler = {
     try {
       reportIDs = yield CrashSubmit.pendingIDsAsync(dateLimit);
     } catch (e) {
       Cu.reportError(e);
       return null;
     }
 
     if (reportIDs.length) {
-      if (CrashNotificationBar.autoSubmit) {
-        CrashNotificationBar.submitReports(reportIDs);
+      if (this.autoSubmit) {
+        this.submitReports(reportIDs);
       } else if (this.shouldShowPendingSubmissionsNotification()) {
         return this.showPendingSubmissionsNotification(reportIDs);
       }
     }
     return null;
   }),
 
   /**
@@ -544,17 +704,17 @@ this.UnsubmittedCrashHandler = {
       return null;
     }
 
     let messageTemplate =
       gNavigatorBundle.GetStringFromName("pendingCrashReports2.label");
 
     let message = PluralForm.get(count, messageTemplate).replace("#1", count);
 
-    let notification = CrashNotificationBar.show({
+    let notification = this.show({
       notificationID: "pending-crash-reports",
       message,
       reportIDs,
       onAction: () => {
         this.showingNotification = false;
       },
     });
 
@@ -573,19 +733,17 @@ this.UnsubmittedCrashHandler = {
    * @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
    * is no browser window), this method exits silently.
    *
    * The notification will allow the user to submit their crash
    * reports. If the user dismissed the notification, the crash