684801 - Timeout failure in /testTabView_SwitchTabs/test1.js | TabView is still open.

Status: RESOLVED FIXED

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

Description

[Mihaela Velimiroviciu (:mihaelav)]

URL for the dashboard report where the failure occurs:
http://mozmill-release.brasstacks.mozilla.com/#/endurance/report/b2f105bf5adf3d9f861770c4a991e7cb

TEST: /testTabView_SwitchTabs/test1.js::testSwitchTabs
ERROR: TimeoutError("TabView is still open.")@resource://mozmill/modules/utils.js:429 waitFor([object Proxy],"TabView is still open.")@resource://mozmill/modules/utils.js:467 
WHEN: 2011-09-05
FIRST: 2011-09-05
FREQ: 1
BRANCHES: aurora

Comment 1

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

Mihaela, thank you for logging this failure. That one is a one-time failure on Nightly. So not sure if this is only related to a test-run which has been interfered by the user or another application. Means we lost the focus on the application. Is it reproducible?

Comment 2

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Your report mentioned that this occurred on Aurora, however the test result indicates that it was in fact on Nightly. I have attempted to replicate the failure locally on the latest Aurora and Nightly builds without success. I would like to see if this issue recurs before investigating further.

Comment 3

[u279076]

FWIW, I see an instance of this failure on the Nightly Mac OSX 10.6.7 testrun on Sept 5 and Sept 6.

Comment 4

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

Could that be a new regression specifically on OS X?

Comment 5

[Mihaela Velimiroviciu (:mihaelav)]

Confirming same fail on Mac 10.6.7 x86_64 with build 20110906030844 from Sept 6

Comment 6

[u415141]

A Pivotal Tracker story has been created for this Bug: https://www.pivotaltracker.com/story/show/17952765

Comment 7

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

Mihaela, are you able to reproduce this failure locally? If yes, does it also happen for Aurora builds? If not can you check some older builds of Minefield so we could get a regression range? thanks.

Comment 8

[Mihaela Velimiroviciu (:mihaelav)]

We tried different scenarios locally on Mac and Ubuntu but we didn't catch the failure.

Comment 9

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

I have executed the testrun_all.py script on qa-horus which is used by the daily script. No failures have been reported:

http://mozmill-archive.brasstacks.mozilla.com/#/endurance/report/5fdc9d8340b415e9a2e6a934ad3996c4

So not sure what's going on. We should check the results in the next couple of days. Anthony, if you could watch the daily test-run and just check for abnormalities on that box, that would be great.

Comment 10

[Mihaela Velimiroviciu (:mihaelav)]

No failures in the results of endurance tests run on build 20110907030853 from Sept 7th

Comment 11

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

Same yesterday. So I would propose we close this bug as WFM and reopen if it reoccurs.

Comment 12

[u415141]

Anthony Hughes added a comment in Pivotal Tracker:   
   
WORKSFORME

Comment 13

[Mihaela Velimiroviciu (:mihaelav)]

Same failure occured again for Mac 10.6 with build 20111019031031.

URL for the dashboard report where the failure occurs:
http://mozmill-release.brasstacks.mozilla.com/#/endurance/report/7f11426eacdc891f09ffcc3839ecbdc8

TEST: /testTabView_SwitchTabs/test1.js::testSwitchTabs
ERROR: TimeoutError("TabView is still open.")@resource://mozmill/modules/utils.js:429 waitFor([object Proxy],"TabView is still open.")@resource://mozmill/modules/utils.js:467 
WHEN: 2011-10-19
FIRST: 2011-09-05
FREQ: 2
BRANCHES: nightly

Comment 14

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Have you been able to replicate this locally? It appears that it hasn't failed on Mac since the 19th October, but has failed a couple of times on other platforms. It might be worth trying to run this with more iterations to see if this is an issue of degradation over time, which we're only just hitting occasionally.

Comment 15

[u279076]

Mihaela, please investigate RE: comment 14. Thanks

Comment 16

[Dave Hunt [:davehunt] [he/him] ⌚BST]

This still appears to be an issue on Mac:

http://mozmill-ci.blargon7.com/#/endurance/report/fdec829b93b19c73985be1d3883a19e3
http://mozmill-ci.blargon7.com/#/endurance/report/fdec829b93b19c73985be1d38837172a

Comment 17

[Mihaela Velimiroviciu (:mihaelav)]

Failure appeared again on Mac OSX 10.6.8, Firefox 16.0a1, on June 6 and 7:
http://mozmill-ci.blargon7.com/#/endurance/report/fdec829b93b19c73985be1d388774a92
http://mozmill-ci.blargon7.com/#/endurance/report/fdec829b93b19c73985be1d3887c9bd7

Comment 18

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

Any progress here? It happened again today for 13.0.1 builds:

http://mozmill-ci.blargon7.com/#/endurance/report/e67171ea696231bb192f5656151cfd29

Comment 19

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

Dave, if we can't get this fixed we will have to disable the test. It's now happening for a long time with no progress made. There were a lot of failures for the latest release tests.

Comment 20

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I agree, although I'm reluctant to skip if we can replicate it so frequently on our CI. Has anyone been able to replicate this locally? I can spend some time on it, but in the meantime we should get a patch to disable the test.

Comment 21

[Maniac Vlad Florin (:vladmaniac)]

Attachment: skip patch v1.0

Skip patch

Comment 22

[Maniac Vlad Florin (:vladmaniac)]

the skip patch was added due to comment 20

Comment 23

[Dave Hunt [:davehunt] [he/him] ⌚BST]

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

Taking this review.

Comment 24

[Dave Hunt [:davehunt] [he/him] ⌚BST]

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

r+ with a minor nit addressed in the skip message and comment.

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/793f4fdd1a93 (default)

Comment 25

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Is ESR10 affected? Let's see this skipped in the next run before transplanting the skip.

Comment 26

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

Yes, esr10 is also affected:
http://mozmill-ci.blargon7.com/#/endurance/report/a7655636e327552d4750d1013c063a96

Please land the skip patch on all branches right away.

Comment 27

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Okay, so we land skips across all affected branches immediately? Noted for future.

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/b3b55bfa89aa (mozilla-aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/e66df6dcb970 (mozilla-beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/caeba250e282 (mozilla-release)
http://hg.mozilla.org/qa/mozmill-tests/rev/93b8b1cd1cb6 (mozilla-esr10)

Comment 28

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I'm unable to replicate this on Mac OS X 10.7.4

Comment 29

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

We can't work on this bug this week given the other priorities.

Comment 30

[Daniela Petrovici]

I have run the tests on 21.0a1 build and I couldn't reproduce the issue at all (tried 400 runs over the past few days). Can we re-enable the test since it was skipped in 16.0a1?

Comment 31

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Sounds good. Let's re-enable this test on default and watch for failures. I'm no longer able the backout the original patch, so please attach a backout patch for default.

Comment 32

[Daniela Petrovici]

Attachment: patch v1.0 to re-enable test

Patch to re-enable test case. The report for MAC is: http://mozmill-crowd.blargon7.com/#/endurance/report/a83c700664548dba07298b74bf30aae6

Comment 33

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 713373 [details] [diff] [review]
patch v1.0 to re-enable test

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

Landed on default:
http://hg.mozilla.org/qa/mozmill-tests/rev/88f34d464198 (default)

Comment 34

[Daniela Petrovici]

Reproducible in FF Nightly FR version on MAC 10.6.8:
http://mozmill-ci.blargon7.com/#/endurance/report/a83c700664548dba07298b74bf764c42

I will try to reproduce the issue today.

Comment 35

[Daniela Petrovici]

I was not able to reproduce it locally or on the remote machine I have tried. I will continue on Monday to see if it reproduces remotely.

Comment 36

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

How often did it fail? Only once over the last couple of days?

Comment 37

[Daniela Petrovici]

Yes, it only reproduced on 2/15 once.

Comment 38

[Andreea Matei [:AndreeaMatei]]

Happened again on OS X 10.8.2, aurora, fr locale:
http://mozmill-ci.blargon7.com/#/endurance/report/f36358d058daf73ddbf7815016b23b1b

I looked over the code and I see in the library we have at close() method a XXX with bug 648294, but that was fixed in the mean time, so we don't need to update the '.toLocaleUpperCase()' part as well.

Comment 39

[Daniela Petrovici]

Actually this fails in waitForClosed() function where close() is not used. 

The close() function is not used by this test and I have logged bug no. 844787 for this issue.

Comment 40

[mario garbi]

Happened again today on Mac OS X 10.8.2 - Firefox 21.0a2:
http://mozmill-ci.blargon7.com/#/endurance/report/66aad0ebfa7a01580628b3af4cf49af4

Comment 41

[mario garbi]

And again today on Mac OS X 10.7.5 with Firefox 22.0a1 fr:
http://mozmill-ci.blargon7.com/#/endurance/report/2a6536e9db9f5f44ed48c5851004d418

Comment 42

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

(In reply to mario garbi from comment #41)
> And again today on Mac OS X 10.7.5 with Firefox 22.0a1 fr:
> http://mozmill-ci.blargon7.com/#/endurance/report/
> 2a6536e9db9f5f44ed48c5851004d418

Just to clarify, this was a test failure from Friday and not today.

Comment 43

[Andreea Matei [:AndreeaMatei]]

Reproduced today on OS X 10.7.5, with Nightly en-US locale:
http://mozmill-ci.blargon7.com/#/endurance/report/2a6536e9db9f5f44ed48c5851053cf09

Comment 44

[Andreea Matei [:AndreeaMatei]]

One more time yesterday, on the same machine:
http://mozmill-ci.blargon7.com/#/endurance/report/2a6536e9db9f5f44ed48c58510a72237

Comment 45

[Daniela Petrovici]

Unable to reproduce with build from 03/09 on the machine it happened. And I am still unable to reproduce locally.

Comment 46

[Andrei Eftimie]

Again on OSX, 22.x:
http://mozmill-ci.blargon7.com/#/endurance/report/36bf148e4d23c6904e5ade75a4859a69

Comment 47

[mario garbi]

Happened again today on Mac OS X 10.7.5 (x86_64) with Firefox 22.0a2 en-US
http://mozmill-ci.blargon7.com/#/endurance/report/452ec32f8deec0960aea87aa06f25f09

Comment 48

[Daniela Petrovici]

Attachment: patch v1.0 to re-enable the test for ESR17

This issue did not reproduce for two months. The test is unskipped on all versions except for ESR17. I think we should enable the test for this branch too.

The patch for default does not apply cleanly on ESR17 so I created a new one.

Reports:
MAC:
http://mozmill-crowd.blargon7.com/#/endurance/report/a1b02004612785c13cf7c6bf1e9bdfc7
Linux:
http://mozmill-crowd.blargon7.com/#/endurance/report/a1b02004612785c13cf7c6bf1e9bd5d0
Windows:
http://mozmill-crowd.blargon7.com/#/endurance/report/a1b02004612785c13cf7c6bf1e9c63d8

Comment 49

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 770715 [details] [diff] [review]
patch v1.0 to re-enable the test for ESR17

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

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/423e8a5fe978 (esr17)
Hope it sticks now. Thanks!

Comment 50

[mario garbi]

Happened again today on Windows NT 6.1.7601 with ESR17 en-US:
http://mozmill-ci.blargon7.com/#/endurance/report/593227b0561e6d84f2e52c8422215752

Comment 51

[Andrei Eftimie]

We have 1 failure on Aurora, Linux, en-US:
http://mozmill-daily.blargon7.com/#/endurance/report/1039ea48a9d69a5a1cc4fd228c46dbfe

Comment 52

[Cosmin Malutan, [:cosmin-malutan]]

We had one failure on Mac 10.6.8 (x64) with Nightly en-us
http://mozmill-daily.blargon7.com/#/endurance/report/6d6f6a58b02eeffc06eafa8bea616298

Comment 53

[Andreea Matei [:AndreeaMatei]]

Happened today with esr17, on OS X 10.6.8:
http://mozmill-daily.blargon7.com/#/endurance/report/f54ca65f73fd56845c30b7ab7d0d3c8a

Comment 54

[Andrei Eftimie]

It seems this still fails, albeit rarely:
http://mozmill-daily.blargon7.com/#/endurance/report/56542c686e28115c2b3e8181cf1c9aee

Comment 55

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v1.0

Hi, I couldn't reproduce this failure but under a closer look it's obvious what is wrong here.
We attach the listener after we trigger the action so in some cases the event might be already terminated at the time we attach the listener.
    
Reports:
http://mozmill-crowd.blargon7.com/#/endurance/report/361b4f553c3df363a1b22c126429866b
http://mozmill-crowd.blargon7.com/#/endurance/report/361b4f553c3df363a1b22c126428f6f7
http://mozmill-crowd.blargon7.com/#/endurance/report/361b4f553c3df363a1b22c12642d9272

Comment 56

[Andrei Eftimie]

Comment on attachment 8351368 [details] [diff] [review]
patch_v1.0

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

Nice catch!

I'm not sold on passing everything as a callback function.

Wondering if it wouldn't be cleaner to add another method `initClose` that would set up the listener.
So we'll have:
> initClose()
> [action for closing]
> waitForClosed()

I'll give feedback+ for finding the issue, but I'll leave the other reviews flag on to gather more info on how to proceed.

::: firefox/lib/tabview.js
@@ +144,5 @@
>      var self = { closed: false };
>      function checkClosed() { self.closed = true; }
>      this._controller.window.addEventListener("tabviewhidden", checkClosed, false);
>  
> +    // The command should be executed after we attache the listener

nit: attach

Comment 57

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

Comment on attachment 8351368 [details] [diff] [review]
patch_v1.0

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

::: firefox/lib/tabview.js
@@ +141,2 @@
>     */
> +  waitForClosed : function tabView_waitForClosed(aCallback) {

Please kill this method. It doesn't make sense to keep it any longer. Waiting for events should always happen right next to the code which triggered an action.

Comment 58

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v2.0

(In reply to Henrik Skupin (:whimboo) from comment #57)
> > +  waitForClosed : function tabView_waitForClosed(aCallback) {
> Please kill this method. It doesn't make sense to keep it any longer.
> Waiting for events should always happen right next to the code which
> triggered an action.
Right, I removed the method and also the waitForOpen, for consistency.
 
Reports:
http://mozmill-crowd.blargon7.com/#/functional/report/0fc916b47806e5d11cba8af7dc2b913f
http://mozmill-crowd.blargon7.com/#/functional/report/0fc916b47806e5d11cba8af7dc2c4833
http://mozmill-crowd.blargon7.com/#/functional/report/0fc916b47806e5d11cba8af7dc2c67cc

http://mozmill-crowd.blargon7.com/#/endurance/report/0fc916b47806e5d11cba8af7dc2cf2e4
http://mozmill-crowd.blargon7.com/#/endurance/report/0fc916b47806e5d11cba8af7dc2e1fe4
http://mozmill-crowd.blargon7.com/#/endurance/report/0fc916b47806e5d11cba8af7dc2e0fec

Comment 59

[Cosmin Malutan, [:cosmin-malutan]]

We need this fix as it causes other test to fail too. So I reprioritize the bug.

Comment 60

[Andrei Eftimie]

Comment on attachment 8358357 [details] [diff] [review]
patch_v2.0

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

Overall looks good.

Just a few minor issues.

::: firefox/lib/tabview.js
@@ +72,5 @@
> +    var self = { opened: false };
> +    function checkOpened() { self.opened = true; }
> +    this._controller.window.addEventListener("tabviewshown", checkOpened, false);
> +
> +    // Open the view via command key

nit: please move the whole block before the `cmdKey` assignment.
Right now we have this comment duplicated // via keyboard shortcut and via command key

@@ +126,5 @@
>      this._controller.window.addEventListener("tabviewhidden", checkClosed, false);
>  
> +    // Close the view via command key
> +    this._controller.keypress(null, cmdKey, {accelKey: true, shiftKey: true});
> +

nit: same thing here. please move the cmdKey assignment right before this call and preserve only 1 comment line

@@ +427,5 @@
> +      this._controller.window.removeEventListener("tabviewhidden", checkClosed, false);
> +    }
> +
> +    this._groupItemsObject = null;
> +    this._tabItemsObject = null;

I'm wondering: should we also reset `_tabView` and  `_tabViewDoc` as we do in `close()` method?
Opening a new tab does close the TabView, so we might as well. Please check if this is the case.

Comment 61

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v2.1

Thanks for review Andrei.

(In reply to Andrei Eftimie from comment #60)
> I'm wondering: should we also reset `_tabView` and  `_tabViewDoc` as we do
> in `close()` method?
> Opening a new tab does close the TabView, so we might as well. Please check
> if this is the case.
I didn't saw a difference, and I wouldn't change something else that this patch fixes.


http://mozmill-crowd.blargon7.com/#/endurance/report/f892d42cad846376757d785401442830

Comment 62

[Andrei Eftimie]

Comment on attachment 8365033 [details] [diff] [review]
patch_v2.1

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

> I didn't saw a difference, and I wouldn't change something else that this patch fixes.
Great, we won't change these.

Please make the changes I requested in the previous review so we can get this landed.

::: firefox/lib/tabview.js
@@ +70,5 @@
> +
> +    // Add event listener to wait until the tabview has been opened
> +    var self = { opened: false };
> +    function checkOpened() { self.opened = true; }
> +    this._controller.window.addEventListener("tabviewshown", checkOpened, false);

You still didn't move this whole block before the cmdKey assignment.

That assignment and the keypress on that element should be one above the other, and should only have 1 comment (now you have them separated, and the same comment twice)

@@ +125,5 @@
>      function checkClosed() { self.closed = true; }
>      this._controller.window.addEventListener("tabviewhidden", checkClosed, false);
>  
> +    // Close the view via command key
> +    this._controller.keypress(null, cmdKey, {accelKey: true, shiftKey: true});

Same issue, this needs to be right below the cmdKey assignment.
Since the action needs to be _after_ we attach the event listener, we should move the cmdKey assignment here.

Comment 63

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v2.2

I might uploaded the same patch last time.

Comment 64

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

Comment on attachment 8366657 [details] [diff] [review]
patch_v2.2

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

::: firefox/lib/tabview.js
@@ +67,5 @@
>    open : function tabView_open() {
> +    // Add event listener to wait until the tabview has been opened
> +    var self = { opened: false };
> +    function checkOpened() { self.opened = true; }
> +    this._controller.window.addEventListener("tabviewshown", checkOpened, false);

Adding the event listener cannot happen outside of the try/finally. If there is an exception in between, we will leak this listener.

@@ +118,4 @@
>      // Add event listener to wait until the tabview has been closed
>      var self = { closed: false };
>      function checkClosed() { self.closed = true; }
>      this._controller.window.addEventListener("tabviewhidden", checkClosed, false);

Same here as above. Lets get this moved into the try block.

@@ +398,5 @@
>  
> +    // Add event listener to wait until the tabview has been closed
> +    var self = { closed: false };
> +    function checkClosed() { self.closed = true; }
> +    this._controller.window.addEventListener("tabviewhidden", checkClosed, false);

And here again.

::: firefox/tests/endurance/testTabView_SwitchTabs/test1.js
@@ +73,5 @@
> +      }, "TabView is still open.");
> +    }
> +    finally {
> +      controller.window.removeEventListener("tabviewhidden", checkChanged, false);
> +    }

Why do we have to use the listener in a test? All this should be done in the lib.

Comment 65

[Cosmin Malutan, [:cosmin-malutan]]

Comment on attachment 8366657 [details] [diff] [review]
patch_v2.2

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

I updated the library, please let me know how would you like me to approach this in test.

::: firefox/tests/endurance/testTabView_SwitchTabs/test1.js
@@ +73,5 @@
> +      }, "TabView is still open.");
> +    }
> +    finally {
> +      controller.window.removeEventListener("tabviewhidden", checkChanged, false);
> +    }

Then I should add a method in the library which will take a command as an parameter, please see comment 57.

Comment 66

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

In comment 57 I never talked that event listening should happen in the test. We have our abstraction in the libraries to help testers writing their tests. Why should we make it harder for them, and require them to copy and paste the similar code dozen of times?

Comment 67

[Cosmin Malutan, [:cosmin-malutan]]

Thanks for input here, I will add a method change tab in library and use that in test.

Comment 68

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v3.0

I added an method selectTabAtIndex in library and use that in test.

Report:
http://mozmill-crowd.blargon7.com/#/endurance/report/0906e2c8b6f223d6588987bcd157ab4b

Comment 69

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

Comment on attachment 8368575 [details] [diff] [review]
patch_v3.0

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

As we agreed on in the meeting, you should not request review from myself immediately. Only once you get a r+ from your team mates. Please keep this in mind. Thanks

Comment 70

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8368575 [details] [diff] [review]
patch_v3.0

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

::: firefox/lib/tabview.js
@@ +76,5 @@
> +      this._controller.keypress(null, cmdKey, {accelKey: true, shiftKey: true});
> +
> +      assert.waitFor(function () {
> +        return self.opened == true;
> +      }, "TabView is not open.");

We can simply return self.opened right? Also, the messages should be positive, in this case "TabView has been opened". We'll know it's the expected result which has failed.

@@ +128,3 @@
>        assert.waitFor(function () {
>          return self.closed == true;
>        }, "TabView is still open.");

Same here.

@@ +397,5 @@
>      var type = aEventType || "menu";
>  
> +    // Add event listener to wait until the tabview has been closed
> +    var self = { closed: false };
> +    function checkClosed() { self.closed = true; }

checkOpened

@@ +401,5 @@
> +    function checkClosed() { self.closed = true; }
> +    this._controller.window.addEventListener("tabviewhidden", checkClosed, false);
> +
> +    try {
> +      // Close the tabview

Please remove this comment.

@@ +416,5 @@
> +      }
> +
> +      assert.waitFor(function () {
> +        return self.closed == true;
> +      }, "TabView is still open.");

These lines also need updating as mentioned above. The message should be about the new opened tab.

@@ +434,5 @@
> +   *        Index of tab we switch too
> +   * @param {string} aFilter
> +   *        Type of filter to apply
> +   *        (active, title)
> +   *        [optional - default: ""]

Just put this at the parameter name: {string} [aFilter=""]

@@ +445,5 @@
> +    var self = { hidden: false };
> +    function checkChanged() { self.hidden = true; }
> +    this._controller.window.addEventListener("tabviewhidden", checkChanged, false);
> +
> +

There are 2 blank lines here.

@@ +452,5 @@
> +
> +      // Wait for the selected tab to display
> +      assert.waitFor(function () {
> +        return self.hidden == true;
> +      }, "TabView is still open.");

Please update these too.

Comment 71

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v3.1

Here is the updated patch.

(In reply to Andreea Matei [:AndreeaMatei] from comment #70)
> @@ +397,5 @@
> >      var type = aEventType || "menu";
> >  
> > +    // Add event listener to wait until the tabview has been closed
> > +    var self = { closed: false };
> > +    function checkClosed() { self.closed = true; }
> 
> checkOpened
Here we actually check if a tabview has been closed, as opening a new tab should close the active tabview.

Comment 72

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8369904 [details] [diff] [review]
patch_v3.1

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

Just a small update on my part. Please request from Henrik as well with that fixed.

::: firefox/lib/tabview.js
@@ +429,5 @@
> +  /**
> +   * Select a tab view at Index
> +   *
> +   * @param {number} aIndex
> +   *        Index of tab we switch too

nit: "Index of the tab we switch to"

Comment 73

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v3.2

Thanks Andreea for review.

Comment 74

[Andrei Eftimie]

Comment on attachment 8371333 [details] [diff] [review]
patch_v3.2

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

Looks good, but there's a misleading JSdoc comment / default argument logic which I'd like to see addressed before we land this.

::: firefox/lib/tabview.js
@@ +432,5 @@
> +   * @param {number} aIndex
> +   *        Index of the tab we switch to
> +   * @param {string} [aFilter=""]
> +   *        Type of filter to apply
> +   *        (active, title)

This should be "active" and "group"
(title is for the "group" type not for the "tab" type)

@@ +435,5 @@
> +   *        Type of filter to apply
> +   *        (active, title)
> +   */
> +  selectTabAtIndex : function tabView_selectTabAtIndex(aIndex, aFilter) {
> +    var tab =  this.getTabs({filter: aFilter})[aIndex];

This is a bit misleading.
JSdoc says aFilter is optional and will default to empty string.
If you don't set it this will be sent forward as undefined to the getTabs method.
This will not filter at all (which is what we want if we don't set the aFilter argument).
So it will work fine.

But we should either update the JSdoc (to cater for undefined) or the code here to make an empty string as the JSdoc suggests.

Comment 75

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

Comment on attachment 8371333 [details] [diff] [review]
patch_v3.2

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

Additional to Andrei's comments you will get mine. Please see inline.

::: firefox/lib/tabview.js
@@ +67,5 @@
>    open : function tabView_open() {
> +    // Add event listener to wait until the tabview has been opened
> +    var self = { opened: false };
> +    function checkOpened() { self.opened = true; }
> +    this._controller.window.addEventListener("tabviewshown", checkOpened, false);

We can omit the false parameter, which is not necessary at this point.

@@ +82,5 @@
> +      this._groupItemsObject = this._tabViewObject._window.GroupItems;
> +      this._tabItemsObject = this._tabViewObject._window.TabItems;
> +    }
> +    finally {
> +      this._controller.window.removeEventListener("tabviewshown", checkOpened, false);

Same here.

@@ +382,5 @@
>      });
>    },
>  
>    /**
> +   * Open a new tab, this will determine the current tab view to close

Don't mix up the title and descriptions. The second part of this line should get its own line after a blank one.

@@ +390,5 @@
>     *     <dt>menu</dt>
>     *     <dd>The main menu is used</dd>
>     *     <dt>shortcut</dt>
>     *     <dd>The keyboard shortcut is used</dd>
>     *   </dl>

Can you please update the doc for the sub property usage?

@@ +398,5 @@
>  
> +    // Add event listener to wait until the tabview has been closed
> +    var self = { closed: false };
> +    function checkClosed() { self.closed = true; }
> +    this._controller.window.addEventListener("tabviewhidden", checkClosed, false);

We should be able to remove false here.

@@ +423,4 @@
>      }
>  
> +    this._groupItemsObject = null;
> +    this._tabItemsObject = null;

This is mostly duplicated code from the close() method. That's one reason why I think having a callback for the initial action instead of a type string would be better. Can you please file a follow-up bug? This would require a bit amount of work so not sure when we can tackle that. For now it is fine.

::: firefox/tests/endurance/testTabView_SwitchTabs/test1.js
@@ +54,5 @@
>      var allTabs = activeTabView.getTabs();
>  
>      // Select the tab which is NOT active
>      if (activeTab.getNode() === allTabs[0].getNode()) {
> +      activeTabView.selectTabAtIndex(1, "active");

Something is wrong here. Whether its the comment or the filter value.

Comment 76

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v3.3

I updated the patch, thanks for review.

(In reply to Henrik Skupin (:whimboo) [away 02/24 - 02/28] from comment #75)
> @@ +390,5 @@
> >     *     <dt>menu</dt>
> >     *     <dd>The main menu is used</dd>
> >     *     <dt>shortcut</dt>
> >     *     <dd>The keyboard shortcut is used</dd>
> >     *   </dl>
> 
> Can you please update the doc for the sub property usage?
Those are options, and from what I saw in other tests, this is the only way we document them, what do you have in mind for updating this? I can file a new bug for updating all cases. 

Thanks

Comment 77

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

You should look at newer modules like the assertions.js one:
http://hg.mozilla.org/qa/mozmill-tests/file/4e10acdf95ca/lib/assertions.js#l576

Comment 78

[Cosmin Malutan, [:cosmin-malutan]]

Well there is an object sent as parameter with multiple subtypes, here we only have a string that can take two different values. Since this is not something I touched I would't change it in this patch.

Comment 79

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

Oh, right. Ok, so let it as it is.

Comment 80

[Andrei Eftimie]

Comment on attachment 8379696 [details] [diff] [review]
patch_v3.3

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

Looks good.

Checked in: http://hg.mozilla.org/qa/mozmill-tests/rev/905bc0ba7a1e (default)

Comment 81

[Andrei Eftimie]

Cosmin, please check backports, we'll want to land this across branches.

Comment 82

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

(In reply to Henrik Skupin (:whimboo) from comment #75)
> This is mostly duplicated code from the close() method. That's one reason
> why I think having a callback for the initial action instead of a type
> string would be better. Can you please file a follow-up bug? This would
> require a bit amount of work so not sure when we can tackle that. For now it
> is fine.

Cosmin, has this been filed as a bug yet?

Comment 83

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v3.3[release]

I just filled bug 979223 for that.
The patch applies cleanly on aurora and beta, here is the patch for release, and I will return with the patch for esr24.
Thanks

Comment 84

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v3.3[esr24]

Here is the patch for esr24

Reports:
http://mozmill-crowd.blargon7.com/#/endurance/report/a438ea29b921b2e8124749eda9308d5d
http://mozmill-crowd.blargon7.com/#/endurance/report/a438ea29b921b2e8124749eda9303866
http://mozmill-crowd.blargon7.com/#/endurance/report/a438ea29b921b2e8124749eda9307fcb

The reports for release:
http://mozmill-crowd.blargon7.com/#/endurance/report/a438ea29b921b2e8124749eda92eb773
http://mozmill-crowd.blargon7.com/#/endurance/report/a438ea29b921b2e8124749eda92ec774
http://mozmill-crowd.blargon7.com/#/endurance/report/a438ea29b921b2e8124749eda92ec54a

Comment 85

[Andrei Eftimie]

Comment on attachment 8385287 [details] [diff] [review]
patch_v3.3[release]

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

Just a small nit.

::: firefox/lib/tabview.js
@@ +446,5 @@
> +    function checkChanged() { self.hidden = true; }
> +    this._controller.window.addEventListener("tabviewhidden", checkChanged, false);
> +
> +    try {
> +     tab.click();

nit: this is not properly indented
This is good in the original patch.

Comment 86

[Andrei Eftimie]

Comment on attachment 8385304 [details] [diff] [review]
patch_v3.3[esr24]

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

You ran reports but didn't properly checked the results. This patch produces an error.

::: firefox/lib/tabview.js
@@ +131,1 @@
>      } finally {

nit: the finally was moved to a newline in the original patch

@@ +440,5 @@
> +    var tab =  this.getTabs({filter: aFilter})[aIndex];
> +    assert.ok(tab.exists(), "Tab has been found.");
> +
> +    // Add event listener to wait until the tabview has been changed
> +   var self = { hidden: false };

nit: indentation

::: firefox/tests/endurance/testTabView_SwitchTabs/test1.js
@@ +59,1 @@
>      } else {

nit: the else was moved to a newline in the original patch

@@ +61,4 @@
>      }
>  
>      // Wait for the selected tab to display
>      activeTabView.waitForClosed();

You forgot to remove this call.
This has been removed in the original patch.

This fails since this function doesn't exist anymore.

Comment 87

[Andrei Eftimie]

Landed backports for:
http://hg.mozilla.org/qa/mozmill-tests/rev/f62b007b19ee (mozilla-aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/ca037aa84c8f (mozilla-beta)

Comment 88

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v3.3[release]

Fixed the nit.

Comment 89

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v3.3[esr24]

I fixed the backport patch for esr24
http://mozmill-crowd.blargon7.com/#/endurance/report/a438ea29b921b2e8124749eda952a505

Comment 90

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v3.4[esr24]

I uploaded the wrong patch, here is the fixed one

Comment 91

[Andrei Eftimie]

Comment on attachment 8385952 [details] [diff] [review]
patch_v3.3[esr24]

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

Looks good.

Landed both:
http://hg.mozilla.org/qa/mozmill-tests/rev/d7418b387a3f (mozilla-release)
http://hg.mozilla.org/qa/mozmill-tests/rev/0c6f2c094a2a (mozilla-esr24)