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)
Firefox
Session Restore
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.
Comment 1•7 years ago
|
||
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.
Comment 2•7 years ago
|
||
CCing a bunch of browser people.
Comment 3•7 years ago
|
||
(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 | ||
Updated•7 years ago
|
Assignee: nobody → arai.unmht
Assignee | ||
Updated•7 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•7 years ago
|
||
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
Assignee | ||
Comment 5•7 years ago
|
||
another try with several fix for each testcase
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0517cb96a41b5afd871f0df2fe1a68eff1d807e
Assignee | ||
Comment 6•7 years ago
|
||
some more fix, and stop awaiting on removeTab
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9b47a3c5f5528f4b59c4261a1046c22d3575912
Assignee | ||
Comment 7•7 years ago
|
||
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
Assignee | ||
Comment 8•7 years ago
|
||
Separated patches, and fixed some cases, and applied workaround to keep the current behavior:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a269b407b2f11a697b14e3db8ae0ba8538245720
Assignee | ||
Comment 9•7 years ago
|
||
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)
Assignee | ||
Comment 10•7 years ago
|
||
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)
Assignee | ||
Comment 11•7 years ago
|
||
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)
Assignee | ||
Comment 12•7 years ago
|
||
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)
Assignee | ||
Comment 13•7 years ago
|
||
(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.
Assignee | ||
Comment 14•7 years ago
|
||
so, actual Part 4.1 also removes await or .then() from removeTab caller.
Attachment #8958330 -
Flags: review?(dao+bmo)
Assignee | ||
Comment 15•7 years ago
|
||
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)
Assignee | ||
Comment 16•7 years ago
|
||
and finally removed misleading BrowserTestUtils.tabRemoved and stop returning that Promise from BrowserTestUtils.removeTab.
Attachment #8958332 -
Flags: review?(dao+bmo)
Updated•7 years ago
|
Attachment #8958329 -
Flags: review?(dao+bmo) → review+
Updated•7 years ago
|
Attachment #8958330 -
Flags: review?(dao+bmo) → review+
Comment 17•7 years ago
|
||
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 18•7 years ago
|
||
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)
Assignee | ||
Comment 19•7 years ago
|
||
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.
Assignee | ||
Comment 20•7 years ago
|
||
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)
Assignee | ||
Comment 21•7 years ago
|
||
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)
Assignee | ||
Comment 22•7 years ago
|
||
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)
Assignee | ||
Comment 23•7 years ago
|
||
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)
Assignee | ||
Comment 24•7 years ago
|
||
Comment 25•7 years ago
|
||
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 26•7 years ago
|
||
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 27•7 years ago
|
||
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)
Assignee | ||
Comment 28•7 years ago
|
||
Thank you for reviewing :)
removed waitForTabRemoved and TabRemoved event.
Attachment #8958326 -
Attachment is obsolete: true
Attachment #8959733 -
Flags: review?(dao+bmo)
Assignee | ||
Comment 29•7 years ago
|
||
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)
Assignee | ||
Updated•7 years ago
|
Attachment #8959065 -
Attachment is obsolete: true
Comment 30•7 years ago
|
||
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-
Assignee | ||
Comment 31•7 years ago
|
||
(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
Assignee | ||
Comment 32•7 years ago
|
||
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)
Updated•7 years ago
|
Attachment #8959733 -
Flags: review?(dao+bmo) → review+
Comment 33•7 years ago
|
||
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+
Updated•7 years ago
|
Attachment #8959066 -
Flags: review?(dao+bmo) → review+
Comment 34•7 years ago
|
||
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+
Comment 35•7 years ago
|
||
(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!
Assignee | ||
Comment 36•7 years ago
|
||
(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.
Assignee | ||
Comment 37•7 years ago
|
||
(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.
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Comment 38•7 years ago
|
||
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
Assignee | ||
Comment 39•7 years ago
|
||
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
Comment 40•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/cbc65e4c51af
https://hg.mozilla.org/mozilla-central/rev/aa1497ed75e5
https://hg.mozilla.org/mozilla-central/rev/9fbad6edbef1
https://hg.mozilla.org/mozilla-central/rev/67af8170257a
https://hg.mozilla.org/mozilla-central/rev/ba58e9052ab9
https://hg.mozilla.org/mozilla-central/rev/d82a41dc01ab
https://hg.mozilla.org/mozilla-central/rev/9659a30c4bb0
https://hg.mozilla.org/mozilla-central/rev/b09ae094c054
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Comment 41•7 years ago
|
||
Comment 42•7 years ago
|
||
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)
Updated•3 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•