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.

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?

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

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

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?

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.

More comments added

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

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 on attachment 586967 [details] [diff] [review] wip 2.0 Changing feedback request for Anthony

Comment on attachment 586985 [details] [diff] [review] wip 2.1 Changing feedback request for Anthony

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

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.

(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

Initial patch

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

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

Nit fixed - Over to Dave for r

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.

Fixed

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.

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

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.

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)

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.

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

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!

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

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.

(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

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.

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

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.

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

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.

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

Let see if bug 753757 can help us

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.

(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

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

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

(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

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

> > >+ // 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.

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

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.

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

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?

Here is a pastebin of the code http://themaniak.pastebin.mozilla.org/1666776

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

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.

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

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

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.

(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

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

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.

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

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.

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

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

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?

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.

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

10 iterations do not get me the error http://mozmill-crowd.blargon7.com/#/endurance/report/23e194f1171aa4baf774928b262f47f4 Are you still seeing this guys?

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?

(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

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.

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?

(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

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.

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

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

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

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?

(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

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

(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

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.

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.

(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 :)

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

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?

* 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

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

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

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

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

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

Comment on attachment 653694 [details] [diff] [review] patch v12.0 Landed as: http://hg.mozilla.org/qa/mozmill-tests/rev/2ade89438d77

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

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

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

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

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

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)