Bug 1290280 - Improve the logic for flipping the remoteness of the initial browser back to non-remote. r?mikedeboer draft
authorMike Conley <mconley@mozilla.com>
Tue, 09 Aug 2016 13:32:21 -0400
changeset 400474 a7b65a96382d3000db960bfdd4685b7d6bac1313
parent 397822 e78975b53563d80c99ebfbdf8a9fbf6b829a8a48
child 400475 deed3fe61a2fb0ea2c6d5e7d83d40a89d35736bb
child 400483 0d227c6f30866a3cc811be0c100284ef724d44ad
push id26156
push usermconley@mozilla.com
push dateSat, 13 Aug 2016 23:05:35 +0000
reviewersmikedeboer
bugs1290280
milestone51.0a1
Bug 1290280 - Improve the logic for flipping the remoteness of the initial browser back to non-remote. r?mikedeboer The code that checks to see whether or not we should flip the remoteness of a browser before loading the session state into it wasn't accounting for the fact that oftentimes, restoreImmediately isn't included, so it's undefined, which coerces to "false-y". This caused us to very quickly destroy a TabParent, very soon after creating it. In some cases, the IPC layer seems to not like that, and throws an OnChannelError, which causes the TabParent ActorDestroy method to be called with an abnormal shutdown reason, which causes the tab crash observer to fire, which bubbles the tab crash event. We should probably make the IPC layer more resilient to this sort of thing, but we should also probably not flip remoteness when we really don't need to. Now instead, when restoring a tab, we detect whether or not it's going to be restored automatically in the near future. If it's not going to be restored automatically, and the browser is remote, we flip its remoteness - otherwise we leave it alone. MozReview-Commit-ID: 5AmPHvzDZlX
browser/components/sessionstore/SessionStore.jsm
--- a/browser/components/sessionstore/SessionStore.jsm
+++ b/browser/components/sessionstore/SessionStore.jsm
@@ -3211,18 +3211,26 @@ var SessionStoreInternal = {
 
     let restoreImmediately = options.restoreImmediately;
     let loadArguments = options.loadArguments;
     let browser = tab.linkedBrowser;
     let window = tab.ownerGlobal;
     let tabbrowser = window.gBrowser;
     let forceOnDemand = options.forceOnDemand;
 
-    this._maybeUpdateBrowserRemoteness(Object.assign({
-      browser, tabbrowser, tabData }, options));
+    let willRestoreImmediately = restoreImmediately ||
+                                 tabbrowser.selectedBrowser == browser ||
+                                 loadArguments;
+
+    if (!willRestoreImmediately && !forceOnDemand) {
+      TabRestoreQueue.add(tab);
+    }
+
+    this._maybeUpdateBrowserRemoteness({ tabbrowser, tab,
+                                         willRestoreImmediately });
 
     // Increase the busy state counter before modifying the tab.
     this._setWindowStateBusy(window);
 
     // It's important to set the window state to dirty so that
     // we collect their data for the first time when saving state.
     DirtyWindows.add(window);
 
@@ -3322,20 +3330,19 @@ var SessionStoreInternal = {
 
     // Restore tab attributes.
     if ("attributes" in tabData) {
       TabAttributes.set(tab, tabData.attributes);
     }
 
     // This could cause us to ignore MAX_CONCURRENT_TAB_RESTORES a bit, but
     // it ensures each window will have its selected tab loaded.
-    if (restoreImmediately || tabbrowser.selectedBrowser == browser || loadArguments) {
+    if (willRestoreImmediately) {
       this.restoreTabContent(tab, loadArguments);
     } else if (!forceOnDemand) {
-      TabRestoreQueue.add(tab);
       this.restoreNextTab();
     }
 
     // Decrease the busy state counter after we're done.
     this._setWindowStateReady(window);
   },
 
   /**
@@ -3607,55 +3614,57 @@ var SessionStoreInternal = {
     }
 
     SessionSaver.runDelayed();
   },
 
   /* ........ Auxiliary Functions .............. */
 
   /**
-   * Determines whether or not a browser for a tab that is
-   * being restored needs to have its remoteness flipped first.
+   * Determines whether or not a tab that is being restored needs
+   * to have its remoteness flipped first.
    *
    * @param (object) with the following properties:
    *
-   *        browser (<xul:browser>):
-   *          The browser for the tab being restored.
-   *
    *        tabbrowser (<xul:tabbrowser>):
    *          The tabbrowser that the browser belongs to.
    *
-   *        tabData (object):
-   *          The tabData state that we're attempting to
-   *          restore for the tab.
+   *        tab (<xul:tab>):
+   *          The tab being restored
    *
-   *        restoreImmediately (bool):
-   *          true if loading of content should occur immediately
-   *          after restoration.
+   *        willRestoreImmediately (bool):
+   *          true if the tab is going to have its content
+   *          restored immediately by the caller.
    *
-   *        forceOnDemand (bool):
-   *          true if the tab is being put into the restore-on-demand
-   *          background state.
    */
-  _maybeUpdateBrowserRemoteness({ browser, tabbrowser, tabData,
-                                  restoreImmediately, forceOnDemand }) {
+  _maybeUpdateBrowserRemoteness({ tabbrowser, tab,
+                                  willRestoreImmediately }) {
     // If the browser we're attempting to restore happens to be
     // remote, we need to flip it back to non-remote if it's going
     // to go into the pending background tab state. This is to make
-    // sure that the background tab can't crash if it hasn't yet
-    // been restored. Normally, when a window is restored, the tabs
-    // that SessionStore inserts are non-remote - but the initial
-    // browser is, by default, remote, so this check and flip is
-    // mostly for that case.
+    // sure that a background tab can't crash if it hasn't yet
+    // been restored.
     //
-    // Note that pinned tabs are exempt from this flip, since
-    // they'll be loaded immediately anyways.
-    if (browser.isRemoteBrowser &&
-        !tabData.pinned &&
-        (!restoreImmediately || forceOnDemand)) {
+    // Normally, when a window is restored, the tabs that SessionStore
+    // inserts are non-remote - but the initial browser is, by default,
+    // remote, so this check and flip covers this case. The other case
+    // is when window state is overwriting the state of an existing
+    // window with some remote tabs.
+    let browser = tab.linkedBrowser;
+
+    // There are two ways that a tab might start restoring its content
+    // very soon - either the caller is going to restore the content
+    // immediately, or the TabRestoreQueue is set up so that the tab
+    // content is going to be restored in the very near future. In
+    // either case, we don't want to flip remoteness, since the browser
+    // will soon be loading content.
+    let willRestore = willRestoreImmediately ||
+                      TabRestoreQueue.willRestoreSoon(tab);
+
+    if (browser.isRemoteBrowser && !willRestore) {
       tabbrowser.updateBrowserRemoteness(browser, false);
     }
   },
 
   /**
    * Update the session start time and send a telemetry measurement
    * for the number of days elapsed since the session was started.
    *
@@ -4422,17 +4431,46 @@ var TabRestoreQueue = {
   visibleToHidden: function (tab) {
     let {visible, hidden} = this.tabs;
     let index = visible.indexOf(tab);
 
     if (index > -1) {
       visible.splice(index, 1);
       hidden.push(tab);
     }
-  }
+  },
+
+  /**
+   * Returns true if the passed tab is in one of the sets that we're
+   * restoring content in automatically.
+   *
+   * @param tab (<xul:tab>)
+   *        The tab to check
+   * @returns bool
+   */
+  willRestoreSoon: function (tab) {
+    let { priority, hidden, visible } = this.tabs;
+    let { restoreOnDemand, restorePinnedTabsOnDemand,
+          restoreHiddenTabs } = this.prefs;
+    let restorePinned = !(restoreOnDemand && restorePinnedTabsOnDemand);
+    let candidateSet = [];
+
+    if (restorePinned && priority.length)
+      candidateSet.push(...priority);
+
+    if (!restoreOnDemand) {
+      if (visible.length)
+        candidateSet.push(...visible);
+
+      if (restoreHiddenTabs && hidden.length)
+        candidateSet.push(...hidden);
+    }
+
+    return candidateSet.indexOf(tab) > -1;
+  },
 };
 
 // A map storing a closed window's state data until it goes aways (is GC'ed).
 // This ensures that API clients can still read (but not write) states of
 // windows they still hold a reference to but we don't.
 var DyingWindowCache = {
   _data: new WeakMap(),