1112601 - Refactor closeTab() to be in sync with openTab()

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P1)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

closeTab() from the tabBrowser class has a different calling convention than all the other methods, AND does not correctly wait for the tab to be closed.

This is highly risky given that closeAllTabs is used in nearly all tests! So this could be a source for failure.

I will get this fixed because I need it for bug 1020923.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: WIP v1

This is the patch to streamline the calling convention. Something strange I noticed is that multiple transitionend notifications are getting sent out. Not sure why this is happening.

Andreea, could you put this onto the next sprint for investigation? Not sure if we can use the events or if we have to keep the old method in checking the tab count. Thanks

Comment 2

[Andreea Matei [:AndreeaMatei]]

Sure, adding it.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thanks!

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Given that a proper tab handling is required for Marionette we would need that urgently. Bumping priority up to P1.

Comment 5

[Cosmin Malutan, [:cosmin-malutan]]

I have an upcoming patch, it just that I have to test it really well, it touches addons.js too and I had some failures on complete teastruns, I'll return in a bit with all details.

Comment 6

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v1.0

I updated the opening method to wait for all three transition events.
I switched to fat arrow functions to get rid of self variable.
I made use of _waitForTabOpen when in AddonsManager so we wait for transition correctly and don't duplicate code
I kept the tabs length comparation when closing tabs, because the tab node get's removed from the list after it was hidden and animation was ended, closeAllTabs method could call closeTab even when we have only one left, because the one being removed it still count's in length and with the new closed one we close the Firefox and we hit a Disconnect error. 

Reports:
http://mozmill-crowd.blargon7.com/#/functional/report/04e54d608fc589a28217304fe60ce2c5
http://mozmill-crowd.blargon7.com/#/functional/report/04e54d608fc589a28217304fe62a5d0a
http://mozmill-crowd.blargon7.com/#/remote/report/04e54d608fc589a28217304fe62be5c8
http://mozmill-crowd.blargon7.com/#/remote/report/04e54d608fc589a28217304fe62b6156

Comment 7

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8544589 [details] [diff] [review]
patch v1.0

Review of attachment 8544589 [details] [diff] [review]:
-----------------------------------------------------------------

::: firefox/lib/tabs.js
@@ +530,5 @@
> +   * @param {object} [aSpec]
> +   *        Information about how to close the tab
> +   * @param {function} [aSpec.callback]
> +   *        Callback used for closing the tab
> +   * @param {number} [index=selectedIndex]

aSpec.index?

@@ +543,5 @@
> +    var method = aSpec.method || "menu";
> +    var index = (typeof aSpec.index === undefined) ? this.selectedIndex
> +                                                   : aSpec.index;
> +
> +    var closed =false;

whitespace after = and please remove the line below.

@@ +553,5 @@
> +    };
> +
> +    var checkTabClosed = () => { closed = true; }
> +    var checkTabTransitioned = (aEvent) => {
> +      switch (aEvent.propertyName){

whitespace before {

@@ +611,5 @@
> +
> +    try {
> +      callback();
> +
> +      assert.waitFor(() => closed && transitioned.completed && this.length === (length - 1),

New line for the last check

@@ +798,5 @@
>     *        Callback used for opening the tab
>     * @param {string} [aSpec.method="menu"]
>     *        Method used for opening the tab
> +   *        ("callback", "menu", "newTabButton", "shortcut", "tabStrip",
> +   *         "contextMenu, "middleClick")

I think we used to put these with "|"

::: firefox/lib/tests/testSessionStore.js
@@ +43,5 @@
>    var button = session.getElement({type: "button_restoreSession"});
>    expect.equal(button.getNode().getAttribute('oncommand'), "restoreSession();",
>                 "Restore Session button has the correct action");
>  
> +  tabBrowser.openTab();

This was called with 'shortcut' before, the methods have 'menu' as default.

::: firefox/tests/functional/testTabbedBrowsing/testCloseTab.js
@@ +35,5 @@
>      return tabBrowser.length === 4;
>    }, "One tab has been closed via the close button");
>  
>    // Close a tab via File > Close tab:
> +  tabBrowser.closeTab({method: "menu"});

This is default

::: lib/addons.js
@@ +231,5 @@
>  
>      try {
>        if (aomTabs[0].controller.tabs.length > 1) {
> +        var tabBrowser = new tabs.tabBrowser(aomTabs[0].controller);
> +        tabBrowser.closeTab({method: "middleClick", index: aomTabs[0].index, amo:true});

whitespace before true, and on a new line I think. What is this amo, i don't see it in closeTab.

Comment 8

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v2.0

I made the changes.

http://mozmill-crowd.blargon7.com/#/functional/report/04e54d608fc589a28217304fe6e374ce

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8545709 [details] [diff] [review]
patch v2.0

Review of attachment 8545709 [details] [diff] [review]:
-----------------------------------------------------------------

This patch looks great! Functionality wise all should be fine. But really lets have a look at the closeTab function, and check if we can make it simpler if possible.

::: firefox/lib/tabs.js
@@ +612,5 @@
> +    try {
> +      callback();
> +
> +      assert.waitFor(() => closed && transitioned.completed &&
> +                           this.length === (length - 1), "Tab has been closed");

What would happen if we really only wait for the tab length to be one lesser? Shouldn't that work reliable too? I feel so, and compared to open or select, we do not have to wait until all those events have happened. Maybe we can really make it simpler here.

@@ +798,5 @@
>     *        Callback used for opening the tab
>     * @param {string} [aSpec.method="menu"]
>     *        Method used for opening the tab
> +   *        ("callback"|"menu"|"newTabButton"|"shortcut"|"tabStrip"
> +   *         "contextMenu"|"middleClick")

Please use the same method in specifying the possible values as for closeTab(). We do not use '|' in the documentation.

@@ +990,5 @@
> +          break;
> +      }
> +      transitioned.completed = transitioned.visibility &&
> +                               transitioned.minWidth &&
> +                               transitioned.maxWidth;

Very nasty code we have to write here to ensure that the tab has been closed. :( Have we ever filed a bug with a request to make that easier? I think especially add-ons which rely on the TabClose event have huge problems.

::: lib/addons.js
@@ -143,5 @@
> -    function onViewLoaded() { self.loaded = true; }
> -    function checkTabTransitioned() { self.transitioned = true; }
> -
> -    this._tabBrowser._tabs.getNode().addEventListener("transitionend",
> -                                                      checkTabTransitioned);

Hurray for getting rid of it here!

Comment 10

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to Henrik Skupin (:whimboo) from comment #9)
> Comment on attachment 8545709 [details] [diff] [review]
> ::: firefox/lib/tabs.js
> @@ +612,5 @@
> > +    try {
> > +      callback();
> > +
> > +      assert.waitFor(() => closed && transitioned.completed &&
> > +                           this.length === (length - 1), "Tab has been closed");
> 
> What would happen if we really only wait for the tab length to be one
> lesser? Shouldn't that work reliable too? I feel so, and compared to open or
> select, we do not have to wait until all those events have happened. Maybe
> we can really make it simpler here.
That was the case before this patch, it was fine and it's fine, it doesn't really bother us, the scope of this bug was to make the two methods alike. I need to keep the length check for cases when we have way to many tabs opened, then we have no UI events when closing tabs, and we will call closeTab too fast in closeAllTabs method resulting in an attempt to close tabs that are already closed.

> > +      }
> > +      transitioned.completed = transitioned.visibility &&
> > +                               transitioned.minWidth &&
> > +                               transitioned.maxWidth;
> 
> Very nasty code we have to write here to ensure that the tab has been
> closed. :( Have we ever filed a bug with a request to make that easier? I
> think especially add-ons which rely on the TabClose event have huge problems.
This is for openTab method, and we really need this, that's why I took over this bug, as discussed in bug 999357 comment 33 and bug 999357 comment 34.
I personally don't like the idea of making a bug for developers for easing the automation process if that's not really necessary, but I have no problem in doing so if you like.
Regarding the addons, they will have problems only if they will try to dispatch an event to the tab while it's animating, which I doubt will be the case.

Could I go ahead and update the other nits? Or we are strong on the two requests above?

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ok, so lets keep what we have above. In regard of a Firefox bug it might be good to start a thread in the dev.platform mailing list.

Comment 12

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v2.1

I fixed the nit, thanks.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8548893 [details] [diff] [review]
patch v2.1

Review of attachment 8548893 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! So lets see how this improves our stability regarding tab handling. If that helps a lot lets consider this to be backported to maybe all older branches?

Comment 14

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8548893 [details] [diff] [review]
patch v2.1

Review of attachment 8548893 [details] [diff] [review]:
-----------------------------------------------------------------

http://hg.mozilla.org/qa/mozmill-tests/rev/1d30e702e25c (default)

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I would really be interested in how more stable we will get with that patch being landed. Lets see tomorrow.

Comment 16

[Cosmin Malutan, [:cosmin-malutan]]

Andreea this ran well on Nightly, the patch applies cleanly on the rest of branches, let's backport the patch.

Comment 17

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v2.1 aurora

Andreea this is the patch for Aurora.

Comment 18

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8551248 [details] [diff] [review]
patch v2.1 aurora

Review of attachment 8551248 [details] [diff] [review]:
-----------------------------------------------------------------

http://hg.mozilla.org/qa/mozmill-tests/rev/735a91a6c375 (aurora)

Comment 19

[Cosmin Malutan, [:cosmin-malutan]]

There were some failures on Aurora,
http://mozmill-daily.blargon7.com/#/functional/failure?app=Firefox&branch=37&platform=Win&from=2015-01-20&to=2015-01-21&test=%2FtestTabbedBrowsing%2FtestOpenInForeground.js&func=testOpenInForegroundTab

Comment 20

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: follow-up patch

Those failures are fall from the missing the transitionend events, which dowsn't tell if the browser closed or opened. In the first place we listen for those events to be sure that we can operate on tab safely, but if the tab is opened and we fail to catch the animation events, I wouldn't invalidate the test, after an timeout exception(5s) the tab will definitely be done animating, and we can operate on it.
My point here is that we should catch the timeout and check again if the tab is opened, this will make the test more robust but more reliable.  
 
http://mozmill-crowd.blargon7.com/#/functional/report/dd0cbbf05a3aab4a7676502a86e464d7
http://mozmill-crowd.blargon7.com/#/remote/report/dd0cbbf05a3aab4a7676502a86e46774

Comment 21

[Mihaela Velimiroviciu (:mihaelav)]

Comment on attachment 8552940 [details] [diff] [review]
follow-up patch

Review of attachment 8552940 [details] [diff] [review]:
-----------------------------------------------------------------

The commit message should be updated: "rely" instead of "relay". Otherwise it looks good to me.

Comment 22

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8552940 [details] [diff] [review]
follow-up patch

Review of attachment 8552940 [details] [diff] [review]:
-----------------------------------------------------------------

Please update and request from Henrik.

::: firefox/lib/tabs.js
@@ +618,5 @@
> +    catch (e) {
> +      if (this.length === (length - 1)) {
> +        return;
> +      }
> + 

Trailing whitespace

Comment 23

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: follow-up patch v2.0

Thanks

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8553027 [details] [diff] [review]
follow-up patch v2.0

Review of attachment 8553027 [details] [diff] [review]:
-----------------------------------------------------------------

::: firefox/lib/tabs.js
@@ +617,5 @@
>      }
> +    catch (e) {
> +      if (this.length === (length - 1)) {
> +        return;
> +      }

So can you please explain why we should miss those events? We register them all before the tab gets opened, so we should see them all. Which ones do we actually miss in those cases?

Also return is wrong here! This will not remove the registered event listeners. It's better to check for the state to re-throw.

@@ +1013,5 @@
>        }, "Tab has been opened");
>      }
> +    catch (e) {
> +      if (opened) {
> +        return;

I'm not that happy with that addition either. Have we had a discussion so far with Dao or someone else who knows tabbed browsing well enough, so we can ask for which events we absolutely have to wait?

Comment 25

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: test_case.js

Do you now in which cases we don't get the transitionend events when closing the tabs? That happens, and I don't know why, please run the testcase.
> mozmill -t test_case.js -b <path to Firefox build>

Comment 26

[Dão Gottwald [:dao]]

(In reply to Cosmin Malutan, [:cosmin-malutan] from comment #25)
> Do you now in which cases we don't get the transitionend events when closing
> the tabs?

See http://hg.mozilla.org/mozilla-central/annotate/b2b10231606b/browser/base/content/tabbrowser.xml#l2003

Comment 27

[Cosmin Malutan, [:cosmin-malutan]]

Thanks Dao.

Whimboo, the events we wait for are correct, but we have lot's of cases in which we don't animate, my opinion is that we can't check or at least, checking all the cases from the link above is error prone.
We can either add a try/catch or get rid of animation listener when we close tabs and rely on tabs length, which is get's updated the latest.

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Seeing the list of exceptions in case of animated tab opening and closing, I'm getting worried that our tests will be stable. As long as there is no final event send out when a tab has fully been opened, I would suggest we get rid of all that cruft and simply disable tab animation! That would solve a lot of problems we currently have. Also I don't think its something we should cover with our tests. There might be browser chrome tests in place which do checks for that. We could include this preference change in the upcoming 2.1 release of Mozmill.

Comment 29

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: remove the animation listeners patch v01

So as discussed, we will remove the animation listeners, we will disable the animation first in tests methods.

http://mozmill-crowd.blargon7.com/#/functional/report/6b1e6b9bf618fb20da2bf8727490a51f
http://mozmill-crowd.blargon7.com/#/remote/report/6b1e6b9bf618fb20da2bf8727490bc70

Comment 30

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: remove the animation listeners patch v1.1

I forgot a change.

http://mozmill-crowd.blargon7.com/#/remote/report/6b1e6b9bf618fb20da2bf87274923113

Comment 31

[Mihaela Velimiroviciu (:mihaelav)]

Comment on attachment 8555823 [details] [diff] [review]
remove the animation listeners patch v1.1

Review of attachment 8555823 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me with the nit addressed.
Also, the commit message needs updated before landing.

::: firefox/lib/tabs.js
@@ +529,5 @@
>    closeTab : function tabBrowser_closeTab(aSpec={}) {
>      var method = aSpec.method || "menu";
>      var index = (typeof aSpec.index === undefined) ? this.selectedIndex
>                                                     : aSpec.index;
> +    var length = this.length;

nit: You don't use this anymore, please remove it.

Comment 32

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: remove the animation listeners patch v1.2

Thanks Mihaela.

Comment 33

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8556383 [details] [diff] [review]
remove the animation listeners patch v1.2

Review of attachment 8556383 [details] [diff] [review]:
-----------------------------------------------------------------

::: firefox/lib/tabs.js
@@ +928,5 @@
>     */
>    _waitForTabOpened: function tabBrowser_waitForTabOpened(aCallback) {
> +    // Bug 1112601
> +    // TODO: Remove this pref once it has been added in Mozmill
> +    prefs.setPref(PREF_TABS_ANIMATE, false);

I hope that all of our tests which open/close tabs are using this method when doing so. Maybe have a quick look over those if not done yet. It could still be that some don't, and animation would still be on.

Comment 34

[Cosmin Malutan, [:cosmin-malutan]]

They do, I checked and I made complete testruns, see the links above.
Thanks

Comment 35

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8556383 [details] [diff] [review]
remove the animation listeners patch v1.2

Review of attachment 8556383 [details] [diff] [review]:
-----------------------------------------------------------------

Lets see how it goes with nightly:
https://hg.mozilla.org/qa/mozmill-tests/rev/cab16cbc6ba8 (default)

Comment 36

[Cosmin Malutan, [:cosmin-malutan]]

Removing the animation events listeners, brings back to life failures from bug 890181, for which we enabled the animations in the first place:
Those fail on windows, and only when closing tabs with close button.
If we want to keep the animations disabled we have to change the method that we use for closing tabs from button to method, and keep only one test for close button, witch it must not to be the first tab that we close. 

http://mozmill-daily.blargon7.com/#/functional/failure?app=Firefox&branch=All&platform=All&from=2015-01-29&to=&test=%2FtestTabbedBrowsing%2FtestOpenInForeground.js&func=testOpenInForegroundTab

Comment 37

[Cosmin Malutan, [:cosmin-malutan]]

Those failures were on Aurora, not on default, I tried to reproduce a failure from morning, but the bug is fixed on default:
http://mozmill-crowd.blargon7.com/#/functional/reports?app=All&branch=38&platform=Win&from=2015-02-02&to=2015-02-02

I'll make a patch for aurora.

Comment 38

[Cosmin Malutan, [:cosmin-malutan]]

The patch applies cleanly on aurora and it works:
http://mozmill-crowd.blargon7.com/#/functional/report/4d9c8aea57549df57dc5846cc003e3c2

Andreea could you backport the fix?

Comment 39

[Andreea Matei [:AndreeaMatei]]

https://hg.mozilla.org/qa/mozmill-tests/rev/6ad4f04a95f8 (aurora)

Comment 40

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch v1.2 [beta]

Here are the merged patches, they apply on beta.
http://mozmill-crowd.blargon7.com/#/functional/report/4d9c8aea57549df57dc5846cc032d5c2
http://mozmill-crowd.blargon7.com/#/remote/report/4d9c8aea57549df57dc5846cc0330eab

Comment 41

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8558360 [details] [diff] [review]
patch v1.2 [beta]

Review of attachment 8558360 [details] [diff] [review]:
-----------------------------------------------------------------

https://hg.mozilla.org/qa/mozmill-tests/rev/318faa7b8640 (beta)

Comment 42

[Cosmin Malutan, [:cosmin-malutan]]

This is an library improvement and we should let the train to take care of release branch.

Comment 43

[Henrik Skupin [:whimboo][⌚️UTC+1]]

With Mozmill 2.0.10 released we need this fix on the esr branch too. I will have a look into it on Monday.

Comment 44

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: backport esr31 (bustage) v1

This is the backport patch for esr31. Local runs all passing with it applied. Given that it is a bustage fix and no-one is around for review right now I will get it landed immediately to stop the massive failures on that branch.

Comment 45

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Landed on mozilla-esr31 as https://hg.mozilla.org/qa/mozmill-tests/rev/24e37ac17b3a.