Bug 1290280 - Improve the logic for flipping the remoteness of the initial browser back to non-remote. r=mikedeboer
authorMike Conley <mconley@mozilla.com>
Tue, 09 Aug 2016 13:32:21 -0400
changeset 309363 3bce29db23296122298437f8a2eeb6043aac378c
parent 309362 cec1738be04c0e2ec1dfa0b5f174cf8dbf3b8ac3
child 309364 e5ab45eab1be2dc9a70fc011af44716c429b89b9
push id20308
push userkwierso@gmail.com
push dateMon, 15 Aug 2016 22:04:54 +0000
treeherderfx-team@2697bf7ad45d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
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(),