From 532800137b4d5f01ff88d13fd76d73ddd3874c2b Mon Sep 17 00:00:00 2001 From: Sebastian Hengst Date: Wed, 3 May 2017 23:15:56 +0200 Subject: [PATCH] Backed out changeset db9f3b377504 (bug 1054740) for failing Marionette's test_prefs.py TestPreferences.test_clear_pref, at least on Linux in non-e10s mode. r=backout on a CLOSED TREE --- .../general/browser_tab_detach_restore.js | 2 +- .../components/sessionstore/SessionStore.jsm | 149 ++++++++++++++---- .../sessionstore/test/browser_423132.js | 2 +- .../sessionstore/test/browser_586147.js | 12 +- .../sessionstore/test/browser_590563.js | 15 +- .../sessionstore/test/browser_624727.js | 2 + .../browser_remoteness_flip_on_restore.js | 113 ++++++++++++- .../tests/unit/unit-tests.ini | 2 +- 8 files changed, 245 insertions(+), 52 deletions(-) diff --git a/browser/base/content/test/general/browser_tab_detach_restore.js b/browser/base/content/test/general/browser_tab_detach_restore.js index fcd3a5a81995..d482edc26047 100644 --- a/browser/base/content/test/general/browser_tab_detach_restore.js +++ b/browser/base/content/test/general/browser_tab_detach_restore.js @@ -25,7 +25,7 @@ add_task(function*() { win = SessionStore.undoCloseWindow(0); yield BrowserTestUtils.waitForEvent(win, "load"); - yield BrowserTestUtils.waitForEvent(win.gBrowser.tabContainer, "SSTabRestored"); + yield BrowserTestUtils.waitForEvent(win.gBrowser.tabs[0], "SSTabRestored"); is(win.gBrowser.tabs.length, 1, "Should have restored one tab"); is(win.gBrowser.selectedBrowser.currentURI.spec, uri, "Should have restored the right page"); diff --git a/browser/components/sessionstore/SessionStore.jsm b/browser/components/sessionstore/SessionStore.jsm index a740c0f02b05..25cdfa5cf250 100644 --- a/browser/components/sessionstore/SessionStore.jsm +++ b/browser/components/sessionstore/SessionStore.jsm @@ -3228,6 +3228,9 @@ var SessionStoreInternal = { let overwriteTabs = aOptions && aOptions.overwriteTabs; let isFollowUp = aOptions && aOptions.isFollowUp; let firstWindow = aOptions && aOptions.firstWindow; + // See SessionStoreInternal.restoreTabs for a description of what + // selectTab represents. + let selectTab = (overwriteTabs ? parseInt(winData.selected || 1, 10) : 0); if (isFollowUp) { this.windowToFocus = aWindow; @@ -3252,25 +3255,22 @@ var SessionStoreInternal = { winData.tabs = []; } - // See SessionStoreInternal.restoreTabs for a description of what - // selectTab represents. - let selectTab = 0; - if (overwriteTabs) { - selectTab = parseInt(winData.selected || 1, 10); - selectTab = Math.max(selectTab, 1); - selectTab = Math.min(selectTab, winData.tabs.length); - } - - let tabbrowser = aWindow.gBrowser; - let tabsToRemove = overwriteTabs ? tabbrowser.browsers.length : 0; - let newTabCount = winData.tabs.length; + var tabbrowser = aWindow.gBrowser; + var openTabCount = overwriteTabs ? tabbrowser.browsers.length : -1; + var newTabCount = winData.tabs.length; var tabs = []; // disable smooth scrolling while adding, moving, removing and selecting tabs - let tabstrip = tabbrowser.tabContainer.mTabstrip; - let smoothScroll = tabstrip.smoothScroll; + var tabstrip = tabbrowser.tabContainer.mTabstrip; + var smoothScroll = tabstrip.smoothScroll; tabstrip.smoothScroll = false; + // unpin all tabs to ensure they are not reordered in the next loop + if (overwriteTabs) { + for (let t = tabbrowser._numPinnedTabs - 1; t > -1; t--) + tabbrowser.unpinTab(tabbrowser.tabs[t]); + } + // We need to keep track of the initially open tabs so that they // can be moved to the end of the restored tabs. let initialTabs = []; @@ -3278,31 +3278,36 @@ var SessionStoreInternal = { initialTabs = Array.slice(tabbrowser.tabs); } + // make sure that the selected tab won't be closed in order to + // prevent unnecessary flickering + if (overwriteTabs && tabbrowser.selectedTab._tPos >= newTabCount) + tabbrowser.moveTabTo(tabbrowser.selectedTab, newTabCount - 1); + let numVisibleTabs = 0; let restoreTabsLazily = this._prefBranch.getBoolPref("sessionstore.restore_tabs_lazily") && this._prefBranch.getBoolPref("sessionstore.restore_on_demand"); 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 select = t == selectTab - 1; - let createLazyBrowser = restoreTabsLazily && !select && !winData.tabs[t].pinned; - let tab = tabbrowser.addTab("about:blank", - { createLazyBrowser, - skipAnimation: true, - userContextId, - skipBackgroundNotify: true }); + let createLazyBrowser = restoreTabsLazily && !winData.tabs[t].pinned; + let reuseExisting = t < openTabCount && + (tabbrowser.tabs[t].getAttribute("usercontextid") == (userContextId || "")); + let tab = reuseExisting ? this._maybeUpdateBrowserRemoteness(tabbrowser.tabs[t]) + : tabbrowser.addTab("about:blank", + { createLazyBrowser, + skipAnimation: true, + userContextId, + skipBackgroundNotify: true }); - if (select) { - // Select a new tab first to prevent the removeTab loop from changing - // the selected tab over and over again. - tabbrowser.selectedTab = tab; - - // Remove superfluous tabs. - for (let i = 0; i < tabsToRemove; i++) { - tabbrowser.removeTab(tabbrowser.tabs[0]); - } - tabsToRemove = 0; + // 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); } tabs.push(tab); @@ -3313,6 +3318,47 @@ var SessionStoreInternal = { tabbrowser.showTab(tabs[t]); numVisibleTabs++; } + + if (!!winData.tabs[t].muted != tabs[t].linkedBrowser.audioMuted) { + tabs[t].toggleMuteAudio(winData.tabs[t].muteReason); + } + } + + if (selectTab > 0 && selectTab <= tabs.length) { + // The state we're restoring wants to select a particular tab. This + // implies that we're overwriting tabs. + let currentIndex = tabbrowser.tabContainer.selectedIndex; + let targetIndex = selectTab - 1; + + if (currentIndex != targetIndex) { + // We need to change the selected tab. There are two ways of doing this: + // + // 1) The fast path: swap the currently selected tab with the one in the + // position of the selected tab in the restored state. Note that this + // can only work if the user contexts between the two tabs being swapped + // match. This should be the common case. + // + // 2) The slow path: switch to the selected tab. + // + // We'll try to do (1), and then fallback to (2). + + let selectedTab = tabbrowser.selectedTab; + let tabAtTargetIndex = tabs[targetIndex]; + let userContextsMatch = selectedTab.userContextId == tabAtTargetIndex.userContextId; + + if (userContextsMatch) { + tabbrowser.moveTabTo(selectedTab, targetIndex); + tabbrowser.moveTabTo(tabAtTargetIndex, currentIndex); + // We also have to do a similar "move" in the aTabs Array to + // make sure that the restored content shows up in the right + // order. + tabs[targetIndex] = tabs[currentIndex]; + tabs[currentIndex] = tabAtTargetIndex; + } else { + // Otherwise, go the slow path, and switch to the target tab. + tabbrowser.selectedTab = tabs[targetIndex]; + } + } } for (let i = 0; i < newTabCount; ++i) { @@ -3330,7 +3376,6 @@ var SessionStoreInternal = { // Move the originally open tabs to the end let endPosition = tabbrowser.tabs.length - 1; for (let i = 0; i < initialTabs.length; i++) { - tabbrowser.unpinTab(initialTabs[i]); tabbrowser.moveTabTo(initialTabs[i], endPosition); } } @@ -3341,6 +3386,18 @@ var SessionStoreInternal = { tabbrowser.showTab(tabs[0]); } + // If overwriting tabs, we want to reset each tab's "restoring" state. Since + // we're overwriting those tabs, they should no longer be restoring. The + // tabs will be rebuilt and marked if they need to be restored after loading + // state (in restoreTabs). + if (overwriteTabs) { + for (let i = 0; i < tabbrowser.tabs.length; i++) { + let tab = tabbrowser.tabs[i]; + if (tabbrowser.browsers[i].__SS_restoreState) + this._resetTabRestoringState(tab); + } + } + // We want to correlate the window with data from the last session, so // assign another id if we have one. Otherwise clear so we don't do // anything with it. @@ -3348,6 +3405,12 @@ var SessionStoreInternal = { if (winData.__lastSessionWindowID) aWindow.__SS_lastSessionWindowID = winData.__lastSessionWindowID; + // when overwriting tabs, remove all superflous ones + if (overwriteTabs && newTabCount < openTabCount) { + Array.slice(tabbrowser.tabs, newTabCount, openTabCount) + .forEach(tabbrowser.removeTab, tabbrowser); + } + if (overwriteTabs) { this.restoreWindowFeatures(aWindow, winData); delete this._windows[aWindow.__SSi].extData; @@ -3766,6 +3829,7 @@ var SessionStoreInternal = { loadArguments: aLoadArguments, isRemotenessUpdate, }); + } // If the restored browser wants to show view source content, start up a @@ -4013,6 +4077,27 @@ var SessionStoreInternal = { }, 0); }, + /** + * Determines whether or not a tab that is being restored needs + * to have its remoteness flipped first. + * + * @param tab (): + * The tab being restored. + * + * @returns tab () + * The tab that was passed. + */ + _maybeUpdateBrowserRemoteness(tab) { + let win = tab.ownerGlobal; + let tabbrowser = win.gBrowser; + let browser = tab.linkedBrowser; + if (win.gMultiProcessBrowser && !browser.isRemoteBrowser) { + tabbrowser.updateBrowserRemoteness(browser, true); + } + + return tab; + }, + /** * Update the session start time and send a telemetry measurement * for the number of days elapsed since the session was started. diff --git a/browser/components/sessionstore/test/browser_423132.js b/browser/components/sessionstore/test/browser_423132.js index 79f1a4514de3..72e7474071f8 100644 --- a/browser/components/sessionstore/test/browser_423132.js +++ b/browser/components/sessionstore/test/browser_423132.js @@ -52,5 +52,5 @@ add_task(function*() { // clean up Services.cookies.removeAll(); - yield promiseRemoveTab(gBrowser.tabs[1]); + yield promiseRemoveTab(tab); }); diff --git a/browser/components/sessionstore/test/browser_586147.js b/browser/components/sessionstore/test/browser_586147.js index 57d6f27841f6..4b917bc51a7d 100644 --- a/browser/components/sessionstore/test/browser_586147.js +++ b/browser/components/sessionstore/test/browser_586147.js @@ -22,7 +22,7 @@ function test() { is(gBrowser.visibleTabs.length, 1, "only 1 after hiding"); ok(hiddenTab.hidden, "sanity check that it's hidden"); - gBrowser.addTab(); + let extraTab = gBrowser.addTab(); let state = ss.getBrowserState(); let stateObj = JSON.parse(state); let tabs = stateObj.windows[0].tabs; @@ -35,16 +35,16 @@ function test() { tabs[2].hidden = true; observeOneRestore(function() { - is(gBrowser.visibleTabs.length, 1, "only restored 1 visible tab"); - let restoredTabs = gBrowser.tabs; - + let testWindow = Services.wm.getEnumerator("navigator:browser").getNext(); + is(testWindow.gBrowser.visibleTabs.length, 1, "only restored 1 visible tab"); + let restoredTabs = testWindow.gBrowser.tabs; ok(!restoredTabs[0].hidden, "first is still visible"); ok(restoredTabs[1].hidden, "second tab is still hidden"); ok(restoredTabs[2].hidden, "third tab is now hidden"); // Restore the original state and clean up now that we're done - gBrowser.removeTab(gBrowser.tabs[1]); - gBrowser.removeTab(gBrowser.tabs[1]); + gBrowser.removeTab(hiddenTab); + gBrowser.removeTab(extraTab); finish(); }); diff --git a/browser/components/sessionstore/test/browser_590563.js b/browser/components/sessionstore/test/browser_590563.js index 58a94d8bbfa0..3e40df594f85 100644 --- a/browser/components/sessionstore/test/browser_590563.js +++ b/browser/components/sessionstore/test/browser_590563.js @@ -56,16 +56,13 @@ function newWindowWithState(state, callback) { let win = window.openDialog(getBrowserURL(), "_blank", opts); win.addEventListener("load", function() { + let tab = win.gBrowser.selectedTab; + // The form data will be restored before SSTabRestored, so we want to listen - // for that on the currently selected tab - let onSSTabRestored = event => { - let tab = event.target; - if (tab.selected) { - win.gBrowser.tabContainer.removeEventListener("SSTabRestored", onSSTabRestored, true); - callback(win); - } - }; - win.gBrowser.tabContainer.addEventListener("SSTabRestored", onSSTabRestored, true); + // for that on the currently selected tab (it will be reused) + tab.addEventListener("SSTabRestored", function() { + callback(win); + }, {capture: true, once: true}); executeSoon(function() { ss.setWindowState(win, JSON.stringify(state), true); diff --git a/browser/components/sessionstore/test/browser_624727.js b/browser/components/sessionstore/test/browser_624727.js index 344751184aed..85d6ff042376 100644 --- a/browser/components/sessionstore/test/browser_624727.js +++ b/browser/components/sessionstore/test/browser_624727.js @@ -21,6 +21,7 @@ add_task(function* () { assertNumberOfTabs(2, "there are two tabs, now"); let [tab1, tab2] = gBrowser.tabs; + let linkedBrowser = tab1.linkedBrowser; gBrowser.pinTab(tab1); gBrowser.pinTab(tab2); assertNumberOfPinnedTabs(2, "both tabs are now pinned"); @@ -30,4 +31,5 @@ add_task(function* () { assertNumberOfTabs(1, "one tab left after setBrowserState()"); assertNumberOfPinnedTabs(0, "there are no pinned tabs"); + is(gBrowser.tabs[0].linkedBrowser, linkedBrowser, "first tab's browser got re-used"); }); diff --git a/browser/components/sessionstore/test/browser_remoteness_flip_on_restore.js b/browser/components/sessionstore/test/browser_remoteness_flip_on_restore.js index 58c4f3e8af76..5d9c124e3a81 100644 --- a/browser/components/sessionstore/test/browser_remoteness_flip_on_restore.js +++ b/browser/components/sessionstore/test/browser_remoteness_flip_on_restore.js @@ -86,6 +86,14 @@ const PINNED_STATE = { * restored window. Leave this at 0 if you don't want to change * the selection from the initial window state. * + * expectedFlips: + * an Array that represents the window that we end up with after + * restoring state. Each bool in the Array represents the window tabs, + * in order. A "true" indicates that the tab should have flipped + * its remoteness once. "false" indicates that the tab should never + * have flipped remoteness. Note that any tab that flips its remoteness + * more than once will cause a test failure. + * * expectedRemoteness: * an Array that represents the window that we end up with after * restoring state. Each bool in the Array represents the window @@ -98,6 +106,10 @@ const PINNED_STATE = { function* runScenarios(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"); @@ -130,15 +142,62 @@ function* runScenarios(scenarios) { yield BrowserTestUtils.switchTab(tabbrowser, tabToSelect); } + // Hook up an event listener to make sure that the right + // tabs flip remoteness, and only once. + let flipListener = { + seenBeforeTabs: new Set(), + seenAfterTabs: new Set(), + handleEvent(e) { + let index = Array.from(tabbrowser.tabs).indexOf(e.target); + switch (e.type) { + case "BeforeTabRemotenessChange": + info(`Saw tab at index ${index} before remoteness flip`); + if (this.seenBeforeTabs.has(e.target)) { + Assert.ok(false, "Saw tab before remoteness flip more than once"); + } + this.seenBeforeTabs.add(e.target); + break; + case "TabRemotenessChange": + info(`Saw tab at index ${index} after remoteness flip`); + if (this.seenAfterTabs.has(e.target)) { + Assert.ok(false, "Saw tab after remoteness flip more than once"); + } + this.seenAfterTabs.add(e.target); + break; + } + }, + }; + + win.addEventListener("BeforeTabRemotenessChange", flipListener); + win.addEventListener("TabRemotenessChange", flipListener); + // Okay, time to test! let state = prepareState(scenario.stateToRestore, scenario.selectedTab); SessionStore.setWindowState(win, state, true); - for (let i = 0; i < scenario.expectedRemoteness.length; ++i) { + win.removeEventListener("BeforeTabRemotenessChange", flipListener); + win.removeEventListener("TabRemotenessChange", flipListener); + + // Because we know that scenario.expectedFlips and + // scenario.expectedRemoteness have the same length, we + // can check that we satisfied both with the same loop. + for (let i = 0; i < scenario.expectedFlips.length; ++i) { + let expectedToFlip = scenario.expectedFlips[i]; let expectedRemoteness = scenario.expectedRemoteness[i]; let tab = tabbrowser.tabs[i]; + if (expectedToFlip) { + Assert.ok(flipListener.seenBeforeTabs.has(tab), + `We should have seen tab at index ${i} before remoteness flip`); + Assert.ok(flipListener.seenAfterTabs.has(tab), + `We should have seen tab at index ${i} after remoteness flip`); + } else { + Assert.ok(!flipListener.seenBeforeTabs.has(tab), + `We should not have seen tab at index ${i} before remoteness flip`); + Assert.ok(!flipListener.seenAfterTabs.has(tab), + `We should not have seen tab at index ${i} after remoteness flip`); + } Assert.equal(tab.linkedBrowser.isRemoteBrowser, expectedRemoteness, "Should have gotten the expected remoteness " + @@ -151,7 +210,8 @@ function* runScenarios(scenarios) { /** * Tests that if we restore state to browser windows with - * a variety of initial remoteness states. For this particular + * a variety of initial remoteness states, that we only flip + * the remoteness on the necessary tabs. For this particular * set of tests, we assume that tabs are restoring on demand. */ add_task(function*() { @@ -173,6 +233,11 @@ add_task(function*() { 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 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], }, @@ -184,6 +249,10 @@ add_task(function*() { 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 + // initialize as remote unrestored background tabs. + expectedFlips: [false, false, false], // All tabs should now be remote. expectedRemoteness: [true, true, true], }, @@ -196,6 +265,10 @@ add_task(function*() { 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 + // initialize as remote unrestored background tabs. + expectedFlips: [false, false, false], // All tabs should now be remote. expectedRemoteness: [true, true, true], }, @@ -208,6 +281,12 @@ add_task(function*() { 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 remote. + expectedFlips: [false, false, false], // Both pinned tabs and the selected tabs should all // end up being remote. expectedRemoteness: [true, true, true], @@ -219,6 +298,14 @@ add_task(function*() { initialSelectedTab: 1, stateToRestore: SIMPLE_STATE, selectedTab: 2, + // The initial tab is non-remote and will flip. The two tabs that + // are added after should be initialized as remote. Since the selected + // tab is changing, SessionStore should swap the initial tab with the + // tab thats in the selectedTab slot. That'll result in a remoteness + // state like this: + // + // [true, false, true] + expectedFlips: [false, true, false], // All tabs should now be remote. expectedRemoteness: [true, true, true], }, @@ -229,6 +316,16 @@ add_task(function*() { initialSelectedTab: 1, stateToRestore: SIMPLE_STATE, selectedTab: 3, + // The initial tab is remote and should stay that way, even + // when put into the background. The initial tab is going to be + // swapped into the selectedTab slot, and both of those tabs are + // remote already. That will result in the tabs remoteness being + // in this order still: + // + // [true, false, true] + // + // This means that we'll only need to flip the second tab. + expectedFlips: [false, true, false], // All tabs should now be remote. expectedRemoteness: [true, true, true], }, @@ -241,6 +338,18 @@ add_task(function*() { initialSelectedTab: 1, stateToRestore: PINNED_STATE, selectedTab: 3, + // The initial tab is going to swap into the selected slot, + // and so after the other two tabs from the PINNED_STATE are + // inserted, we'll have a remoteness state like this: + // + // [true, true, false] + // + // The two pinned tabs at the start of the PINNED_STATE should + // load right away, but should not flip remoteness. + // + // The third tab is selected, and should load right away, so + // it should flip remoteness. + expectedFlips: [false, false, true], // All tabs should now be remote. expectedRemoteness: [true, true, true], }, diff --git a/testing/marionette/harness/marionette_harness/tests/unit/unit-tests.ini b/testing/marionette/harness/marionette_harness/tests/unit/unit-tests.ini index 34df3eadf99f..d6ec94f0b369 100644 --- a/testing/marionette/harness/marionette_harness/tests/unit/unit-tests.ini +++ b/testing/marionette/harness/marionette_harness/tests/unit/unit-tests.ini @@ -101,7 +101,7 @@ skip-if = true # Bug 925688 [test_profile_management.py] skip-if = manage_instance == false || appname == 'fennec' # Bug 1298921 and bug 1322993 [test_quit_restart.py] -skip-if = manage_instance == false || appname == 'fennec' || true # Bug 1298921, bug 1322993, bug 1361837 +skip-if = manage_instance == false || appname == 'fennec' # Bug 1298921 and bug 1322993 [test_with_using_context.py] [test_modal_dialogs.py]