Bug 1256472 - When restoring a window, initialize each browser tab as remote if possible. r?mikedeboer draft
authorMike Conley <mconley@mozilla.com>
Tue, 14 Mar 2017 10:01:38 -0400
changeset 498485 7ce620aff2d5bf2338a31f929b90aaf02e21d40f
parent 498484 5b6d43de7d00bd2b77d141d4606656da09e0ba5a
child 498486 f651695b7a3e1a09bdba124fc2a9eab98d43538b
push id49206
push usermconley@mozilla.com
push dateTue, 14 Mar 2017 20:46:25 +0000
reviewersmikedeboer
bugs1256472, 1241459
milestone55.0a1
Bug 1256472 - When restoring a window, initialize each browser tab as remote if possible. r?mikedeboer Originally, we were forcing these restored background tabs to be non-remote by default. This was because we didn't want them to show the crashed tab favicon nor show about:tabcrashed if the user hadn't restored them before. Bug 1241459 added infrastructure that makes it possible to put crashed background tabs into the "restore on demand" state again, without showing about:tabcrashed or showing the crashed tab favicon. This means we should be able to restore tabs in the content process again which should take some load off of the parent process during session restore, which is good for perceived performance. Note that if the content process does crash, the background tabs are then loaded in the parent process. Restoring them on demand will then do the remoteness flip. MozReview-Commit-ID: 1mWe0td6geB
browser/components/sessionstore/SessionStore.jsm
browser/components/sessionstore/test/browser_remoteness_flip_on_restore.js
--- a/browser/components/sessionstore/SessionStore.jsm
+++ b/browser/components/sessionstore/SessionStore.jsm
@@ -3204,26 +3204,28 @@ var SessionStoreInternal = {
     let numVisibleTabs = 0;
 
     for (var t = 0; t < newTabCount; t++) {
       // When trying to restore into existing tab, we also take the userContextId
       // into account if present.
       let userContextId = winData.tabs[t].userContextId;
       let reuseExisting = t < openTabCount &&
                           (tabbrowser.tabs[t].getAttribute("usercontextid") == (userContextId || ""));
-      // If the tab is pinned, then we'll be loading it right away, and
-      // there's no need to cause a remoteness flip by loading it initially
-      // non-remote.
-      let forceNotRemote = !winData.tabs[t].pinned;
       let tab = reuseExisting ? tabbrowser.tabs[t] :
                                 tabbrowser.addTab("about:blank",
                                                   {skipAnimation: true,
-                                                   forceNotRemote,
                                                    userContextId});
 
+      if (reuseExisting) {
+        this._maybeUpdateBrowserRemoteness({
+          tabbrowser,
+          browser: tab.linkedBrowser
+        });
+      }
+
       // If we inserted a new tab because the userContextId didn't match with the
       // open tab, even though `t < openTabCount`, we need to remove that open tab
       // and put the newly added tab in its place.
       if (!reuseExisting && t < openTabCount) {
         tabbrowser.removeTab(tabbrowser.tabs[t]);
         tabbrowser.moveTabTo(tab, t);
       }
 
@@ -3482,19 +3484,16 @@ var SessionStoreInternal = {
     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);
 
     // In case we didn't collect/receive data for any tabs yet we'll have to
@@ -3933,47 +3932,21 @@ var SessionStoreInternal = {
    * @param (object) with the following properties:
    *
    *        tabbrowser (<xul:tabbrowser>):
    *          The tabbrowser that the browser belongs to.
    *
    *        tab (<xul:tab>):
    *          The tab being restored
    *
-   *        willRestoreImmediately (bool):
-   *          true if the tab is going to have its content
-   *          restored immediately by the caller.
-   *
    */
-  _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 a 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 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);
+  _maybeUpdateBrowserRemoteness({ tabbrowser, browser }) {
+    let win = tabbrowser.ownerGlobal;
+    if (win.gMultiProcessBrowser && !browser.isRemoteBrowser) {
+      tabbrowser.updateBrowserRemoteness(browser, true);
     }
   },
 
   /**
    * Update the session start time and send a telemetry measurement
    * for the number of days elapsed since the session was started.
    *
    * @param state
--- a/browser/components/sessionstore/test/browser_remoteness_flip_on_restore.js
+++ b/browser/components/sessionstore/test/browser_remoteness_flip_on_restore.js
@@ -99,17 +99,18 @@ const PINNED_STATE = {
  *   restoring state. Each bool in the Array represents the window
  *   tabs in order. A "true" indicates that the tab be remote, and
  *   a "false" indicates that the tab should be "non-remote". We
  *   need this Array in order to test pinned tabs which will also
  *   be loaded by default, and therefore should end up remote.
  *
  */
 function* runScenarios(scenarios) {
-  for (let scenario of scenarios) {
+  for (let [scenarioIndex, scenario] of scenarios.entries()) {
+    info("Running scenario " + scenarioIndex);
     // Let's make sure our scenario is sane first.
     Assert.equal(scenario.expectedFlips.length,
                  scenario.expectedRemoteness.length,
                  "All expected flips and remoteness needs to be supplied");
     Assert.ok(scenario.initialSelectedTab > 0,
               "You must define an initially selected tab");
 
     // First, we need to create the initial conditions, so we
@@ -229,114 +230,114 @@ add_task(function*() {
     // when the restored window is being opened.
     {
       initialRemoteness: [true],
       initialSelectedTab: 1,
       stateToRestore: SIMPLE_STATE,
       selectedTab: 3,
       // The initial tab is remote and should go into
       // the background state. The second and third tabs
-      // are new and should be initialized non-remote.
-      expectedFlips: [true, false, true],
-      // Only the selected tab should be remote.
-      expectedRemoteness: [false, false, true],
+      // are new and should initialize remotely as well.
+      // There should therefore be no remoteness flips.
+      expectedFlips: [false, false, false],
+      // All tabs should now be remote.
+      expectedRemoteness: [true, true, true],
     },
 
     // A single remote tab, and this is the one that's going
     // to be selected once state is restored.
     {
       initialRemoteness: [true],
       initialSelectedTab: 1,
       stateToRestore: SIMPLE_STATE,
       selectedTab: 1,
       // The initial tab is remote and selected, so it should
       // not flip remoteness. The other two new tabs should
-      // be non-remote by default.
+      // initialize as remote unrestored background tabs.
       expectedFlips: [false, false, false],
-      // Only the selected tab should be remote.
-      expectedRemoteness: [true, false, false],
+      // All tabs should now be remote.
+      expectedRemoteness: [true, true, true],
     },
 
     // A single remote tab which starts selected. We set the
     // selectedTab to 0 which is equivalent to "don't change
     // the tab selection in the window".
     {
       initialRemoteness: [true],
       initialSelectedTab: 1,
       stateToRestore: SIMPLE_STATE,
       selectedTab: 0,
       // The initial tab is remote and selected, so it should
       // not flip remoteness. The other two new tabs should
-      // be non-remote by default.
+      // initialize as remote unrestored background tabs.
       expectedFlips: [false, false, false],
-      // Only the selected tab should be remote.
-      expectedRemoteness: [true, false, false],
+      // All tabs should now be remote.
+      expectedRemoteness: [true, true, true],
     },
 
     // An initially remote tab, but we're going to load
     // some pinned tabs now, and the pinned tabs should load
     // right away.
     {
       initialRemoteness: [true],
       initialSelectedTab: 1,
       stateToRestore: PINNED_STATE,
       selectedTab: 3,
       // The initial tab is pinned and will load right away,
       // so it should stay remote. The second tab is new
       // and pinned, so it should start remote and not flip.
       // The third tab is not pinned, but it is selected,
-      // so it will start non-remote, and then flip remoteness.
-      expectedFlips: [false, false, true],
+      // so it will start remote.
+      expectedFlips: [false, false, false],
       // Both pinned tabs and the selected tabs should all
       // end up being remote.
       expectedRemoteness: [true, true, true],
     },
 
     // A single non-remote tab.
     {
       initialRemoteness: [false],
       initialSelectedTab: 1,
       stateToRestore: SIMPLE_STATE,
       selectedTab: 2,
-      // The initial tab is non-remote and should stay
-      // that way. The second and third tabs are new and
-      // should be initialized non-remote.
-      expectedFlips: [false, true, false],
-      // Only the selected tab should be remote.
-      expectedRemoteness: [false, true, false],
+      // The initial tab is non-remote and should become remote.
+      // The second and third tabs are new and should be initialized
+      // as remote.
+      expectedFlips: [true, false, false],
+      // All tabs should now be remote.
+      expectedRemoteness: [true, true, true],
     },
 
     // A mixture of remote and non-remote tabs.
     {
       initialRemoteness: [true, false, true],
       initialSelectedTab: 1,
       stateToRestore: SIMPLE_STATE,
       selectedTab: 3,
-      // The initial tab is remote and should flip to non-remote
-      // as it is put into the background. The second tab should
-      // stay non-remote, and the third one should stay remote.
-      expectedFlips: [true, false, false],
-      // Only the selected tab should be remote.
-      expectedRemoteness: [false, false, true],
+      // The initial tab is remote and should stay that way, even
+      // when put into the background. The second tab should flip
+      // remoteness, and the third one should stay remote.
+      expectedFlips: [false, true, false],
+      // All tabs should now be remote.
+      expectedRemoteness: [true, true, true],
     },
 
     // An initially non-remote tab, but we're going to load
     // some pinned tabs now, and the pinned tabs should load
     // right away.
     {
       initialRemoteness: [false],
       initialSelectedTab: 1,
       stateToRestore: PINNED_STATE,
       selectedTab: 3,
       // The initial tab is pinned and will load right away,
       // so it should flip remoteness. The second tab is new
       // and pinned, so it should start remote and not flip.
       // The third tab is not pinned, but it is selected,
-      // so it will start non-remote, and then flip remoteness.
-      expectedFlips: [true, false, true],
-      // Both pinned tabs and the selected tabs should all
-      // end up being remote.
+      // so it will start remote, and not flip remoteness.
+      expectedFlips: [true, false, false],
+      // All tabs should now be remote.
       expectedRemoteness: [true, true, true],
     },
   ];
 
   yield* runScenarios(TEST_SCENARIOS);
 });