711360 - Mozmill endurance test for opening new window

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Maniac Vlad Florin (:vladmaniac)]

Develop a new mozmill test for opening a closing multiple windows. 

The test will make usage of entities to open a given number of windows, then close them one by one.

Comment 1

[Maniac Vlad Florin (:vladmaniac)]

Dave, what exactly do we want to test here? Is it going to be similar with the opening of a new tab test or do we want something different?

Comment 2

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

I would suggest this is very similar to the new tab test.

Comment 3

[Maniac Vlad Florin (:vladmaniac)]

Then will open a new window (entities number of windows to be exact) and close them after

Comment 4

[Maniac Vlad Florin (:vladmaniac)]

Attachment: wip 1.0

I've tried to get this done but it seems that we are not handling windows well - so sometimes we get failures for opening and closing windows - 

One possible dependency bug is already been filed for this bug 628576 

My WIP patch does not fail now but it uses "sleep" which we all know its bad - have any other ideas?

Comment 5

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

I would prefer to see some comments in the code which explain what steps will be performed. It's very hard to understand everything in those two nested loops otherwise.

Comment 6

[Maniac Vlad Florin (:vladmaniac)]

Attachment: wip 2.0

More comments added

Comment 7

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #5)
> I would prefer to see some comments in the code which explain what steps
> will be performed. It's very hard to understand everything in those two
> nested loops otherwise.

Added a new patch, hopefully more specific in wording than last one - asked feedback from you since you have stepped in (thanks, btw :) ) 

I'll ask Anthony's opinion afterwards.

Comment 8

[Maniac Vlad Florin (:vladmaniac)]

Attachment: wip 2.1

Remus helped building a patch using handleWindow() from utils.js shared module - it's an alternative of wip patch 2.0 but it does not fix our issues 

It uses the known workaround with "controller.sleep(0)" form handleWindow() helper function

Comment 9

[Maniac Vlad Florin (:vladmaniac)]

Comment on attachment 586967 [details] [diff] [review]
wip 2.0

Changing feedback request for Anthony

Comment 10

[Maniac Vlad Florin (:vladmaniac)]

Comment on attachment 586985 [details] [diff] [review]
wip 2.1

Changing feedback request for Anthony

Comment 11

[u279076]

Comment on attachment 586967 [details] [diff] [review]
wip 2.0

I'll review each patch on it's own before doing a comparison one way or another.

>+* The Initial Developer of the Original Code is Mozilla Foundation.
>+* Portions created by the Initial Developer are Copyright (C) 2011
>+* the Initial Developer. All Rights Reserved.

nit: Year is now 2012
 
>+      // Get the controller of the newly opened window
>+      windows[windowCounter] = mozmill.getBrowserController();
>+
>+      // Incremenent the window counter
>+      windowCounter++;

I think you can combine these two lines... 
windows[windowCounter++] should increment windowCounter after it is used.

Seems like a decent solution so far. Using sleep() *should* be a last resort. That said, if sleep() works and there is no way around it, let's use it.

Comment 12

[u279076]

Comment on attachment 586985 [details] [diff] [review]
wip 2.1

>+* The Initial Developer of the Original Code is Mozilla Foundation.
>+* Portions created by the Initial Developer are Copyright (C) 2011
>+* the Initial Developer. All Rights Reserved.

nit: 2012

>+function setupModule() {
>+  controller = mozmill.getBrowserController();
>+  newController = null;

I don't see newController being used anywhere in the test...

>+      // Open a new window
>+      controller.mainMenu.click("#menu_newNavigator");
>+      controller.sleep(0);

So without this sleep we fail?

>+      controllers[i] = utils.handleWindow("type", 'navigator:browser', undefined, false);
>+      i++;

controllers[i++] should work.

>+    // Trying to close windows one by one
>+    for (var i = 0; i < controllers.length; i++) {
>+      // Do not close the first window
>+        controllers[i].window.close();
>+        mozmill.utils.waitFor(function () {
>+          return controllers[i].window.closed;
>+        }, "Window has been closed.");
>+    }

I'm wondering if this should be done in its own enduranceManager.loop(). In other words, we should be recording endurance data for the closing of the windows as well.

Comment 13

[u279076]

The major difference I see between the two patches is:
* 2.0 clicks the New Window menu item then sleeps for 1.5 seconds
* 2.1 clicks the New Window menu item then utils.handleWindow()

2.1 is "safer" in my opinion.

Comment 14

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #13)
> The major difference I see between the two patches is:
> * 2.0 clicks the New Window menu item then sleeps for 1.5 seconds
> * 2.1 clicks the New Window menu item then utils.handleWindow()
> 
> 2.1 is "safer" in my opinion.

We will go for 2.1 then - let's see if we can optimize the patch more given the existent limitations

Comment 15

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v1.0 - initial patch

Initial patch

Comment 16

[Maniac Vlad Florin (:vladmaniac)]

> nit: 2012
> 
nit fixed in initial patch v1.0 
> 
> I don't see newController being used anywhere in the test...
Correct. Removed
> 
> >+      // Open a new window
> >+      controller.mainMenu.click("#menu_newNavigator");
> >+      controller.sleep(0);
> 
> So without this sleep we fail?
Yes. Please see utils.js shared module and look for the handleWindow() helper function
> 
> >+      controllers[i] = utils.handleWindow("type", 'navigator:browser', undefined, false);
> >+      i++;
> 
> controllers[i++] should work.
Agreed. Changed in patch. 

> I'm wondering if this should be done in its own enduranceManager.loop(). In
> other words, we should be recording endurance data for the closing of the
> windows as well.

I won't create another enduranceManager.loop for this. Also enduranceManager.run(), meaning the handler of iterations can gather data - we have tests which do not use loop and still can gather data. 

the missing here was that we did not add a checkpoint in the first place for the closing part - added now in the latest patch. 

Thanks Anthony

Comment 17

[u279076]

Comment on attachment 587622 [details] [diff] [review]
patch v1.0 - initial patch

>+    controllers[i++] = utils.handleWindow("type", 'navigator:browser', undefined, false);

I'm not sure an iterator is needed in the above code. Also, it creates ambiguity with the iterator below. Instead, I recommend using array.push().

>+    // Close windows one by one
>+    for (var i = 0; i < controllers.length; i++) { 
>+      enduranceManager.addCheckpoint("Close a window");

Comment 18

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v2.0

Done - changed the "i" array index to use arrayName.push(element) instead. 

However, in the second part of code I still need to use the for because I need to iterate through the controllers array in order to close all the windows properly, one by one, so we can get endurance data about closing one window at a time. array.push does only append a new element at the end of the array and return the new index

Comment 19

[u279076]

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

>+      controller.mainMenu.click("#menu_newNavigator");
>+      // XXX: Bug 628576 - need to find an event handler for when a window

nit: separate with a newline

Assuming that is fixed, please send this over to Dave Hunt for final review.

Comment 20

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v3.0

Nit fixed - Over to Dave for r

Comment 21

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

Comment on attachment 592624 [details] [diff] [review]
patch v3.0

>+    // Close windows one by one
>+    for (var i = 0; i < controllers.length; i++) { 

We should use enduranceManager.loop again here to close all the windows.

Comment 22

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v4.0

Fixed

Comment 23

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

Comment on attachment 592656 [details] [diff] [review]
patch v4.0

>+    enduranceManager.loop(function () {
>+      if (count != controllers.length) {
>+        // Close windows one by one
>+        enduranceManager.addCheckpoint("Close a window"); 
>+        controllers[count].window.close();
>+
>+        mozmill.utils.waitFor(function () {
>+          return controllers[count].window.closed;
>+        }, "Window has been closed.");
>+
>+        count++; 
>+        enduranceManager.addCheckpoint("A window has been closed");
>+      }
>+    }); 

Why are you using a count variable? You should just use the enduranceManager.currentEntity.

Comment 24

[Maniac Vlad Florin (:vladmaniac)]

> 
> Why are you using a count variable? You should just use the
> enduranceManager.currentEntity.

Window count starts from 0 in Firefox (and tabs). Entity index starts from 1. So if we were to be equivalent with the window index, we'd have to use enduranceManager.currentEntity - 1 in the array index, and if so we skip closing the last opened window. I do not like using the count as well, but it feels more elegant to me because of the indexing. 

Surely, this was only an answer on the "why" question. If you want me to use currentEntity, no problem :)

Comment 25

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

As mentioned on IRC please use currentEntity so we are closing all the expected windows rather than all the opened windows. Either way all windows should be closed.

When running this locally I often saw windows remaining open. This seems intermittent and is not always the same window left open. I suspect an issue with window.close and window.closed. Please investigate this and raise a blocking bug if needed.

Comment 26

[Maniac Vlad Florin (:vladmaniac)]

We might be closing windows too fast. 

In any case 'gBroswer' is loaded in every window and the backed API does not show 
that any windows are left unclosed 

 nsWindowMediator::GetMostRecentWindow(const PRUnichar* inType, nsIDOMWindow** outWindow)

What I did is used and Enumerator to iterate thourgh all windows (enum.getNext()) and wait for 'gBroswer' in window. This did not show me any windows being left opened. 

Well - I will go on with this investigation 

As a final final solution, I suggest we do a final check at the end of the test to see if there is a window still opened, and if there is, force close. This way we will be sure we will not fail. (I mean at least two windows opened)

Comment 27

[Maniac Vlad Florin (:vladmaniac)]

This needs to be refurbished due to bug 695480 

The endurance test development has been put on hold for the moment, but I think we can ship one more in.

Comment 28

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

Yes, let's try to get this one in.

Comment 29

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v5.0

I think we got it right this time. 

Added an event listener for closing the each and every window. 
Tested patch on Firefox 15, which is our first land-target and using Mac OS x
http://mozmill-crowd.blargon7.com/#/endurance/report/c2b72632f20450b6d99d14c709a4344c 

Dave, would appreciate if you test the patch locally yourself also and see the actual behavior and data. 

Thanks!

Comment 30

[Maniac Vlad Florin (:vladmaniac)]

Please also consider looking into some test results for Ubuntu Linux 
http://mozmill-crowd.blargon7.com/#/endurance/report/c2b72632f20450b6d99d14c709a4406a

Comment 31

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

Comment on attachment 622326 [details] [diff] [review]
patch v5.0

I ran this with 2 iterations and 20 entities and a window was orphaned so the issue still exists.

Comment 32

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Dave Hunt (:davehunt) from comment #31)
> Comment on attachment 622326 [details] [diff] [review]
> patch v5.0
> 
> I ran this with 2 iterations and 20 entities and a window was orphaned so
> the issue still exists.

Do you see any errors? Or its just visual? 
I'll try myself too, clearly

Comment 33

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

No errors/failures, but the window is left open and the tests continue to run against the new window. Perhaps we need an assertion on the window count so that we get a failure if a window is not closed.

Comment 34

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

We need code in teardownModule which makes sure that all other windows are closed. If not they have to be forced to close.

Comment 35

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

I agree with Henrik, however shouldn't this test fail if there are windows left open? All this test does is open and close windows, so if that fails then the endurance results are compromised.

Comment 36

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

Yes, absolutely. If there are no checks yet, please add those.

Comment 37

[Maniac Vlad Florin (:vladmaniac)]

I am sorry guys but I don't see this orphan window locally nor on mac or linux, ran with 2 iterations 20 entities. 

Dave, if you are talking about a window being left unclosed, and no more than one, well that's the default browser window and we cannot close that one. the test is about open and close new windows. 

On the other hand, if you see more than one window left opened, we have a problem. Unfortunately, I cannot see it.

Comment 38

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Dave Hunt (:davehunt) from comment #33)
> No errors/failures, but the window is left open and the tests continue to
> run against the new window. Perhaps we need an assertion on the window count
> so that we get a failure if a window is not closed.

You cannot practically assert on the closed window counts because after closing the window object is destroyed. 

The thing we could do is use the 'count' variable but that is a band-aid, not a true assertion of the event. 

The waitFor in the test is responsible with checking that the window is closed, that is why I asked if we see errors in the first place...

Comment 39

[Maniac Vlad Florin (:vladmaniac)]

Let see if bug 753757 can help us

Comment 40

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

I'm not talking about the main window being left open, I am talking about one of the windows opened by the test being left open. Like I say I saw this on my first and only run of this patch. I will try the patch again now, but even if I'm unable to replicate the issue of a window being orphaned I would be much more comfortable approving this if we had an assert to ensure that if this did recur we would have a test failure.

Comment 41

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Dave Hunt (:davehunt) from comment #40)
> I'm not talking about the main window being left open, I am talking about
> one of the windows opened by the test being left open. Like I say I saw this
> on my first and only run of this patch. I will try the patch again now, but
> even if I'm unable to replicate the issue of a window being orphaned I would
> be much more comfortable approving this if we had an assert to ensure that
> if this did recur we would have a test failure.

Assert is on its way Dave, that is no problem

Comment 42

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v6.0

Added the extra-check: 
* after we close all windows, we assert for the count variable and the length of the controller array to make sure we did that exact amount of "window.close()" operations. 

Patch was tested with all green result: 
http://mozmill-crowd.blargon7.com/#/endurance/report/f87375a634b1a5ba746e5f763a00c9f4

Comment 43

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

Comment on attachment 623642 [details] [diff] [review]
patch v6.0

Sorry to steal the review request Dave, but there is something to say here.

>+function teardownModule() {
>+  tabBrowser.closeAllTabs();
>+}

Ensure to force-kill all open browser windows excluding the last one.

>+      // XXX: Bug 628576 - need to find an event handler for when a window
>+      //                   is truly closed.
>+      controller.sleep(0);

This has been fixed. Please update the test accordingly. Also not sure why this affects this code when we open new windows.

>+      if (count != controllers.length) {

Please don't do this because we could end-up in an endless loop if a window doesn't close and the code below updated. Better iterate throw controllers.

>+        // Close windows one by one
>+        enduranceManager.addCheckpoint("Close a window"); 
>+        controllers[count].window.addEventListener("unload", function () { closed = true; }, false);
>+        controllers[count].window.close();
>+        // Make sure the window is closed
>+        controllers[count].waitFor(function () {
>+          return closed;
>+        }, "Window has been closed.");

See above. Our handling has been changed. Also don't use waitFor in this loop, or if it is necessary transform it into an expect assertion. We should not bail out earlier. 

>+    // Make sure we have no windows left opened 
>+    assert.equal(controllers.length, count, "Windows have been closed");    

With the changes above we wouldn't need a final assert.

Comment 44

[Maniac Vlad Florin (:vladmaniac)]

AFAIK windows have ids. If I know right, then I will want to re-think this to use the ids of the windows so we will exactly know at every moment with which window we are working

Comment 45

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Maniac Vlad Florin (:vladmaniac) from comment #44)
> AFAIK windows have ids. If I know right, then I will want to re-think this
> to use the ids of the windows so we will exactly know at every moment with
> which window we are working

Just FYI, I have to postpone this a bit until I have any free time left. 
Failures, imminent refactoring patches and smoketests coverage are P1 at the moment. 

Dave, thanks for understanding 
As in comment 44, I am planning to re-think this from scratch

Comment 46

[Maniac Vlad Florin (:vladmaniac)]

Attachment: wip patch

This is a WIP patch 

Dumps data into the console to demonstrate we are now closing all the windows properly now and none remain orphaned. 
If approved, a test final patch will follow and we could make usage of this test

Dave, mind testing this and give your feedback?

Comment 47

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

Comment on attachment 629185 [details] [diff] [review]
wip patch

When testing all windows opened by the test were successfully closed.

+      controller.mainMenu.click("#menu_newNavigator");
+      // XXX: Need this sleep, otherwise we would not catch the controller and id 
+      //      of the newly opened window
+      controller.sleep(100);

Why do we need this sleep? Could we use a wait instead?

+      if(windowIds[count] != firstWindowId) {

The first window is not pushed to the array. If it appears in the array it would mean that the first window was not opened and therefore we should probably fail the test.

+      count++;

Could you use enduranceManager.currentEntity for keeping track of the open window IDs and subsequent closing of them?

Comment 48

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

Comment on attachment 629185 [details] [diff] [review]
wip patch

>+  // Get the id of the initial window
>+  firstWindowId = mozmill.utils.getWindowId(controller.window);

Why do we need this?

>+      // Open a new window
>+      controller.mainMenu.click("#menu_newNavigator");
>+      // XXX: Need this sleep, otherwise we would not catch the controller and id 
>+      //      of the newly opened window
>+      controller.sleep(100);      

Never sleep here. Compare the number of open windows before and after this action.

>+      // Save the controllers and windowIds of the opened windows in arrays
>+      controllers.push(currentController);
>+      windowIds.push(windowId);

You do not have to save both. Save the controller or the window ids. given that you want to close the windows it should be the controller objects. The id you can always reconstruct from its window.

>+      if(windowIds[count] != firstWindowId) {

Please never use 'count' for an index variable. Also always use triple operators.

>+      enduranceManager.addCheckpoint("A window has been closed");

You should include the window id of the window which has been closed in this iteration.

Comment 49

[Maniac Vlad Florin (:vladmaniac)]

> 
> >+      // Open a new window
> >+      controller.mainMenu.click("#menu_newNavigator");
> >+      // XXX: Need this sleep, otherwise we would not catch the controller and id 
> >+      //      of the newly opened window
> >+      controller.sleep(100);      
> 
> Never sleep here. Compare the number of open windows before and after this
> action.
> 
 Can you please provide a code snippet with your feedback on this one because I put the sleep there for the reason that it needs to wait a bit before the window gets registered and the "activate" event gets fired. Also, I cannot listen for any events because I cannot retrieve the controller of the newly opened window before performing the menu click.

Comment 50

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

We already have code like that in our newWindow.js test.

Comment 51

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v7.0

Fixed 

Note: Added a '0' entry in controllers[0] because entity count starts from 1 and array index from 0. This way we have concordance between them.

Comment 52

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #50)
> We already have code like that in our newWindow.js test.

Exactly what I needed, thanks Henrik!

Comment 53

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

Comment on attachment 630537 [details] [diff] [review]
patch v7.0

>+    // Insert '0' in the first position of the controller array 
>+    controllers.push(0);

A comment has to explain code. This one just reflects what the code already tells me. What is the purpose of that line? It could have been done immediately with the array instantiation.

>+      enduranceManager.addCheckpoint("Open a new window");
>+
>+      // Open a new window
>+      controller.mainMenu.click("#menu_newNavigator");

I don't see a reason why we have to add comments to each single line of code. The checkpoint above already gives that information.

>+      // Make sure we have opened a new window
>+      controller.waitFor(function () {
>+        var windows = mozmill.utils.getWindows("navigator:browser");
>+
>+        return (windows.length === controllers.length + 1);
>+      }, "A new window has been opened");

I think it would help a lot when you also add the iteration (window count) to the message. It makes it easier later for debugging if failures occur.

>+      // Get the controller of the newly opened window and add it to the array
>+      var currentController = utils.handleWindow("type", 'navigator:browser', 
>+                                                 undefined, false);      
>+      controllers.push(currentController);

Same here with the comment. For such simple code we should stop adding comments. Please also revise later comments in this patch.

>+      var windowId = mozmill.utils.getWindowId(controllers[enduranceManager.currentEntity].window);

Please create a temporarily variable for the controller retrieved from the array.

Comment 54

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v8.0

Comment 55

[Maniac Vlad Florin (:vladmaniac)]

Comment on attachment 633102 [details] [diff] [review]
patch v8.0

I am using controllers.push(0) because I need only controllers[0] to be '0' or another value except a controller object

I need this because array indexes starts with 0 in JS and enduranceManager.entities list starts from 1. 

Otherwise, fixed all

Comment 56

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

Comment on attachment 633102 [details] [diff] [review]
patch v8.0

>+    var controllers = new Array(); 

Forgot that you should avoid using 'new Array()' as best as possible. Instead simply do 'var controllers = [ ]'.

>+    // We are not using the first position in the array. Placing '0'
>+    controllers.push(0);

This is a no-op and inserts an useless entry in the array. Please remove it. Instead substract 1 from the iteration count to start with 0.

>+      // Make sure we have opened a new window
>+      controller.waitFor(function () {
>+        var windows = mozmill.utils.getWindows("navigator:browser");
>+
>+        return (windows.length === controllers.length + 1);
>+      }, "Window number '" + enduranceManager.currentEntity + "' has been opened");

Again the above comment is a duplicate from the waitFor message. Please kill that.

>+      // Close windows one by one
>+      enduranceManager.addCheckpoint("Close a window"); 

Same here.

Comment 57

[Maniac Vlad Florin (:vladmaniac)]

Tried substracting 1 from the entity count and windows are left orphaned on closing and the test fails 

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

Do you have any idea why this might happen?

Comment 58

[Maniac Vlad Florin (:vladmaniac)]

Here is a pastebin of the code 

http://themaniak.pastebin.mozilla.org/1666776

Comment 59

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

(In reply to Maniac Vlad Florin (:vladmaniac) from comment #57)
> Tried substracting 1 from the entity count and windows are left orphaned on
> closing and the test fails 
> 
> http://mozmill-crowd.blargon7.com/#/endurance/report/
> 8bf3fa70d3d9a46d3e7617383b0fb8be
> 
> Do you have any idea why this might happen?

This failure doesn't look like it has anything to do with subtracting 1 from currentEntity. As far as I can tell that is only affecting the failure message and not the assertion.

Comment 60

[Maniac Vlad Florin (:vladmaniac)]

 As far as I can tell that is only affecting the failure
> message and not the assertion.

We use enduranceManager.currentEntity when iterating through the controller array when closing the windows. This is where the problem is, and, its very strange.

Comment 61

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

(In reply to Maniac Vlad Florin (:vladmaniac) from comment #60)
>  As far as I can tell that is only affecting the failure
> > message and not the assertion.
> 
> We use enduranceManager.currentEntity when iterating through the controller
> array when closing the windows. This is where the problem is, and, its very
> strange.

That's not what your linked report shows. It shows "Window number '0' has been opened" as the failure message, which as far as I can tell would relate to opening the first new window.

Comment 62

[Maniac Vlad Florin (:vladmaniac)]

What is very strange is that if I do 

controllers.push(0), meaning insert a '0' in the first position of the controllers array, the test behaves normally. If I comment that line and just substract 1 from the enduranceManager.currentEntity the test fails, it lefts windows unclosed. That's why the error 

"Window number '0' has been opened" 

is there, because windows.length is no longer equal with controllers.length because windows are left opened. 

It will take some time for me to figure out this, but if you have the time to try the code see what I mean, maybe you can help a bit

Comment 63

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

I've just tested this myself and it works fine. You don't need to push an initial entry for controllers, but when comparing the number of windows you need to ensure that windows.length is equal to controllers.length + 2 (one is the original window, and one is the new window). Then when closing the windows use currentEntity - 1 to get the correct index.

Comment 64

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Dave Hunt (:davehunt) from comment #63)
> I've just tested this myself and it works fine. You don't need to push an
> initial entry for controllers, but when comparing the number of windows you
> need to ensure that windows.length is equal to controllers.length + 2 (one
> is the original window, and one is the new window). Then when closing the
> windows use currentEntity - 1 to get the correct index.

++ Dave, thanks for this one

Comment 65

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v9.0

We got it right this time, thanks to Dave's help

Comment 66

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

Comment on attachment 637084 [details] [diff] [review]
patch v9.0

>+        return (windows.length === controllers.length + 2);
>+      }, "Window number '" + (enduranceManager.currentEntity - 1) + "' has been opened");

Why are you subtracting 1 from currentEntity here?

>+    enduranceManager.loop(function () {
>+      // Close windows one by one
>+      enduranceManager.addCheckpoint("Close a window");

In comment 56, Henrik suggested removing the comment. I tend to agree as the checkpoint is clear enough.

>+      // Make sure the window is closed
>+      controller.waitFor(function () {
>+        return !mozmill.controller.windowMap.contains(windowId);
>+      }, "Window '" + windowId + "' has been closed.");     
>+      enduranceManager.addCheckpoint("Window '" + windowId + "' has been closed");

This comment would also seem redundant given the waitFor and checkpoint messages.

Comment 67

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v10.0

fixed in this patch - thanks dave - forgot about the comments since this was left a bit in the backlog

Comment 68

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

Comment on attachment 637095 [details] [diff] [review]
patch v10.0

Looks good. Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/d997b0b91b92 (default)

Let's see how it goes before transplanting it.

Comment 69

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

I think it's worth transplanting the endurance tests to all the major branches but not esr branches.

Comment 70

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

This test has been failed:
http://mozmill-ci.blargon7.com/#/endurance/report/23e194f1171aa4baf774928b2605ce7e

Also because of:

this.controller.window.gBrowser.browsers is undefined 

Backed out the test:
http://hg.mozilla.org/qa/mozmill-tests/rev/ee9e9f593f35

Comment 71

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

Thanks for backing this out Henrik. It looks like it passed on Mac (with a big memory spike for that test), but failed on Ubuntu. I suspect it was backed out before there was a chance for it to run on Windows.

Vlad: Can you test this against all platforms and come up with a fix?

Comment 72

[Maniac Vlad Florin (:vladmaniac)]

I was testing it on Ubuntu and worked - could be something intermittent. 
Lets see 

I will run it against Linux 11.10, Mac Lion and a win XP VM.

Comment 73

[Maniac Vlad Florin (:vladmaniac)]

I am not able to reproduce this locally on Linux, where the failure was first seen, ran this 20 times - the last report is 

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

I ran them also with mozmill 1.5.12 and 1.5.13 just for sanity 

I will run tests with more iterations I am sensing this will cause our problem

Comment 74

[Maniac Vlad Florin (:vladmaniac)]

10 iterations do not get me the error 

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

Are you still seeing this guys?

Comment 75

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

We're not seeing it because it's been disabled. Looking at your results, it appears that the memory usage climbs throughout, so it may be that we're hitting a limit for the VM used by Mozmill CI. What if you increase the iterations, or perhaps otherwise consume resources on the machine running the tests?

Comment 76

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Dave Hunt (:davehunt) [Away until July 3rd] from comment #75)
> We're not seeing it because it's been disabled. Looking at your results, it
> appears that the memory usage climbs throughout, so it may be that we're
> hitting a limit for the VM used by Mozmill CI. What if you increase the
> iterations, or perhaps otherwise consume resources on the machine running
> the tests?

I will increase iterations to 20, and I will build firefox or something locally. that will definitely increase resource consumption

Comment 77

[Maniac Vlad Florin (:vladmaniac)]

Caught the failure intermittently with both heavy system loading by building Firefox locally 

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

and without system load but 20 iterations 
Now we need to figure out the cause.

Comment 78

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

I would've guessed at a memory resource issue, but your failure indicates that the first window couldn't be opened... what did you see with this running? Did Firefox hang?

Comment 79

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Dave Hunt (:davehunt) from comment #78)
> I would've guessed at a memory resource issue, but your failure indicates
> that the first window couldn't be opened... what did you see with this
> running? Did Firefox hang?

Yes, in this case Firefox hangs with a number of windows being left opened. Because of that, 
the statement in the waitFor is false all the time

Comment 80

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

Interesting. If the windows are successfully opened then why is the failure "Window number '1' has been opened"? I would expect that with that failure, the first window would not be opened.

Comment 81

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

I would say dump windows.length and controllers.length to the console. I believe something is wrong with windows.length.

Comment 82

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #81)
> I would say dump windows.length and controllers.length to the console. I
> believe something is wrong with windows.length.

Agree, was doing that as we speak, its strange how its degrading when iterations are increased.

Comment 83

[Maniac Vlad Florin (:vladmaniac)]

 WINDOWS.LENGTH == 11
 CONTROLLERS.LENGTH + 2 == 11

 WINDOWS.LENGTH == 3 // FAIL - ONE WINDOW IS LEFT ORPHANED
 CONTROLLERS.LENGTH + 2 == 2

 WINDOWS.LENGTH == 4 // FAIL - TWO WINDOWS ARE LEFT ORPHANED 
 CONTROLLERS.LENGTH + 2 == 2

Test report 
http://mozmill-crowd.blargon7.com/#/endurance/report/4c461f9adf1253771fc64556670aed8b

Comment 84

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

It would appear that windows.length is incorrect as this is during the opening of the first additional window. Are there other windows open? What do they contain?

Comment 85

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Dave Hunt (:davehunt) from comment #84)
> It would appear that windows.length is incorrect as this is during the
> opening of the first additional window. Are there other windows open? What
> do they contain?

Not sure what you mean, but windows.length should equal 2, not 3 or 4 at the moment the new iteration begins and a new window is opened. because one and then two are left orphaned, windows.length is higher than expected

Comment 86

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

Right. It wasn't clear that this was not the first iteration. If this is an issue with closing windows then I would expect the test to fail when the windows are being closed. Is the current wait not suitable..?

Comment 87

[Maniac Vlad Florin (:vladmaniac)]

 (In reply to Dave Hunt (:davehunt) from comment #86)
> Right. It wasn't clear that this was not the first iteration. If this is an
> issue with closing windows then I would expect the test to fail when the
> windows are being closed. Is the current wait not suitable..?

Probably so but if this is the case, then we have a problem with this line 

  return !mozmill.controller.windowMap.contains(windowId); 

I'll try to replace it with a band-aid and see where it gets us

Comment 88

[Maniac Vlad Florin (:vladmaniac)]

Its clear now 

The condition mozmill.controller.windowMap.contains(windowId); is not enough, that why we sometimes leave windows opened. We need to somehow force the test to close all the windows, and wait for the enduranceManager.entities closed windows as a total.

Comment 89

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

Not sure what you mean, Vlad. Calling 'controller.window.close()' is doing a hard-close of the window. If that leaves the window behind then it's clear a bug in Firefox.

But looking at the failure we do not reach this line of code. We already fail before.

Comment 90

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #89)
> Not sure what you mean, Vlad. Calling 'controller.window.close()' is doing a
> hard-close of the window. If that leaves the window behind then it's clear a
> bug in Firefox.
> 
> But looking at the failure we do not reach this line of code. We already
> fail before.

As I said in comment 88, the condition in the closing waitFor is not enough, it will be true even if all windows are not close. because windows are left opened, the array 'windows' has more elements than expected. That's why windows.length != controllers.length + 2 in the assert message so that's why the failure occurs. Hope I found my words correctly this time :)

Comment 91

[Maniac Vlad Florin (:vladmaniac)]

And yes, a bug in Firefox is not to be excluded at the moment

Comment 92

[Maniac Vlad Florin (:vladmaniac)]

I've digged into this last week. Tried: 

* Replaced waitFors in the test with event listeners for "load", "unload" events for loading and closing the windows. 
* Deleted all extra checks in the test and left it as minimal as possible so that it only opens and closes windows without any check 

In both cases, for iteration count > 10, the test degrades in time and leaves windows opened 
Maybe window.close() is not that reliable?

Comment 93

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v11.0

* fixed this test by replacing controller.waitFor with mozmill.utils.waitFor when closing windows. controller.waitFor does not apply here since we are polling for a value from Mozmill utils API

Comment 94

[Maniac Vlad Florin (:vladmaniac)]

This was tested under the normal environment conditions and its a PASS: 
http://mozmill-crowd.blargon7.com/#/endurance/report/d87d47fd1034f072b9bece6ee64a0400
and under heavy loaded environment conditions and double the usual iterations and entities number (meaning 20 of each) - and still a PASS: 
http://mozmill-crowd.blargon7.com/#/endurance/report/d87d47fd1034f072b9bece6ee64b639c

Note: This patch will no apply cleanly for mozilla-esr10

Comment 95

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

I have tested the patch and can confirm that with 50 entities and 5 iterations no windows were orphaned, however as far as I can tell controller.waitFor simply calls utils.waitFor... I'd like to get Henrik's thoughts on this.

Comment 96

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

Comment on attachment 652466 [details] [diff] [review]
patch v11.0

(In reply to Maniac Vlad Florin (:vladmaniac) from comment #93)
> * fixed this test by replacing controller.waitFor with mozmill.utils.waitFor
> when closing windows. controller.waitFor does not apply here since we are
> polling for a value from Mozmill utils API

Huh? Can you please explain this in detail? I don't understand this comment at all. As Dave has mentioned controller.waitFor is just a pass-through to mozmill.utils.waitFor(). So I really can't give a r+ for it.

Comment 97

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #96)
> Comment on attachment 652466 [details] [diff] [review]
> patch v11.0
> 
> (In reply to Maniac Vlad Florin (:vladmaniac) from comment #93)
> > * fixed this test by replacing controller.waitFor with mozmill.utils.waitFor
> > when closing windows. controller.waitFor does not apply here since we are
> > polling for a value from Mozmill utils API
> 
> Huh? Can you please explain this in detail? I don't understand this comment
> at all. As Dave has mentioned controller.waitFor is just a pass-through to
> mozmill.utils.waitFor(). So I really can't give a r+ for it.

I was wrongly assuming they work different, still: 

* controller.waitFor causes the test to fail
* mozmill.utils.waitFor fixes the problem 

Dave also confirmed this in comment 95. 
So it seems the controller.waitFor has a problem, a probably we should have a bug for it. 

More, while seeking for an answer I found this fix, which also replaces controller.waitFor with mozmill.utils.waitFor 

https://bug761569.bugzilla.mozilla.org/attachment.cgi?id=630768

-  controller.waitFor(function () {
+  mozmill.utils.waitFor(function () {

Why the replace if they are the same?

Comment 98

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

Hm, good question. The controller is based on a window and when destroying a window it looks like that somehow we can't make use of controller.waitFor. I have no idea why. I think it can only be related to the already dead window property even that it is not used in this method. So I will check the patch again.

Comment 99

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

Comment on attachment 652466 [details] [diff] [review]
patch v11.0

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

Ok, I think it looks fine except some nits. With those fixed I'm fine. So next time only ask for review from Dave.

::: tests/endurance/testTabbedBrowsing_OpenNewWindow/test1.js
@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> + 

nit: unnecessary blank.

@@ +10,5 @@
> +
> +function setupModule() {
> +  controller = mozmill.getBrowserController();
> +  enduranceManager = new endurance.EnduranceManager(controller);
> + 

nit: unnecessary blank.

@@ +35,5 @@
> +        return (windows.length === controllers.length + 2);
> +      }, "Window number '" + enduranceManager.currentEntity + "' has been opened");
> +
> +      var currentController = mozmill.getBrowserController();
> +      controllers.push(currentController);

Why do we need the currentController variable? You can directly add the new entry to the controllers array.

@@ +42,5 @@
> +
> +    enduranceManager.loop(function () {
> +      enduranceManager.addCheckpoint("Close a window");
> +      var currentEntity = enduranceManager.currentEntity;
> +      var controller = controllers[currentEntity - 1];

why do we need the currentEntity variable? It's used only once and we will not reach the line limit.

Comment 100

[Maniac Vlad Florin (:vladmaniac)]

Attachment: patch v12.0

* nits fixed 
* removed trailing whitespaces 
* tested only with default branch for now

Comment 101

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

Comment on attachment 653694 [details] [diff] [review]
patch v12.0

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/2ade89438d77

Comment 102

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

Dave, can you please land the patch on older branches? Looks like it slipped through.

Comment 103

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

It didn't slip through, it's still on my list. I haven't landed it due to concerns on bug 784793.

Comment 104

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

(In reply to Henrik Skupin (:whimboo) from comment #98)
> Hm, good question. The controller is based on a window and when destroying a
> window it looks like that somehow we can't make use of controller.waitFor. I
> have no idea why. I think it can only be related to the already dead window
> property even that it is not used in this method. So I will check the patch
> again.

It appears that by using mozmill.utils.waitFor we have introduced a potential disconnect when the closing of the windows exceeds the timeout. It appears that controller.waitFor includes a message back to the broker:

broker.pass({'function':'controller.waitFor()'});

Nothing like this appears to be present in mozmill.utils.waitFor. I suspect this may also affect the waitFor methods in assertions.js from bug 787020.

Comment 105

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

Right, please file a new bug so we can get this issue fixed in Mozmill proper.

Comment 106

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

On investigating bug 795579 I discovered that the manifest file entry is incorrect.

The manifest lists:
[include:testTabView_OpenNewWindow/manifest.ini]

Where it should be:
[include:testTabbedBrowsing_OpenNewWindow/manifest.ini

I will submit a follow-up patch shortly to fix this.

Comment 107

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

Attachment: Fix manifest file to correctly include the test. v1.0

Comment 108

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 684852 [details] [diff] [review]
Fix manifest file to correctly include the test. v1.0

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

Looks good to me.

Comment 109

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

http://hg.mozilla.org/qa/mozmill-tests/rev/b14aab8106e3 (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/8eb59353d6e4 (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/155540aeea0e (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/22a7f9288d54 (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/ed2b4f057c6e (esr17)