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

Status: RESOLVED FIXED

(Firefox :: Session Restore, enhancement)

Description

[Tooru Fujisawa [:arai]]

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

[Hiroyuki Ikezoe (:hiro)]

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

[Hiroyuki Ikezoe (:hiro)]

CCing a bunch of browser people.

Comment 3

[Dão Gottwald [:dao]]

(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.

Comment 4

[Tooru Fujisawa [:arai]]

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

Comment 5

[Tooru Fujisawa [:arai]]

another try with several fix for each testcase

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0517cb96a41b5afd871f0df2fe1a68eff1d807e

Comment 6

[Tooru Fujisawa [:arai]]

some more fix, and stop awaiting on removeTab

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9b47a3c5f5528f4b59c4261a1046c22d3575912

Comment 7

[Tooru Fujisawa [:arai]]

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

Comment 8

[Tooru Fujisawa [:arai]]

Separated patches, and fixed some cases, and applied workaround to keep the current behavior:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a269b407b2f11a697b14e3db8ae0ba8538245720

Comment 9

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Add BrowserTestUtils.{waitForTabClosing,waitForTabRemoved,waitForSessionStoreUpdate}.

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)

Comment 10

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Use BrowserTestUtils.{waitForTabClosing,waitForTabRemoved,waitForSessionStoreUpdate} instead of BrowserTestUtils.tabRemoved.

And replaced BrowserTestUtils.tabRemoved with one of the newly added methods.
BrowserTestUtils.tabRemoved will be removed in the later patch

Comment 11

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Use BrowserTestUtils.{waitForTabClosing,waitForTabRemoved,waitForSessionStoreUpdate} instead of BrowserTestUtils.removeTab.

Also replaced BrowserTestUtils.removeTab with one of newly added methods.
BrowserTestUtils.removeTab will be changed not to return Promise in the later patch.

Comment 12

[Tooru Fujisawa [:arai]]

Attachment: 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

Comment 13

[Tooru Fujisawa [:arai]]

(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.

Comment 14

[Tooru Fujisawa [:arai]]

Attachment: Part 4.1: Stop unnecessarily awaiting on BrowserTestUtils.removeTab (non-simple part).

so, actual Part 4.1 also removes await or .then() from removeTab caller.

Comment 15

[Tooru Fujisawa [:arai]]

Attachment: Part 5: Workaround for some BrowserTestUtils.removeTab cases.

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)

Comment 16

[Tooru Fujisawa [:arai]]

Attachment: Part 6: Stop returning a Promise from BrowserTestUtils.removeTab.

and finally removed misleading BrowserTestUtils.tabRemoved and stop returning that Promise from BrowserTestUtils.removeTab.

Comment 17

[Dão Gottwald [:dao]]

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

Comment 18

[Dão Gottwald [:dao]]

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

Comment 19

[Tooru Fujisawa [:arai]]

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 20

[Tooru Fujisawa [:arai]]

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).

Comment 21

[Tooru Fujisawa [:arai]]

Attachment: Part 2: Use BrowserTestUtils.{waitForTabClosing,waitForTabRemoved,waitForSessionStoreUpdate} instead of BrowserTestUtils.tabRemoved.

Changed browser_withoutDump.js to use BrowserTestUtils.waitForTabClosing, and allowed closing tab in testing/mochitest/browser-test.js

Comment 22

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Use BrowserTestUtils.{waitForTabClosing,waitForTabRemoved,waitForSessionStoreUpdate} instead of BrowserTestUtils.removeTab.

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.

Comment 23

[Tooru Fujisawa [:arai]]

Attachment: Part 5: Workaround for some BrowserTestUtils.removeTab cases.

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)

Comment 24

[Tooru Fujisawa [:arai]]

here's try run
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0afca1f0f22063de51a5b0d7a6a505105cf7cb62&selectedJob=168120784

Comment 25

[Dão Gottwald [:dao]]

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.

Comment 26

[Dão Gottwald [:dao]]

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?

Comment 27

[Dão Gottwald [:dao]]

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.

Comment 28

[Tooru Fujisawa [:arai]]

Attachment: Part 1: Add BrowserTestUtils.{waitForTabClosing,waitForSessionStoreUpdate}.

Thank you for reviewing :)


removed waitForTabRemoved and TabRemoved event.

Comment 29

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Use BrowserTestUtils.{waitForTabClosing,waitForSessionStoreUpdate} instead of BrowserTestUtils.removeTab.

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

Comment 30

[Dão Gottwald [:dao]]

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);

Comment 31

[Tooru Fujisawa [:arai]]

(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

Comment 32

[Tooru Fujisawa [:arai]]

Attachment: Part 3: Use BrowserTestUtils.{waitForTabClosing,waitForSessionStoreUpdate} instead of BrowserTestUtils.removeTab.

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.

Comment 33

[Dão Gottwald [:dao]]

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.

Comment 34

[Dão Gottwald [:dao]]

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.

Comment 35

[Dão Gottwald [:dao]]

(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!

Comment 36

[Tooru Fujisawa [:arai]]

(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.

Comment 37

[Tooru Fujisawa [:arai]]

(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.

Comment 38

[Tooru Fujisawa [:arai]]

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

Comment 39

[Tooru Fujisawa [:arai]]

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

[Arthur Iakab [arthur_iakab]]

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

Comment 41

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/activity-stream/pull/4049

Comment 42

[[github robot]]

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)