Bug 1442694 - Fix failures due to removing selected tab r=Gijs
authorDoug Thayer <dothayer@mozilla.com>
Fri, 01 Mar 2019 18:29:09 +0000
changeset 520116 8b3fe0426ffc
parent 520115 3bd9591627ce
child 520117 5caf48a420eb
push id10862
push userffxbld-merge
push dateMon, 11 Mar 2019 13:01:11 +0000
treeherdermozilla-beta@a2e7f5c935da [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersGijs
bugs1442694
milestone67.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1442694 - Fix failures due to removing selected tab r=Gijs This adds test which reproduce the failure as well as the fix. Essentially, if we hit the edited case in SessionStore with `tab` equal to `tabbrowser.tabs[t]`, we remove the tab and then try to pin it, which obviously blows up. Note: the additional method in SessionStore.jsm was largely to get around complexity requirements inside restoreWindow. Cleaner solutions welcome. Differential Revision: https://phabricator.services.mozilla.com/D21383
browser/components/sessionstore/SessionStore.jsm
browser/components/sessionstore/test/browser_preopened_apptabs.js
--- a/browser/components/sessionstore/SessionStore.jsm
+++ b/browser/components/sessionstore/SessionStore.jsm
@@ -3596,16 +3596,46 @@ var SessionStoreInternal = {
       if (!winData || !winData.tabs || !winData.tabs[0])
         continue;
       this._openWindowWithState({ windows: [winData] });
     }
     return Promise.all([...WINDOW_SHOWING_PROMISES.values()].map(deferred => deferred.promise));
   },
 
   /**
+   * Handles the pinning / unpinning of a selected tab restored with
+   * restoreWindow.
+   *
+   * @param aWindow
+   *        Window reference to the window used for restoration
+   * @param aWinData
+   *        The window data we're restoring
+   * @param aRestoreIndex
+   *        The index of the tab data we're currently restoring
+   * @returns the selected tab
+   */
+  _updateRestoredSelectedTabPinnedState(aWindow, aWinData, aRestoreIndex) {
+    let tabbrowser = aWindow.gBrowser;
+    let tabData = aWinData.tabs[aRestoreIndex];
+    let tab = tabbrowser.selectedTab;
+    let needsUnpin = tab.pinned && !tabData.pinned;
+    let needsPin = !tab.pinned && tabData.pinned;
+    if (needsUnpin) {
+      tabbrowser.unpinTab(tab);
+    } else if (needsPin && tab == tabbrowser.tabs[aRestoreIndex]) {
+      tabbrowser.pinTab(tab);
+    } else if (needsPin) {
+      tabbrowser.removeTab(tabbrowser.tabs[aRestoreIndex]);
+      tabbrowser.pinTab(tab);
+      tabbrowser.moveTabTo(tab, aRestoreIndex);
+    }
+    return tab;
+  },
+
+  /**
    * restore features to a single window
    * @param aWindow
    *        Window reference to the window to use for restoration
    * @param winData
    *        JS object
    * @param aOptions
    *        {overwriteTabs: true} to overwrite existing tabs w/ new ones
    *        {firstWindow: true} if this is the first non-private window we're
@@ -3680,24 +3710,17 @@ var SessionStoreInternal = {
       let userContextId = tabData.userContextId;
       let select = t == selectTab - 1;
       let tab;
 
       // Re-use existing selected tab if possible to avoid the overhead of
       // selecting a new tab.
       if (select &&
           tabbrowser.selectedTab.userContextId == userContextId) {
-        tab = tabbrowser.selectedTab;
-        if (tab.pinned && !tabData.pinned) {
-          tabbrowser.unpinTab(tab);
-        } else if (!tab.pinned && tabData.pinned) {
-          tabbrowser.removeTab(tabbrowser.tabs[t]);
-          tabbrowser.pinTab(tab);
-          tabbrowser.moveTabTo(tab, t);
-        }
+        tab = this._updateRestoredSelectedTabPinnedState(aWindow, winData, t);
 
         tabbrowser.moveTabToEnd();
         if (aWindow.gMultiProcessBrowser && !tab.linkedBrowser.isRemoteBrowser) {
           tabbrowser.updateBrowserRemoteness(tab.linkedBrowser, true);
         }
       } else if (tabData.pinned &&
           tabbrowser.tabs[t] &&
           tabbrowser.tabs[t].pinned &&
--- a/browser/components/sessionstore/test/browser_preopened_apptabs.js
+++ b/browser/components/sessionstore/test/browser_preopened_apptabs.js
@@ -18,121 +18,48 @@ function muffleMainWindowType() {
     });
   }
 }
 
 registerCleanupFunction(function() {
   Services.prefs.clearUserPref(PREF_NUM_PINNED_TABS);
 });
 
-add_task(async function testPrefSynced() {
-  Services.prefs.setIntPref(PREF_NUM_PINNED_TABS, 3);
-
-  let state = { windows: [{ tabs: [
-    { entries: [{ url: "http://example.org/#1", triggeringPrincipal_base64 }], pinned: true },
-    { entries: [{ url: "http://example.org/#2", triggeringPrincipal_base64 }], pinned: true },
-    { entries: [{ url: "http://example.org/#3", triggeringPrincipal_base64 }], pinned: true },
-  ], selected: 3 }] };
-
-  muffleMainWindowType();
-  let win = await BrowserTestUtils.openNewBrowserWindow();
-  Assert.equal([...win.gBrowser.tabs].filter(t => t.pinned).length, 3);
-  Assert.equal([...win.gBrowser.tabs].filter(t => !!t.linkedPanel).length, 1);
-  await setWindowState(win, state, false, true);
-  Assert.equal([...win.gBrowser.tabs].filter(t => t.pinned).length, 3);
-  Assert.equal([...win.gBrowser.tabs].filter(t => !!t.linkedPanel).length, 4);
-  await BrowserTestUtils.closeWindow(win);
-});
-
-add_task(async function testPrefSyncedSelected() {
-  Services.prefs.setIntPref(PREF_NUM_PINNED_TABS, 3);
-
-  let state = { windows: [{ tabs: [
-    { entries: [{ url: "http://example.org/#1", triggeringPrincipal_base64 }], pinned: true },
-    { entries: [{ url: "http://example.org/#2", triggeringPrincipal_base64 }], pinned: true },
-    { entries: [{ url: "http://example.org/#3", triggeringPrincipal_base64 }], pinned: true },
-  ], selected: 1 }] };
-
-  muffleMainWindowType();
-  let win = await BrowserTestUtils.openNewBrowserWindow();
-  Assert.equal([...win.gBrowser.tabs].filter(t => t.pinned).length, 3);
-  Assert.equal([...win.gBrowser.tabs].filter(t => !!t.linkedPanel).length, 1);
-  await setWindowState(win, state, false, true);
-  Assert.equal([...win.gBrowser.tabs].filter(t => t.pinned).length, 3);
-  Assert.equal([...win.gBrowser.tabs].filter(t => !!t.linkedPanel).length, 4);
-  await BrowserTestUtils.closeWindow(win);
-});
-
-add_task(async function testPrefTooHigh() {
-  Services.prefs.setIntPref(PREF_NUM_PINNED_TABS, 5);
-
-  let state = { windows: [{ tabs: [
-    { entries: [{ url: "http://example.org/#1", triggeringPrincipal_base64 }], pinned: true },
-    { entries: [{ url: "http://example.org/#2", triggeringPrincipal_base64 }], pinned: true },
-    { entries: [{ url: "http://example.org/#3", triggeringPrincipal_base64 }], pinned: true },
-  ], selected: 3 }] };
-
-  muffleMainWindowType();
-  let win = await BrowserTestUtils.openNewBrowserWindow();
-  Assert.equal([...win.gBrowser.tabs].filter(t => t.pinned).length, 5);
-  Assert.equal([...win.gBrowser.tabs].filter(t => !!t.linkedPanel).length, 1);
-  await setWindowState(win, state, false, true);
-  Assert.equal([...win.gBrowser.tabs].filter(t => t.pinned).length, 3);
-  Assert.equal([...win.gBrowser.tabs].filter(t => !!t.linkedPanel).length, 4);
-  await BrowserTestUtils.closeWindow(win);
-});
+let testCases = [
+  {numPinnedPref: 5, selected: 3, overrideTabs: false},
+  {numPinnedPref: 5, selected: 3, overrideTabs: true},
+  {numPinnedPref: 5, selected: 1, overrideTabs: false},
+  {numPinnedPref: 5, selected: 1, overrideTabs: true},
+  {numPinnedPref: 3, selected: 3, overrideTabs: false},
+  {numPinnedPref: 3, selected: 3, overrideTabs: true},
+  {numPinnedPref: 3, selected: 1, overrideTabs: false},
+  {numPinnedPref: 3, selected: 1, overrideTabs: true},
+  {numPinnedPref: 1, selected: 3, overrideTabs: false},
+  {numPinnedPref: 1, selected: 3, overrideTabs: true},
+  {numPinnedPref: 1, selected: 1, overrideTabs: false},
+  {numPinnedPref: 1, selected: 1, overrideTabs: true},
+  {numPinnedPref: 0, selected: 3, overrideTabs: false},
+  {numPinnedPref: 0, selected: 3, overrideTabs: true},
+  {numPinnedPref: 0, selected: 1, overrideTabs: false},
+  {numPinnedPref: 0, selected: 1, overrideTabs: true},
+];
 
-add_task(async function testPrefTooLow() {
-  Services.prefs.setIntPref(PREF_NUM_PINNED_TABS, 1);
-
-  let state = { windows: [{ tabs: [
-    { entries: [{ url: "http://example.org/#1", triggeringPrincipal_base64 }], pinned: true },
-    { entries: [{ url: "http://example.org/#2", triggeringPrincipal_base64 }], pinned: true },
-    { entries: [{ url: "http://example.org/#3", triggeringPrincipal_base64 }], pinned: true },
-  ], selected: 3 }] };
+for (let {numPinnedPref, selected, overrideTabs} of testCases) {
+  add_task(async function testPrefSynced() {
+    Services.prefs.setIntPref(PREF_NUM_PINNED_TABS, numPinnedPref);
 
-  muffleMainWindowType();
-  let win = await BrowserTestUtils.openNewBrowserWindow();
-  Assert.equal([...win.gBrowser.tabs].filter(t => t.pinned).length, 1);
-  Assert.equal([...win.gBrowser.tabs].filter(t => !!t.linkedPanel).length, 1);
-  await setWindowState(win, state, false, true);
-  Assert.equal([...win.gBrowser.tabs].filter(t => t.pinned).length, 3);
-  Assert.equal([...win.gBrowser.tabs].filter(t => !!t.linkedPanel).length, 4);
-  await BrowserTestUtils.closeWindow(win);
-});
-
-add_task(async function testPrefTooHighSelected() {
-  Services.prefs.setIntPref(PREF_NUM_PINNED_TABS, 5);
-
-  let state = { windows: [{ tabs: [
-    { entries: [{ url: "http://example.org/#1", triggeringPrincipal_base64 }], pinned: true },
-    { entries: [{ url: "http://example.org/#2", triggeringPrincipal_base64 }], pinned: true },
-    { entries: [{ url: "http://example.org/#3", triggeringPrincipal_base64 }], pinned: true },
-  ], selected: 1 }] };
+    let state = { windows: [{ tabs: [
+      { entries: [{ url: "http://example.org/#1", triggeringPrincipal_base64 }], pinned: true, userContextId: 0 },
+      { entries: [{ url: "http://example.org/#2", triggeringPrincipal_base64 }], pinned: true, userContextId: 0 },
+      { entries: [{ url: "http://example.org/#3", triggeringPrincipal_base64 }], pinned: true, userContextId: 0 },
+    ], selected }] };
 
-  muffleMainWindowType();
-  let win = await BrowserTestUtils.openNewBrowserWindow();
-  Assert.equal([...win.gBrowser.tabs].filter(t => t.pinned).length, 5);
-  Assert.equal([...win.gBrowser.tabs].filter(t => !!t.linkedPanel).length, 1);
-  await setWindowState(win, state, false, true);
-  Assert.equal([...win.gBrowser.tabs].filter(t => t.pinned).length, 3);
-  Assert.equal([...win.gBrowser.tabs].filter(t => !!t.linkedPanel).length, 4);
-  await BrowserTestUtils.closeWindow(win);
-});
-
-add_task(async function testPrefTooLowSelected() {
-  Services.prefs.setIntPref(PREF_NUM_PINNED_TABS, 1);
-
-  let state = { windows: [{ tabs: [
-    { entries: [{ url: "http://example.org/#1", triggeringPrincipal_base64 }], pinned: true },
-    { entries: [{ url: "http://example.org/#2", triggeringPrincipal_base64 }], pinned: true },
-    { entries: [{ url: "http://example.org/#3", triggeringPrincipal_base64 }], pinned: true },
-  ], selected: 1 }] };
-
-  muffleMainWindowType();
-  let win = await BrowserTestUtils.openNewBrowserWindow();
-  Assert.equal([...win.gBrowser.tabs].filter(t => t.pinned).length, 1);
-  Assert.equal([...win.gBrowser.tabs].filter(t => !!t.linkedPanel).length, 1);
-  await setWindowState(win, state, false, true);
-  Assert.equal([...win.gBrowser.tabs].filter(t => t.pinned).length, 3);
-  Assert.equal([...win.gBrowser.tabs].filter(t => !!t.linkedPanel).length, 4);
-  await BrowserTestUtils.closeWindow(win);
-});
+    muffleMainWindowType();
+    let win = await BrowserTestUtils.openNewBrowserWindow();
+    Assert.equal([...win.gBrowser.tabs].filter(t => t.pinned).length, numPinnedPref);
+    Assert.equal([...win.gBrowser.tabs].filter(t => !!t.linkedPanel).length, 1);
+    await setWindowState(win, state, overrideTabs, true);
+    Assert.equal([...win.gBrowser.tabs].filter(t => t.pinned).length, 3);
+    Assert.equal([...win.gBrowser.tabs].filter(t => !!t.linkedPanel).length,
+                 overrideTabs ? 3 : 4);
+    await BrowserTestUtils.closeWindow(win);
+  });
+}