Bug 1256472 - When restoring a window, initialize each browser tab as remote if possible. r?mikedeboer draft
authorMike Conley <mconley@mozilla.com>
Mon, 13 Mar 2017 18:12:40 -0400
changeset 497842 f6359293363c8ad905bff75a9820d5c6fc58af9c
parent 497206 f9362554866b327700c7f9b18050d7b7eb3d2b23
child 549007 c42090bf993026cbafeb28832bce27f3edc749eb
push id49030
push usermconley@mozilla.com
push dateMon, 13 Mar 2017 22:33:43 +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: GlR1PF22UlK
browser/components/sessionstore/SessionStore.jsm
browser/components/sessionstore/content/content-sessionStore.js
browser/components/sessionstore/test/browser_remoteness_flip_on_restore.js
--- a/browser/components/sessionstore/SessionStore.jsm
+++ b/browser/components/sessionstore/SessionStore.jsm
@@ -902,17 +902,17 @@ var SessionStoreInternal = {
         event.initEvent("SSTabRestoring", true, false);
         tab.dispatchEvent(event);
         break;
       case "SessionStore:restoreTabContentStarted":
         if (browser.__SS_restoreState == TAB_STATE_NEEDS_RESTORE) {
           // If a load not initiated by sessionstore was started in a
           // previously pending tab. Mark the tab as no longer pending.
           this.markTabAsRestoring(tab);
-        } else if (!data.isRemotenessUpdate) {
+        } else if (!data.isRemotenessUpdate || data.wasSettingBrowserState) {
           // If the user was typing into the URL bar when we crashed, but hadn't hit
           // enter yet, then we just need to write that value to the URL bar without
           // loading anything. This must happen after the load, as the load will clear
           // userTypedValue.
           let tabData = TabState.collect(tab);
           if (tabData.userTypedValue && !tabData.userTypedClear && !browser.userTypedValue) {
             browser.userTypedValue = tabData.userTypedValue;
             win.URLBarSetURI();
@@ -3203,26 +3203,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);
       }
 
@@ -3479,19 +3481,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
@@ -3647,16 +3646,23 @@ var SessionStoreInternal = {
     let newFrameloader =
       aReloadInFreshProcess || !!browser.frameLoader.groupedSHistory;
     let isRemotenessUpdate =
       tabbrowser.updateBrowserRemotenessByURL(browser, uri, {
         freshProcess: aReloadInFreshProcess,
         newFrameloader: newFrameloader,
       });
 
+    // If we're restoring this tab because we're setting the global browser
+    // state, let the frame script know. That way, when it responds with
+    // restoreTabContentStarted, we can tell the difference between a full
+    // browser state restoration, and a state restoration due to something
+    // like a remoteness flip or a single window restoration.
+    let wasSettingBrowserState = this._browserSetState;
+
     if (isRemotenessUpdate) {
       // We updated the remoteness, so we need to send the history down again.
       //
       // Start a new epoch to discard all frame script messages relating to a
       // previous epoch. All async messages that are still on their way to chrome
       // will be ignored and don't override any tab data set when restoring.
       let epoch = this.startNextEpoch(browser);
 
@@ -3671,17 +3677,17 @@ var SessionStoreInternal = {
 
     // If the restored browser wants to show view source content, start up a
     // view source browser that will load the required frame script.
     if (uri && ViewSourceBrowser.isViewSource(uri)) {
       new ViewSourceBrowser(browser);
     }
 
     browser.messageManager.sendAsyncMessage("SessionStore:restoreTabContent",
-      {loadArguments: aLoadArguments, isRemotenessUpdate});
+      {loadArguments: aLoadArguments, isRemotenessUpdate, wasSettingBrowserState});
   },
 
   /**
    * Marks a given pending tab as restoring.
    *
    * @param aTab
    *        the pending tab to mark as restoring
    */
@@ -3924,47 +3930,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/content/content-sessionStore.js
+++ b/browser/components/sessionstore/content/content-sessionStore.js
@@ -190,27 +190,31 @@ var MessageListener = {
     // SessionStore.jsm so that it can run SSTabRestoring. Users of
     // SSTabRestoring seem to get confused if chrome and content are out of
     // sync about the state of the restore (particularly regarding
     // docShell.currentURI). Using a synchronous message is the easiest way
     // to temporarily synchronize them.
     sendSyncMessage("SessionStore:restoreHistoryComplete", {epoch, isRemotenessUpdate});
   },
 
-  restoreTabContent({loadArguments, isRemotenessUpdate}) {
+  restoreTabContent({loadArguments, isRemotenessUpdate, wasSettingBrowserState}) {
     let epoch = gCurrentEpoch;
 
     // We need to pass the value of didStartLoad back to SessionStore.jsm.
     let didStartLoad = gContentRestore.restoreTabContent(loadArguments, isRemotenessUpdate, () => {
       // Tell SessionStore.jsm that it may want to restore some more tabs,
       // since it restores a max of MAX_CONCURRENT_TAB_RESTORES at a time.
       sendAsyncMessage("SessionStore:restoreTabContentComplete", {epoch, isRemotenessUpdate});
     });
 
-    sendAsyncMessage("SessionStore:restoreTabContentStarted", {epoch, isRemotenessUpdate});
+    sendAsyncMessage("SessionStore:restoreTabContentStarted", {
+      epoch,
+      isRemotenessUpdate,
+      wasSettingBrowserState
+    });
 
     if (!didStartLoad) {
       // Pretend that the load succeeded so that event handlers fire correctly.
       sendAsyncMessage("SessionStore:restoreTabContentComplete", {epoch, isRemotenessUpdate});
     }
   },
 
   flush({id}) {
--- 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);
 });