Closed Bug 1442465 Opened 7 years ago Closed 7 years ago

Remove workaround in BrowserTestUtils.tabRemoved that waits the next event tick

Categories

(Firefox :: Session Restore, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: arai, Assigned: arai)

References

Details

Attachments

(8 files, 6 obsolete files)

(deleted), patch
dao
: review+
Details | Diff | Splinter Review
(deleted), patch
dao
: review+
Details | Diff | Splinter Review
(deleted), patch
dao
: review+
Details | Diff | Splinter Review
(deleted), patch
dao
: review+
Details | Diff | Splinter Review
(deleted), patch
dao
: review+
Details | Diff | Splinter Review
(deleted), patch
dao
: review+
Details | Diff | Splinter Review
(deleted), patch
dao
: review+
Details | Diff | Splinter Review
(deleted), text/x-github-pull-request
Details
In bug 1441872, a workaround is applied to BrowserTestUtils.tabRemoved, that is to wait the next event tick before resolving the returned promise, to make sure that closed tabs information is updated for the tab close. We should apply the appropriate fix to each case and remove the workaround.
I should note what happened in bug 1441872 and what I was thinking. Some test cases expect that the process in SessionStore.receiveMessage has been done in the case where we receive 'SessionStore:update' when the test case receives 'SessionStore:update' as well. But unfortunately in the failure case, the test case receives the message before SessionStore.receiveMessage receives it. I think we should send a new message named 'SessionStore:updated' at the end of the process for 'SessionStore:update' in SessionStore.receiveMessage, then the test case should listen the new message. Also, listening 'SessionStore:update' in BrowserTestUtils.tabRemoved is somewhat misleading. We should also add an appropriate message/event for that.
CCing a bunch of browser people.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #1) > Also, listening 'SessionStore:update' in BrowserTestUtils.tabRemoved is > somewhat misleading. We should also add an appropriate message/event for > that. Yes, this message should only matter to tests that check the closed tabs list or are otherwise involved with sessionstore. Doing this for every BrowserTestUtils.removeTab call makes tests more lax and needlessly slows them down.
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
So far, the almost all places that directly invokes BrowserTestUtils.tabRemoved work with TabClose event, or just that they don't have to wait. The single case that doesn't work is browser/base/content/test/tabcrashed/browser_withoutDump.js, it closes tab in content side, and waits for the tab close. then, at the point of TabClose event, the number of tab doesn't change (intentionally [1]), and test harness complains that there's remaining tab after the test finishes. we should wait for the tabs.length decrement (maybe inside TabBrowser#_endRemoveTab or something) somehow. also removed BrowserTestUtils.tabRemoved call in BrowserTestUtils.removeTab, and running try to see how much test fails: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ebe7c6d79776b419b4c12517bd2cfc381fe0af6f [1] https://searchfox.org/mozilla-central/rev/bffd3e0225b65943364be721881470590b9377c1/browser/base/content/tabbrowser.js#2810-2815
looks like some tests are abusing the wait in removeTab. they should wait for some other thing, and waiting for removeTab (actually SessionStore update) was longer enough for the other thing. I'll investigate each case. https://treeherder.mozilla.org/#/jobs?repo=try&revision=22a797a139e0cd70983ba81821b75730d7ec5c91
Depends on: 1444007
Separated patches, and fixed some cases, and applied workaround to keep the current behavior: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a269b407b2f11a697b14e3db8ae0ba8538245720
Added following dedicate methods: * BrowserTestUtils.waitForTabClosing returns a Promise that is resolved when the tab is being closed (when TabClose is received, that is dispatched from TabBrowser#_beginRemoveTab) * BrowserTestUtils.waitForTabRemoved returns a Promise that is resolved when the tab is removed (when newly-added TabRemoved is received, that is dispatched from TabBrowser#_endRemoveTab) * BrowserTestUtils.waitForSessionStoreUpdate returns a Promise that is resolved when SessinStore info is updated (when SessionStore:update is received, that is the same behavior as BrowserTestUtils.tabRemoved, that is removed in later patch)
Attachment #8958326 - Flags: review?(dao+bmo)
And replaced BrowserTestUtils.tabRemoved with one of the newly added methods. BrowserTestUtils.tabRemoved will be removed in the later patch
Attachment #8958327 - Flags: review?(dao+bmo)
Also replaced BrowserTestUtils.removeTab with one of newly added methods. BrowserTestUtils.removeTab will be changed not to return Promise in the later patch.
Attachment #8958328 - Flags: review?(dao+bmo)
Part 4.1 and 4.2 removes "await" from BrowserTestUtils.removeTab. Part 4.1 just replaces "await BrowserTestUtils.removeTab(...)" with "BrowserTestUtils.removeTab(...)", where the testcase unnecessarily waiting on it
Attachment #8958329 - Flags: review?(dao+bmo)
(In reply to Tooru Fujisawa [:arai] from comment #12) > Created attachment 8958329 [details] [diff] [review] > Part 4.2: Stop unnecessarily awaiting on BrowserTestUtils.removeTab (simple > part). > > Part 4.1 and 4.2 removes "await" from BrowserTestUtils.removeTab. > Part 4.1 just replaces "await BrowserTestUtils.removeTab(...)" with > "BrowserTestUtils.removeTab(...)", > where the testcase unnecessarily waiting on it I mis-numbered :P it was part 4.2 that simply replaces them.
so, actual Part 4.1 also removes await or .then() from removeTab caller.
Attachment #8958330 - Flags: review?(dao+bmo)
And workaround for the remaining case that they need dedicate event. * browser_devices_get_user_media_multi_process.js (bug 1444007) should wait for WebRTC indicator update * browser_favicon_userContextId.js and browser_privatebrowsing_favicon.js should wait for the end of all network requests from the tab, especially tab icon they'll be fixed in their own bugs (I'll file for latter 2)
Attachment #8958331 - Flags: review?(dao+bmo)
and finally removed misleading BrowserTestUtils.tabRemoved and stop returning that Promise from BrowserTestUtils.removeTab.
Attachment #8958332 - Flags: review?(dao+bmo)
Attachment #8958329 - Flags: review?(dao+bmo) → review+
Attachment #8958330 - Flags: review?(dao+bmo) → review+
Comment on attachment 8958327 [details] [diff] [review] Part 2: Use BrowserTestUtils.{waitForTabClosing,waitForTabRemoved,waitForSessionStoreUpdate} instead of BrowserTestUtils.tabRemoved. >--- a/browser/base/content/test/tabcrashed/browser_withoutDump.js >+++ b/browser/base/content/test/tabcrashed/browser_withoutDump.js >@@ -12,17 +12,17 @@ add_task(async function setup() { > add_task(async function test_without_dump() { > return BrowserTestUtils.withNewTab({ > gBrowser, > url: PAGE, > }, async function(browser) { > let tab = gBrowser.getTabForBrowser(browser); > await BrowserTestUtils.crashBrowser(browser); > >- let tabRemovedPromise = BrowserTestUtils.tabRemoved(tab); >+ let tabRemovedPromise = BrowserTestUtils.waitForTabRemoved(tab); I'd prefer just waiting for the TabClose event here and ignoring closing tabs here (by checking lastTab.closing): https://searchfox.org/mozilla-central/rev/99df6dad057b94c2ac4d8b12f7b0b3a7fdf25d04/testing/mochitest/browser-test.js#531-535
Attachment #8958327 - Flags: review?(dao+bmo)
Comment on attachment 8958328 [details] [diff] [review] Part 3: Use BrowserTestUtils.{waitForTabClosing,waitForTabRemoved,waitForSessionStoreUpdate} instead of BrowserTestUtils.removeTab. >--- a/browser/base/content/test/general/browser_hide_removing.js >+++ b/browser/base/content/test/general/browser_hide_removing.js >@@ -9,19 +9,20 @@ add_task(async function() { > let testTab = BrowserTestUtils.addTab(gBrowser, "about:blank", { skipAnimation: true }); > is(gBrowser.visibleTabs.length, 2, "just added a tab, so 2 tabs"); > await BrowserTestUtils.switchTab(gBrowser, testTab); > > let numVisBeforeHide, numVisAfterHide; > > // We have to animate the tab removal in order to get an async > // tab close. >- let tabClosed = BrowserTestUtils.waitForEvent(gBrowser.tabContainer, "TabClose"); >- let tabRemoved = BrowserTestUtils.removeTab(testTab, { animate: true }); >- await tabClosed; >+ let tabClosing = BrowserTestUtils.waitForTabClosing(gBrowser.tabContainer); >+ let tabRemoved = BrowserTestUtils.waitForTabRemoved(testTab); >+ BrowserTestUtils.removeTab(testTab, { animate: true }); >+ await tabClosing; > > numVisBeforeHide = gBrowser.visibleTabs.length; > gBrowser.hideTab(testTab); > numVisAfterHide = gBrowser.visibleTabs.length; > > is(numVisBeforeHide, 1, "animated remove has in 1 tab left"); > is(numVisAfterHide, 1, "hiding a removing tab also has 1 tab"); This shouldn't wait for the tab to be removed. This specifically wants to test hiding a closing tab. >--- a/browser/components/originattributes/test/browser/browser_favicon_firstParty.js >+++ b/browser/components/originattributes/test/browser/browser_favicon_firstParty.js >@@ -229,18 +229,21 @@ async function doTest(aTestPage, aExpect > let tabInfo = await openTab(TEST_SITE_ONE + aTestPage); > > // Waiting until favicon requests are all made. > await promiseObserveFavicon; > > // Waiting until favicon loaded. > await promiseFaviconLoaded; > >- // Close the tab. >- await BrowserTestUtils.removeTab(tabInfo.tab); >+ // Close the tab and make sure the tab is removed, to avoid observing >+ // previous tab info in the next step. >+ let removedPromise = BrowserTestUtils.waitForTabRemoved(tabInfo.tab); >+ BrowserTestUtils.removeTab(tabInfo.tab); >+ await removedPromise; This tab should be removed synchronously so I'm not sure what's there to wait for. >--- a/dom/base/test/browser_messagemanager_loadprocessscript.js >+++ b/dom/base/test/browser_messagemanager_loadprocessscript.js > for (let i = 0; i < 3; i++) { >- await BrowserTestUtils.removeTab(tabs[i]); >+ let removedPromise = BrowserTestUtils.waitForTabRemoved(tabs[i]); >+ BrowserTestUtils.removeTab(tabs[i]); >+ await removedPromise; ditto
Attachment #8958328 - Flags: review?(dao+bmo)
Thank you for reviewing :) (In reply to Dão Gottwald [::dao] from comment #18) > >--- a/browser/components/originattributes/test/browser/browser_favicon_firstParty.js > >+++ b/browser/components/originattributes/test/browser/browser_favicon_firstParty.js > >@@ -229,18 +229,21 @@ async function doTest(aTestPage, aExpect > > let tabInfo = await openTab(TEST_SITE_ONE + aTestPage); > > > > // Waiting until favicon requests are all made. > > await promiseObserveFavicon; > > > > // Waiting until favicon loaded. > > await promiseFaviconLoaded; > > > >- // Close the tab. > >- await BrowserTestUtils.removeTab(tabInfo.tab); > >+ // Close the tab and make sure the tab is removed, to avoid observing > >+ // previous tab info in the next step. > >+ let removedPromise = BrowserTestUtils.waitForTabRemoved(tabInfo.tab); > >+ BrowserTestUtils.removeTab(tabInfo.tab); > >+ await removedPromise; > > This tab should be removed synchronously so I'm not sure what's there to > wait for. > > >--- a/dom/base/test/browser_messagemanager_loadprocessscript.js > >+++ b/dom/base/test/browser_messagemanager_loadprocessscript.js > > > for (let i = 0; i < 3; i++) { > >- await BrowserTestUtils.removeTab(tabs[i]); > >+ let removedPromise = BrowserTestUtils.waitForTabRemoved(tabs[i]); > >+ BrowserTestUtils.removeTab(tabs[i]); > >+ await removedPromise; > > ditto Sorry I forgot to move them into workaround patch (and also fixup into better code...) I'll post updated patch after verifying the fixed workaround works.
Comment on attachment 8958331 [details] [diff] [review] Part 5: Workaround for some BrowserTestUtils.removeTab cases. looks like the workaround can be simplified a bit more. I'll post after some more test (with extra tests moved from Part 3).
Attachment #8958331 - Attachment is obsolete: true
Attachment #8958331 - Flags: review?(dao+bmo)
Changed browser_withoutDump.js to use BrowserTestUtils.waitForTabClosing, and allowed closing tab in testing/mochitest/browser-test.js
Attachment #8958327 - Attachment is obsolete: true
Attachment #8959064 - Flags: review?(dao+bmo)
Removed BrowserTestUtils.waitForTabRemoved call from browser_hide_removing.js (since now closing tab is allowed to be left) and moved browser_favicon_firstParty.js and browser_messagemanager_loadprocessscript.js to Part 5, since they actually need workaround.
Attachment #8958328 - Attachment is obsolete: true
Attachment #8959065 - Flags: review?(dao+bmo)
here's updated workarounds * browser/base/content/test/webrtc/browser_devices_get_user_media_multi_process.js should wait for WebRTC indicator update (bug 1444007) * browser/components/originattributes/test/browser/browser_favicon_firstParty.js * browser/components/originattributes/test/browser/browser_favicon_userContextId.js * browser/components/privatebrowsing/test/browser/browser_privatebrowsing_favicon.js should wait for some timing, to avoid observing the previous tab's favicon load in the next test. looks like just waiting for the next event tick works * dom/base/test/browser_messagemanager_loadprocessscript.js should wait for the tab removal gets reflected to the process count (not yet filed bug)
Attachment #8959066 - Flags: review?(dao+bmo)
Comment on attachment 8958332 [details] [diff] [review] Part 6: Stop returning a Promise from BrowserTestUtils.removeTab. > removeTab(tab, options = {}) { >- let tabRemoved = BrowserTestUtils.tabRemoved(tab); > if (!tab.closing) { > tab.ownerGlobal.gBrowser.removeTab(tab, options); > } We should remove the tab.closing check too. gBrowser supports synchronously removing a tab that was already closing asynchronously.
Attachment #8958332 - Flags: review?(dao+bmo) → review+
Comment on attachment 8959065 [details] [diff] [review] Part 3: Use BrowserTestUtils.{waitForTabClosing,waitForTabRemoved,waitForSessionStoreUpdate} instead of BrowserTestUtils.removeTab. >--- a/browser/base/content/test/tabs/browser_tabCloseProbes.js >+++ b/browser/base/content/test/tabs/browser_tabCloseProbes.js >@@ -40,17 +40,19 @@ add_task(async function setup() { > */ > add_task(async function test_close_time_anim_probe() { > let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser); > await BrowserTestUtils.waitForCondition(() => tab._fullyOpen); > > gAnimHistogram.clear(); > gNoAnimHistogram.clear(); > >- await BrowserTestUtils.removeTab(tab, { animate: true }); >+ let closingPromise = BrowserTestUtils.waitForTabRemoved(tab); >+ BrowserTestUtils.removeTab(tab, { animate: true }); >+ await closingPromise; > > assertCount(gAnimHistogram.snapshot(), 1); > assertCount(gNoAnimHistogram.snapshot(), 0); > > gAnimHistogram.clear(); > gNoAnimHistogram.clear(); > }); So at this point this is the only consumer of waitForTabRemoved. Can we just use BrowserTestUtils.waitForCondition instead?
Attachment #8959065 - Flags: review?(dao+bmo)
Comment on attachment 8958326 [details] [diff] [review] Part 1: Add BrowserTestUtils.{waitForTabClosing,waitForTabRemoved,waitForSessionStoreUpdate}. I'd prefer not implementing waitForTabRemoved as I don't think there's a strong enough use case, and people would likely get confused about when to use it.
Attachment #8958326 - Flags: review?(dao+bmo)
Thank you for reviewing :) removed waitForTabRemoved and TabRemoved event.
Attachment #8958326 - Attachment is obsolete: true
Attachment #8959733 - Flags: review?(dao+bmo)
Changed waitForTabRemoved to `await BrowserTestUtils.waitForCondition(() => tab.collapsed);` which is set here: https://searchfox.org/mozilla-central/rev/6e96a3f1e44e286ddae5fdafab737709741d237a/browser/base/content/tabbrowser.js#2851
Attachment #8959734 - Flags: review?(dao+bmo)
Attachment #8959065 - Attachment is obsolete: true
Comment on attachment 8959734 [details] [diff] [review] Part 3: Use BrowserTestUtils.{waitForTabClosing,waitForSessionStoreUpdate} instead of BrowserTestUtils.removeTab. >- await BrowserTestUtils.removeTab(tab, { animate: true }); >+ BrowserTestUtils.removeTab(tab, { animate: true }); >+ await BrowserTestUtils.waitForCondition(() => tab.collapsed); > > assertCount(gAnimHistogram.snapshot(), 1); No, I was thinking: await BrowserTestUtils.waitForCondition(gAnimHistogram.snapshot() == 1);
Attachment #8959734 - Flags: review?(dao+bmo) → review-
(In reply to Dão Gottwald [::dao] from comment #30) > Comment on attachment 8959734 [details] [diff] [review] > Part 3: Use BrowserTestUtils.{waitForTabClosing,waitForSessionStoreUpdate} > instead of BrowserTestUtils.removeTab. > > >- await BrowserTestUtils.removeTab(tab, { animate: true }); > >+ BrowserTestUtils.removeTab(tab, { animate: true }); > >+ await BrowserTestUtils.waitForCondition(() => tab.collapsed); > > > > assertCount(gAnimHistogram.snapshot(), 1); > > No, I was thinking: > > await BrowserTestUtils.waitForCondition(gAnimHistogram.snapshot() == 1); in that way we need the following expression there too. https://searchfox.org/mozilla-central/rev/6e96a3f1e44e286ddae5fdafab737709741d237a/browser/base/content/test/tabs/browser_tabCloseProbes.js#23-24
Added the following functions: * snapshotCount returns the sum of snapshot counts * waitForSnapshotCount returns a promise that resolves when the histgram's snapshot count becomes the expected count and used waitForSnapshotCount both in anim case and no-anim case, for consistency.
Attachment #8959734 - Attachment is obsolete: true
Attachment #8959902 - Flags: review?(dao+bmo)
Attachment #8959733 - Flags: review?(dao+bmo) → review+
Comment on attachment 8959064 [details] [diff] [review] Part 2: Use BrowserTestUtils.{waitForTabClosing,waitForTabRemoved,waitForSessionStoreUpdate} instead of BrowserTestUtils.tabRemoved. >--- a/browser/base/content/test/general/browser_bug455852.js >+++ b/browser/base/content/test/general/browser_bug455852.js >@@ -2,19 +2,19 @@ add_task(async function() { > is(gBrowser.tabs.length, 1, "one tab is open"); > > gBrowser.selectedBrowser.focus(); > isnot(document.activeElement, gURLBar.inputField, "location bar is not focused"); > > var tab = gBrowser.selectedTab; > Services.prefs.setBoolPref("browser.tabs.closeWindowWithLastTab", false); > >- let tabClosedPromise = BrowserTestUtils.tabRemoved(tab); >+ let tabClosingPromise = BrowserTestUtils.waitForTabClosing(tab); > EventUtils.synthesizeKey("w", { accelKey: true }); >- await tabClosedPromise; >+ await tabClosingPromise; > > is(tab.parentNode, null, "ctrl+w removes the tab"); This should work synchronously, so I think you can get rid of tabClosingPromise. >--- a/browser/base/content/test/general/browser_tabs_close_beforeunload.js >+++ b/browser/base/content/test/general/browser_tabs_close_beforeunload.js >@@ -27,21 +27,21 @@ add_task(async function() { > content.document.getElementsByTagName("a")[0].click(); > }); > info("Waiting for the second tab to be opened"); > await tabOpened; > info("Waiting for the load in that tab to finish"); > await secondTabLoadedPromise; > > let closeBtn = document.getAnonymousElementByAttribute(secondTab, "anonid", "close-button"); >- let closePromise = BrowserTestUtils.tabRemoved(secondTab); >+ let closingPromise = BrowserTestUtils.waitForTabClosing(secondTab); > info("closing second tab (which will self-close in beforeunload)"); > closeBtn.click(); > ok(secondTab.closing, "Second tab should be marked as closing synchronously."); >- await closePromise; >+ await closingPromise; > ok(secondTab.closing, "Second tab should still be marked as closing"); > ok(!secondTab.linkedBrowser, "Second tab's browser should be dead"); I'm a bit surprised that this works. Doesn't this use the closing animation, i.e. the browser should be closed much later than the TabClose event is fired? I also don't really see the point of closingPromise here.
Attachment #8959064 - Flags: review?(dao+bmo) → review+
Attachment #8959066 - Flags: review?(dao+bmo) → review+
Comment on attachment 8959902 [details] [diff] [review] Part 3: Use BrowserTestUtils.{waitForTabClosing,waitForSessionStoreUpdate} instead of BrowserTestUtils.removeTab. >diff --git a/browser/base/content/test/general/browser_ctrlTab.js b/browser/base/content/test/general/browser_ctrlTab.js >--- a/browser/base/content/test/general/browser_ctrlTab.js >+++ b/browser/base/content/test/general/browser_ctrlTab.js >@@ -56,17 +56,19 @@ add_task(async function() { > checkTabs(3); > await ctrlTabTest([2, 1, 0], 7, 1); > > { // test for bug 1292049 > let tabToClose = await BrowserTestUtils.openNewForegroundTab(gBrowser, "about:buildconfig"); > checkTabs(4); > selectTabs([0, 1, 2, 3]); > >- await BrowserTestUtils.removeTab(tabToClose); >+ let promise = BrowserTestUtils.waitForSessionStoreUpdate(tabToClose); >+ BrowserTestUtils.removeTab(tabToClose); >+ await promise; > checkTabs(3); > undoCloseTab(); > checkTabs(4); > is(gBrowser.tabContainer.selectedIndex, 3, "tab is selected after closing and restoring it"); Is waitForSessionStoreUpdate really needed here? >--- a/browser/base/content/test/general/browser_hide_removing.js >+++ b/browser/base/content/test/general/browser_hide_removing.js >@@ -9,21 +9,19 @@ add_task(async function() { > let testTab = BrowserTestUtils.addTab(gBrowser, "about:blank", { skipAnimation: true }); > is(gBrowser.visibleTabs.length, 2, "just added a tab, so 2 tabs"); > await BrowserTestUtils.switchTab(gBrowser, testTab); > > let numVisBeforeHide, numVisAfterHide; > > // We have to animate the tab removal in order to get an async > // tab close. >- let tabClosed = BrowserTestUtils.waitForEvent(gBrowser.tabContainer, "TabClose"); >- let tabRemoved = BrowserTestUtils.removeTab(testTab, { animate: true }); >- await tabClosed; >+ let tabClosing = BrowserTestUtils.waitForTabClosing(gBrowser.tabContainer); >+ BrowserTestUtils.removeTab(testTab, { animate: true }); >+ await tabClosing; > > numVisBeforeHide = gBrowser.visibleTabs.length; Afaik this should work synchronously, no need to wait for tabClosing.
Attachment #8959902 - Flags: review?(dao+bmo) → review+
(In reply to Dão Gottwald [::dao] from comment #25) > Comment on attachment 8958332 [details] [diff] [review] > Part 6: Stop returning a Promise from BrowserTestUtils.removeTab. > > > removeTab(tab, options = {}) { > >- let tabRemoved = BrowserTestUtils.tabRemoved(tab); > > if (!tab.closing) { > > tab.ownerGlobal.gBrowser.removeTab(tab, options); > > } > > We should remove the tab.closing check too. gBrowser supports synchronously > removing a tab that was already closing asynchronously. Please don't forget to address this and other review comments. Thanks!
(In reply to Dão Gottwald [::dao] from comment #34) > Comment on attachment 8959902 [details] [diff] [review] > Part 3: Use BrowserTestUtils.{waitForTabClosing,waitForSessionStoreUpdate} > instead of BrowserTestUtils.removeTab. > > >diff --git a/browser/base/content/test/general/browser_ctrlTab.js b/browser/base/content/test/general/browser_ctrlTab.js > >--- a/browser/base/content/test/general/browser_ctrlTab.js > >+++ b/browser/base/content/test/general/browser_ctrlTab.js > >@@ -56,17 +56,19 @@ add_task(async function() { > > checkTabs(3); > > await ctrlTabTest([2, 1, 0], 7, 1); > > > > { // test for bug 1292049 > > let tabToClose = await BrowserTestUtils.openNewForegroundTab(gBrowser, "about:buildconfig"); > > checkTabs(4); > > selectTabs([0, 1, 2, 3]); > > > >- await BrowserTestUtils.removeTab(tabToClose); > >+ let promise = BrowserTestUtils.waitForSessionStoreUpdate(tabToClose); > >+ BrowserTestUtils.removeTab(tabToClose); > >+ await promise; > > checkTabs(3); > > undoCloseTab(); > > checkTabs(4); > > is(gBrowser.tabContainer.selectedIndex, 3, "tab is selected after closing and restoring it"); > > Is waitForSessionStoreUpdate really needed here? yes, the "undo" after that should be performed on the updated state, otherwise it tries to undo the close of different tab.
(In reply to Dão Gottwald [::dao] from comment #33) > >--- a/browser/base/content/test/general/browser_tabs_close_beforeunload.js > >+++ b/browser/base/content/test/general/browser_tabs_close_beforeunload.js > >@@ -27,21 +27,21 @@ add_task(async function() { > > content.document.getElementsByTagName("a")[0].click(); > > }); > > info("Waiting for the second tab to be opened"); > > await tabOpened; > > info("Waiting for the load in that tab to finish"); > > await secondTabLoadedPromise; > > > > let closeBtn = document.getAnonymousElementByAttribute(secondTab, "anonid", "close-button"); > >- let closePromise = BrowserTestUtils.tabRemoved(secondTab); > >+ let closingPromise = BrowserTestUtils.waitForTabClosing(secondTab); > > info("closing second tab (which will self-close in beforeunload)"); > > closeBtn.click(); > > ok(secondTab.closing, "Second tab should be marked as closing synchronously."); > >- await closePromise; > >+ await closingPromise; > > ok(secondTab.closing, "Second tab should still be marked as closing"); > > ok(!secondTab.linkedBrowser, "Second tab's browser should be dead"); > > I'm a bit surprised that this works. Doesn't this use the closing animation, > i.e. the browser should be closed much later than the TabClose event is > fired? I also don't really see the point of closingPromise here. looks like everything works synchronously, at least on macOS. I don't see animation when the tab closes, and also the test works even if I remove `await`. I'll remove the promise.
Depends on: 1446725
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbc65e4c51af968fee475e64edcdd885ac8ab657 Bug 1442465 - Part 1: Add BrowserTestUtils.{waitForTabClosing,waitForSessionStoreUpdate}. r=dao https://hg.mozilla.org/integration/mozilla-inbound/rev/aa1497ed75e50222faaed7e65b8c41818217681e Bug 1442465 - Part 2: Use BrowserTestUtils.{waitForTabClosing,waitForSessionStoreUpdate} instead of BrowserTestUtils.tabRemoved. r=dao https://hg.mozilla.org/integration/mozilla-inbound/rev/9fbad6edbef1c8a6ff76838031ae2120ea2512c5 Bug 1442465 - Part 3: Use BrowserTestUtils.{waitForTabClosing,waitForSessionStoreUpdate} instead of BrowserTestUtils.removeTab. r=dao https://hg.mozilla.org/integration/mozilla-inbound/rev/67af8170257afb47e77ccad1a449733b6257fe07 Bug 1442465 - Part 4.1: Stop unnecessarily awaiting on BrowserTestUtils.removeTab (non-simple part). r=dao https://hg.mozilla.org/integration/mozilla-inbound/rev/ba58e9052ab972dfad832bb33d35652500fbe54c Bug 1442465 - Part 4.2: Stop unnecessarily awaiting on BrowserTestUtils.removeTab (simple part). r=dao https://hg.mozilla.org/integration/mozilla-inbound/rev/d82a41dc01ab4a335e5c07ed6fc0a426be2ddf24 Bug 1442465 - Part 5: Workaround for some BrowserTestUtils.removeTab cases. r=dao https://hg.mozilla.org/integration/mozilla-inbound/rev/9659a30c4bb04e36661f5ac3d44a8d0b73173708 Bug 1442465 - Part 6: Stop returning a Promise from BrowserTestUtils.removeTab. r=dao
https://hg.mozilla.org/integration/mozilla-inbound/rev/b09ae094c0542194c212657923be635011387518 Bug 1442465 - followup: Wait for session store update in browser/components/migration/tests/browser/browser_undo_notification.js. r=me
Commit pushed to master at https://github.com/mozilla/activity-stream https://github.com/mozilla/activity-stream/commit/44ed3e043d3999d4c725aa6750b81fe605e38f1d chore(mc): Port Bug 1442465 - Part 4.2: Stop unnecessarily awaiting on BrowserTestUtils.removeTab (simple part). r=dao (#4049)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: