Seen this on October 9th and waited to see other reports before filling the bug. But today I looked at older reports and found this has happened before on October 3rd and September 30th: http://mozmill-ci.blargon7.com/#/endurance/report/d11b1de413a0179d904e7372304b3ec5 http://mozmill-ci.blargon7.com/#/endurance/report/d11b1de413a0179d904e737230828a38 All reports are on Ubuntu 12.04 x86, with Nightly, both Mozmill 1.5.18 and 1.5.19.

We had a similar issue with closing the windows when we were using controller.waitFor. I wonder if it would help to switch this test to use assert.waitFor in both places. This will be covered by bug 793092.

It's happening more and more. Can we get this at least investigated and checked as given by Dave's comment 1? If that doesn't help we should skip the test.

I will investigate and check if using assert.waitFor() helps, otherwise will have a skip patch until we find a solution.

We are failing too often. Please ensure that you can get it investigated on Monday. As you say, please skip if there is no solution.

Skipping test until we figure out the issue.

Comment on attachment 676106 [details] [diff] [review] skip patch Review of attachment 676106 [details] [diff] [review]: ----------------------------------------------------------------- Before we skip please make the change as given by Dave in comment 1.

Replacing the controller.waitFor calls with assert.waitFor(). For me it hasn't reproduced this way, we should see how it works on mozmill-ci.

Comment on attachment 676190 [details] [diff] [review] patch v1 Review of attachment 676190 [details] [diff] [review]: ----------------------------------------------------------------- Lets see how it works on default. If everything is fine lets get it backported asap to the other branches.

Comment on attachment 676190 [details] [diff] [review] patch v1 http://hg.mozilla.org/qa/mozmill-tests/rev/869473e61519 (default)

Not sure why the test is not present on release and esr10 but we want to have this fixed for aurora and beta too.

I don't see any more reports from what mozmill-ci could show today, but I've checked the backport and this patch applies cleanly to aurora and beta.

Transplanted as: http://hg.mozilla.org/qa/mozmill-tests/rev/1735aedd463b (mozilla-aurora) http://hg.mozilla.org/qa/mozmill-tests/rev/53f0be20eb91 (mozilla-beta) The test is not on release or beta because of the dependency on bug 788531. At this rate by the time that's resolved, the test will probably be present on all branches anyway... Let's reopen this bug if the issue reoccurs.

Unfortunately it has, with Nightly 19.0a1 on Linux: http://mozmill-ci.blargon7.com/#/endurance/report/190d57cf64e3c5c3996c0c10596411b8 http://mozmill-ci.blargon7.com/#/endurance/report/190d57cf64e3c5c3996c0c10596396db

Andrea, beside commenting please also change the bug status accordingly. Lets reopen this bug now.

Looks like it's failing in the 5th iteration in the first report from comment 13 and the 8th iteration in the second. Have we tried replicating this with Ubuntu 12.04 x86 with a large number of iterations?

This is often reproducing on linux OS or VM, with latest Nightly, 10 iterations and 10 entities. I used some dumps to see what happens and it always fails because controllers.length is 0. For example we have 4 windows opened, but the (controllers.length + 2) is 2. Increasing the timeout doesn't help, controllers don't get updated.

Let's go ahead and skip this, however you will need to wait until the follow-up patch on bug 711360 has landed, as the entry in the manifest was incorrect.

This failed again on Ubuntu Aurora en_US on 11/23 http://mozmill-ci.blargon7.com/#/endurance/report/674977957b923f4905160d1b9ab73ae6 Again on Ubuntu Nightly en_US on 11/23 http://mozmill-ci.blargon7.com/#/endurance/report/674977957b923f4905160d1b9abb1f39

Another failure in Nightly en_US Ubuntu on 11/24 http://mozmill-ci.blargon7.com/#/endurance/report/674977957b923f4905160d1b9acd7314

Aurora en_US Ubuntu - in 11/25 http://mozmill-ci.blargon7.com/#/endurance/report/674977957b923f4905160d1b9acff3d0 Nightly en_US Ubuntu - in 11/25 http://mozmill-ci.blargon7.com/#/endurance/report/674977957b923f4905160d1b9ad61125

Another failure appeared today (11/26) with new error message "Window number '4' has been opened" on Ubuntu 12.04 x86_64 http://mozmill-ci.blargon7.com/#/endurance/report/674977957b923f4905160d1b9adceb51

Happened yesterday 11/26 on: Aurora and Ubuntu 12.04 (x86_64) http://mozmill-ci.blargon7.com/#/endurance/report/674977957b923f4905160d1b9ae9b643 http://mozmill-ci.blargon7.com/#/endurance/report/674977957b923f4905160d1b9aec2b0b Aurora and Ubuntu 12.04 (x86) http://mozmill-ci.blargon7.com/#/update/report/674977957b923f4905160d1b9aea941e

Happened again today on Aurora build with Ubuntu 12.04 32bit: http://mozmill-ci.blargon7.com/#/endurance/report/674977957b923f4905160d1b9afbec8c Happened yesterday evening too on: 1) ESR17 Ubuntu x86_64: http://mozmill-ci.blargon7.com/#/endurance/report/674977957b923f4905160d1b9af1c0ca 2) Aurora Ubuntu x86: http://mozmill-ci.blargon7.com/#/endurance/report/674977957b923f4905160d1b9af426ba 3) Aurora Ubuntu 12.04 (x86_64): http://mozmill-ci.blargon7.com/#/endurance/report/674977957b923f4905160d1b9af67226

Happened again today: On ESR17 with Ubuntu 12.04 (x86_64): http://mozmill-ci.blargon7.com/#/endurance/report/352218d7e3125c857fc1d37167005995 On Nightly with Linux Ubuntu 12.04 (x86_64): http://mozmill-ci.blargon7.com/#/endurance/report/352218d7e3125c857fc1d3716703852d On Aurora FR with Ubuntu 12.04 (x86_64): http://mozmill-ci.blargon7.com/#/endurance/report/674977957b923f4905160d1b9afd3d2e

Happened yesterday on Firefox 18.0 - Linux Ubuntu 12.04 (x86_64): http://mozmill-ci.blargon7.com/#/endurance/report/352218d7e3125c857fc1d37167082759 On Ubuntu 12.04 (x86) and Aurora: http://mozmill-ci.blargon7.com/#/endurance/report/352218d7e3125c857fc1d3716708d883 On Ubuntu x86 and Aurora FR: http://mozmill-ci.blargon7.com/#/endurance/report/352218d7e3125c857fc1d37167151c17 Ubuntu 12.04 (x86_64) and Firefox 17: http://mozmill-ci.blargon7.com/#/endurance/report/352218d7e3125c857fc1d37167172520 Happened yesterday on Aurora and Ubuntu 12.04 (x86_64): http://mozmill-ci.blargon7.com/#/endurance/report/352218d7e3125c857fc1d3716708e47c

(In reply to Dave Hunt (:davehunt) from comment #17) > Let's go ahead and skip this, however you will need to wait until the > follow-up patch on bug 711360 has landed, as the entry in the manifest was > incorrect. Dave has requested to get this test skipped about 5 days ago. Can we finally please get to that task? Thanks.

Disabling test patch.

Comment on attachment 686549 [details] [diff] [review] skip patch Review of attachment 686549 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Andreea, landed as: http://hg.mozilla.org/qa/mozmill-tests/rev/89e570e67625

Dave, please land across branches because all are affected. Thanks.

Transplanted as: http://hg.mozilla.org/qa/mozmill-tests/rev/6ca74bd183df (mozilla-aurora) http://hg.mozilla.org/qa/mozmill-tests/rev/2424bf1481ae (mozilla-beta) http://hg.mozilla.org/qa/mozmill-tests/rev/89e570e67625 (mozilla-esr17)

Dave, it still missed the backport for release. Also please set the whiteboard entry and update the flags next time you land skip patches. Thanks.

(In reply to Dave Hunt (:davehunt) from comment #30) > Transplanted as: > http://hg.mozilla.org/qa/mozmill-tests/rev/89e570e67625 (mozilla-esr17) This transplant never went up into our repository! Please check all landings via the pushlog for posting links here. I don't want to have to check all the checkins. http://hg.mozilla.org/qa/mozmill-tests/rev/1256285951d0 (release) http://hg.mozilla.org/qa/mozmill-tests/rev/f7388a28c923 (esr17)

Happened on Aurora FR with Ubuntu 12.04 x86: http://mozmill-ci.blargon7.com/#/endurance/report/352218d7e3125c857fc1d371673c9b29

(In reply to Henrik Skupin (:whimboo) from comment #32) > (In reply to Dave Hunt (:davehunt) from comment #30) > > Transplanted as: > > http://hg.mozilla.org/qa/mozmill-tests/rev/89e570e67625 (mozilla-esr17) > > This transplant never went up into our repository! Please check all landings > via the pushlog for posting links here. I don't want to have to check all > the checkins. Could you clarify what was wrong here? The original URL looks fine to me... > http://hg.mozilla.org/qa/mozmill-tests/rev/1256285951d0 (release) Apologies for missing release.

I have used some dumps to see what happens here. It appears we are closing the main window that starts the test too and remain with an opened one, that is why controllers are null afterwards. So I've added a for loop that ensures we close all windows after the 2 loops in the test and we compare the ID in order to not close the main window. The strange thing I came across with is that we have a opened window with an unknown ID number, right before we fail. Don't know how that gets opened or if it's an error of reading id's.

What is ID? As mentioned some days back it is important to see the code you have used. So please add this to the dump in the future. Why does 'windows count before 1' prints out everything twice? The numbers under 'Closing windows' don't make sense at all.

Added the dumps code and the report for it. The numbers are the ID of the opened windows, that are used also in the second loop for closing them. I wanted to see them for the opening as well because as you can see at the end of the loop, another "zombie" window ID appears as opened, despite the fact it's not in the opened list. But in the opened list, the same ID appears twice, when it should have been the "zombie" ID one. If you un-comment the code in the forEach I created, you will see the test working, since we no longer close the main window (ID = 3) - the reason this test fails now, but the "zombie" one.

What are the windows with ID 57 in the first loop and 200 in the second loop? Where do they come from?

ID 57 I assume is the one opened under ID 26 in the first loop, if you check the list the 26 appears twice. On the 3rd loop is the same case, ID 217 appears twice, instead of being ID 256. IDs 200 and 305 are a mystery. Maybe Dave has an idea.. Could be that where we push mozmill.getBrowserController() into controllers array we also open a new window? Shouldn't be that since we have opened windows, it's not necessary to open one.

We shouldn't but we should also not call 'mozmill.getBrowserController()' for this case. Instead we have to get the most recent opened browser window. Do we need a controller at all? If yes lets create it via mozmill.controller.MozMillController.

We need the array of controllers in order to close the windows on the second loop (as it is, or could be changed using the forEach I've created). mozmill.getBrowserController() opens a new window if somwhow fails to get the controller for the current one.

It appears changing the controller to push the one for the current opened window does the trick. No more zombie windows opened. Reports for OS X and Linux: * http://mozmill-crowd.blargon7.com/#/endurance/report/a11aa7b15f4e571fd6fe21a2db7901da * http://mozmill-crowd.blargon7.com/#/endurance/report/a11aa7b15f4e571fd6fe21a2db7854a0

Comment on attachment 693411 [details] [diff] [review] patch v2 Review of attachment 693411 [details] [diff] [review]: ----------------------------------------------------------------- ::: tests/endurance/testTabbedBrowsing_OpenNewWindow/test1.js @@ +33,2 @@ > assert.waitFor(function () { > + windows = mozmill.utils.getWindows("navigator:browser"); You do not have to call getWindows() when declaring the variable. It will be done as the first command in the waitFor callback. @@ +36,5 @@ > }, "Window number '" + enduranceManager.currentEntity + "' has been opened"); > > + var windowController = new mozmill.controller. > + MozMillController(windows[enduranceManager. > + currentEntity]); I would not do such logic. It can cause brokenness. Instead get the most recently opened browser window. You might already do that in the waitFor and check for the window id being different from the last opened window. That would certainly make our test faster. Given that we don't even need the controller for the opened window, we could store the window ids in the array and close all windows except the main window later.

We still need to have the controller of each window in order to close it in the second loop. I can do those changes for the first loop, but in the second I will still need to go through all windows in a forEach loop, get the controller as above with MozmillController(window) and if it's ID is different then the main window's one, close it. But it looks to me that it'll be the same as the fix above. Henrik, why would it cause brokenness? Dave, any other thoughts on this? Thanks.

I talked with Andreea on IRC and as we agreed on we do not have to store any controller given that the only operation we do is to close the window. And that can be done via window.close().

We're comparing in waitFor() the id of the most recent window and the previous one from the array of opened windows (for the first entity it will be the id of the main window). There was no need for an array of Ids since in the closing part, we need to go through all windows and it was simpler to take the ID there instead of making use of loop-in-loop. We check that the ID of each window is different from the main's one, so we can close it. Reports: * http://mozmill-crowd.blargon7.com/#/endurance/report/a11aa7b15f4e571fd6fe21a2dbb69f8b * http://mozmill-crowd.blargon7.com/#/endurance/report/a11aa7b15f4e571fd6fe21a2dbb690d1

Comment on attachment 694804 [details] [diff] [review] patch v2.1 Review of attachment 694804 [details] [diff] [review]: ----------------------------------------------------------------- ::: tests/endurance/testTabbedBrowsing_OpenNewWindow/test1.js @@ +11,5 @@ > function setupModule() { > controller = mozmill.getBrowserController(); > enduranceManager = new endurance.EnduranceManager(controller); > tabBrowser = new tabs.tabBrowser(controller); > + wm = Components.classes["@mozilla.org/appshell/window-mediator;1"]. Please use Cc in place of Components.classes. Also, please insert a blank line before. @@ +35,2 @@ > assert.waitFor(function () { > + var currentId = mozmill.utils.getWindowId(wm.getMostRecentWindow('navigator:browser')); Is it necessary to compare the IDs? I understand that you used this to determine the failure, however we only need to wait for a new window to be open here. @@ +45,5 @@ > enduranceManager.loop(function () { > enduranceManager.addCheckpoint("Close a window"); > + var windows = mozmill.utils.getWindows("navigator:browser"); > + var windowId; > + windows.forEach(function (openedWindow) { This will close all windows within the first entity loop, which means the checkpoint is out of place. The loop method is executed for each entity. We should only attempt to close as many windows as we expect to be open. If we associated each entity with a window ID then we should be able to close only the appropriate window here.

That's correct. Please only close those windows which have been opened by this test and not more or less.

(In reply to Dave Hunt (:davehunt) from comment #47) > Is it necessary to compare the IDs? I understand that you used this to > determine the failure, however we only need to wait for a new window to be > open here. I used that comparison to ensure the opening of a new window, the id of the most recent window should be different from the previous opened one, as asked in comment 43. An easier idea would be to simply compare the opened windows length with the currentEntity + 1 (that one being the main window). > The loop method is executed for each entity. We > should only attempt to close as many windows as we expect to be open. But at the first entity we expect to have all the new 10 windows opened. > If we associated each entity with a window ID then we should be able > to close only the appropriate window here. We could close only one window per entity, the last one from the windows array (that is also the last opened one) and keep the condition to have a different id then the main window, just to be sure we don't close that one. Does this sound right for you?

(In reply to Andreea Matei [:AndreeaMatei] from comment #49) > (In reply to Dave Hunt (:davehunt) from comment #47) > > Is it necessary to compare the IDs? I understand that you used this to > > determine the failure, however we only need to wait for a new window to be > > open here. > I used that comparison to ensure the opening of a new window, the id of the > most recent window should be different from the previous opened one, as > asked in comment 43. An easier idea would be to simply compare the opened > windows length with the currentEntity + 1 (that one being the main window). Yes, I believe this is how it was done before. > > The loop method is executed for each entity. We > > should only attempt to close as many windows as we expect to be open. > But at the first entity we expect to have all the new 10 windows opened. No, in teh case of this test the windows are the entities, so the first entity is the first window, the 10th entity is the 10th window. > > If we associated each entity with a window ID then we should be able > > to close only the appropriate window here. > We could close only one window per entity, the last one from the windows > array (that is also the last opened one) and keep the condition to have a > different id then the main window, just to be sure we don't close that one. > Does this sound right for you? We should close the window associated with the entity, not the last one opened, however I would expect these to be the same.

We're opening a new window, waiting for the number of windows to be equal to the currentEntity + 1 (the main window) and closing the window associated to each entity. Reports: * http://mozmill-crowd.blargon7.com/#/endurance/report/23d8fbdd0190d4b0496d6b129fd83ac7 * http://mozmill-crowd.blargon7.com/#/endurance/report/23d8fbdd0190d4b0496d6b129fd8ebbe

Comment on attachment 699760 [details] [diff] [review] patch v3 Review of attachment 699760 [details] [diff] [review]: ----------------------------------------------------------------- ::: tests/endurance/testTabbedBrowsing_OpenNewWindow/test1.js @@ +21,5 @@ > * Test opening new windows > */ > function testOpenAndCloseMultipleWindows() { > enduranceManager.run(function () { > + var windows = mozmill.utils.getWindows("navigator:browser"); We don't need to assign a value to this variable here, as we don't use it before assigning it within the first entity loop. @@ +32,2 @@ > > + return windows.length == enduranceManager.currentEntity + 1; === please @@ +44,4 @@ > assert.waitFor(function () { > return !mozmill.controller.windowMap.contains(windowId); > }, "Window '" + windowId + "' has been closed."); > enduranceManager.addCheckpoint("Window '" + windowId + "' has been closed"); Let's make this a little clearer. Something like "Window number '" + enduranceManager.currentEntity + "' with ID '" + windowId + "' has been closed". Also, as we use this string twice, let's create a variable for it.

Updated as requested in the latest review.

Comment on attachment 700258 [details] [diff] [review] patch v3.1 Review of attachment 700258 [details] [diff] [review]: ----------------------------------------------------------------- Running this I see that the windows start being closed before they've all finished opening. Could we use an observer here to prevent this?

I can look for an observer to use, but I don't see that behavior. I've dumped the mozmill.utils.getWindows("navigator:browser").length at the end of first loop, in between loops and in the second and each time we have 11 windows after the first loop and when we start the second one. Visually can be misleading, though I can count them all opening, but we start closing the very first ones, which are hidden in the background.

Dave, really? Would you mind to slow down the test and check again? I can't believe that given that we check for the window length and wait until the new window has been opened.

(In reply to Henrik Skupin (:whimboo) from comment #56) > Dave, really? Would you mind to slow down the test and check again? I can't > believe that given that we check for the window length and wait until the > new window has been opened. Wouldn't slowing the test down just mask this issue? We're waiting for the window length to increase. It doesn't seem unreasonable for the window count to increase fractionally before the window is finally visible to the user. In the first iteration the new windows are behind the initial window, but subsequent iterations they cascade. Using 10 entities I clearly see windows start closing by the time the 8th window is visible. I would be happy to record a screencast of this.

I wonder if we should subscribe to the toplevel-window-ready observer notifications and wait until the newly opened window has been fully loaded. Might be a great enhancement for any test which deals with newly opened windows.

Added the observer Henrik suggested and a waitFor the window to have isLoaded value set to true: http://mxr.mozilla.org/mozilla-central/source/testing/peptest/peptest/extension/resource/mozmill/driver/controller.js#405 To ensure the observer gets registered, we have a counter that in the end has to equal the number of entities. Ran it on OS X and I see now all windows being opened and just right after the last one is opened, they start being closed. Report: http://mozmill-crowd.blargon7.com/#/endurance/report/9e41582ed5e806fa373351d9d31ddf1a

Comment on attachment 703308 [details] [diff] [review] patch v4 Review of attachment 703308 [details] [diff] [review]: ----------------------------------------------------------------- ::: lib/endurance.js @@ +15,5 @@ > + * > + * @see http://mxr.mozilla.org/mozilla-central (nsIObserverService.idl) > + */ > +var observerService = Cc["@mozilla.org/observer-service;1"]. > + getService(Ci.nsIObserverService); Please use the Services.jsm module from now on. See bug 830688 as example. @@ +94,5 @@ > + * @returns {Number} Number of windows > + */ > + get counter() { > + return this._counter; > + }, I do not think we want to have that property on the endurance object. It doesn't seem to be necessary. See below. @@ +159,5 @@ > + > + /** > + * Open a new window via menu and wait for it to load > + */ > + open: function endurance_open() { This is not a method for the endurance module. I agree that this can be useful but then we have it in a window.js module which then can be used by any tests. Once it's in we should file a follow-up to get all the other tests updated which open new windows. @@ +161,5 @@ > + * Open a new window via menu and wait for it to load > + */ > + open: function endurance_open() { > + const TOPLEVEL_WINDOW_READY = "toplevel-window-ready"; > + var that = this; Just for the future, in our suite we use 'self' instead of 'that'. @@ +170,5 @@ > + } > + > + try { > + observerService.addObserver(observer, TOPLEVEL_WINDOW_READY, false); > + this._controller.mainMenu.click("#menu_newNavigator"); This should be similar to the pb code for opening a new window. We might want to combine the code later. @@ +181,5 @@ > + var win = enumerator.getNext(); > + mozmill.utils.waitFor(function () { > + return that._controller.isLoaded(win); > + }, "New window is loaded"); > + } The above code should end-up in the test itself. ::: tests/endurance/testTabbedBrowsing_OpenNewWindow/test1.js @@ +33,2 @@ > > + return windows.length === enduranceManager.currentEntity + 1; nit: please re-add the brackets.

Here are the requested changes, added a new lib called windows.js, with open and close methods. Updated the test so it uses both of them. Report: http://mozmill-crowd.blargon7.com/#/endurance/report/0dbaa964aec88a2f5637c68c9620fd18

Comment on attachment 708560 [details] [diff] [review] patch v5 Review of attachment 708560 [details] [diff] [review]: ----------------------------------------------------------------- Is patch is broken and removes nearly every endurance test.

Sorry about that, I missplaced the patches.

Comment on attachment 710090 [details] [diff] [review] patch v5 Review of attachment 710090 [details] [diff] [review]: ----------------------------------------------------------------- ::: lib/windows.js @@ +80,5 @@ > + aController.keypress(null, "N", {ctrlKey: true}); > + break; > + case "callback": > + if (typeof aCallback !== "function") { > + throw new Error("No callback given for opening the private browsing window"); Is this specific to private browsing? @@ +100,5 @@ > + finally { > + Services.obs.removeObserver(observer, TOPLEVEL_WINDOW_READY); > + } > + }, > + nit: trailing space @@ +123,5 @@ > + // Try to close the window in case it is not already closed > + this._controller.window.close(); > + > + // Wait for it to finish being closed > + mozmill.utils.waitFor(function () { Like the open, should we add a check for the window count and decrement the counter? ::: tests/endurance/testTabbedBrowsing_OpenNewWindow/test1.js @@ +27,5 @@ > */ > function testOpenAndCloseMultipleWindows() { > enduranceManager.run(function () { > + var windows; > + windowManager.counter = 0; If we decremented this in the window manager whenever we close a window then we wouldn't need to set this to 0. @@ +40,5 @@ > > + // Check each opened window is loaded > + var enumerator = Services.wm.getEnumerator(""); > + while (enumerator.hasMoreElements()) { > + var win = enumerator.getNext(); nit: could we use 'window' instead of 'win' @@ +63,5 @@ > assert.waitFor(function () { > return !mozmill.controller.windowMap.contains(windowId); > }, "Window '" + windowId + "' has been closed."); > enduranceManager.addCheckpoint("Window '" + windowId + "' has been closed"); > }); I think we should assert that there are 0 windows open here, and if we decrement the counter in windowManager.close() then that should be simple enough.

Updated as requested. Since Henrik is in PTO, asking review from Dave only. Report: http://mozmill-crowd.blargon7.com/#/endurance/report/1641bc2f6438595b86015bd9019473f9

Comment on attachment 711830 [details] [diff] [review] patch v5.1 Review of attachment 711830 [details] [diff] [review]: ----------------------------------------------------------------- A couple of nits, but looking good, and tests well. I would prefer if we could close the windows in the same order we opened them, but wouldn't let this block landing the patch. I am going to block this on bug 788531 though because landing this patch will otherwise give a huge jump in memory usage, but I should be able to unblock that today. ::: lib/windows.js @@ +89,5 @@ > + } > + > + assert.waitFor(function () { > + var windows = mozmill.utils.getWindows("navigator:browser"); > + return (windows.length === (self._counter + 1)); I believe this could return early if processed before the window has opened. Let's say windows.length is 1, and self._counter is 0 (initial values), this statement is true, but we've started to open a new window... I think adding a flag in the observer code would make this more robust: return (windowOpened && windows.length === (self._counter + 1)); What do you think? @@ +124,5 @@ > + > + // Wait for it to finish being closed > + assert.waitFor(function () { > + var windows = mozmill.utils.getWindows("navigator:browser"); > + return (windows.length === (self._counter + 1)); Same as above here, I'd bring back the windowClosed flag and use that, perhaps in addition to the length/counter. ::: tests/endurance/testTabbedBrowsing_OpenNewWindow/test1.js @@ +61,4 @@ > enduranceManager.addCheckpoint("Window '" + windowId + "' has been closed"); > }); > + > + assert.waitFor(function () { We already wait for the window to be closed, so is it necessary for this to be a wait or can it just be a straightforward assert?

Updated requested changes. Indeed it works with assert.equal instead assert.waitFor at the final test check. Report with the old delay: http://mozmill-crowd.blargon7.com/#/endurance/report/a83c700664548dba07298b74bf2d51e0 The 5s delay is still running, think it will be done in half an hour or so. I'll update the bug when that's finished.

Comment on attachment 713406 [details] [diff] [review] patch v6 Review of attachment 713406 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, this no longer applies cleanly since bug 839705 landed on default. Also, we should probably block this until https://github.com/mozilla/mozmill-ci/issues/201 is resolved. :( Otherwise, the patch looks great! :)

Also, I don't think your full run came through on the mozmill-crowd dashboard...

Looked now for my report and I believe is this one: http://mozmill-crowd.blargon7.com/#/endurance/report/a83c700664548dba07298b74bf4577d2 It took foreveeer and seems it has failed at the openNewTab test, testrun started at about 3:30 PM and finished at about 10PM, Romanian time. I'll wait for that issue to be solved and in the meantime will try some more testruns.

That "forever" report was due to the fact that the machine was getting into stand-by mode, I have changed that now and it takes 90 minutes as it should. After issue 201 gets fixed I'll update the patch here.

(In reply to Andreea Matei [:AndreeaMatei] from comment #71) > That "forever" report was due to the fact that the machine was getting into > stand-by mode, I have changed that now and it takes 90 minutes as it should. > After issue 201 gets fixed I'll update the patch here. Ah, thanks! :)

Comment on attachment 713406 [details] [diff] [review] patch v6 Review of attachment 713406 [details] [diff] [review]: ----------------------------------------------------------------- I want to add some more comments to this patch given the work Geo and I did a couple of months back to refactor the tests. We should check that code and find a way to make sure to be mostly be in sync. It will help us a lot if we are going with the refactor at some point. ::: lib/windows.js @@ +15,5 @@ > + * > + * @param {MozMillController} controller > + * MozMillController of the window to operate on > + */ > +function WindowManager(aController) { Please call it WindowWrapper so that we are in sync with the potential refactor we started as POC a while back: https://github.com/whimboo/mozmill-test-refactor/blob/master/lib/api/windows.js @@ +54,5 @@ > + > + /** > + * Open a new window and wait for it to load > + */ > + open: function window_open(aController, aMethod, aCallback) { nit: instead of window_open call it WW_open (which relates to the proposed WindowWrapper name) @@ +57,5 @@ > + */ > + open: function window_open(aController, aMethod, aCallback) { > + var method = aMethod || "menu"; > + > + assert.ok(aController, "A controller has to be specified"); Please try to not divide the declaration block with this assert call. It should be located in the first line or before the try statement. @@ +76,5 @@ > + case "menu": > + aController.mainMenu.click("#menu_newNavigator"); > + break; > + case "shortcut": > + aController.keypress(null, "N", {ctrlKey: true}); Never use a hard-coded value for keypress. Retrieve the appropriate key from the DTD entity. Also this will fail on OS X, so make use of the accelKey. @@ +81,5 @@ > + break; > + case "callback": > + if (typeof aCallback !== "function") { > + throw new Error("No callback given for opening the window"); > + } Do not throw but call assert.*() here. @@ +85,5 @@ > + } > + aCallback(aController); > + break; > + default: > + throw new Error(arguments.callee.name + ": Unknown opening method - " + method); Same here. @@ +90,5 @@ > + } > + > + assert.waitFor(function () { > + var windows = mozmill.utils.getWindows("navigator:browser"); > + return (windowOpened && windows.length === (self._counter + 1)); I do not see the usefulness of _counter. Why has to to be a property? It will get out of sync if any other code outside of this class causes a new window to open. @@ +94,5 @@ > + return (windowOpened && windows.length === (self._counter + 1)); > + }, "A new window has been opened"); > + > + this._controller = utils.handleWindow("type", "navigator:browser", undefined, false); > + return this._controller; Please do not overwrite the formerly set controller of this wrapper with the newly opened window's controller. This totally mixes up the underlying object. If you want a real window manager it should manage a list of window wrappers. @@ +122,5 @@ > + try { > + Services.obs.addObserver(observer, DOM_WINDOW_CLOSED, false); > + > + // Try to close the window in case it is not already closed > + this._controller.window.close(); I don't think that we should always close the window in the hard way. We should add an option to the method like aForce, which let us decide if we have to use UI elements or the backend API. @@ +132,5 @@ > + }, "Window has been closed"); > + } > + finally { > + Services.obs.removeObserver(observer, DOM_WINDOW_CLOSED); > + this._controller = utils.handleWindow("type", "navigator:browser", undefined, false); Same as mentioned above. Pleaes don't override the _controller property.

Giving that we are going to update tests to use this class for opening/closing windows, that counter is not affecting us. Without it, we get the wrong controller starting with the main window instead of the new window one. For this test, the closing of the windows can only be by "force", since the other methods will close all windows (including the main one) - this is expected behavior, also done manually. Passing report: http://mozmill-crowd.blargon7.com/#/endurance/report/172b2945b09ec259eb6e92f644593135

Comment on attachment 722791 [details] [diff] [review] patch v6.1 Review of attachment 722791 [details] [diff] [review]: ----------------------------------------------------------------- Leaving review flag for Henrik as the recent changes address his detailed review. ::: lib/windows.js @@ +90,5 @@ > + case "menu": > + aController.mainMenu.click("#menu_newNavigator"); > + break; > + case "shortcut": > + var dtds = ["chrome://browser/locale/browser.dtd"]; Please create a getDtds method as we have in other modules. @@ +100,5 @@ > + "No callback given for opening the window"); > + aCallback(aController); > + break; > + default: > + assert.equal(typeof aCallback, "function", Why are we checking that aCallback is a function here? @@ +142,5 @@ > + Services.obs.addObserver(observer, DOM_WINDOW_CLOSED, false); > + // Try to close the window in case it is not already closed > + switch (method) { > + case "menu": > + aController.mainMenu.click("#menu_FileQuitItem"); We don't want to quit the application, just close the current window. @@ +145,5 @@ > + case "menu": > + aController.mainMenu.click("#menu_FileQuitItem"); > + break; > + case "shortcut": > + var dtds = ["chrome://browser/locale/browser.dtd"]; As above, use getDtds @@ +146,5 @@ > + aController.mainMenu.click("#menu_FileQuitItem"); > + break; > + case "shortcut": > + var dtds = ["chrome://browser/locale/browser.dtd"]; > + var cmdKey = utils.getEntity(dtds, "quitApplicationCmdMac.key"); This should be the close window command, not the quit application one. @@ +158,5 @@ > + case "force": > + aController.window.close(); > + break; > + default: > + assert.equal(typeof aCallback, "function", As above, why check aCallback is a function here, we should just throw an exception.

Thanks Dave! (In reply to Dave Hunt (:davehunt) from comment #75) > > + case "menu": > > + aController.mainMenu.click("#menu_FileQuitItem"); > > We don't want to quit the application, just close the current window. I'm not sure if we can do this from menu then, since we only have quit/exit now. The only way I see is Shift + Ctrl + W, but that's what the shortcut case does. Any ideas or should I just remove the "menu" case?

I can see 'Close Window' in my File menu... http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-menubar.inc#47

Addressed Dave's review, indeed close Window is available throough Alt + f, it's hidden otherwise. Report here with all endurance tests unskipped :) http://mozmill-crowd.blargon7.com/#/endurance/report/2a6536e9db9f5f44ed48c58510ae7b85

Comment on attachment 725396 [details] [diff] [review] patch v6.2 Review of attachment 725396 [details] [diff] [review]: ----------------------------------------------------------------- I'm happy with this. Henrik, please could you either give this a final look over before landing, or if you're happy with my review let me know and I'll go ahead and land it.

Updating the patch cause it was not applying cleanly anymore. Dave already r+ this. New reports: * http://mozmill-crowd.blargon7.com/#/endurance/report/f2d3b6136b11cb7b6708636cff2ab99c * http://mozmill-crowd.blargon7.com/#/endurance/report/f2d3b6136b11cb7b6708636cff2a73c4

I'd like to put this in the P1 bucket list given that it also includes a new library and is blocking bug 711129.

Thanks Andreea. Let's ping Henrik about a final review for this. As it's been a while since the last version of the patch could you check that it hasn't suffered any bitrot?

This still applies cleanly and works as expected.

Comment on attachment 762647 [details] [diff] [review] patch v6.2 Review of attachment 762647 [details] [diff] [review]: ----------------------------------------------------------------- I haven't been able to reproduce the initial reported issue at all (with or without this patch). This patch seems to work fine for me (on OSX). Yet there are still some issues to take care of: ::: lib/windows.js @@ +5,5 @@ > +// Import the required modules > +var { assert } = require("assertions"); > +var utils = require("utils"); > + > +Components.utils.import("resource://gre/modules/Services.jsm"); We should use Cu here @@ +14,5 @@ > + * MozMillController of the window to operate on > + */ > +function WindowWrapper(aController) { > + this._controller = aController || null; > + this._controllers = []; I would rename this into controllerList. But take this as a personal preference, and leave it like this if you like it better. @@ +45,5 @@ > + * Set the length of controllers list for the new windows > + * @param {Number} aValue > + * Number of controllers > + * > + * @returns {Number} Number of window controllers Does not return anything @@ +47,5 @@ > + * Number of controllers > + * > + * @returns {Number} Number of window controllers > + */ > + set controllers(aValue) { This setter is inappropriate, it does not actually set the _controllers array (I would have expected an array as an argument). We only use this to reset the controllers array. More appropriate would be a method that does that: Something like: resetControllerList() @@ +52,5 @@ > + this._controllers.length = aValue; > + }, > + > + /** > + * Returns the list of controllers of the new windows Might rephrase as "Returns the controller list[...]" as this sounds a bit odd. @@ +63,5 @@ > + > + /** > + * Gets all the needed external DTD urls as an array > + * > + * @returns {Array} Array of external DTD urls This should be {String[]} @@ +74,5 @@ > + /** > + * Open a new window and wait for it to load > + * @param {MozMillController} aController > + Controller of the window > + * @param {string} aMethod Specify that this is optional, and which default value it has @@ +82,5 @@ > + */ > + open: function WW_open(aController, aMethod, aCallback) { > + assert.ok(aController, "A controller has to be specified"); > + > + const TOPLEVEL_WINDOW_READY = "toplevel-window-ready"; We should declare this at the top of the module to save some memory @@ +130,5 @@ > + /** > + * Close a window > + * @param {MozMillController} aController > + Controller of the window > + * @param {string} aMethod Optional parameter + default value @@ +134,5 @@ > + * @param {string} aMethod > + * Specifies a method for closing the window > + */ > + close: function WW_close(aController, aMethod) { > + const DOM_WINDOW_CLOSED = "domwindowclosed"; This should also be placed at the top of the module @@ +168,5 @@ > + aController.window.close(); > + break; > + default: > + throw new Error(arguments.callee.name + ": Unknown opening method - " + method); > + } nit: this block is not properly indented ::: tests/endurance/testTabbedBrowsing_OpenNewWindow/test1.js @@ +45,5 @@ > + var window = enumerator.getNext(); > + assert.waitFor(function () { > + return controller.isLoaded(window); > + }, "New window is loaded"); > + } Why do we wait for all windows to be loaded at each step? Isn't it enough to wait for the current window to load?

Comment on attachment 762647 [details] [diff] [review] patch v6.2 Review of attachment 762647 [details] [diff] [review]: ----------------------------------------------------------------- ::: lib/windows.js @@ +14,5 @@ > + * MozMillController of the window to operate on > + */ > +function WindowWrapper(aController) { > + this._controller = aController || null; > + this._controllers = []; I still think that we might want to fully re-check what we want to do here. A window wrapper is not handling a list of controllers but should have a 1:1 relationship. That's what I mentioned already in one of the previous reviews. So I'm still against the inclusion of such a class.

Updated to be 1:1 relationship, meaning we're back to a controllers array in the test to be able to first open all windows and then close them one by one. Reports with just this test running in 10 entities, if it's ok I'll provide full reports (it takes 2 hours for a full one): 2.0: http://mozmill-crowd.blargon7.com/#/endurance/report/6ec6776efe900da3fd2b64a750ccf84c http://mozmill-crowd.blargon7.com/#/endurance/report/6ec6776efe900da3fd2b64a750cc652d 1.5: http://mozmill-crowd.blargon7.com/#/endurance/report/6ec6776efe900da3fd2b64a750cd26fc

Comment on attachment 813125 [details] [diff] [review] patch v7 Review of attachment 813125 [details] [diff] [review]: ----------------------------------------------------------------- We still have some work to do here. ::: lib/windows.js @@ +85,5 @@ > + default: > + throw new Error(arguments.callee.name + ": Unknown opening method - " + method); > + } > + > + assert.waitFor(function () { nit: space between function and brackets @@ +90,5 @@ > + var windows = mozmill.utils.getWindows("navigator:browser"); > + return (windowOpened && windows.length === (self._counter + 1)); > + }, "A new window has been opened"); > + > + this._controller = utils.handleWindow("type", "navigator:browser", undefined, false); This doesn't look very clean. the windowManager class only hold a reference to the latest opened window as we replace _controller on each window open. We shouldn't do this. @@ +116,5 @@ > + windowClosed = true; > + } > + } > + > + if (aController) { If we need a controller we should fail the same way we do in open(), not skip the whole method and fail to close the window silently. ::: tests/endurance/testTabbedBrowsing_OpenNewWindow/test1.js @@ +21,4 @@ > } > > function teardownModule(aModule) { > aModule.tabBrowser.closeAllTabs(); We do not open any tab during this test. We are opening new windows, if the test should fail, we should clean up all windows. the closeAllTabs() will not clean multiple windows opened. @@ +38,1 @@ > assert.waitFor(function () { Do we need this waitFor? We already wait for the window to open in windows.js We do have an additional check here for the length to match the current entity iteration, but if the window opening failes, we will fail in windows.js, so this check is probably not needed. @@ +38,2 @@ > assert.waitFor(function () { > + windows = mozmill.utils.getWindows("navigator:browser"); You didn't use `var` here so you are overriding the `windows` library loaded in line 12. You could take advantage of the closure and declare this with `var` or maybe better to use another name to avoid any confusion. @@ +41,5 @@ > }, "Window number '" + enduranceManager.currentEntity + "' has been opened"); > > + // Check each opened window is loaded > + var enumerator = Services.wm.getEnumerator("navigator:browser"); > + while (enumerator.hasMoreElements()) { Why do we check *all* windows have loaded on each step? We've already checked all previous opened windows in previous endurance loops. It should suffice to wait for the current window to load. @@ +45,5 @@ > + while (enumerator.hasMoreElements()) { > + var window = enumerator.getNext(); > + assert.waitFor(function () { > + return windowManager.controller.isLoaded(window); > + }, "New window is loaded"); Can we change the tense of the message to be in line with all other? Something like: "New window has been loaded" @@ +60,3 @@ > enduranceManager.loop(function () { > enduranceManager.addCheckpoint("Close a window"); > + var windowController = controllers[enduranceManager.currentEntity-1]; nit: leave spaces around the minus operand @@ +60,4 @@ > enduranceManager.loop(function () { > enduranceManager.addCheckpoint("Close a window"); > + var windowController = controllers[enduranceManager.currentEntity-1]; > + var windowId = mozmill.utils.getWindowId(windows[enduranceManager.currentEntity]); Who is `windows` in this context? Right now it references `mozmill.utils.getWindows("navigator:browser");` from the last iteration from the first enduranceManeger.loop() call. We should probably save a reference to it between the endurance loops. Also, shouldn't `controllers` and `windows` be in sync? I'm not sure why they are offset by 1...

I'm not sure I follow what is requested here. In comment 73 it's said to not overwrite the controller of the wrapper with the newly opened window but instead to have a list of window wrappers. But we don't want to have a list with all the controllers from the opened windows, cause we want 1:1 relationship. So should I create another class like WindowManager and have the current WindowWrapper as a subclass? In WindowManager class I should add the controllers and have like a controllers map? (In reply to Andrei Eftimie from comment #87) > > + > > + assert.waitFor(function () { > > nit: space between function and brackets I don't see anything wrong here. > > @@ +38,1 @@ > > assert.waitFor(function () { > > Do we need this waitFor? > We already wait for the window to open in windows.js > > We do have an additional check here for the length to match the current > entity iteration, but if the window opening failes, we will fail in > windows.js, so this check is probably not needed. This also gives us the clue of which iteration has failed, like in the failure from this summary bug. > @@ +60,4 @@ > > enduranceManager.loop(function () { > > enduranceManager.addCheckpoint("Close a window"); > > + var windowController = controllers[enduranceManager.currentEntity-1]; > > + var windowId = mozmill.utils.getWindowId(windows[enduranceManager.currentEntity]); > > Who is `windows` in this context? > Right now it references `mozmill.utils.getWindows("navigator:browser");` > from the last iteration from the first enduranceManeger.loop() call. > > We should probably save a reference to it between the endurance loops. That is exactly who it should reference: mozmill.utils.getWindows("navigator:browser"); > Also, shouldn't `controllers` and `windows` be in sync? > I'm not sure why they are offset by 1... Cause controllers is an array and it's first position is 0, while the entities start at 1.

Hi guys I talk with Andreea and I took this bug as she's busy with metro. In order to have a 1-1 reference between controller and window object I suggest in open method we wait for the new window and initialize a new controller with it, and return the controller. And since is impossible to store all references off controllers inside the class(because we might have window objects that were not opened with WindowWrapper) we could handle the in tests exactly as before. So we won't change the logic of test here. Reports: http://mozmill-crowd.blargon7.com/#/endurance/report/50df0bef5c9d0b802925c134be2bc462 http://mozmill-crowd.blargon7.com/#/endurance/report/50df0bef5c9d0b802925c134be2bd2d3 http://mozmill-crowd.blargon7.com/#/endurance/report/50df0bef5c9d0b802925c134be2bd2d3

Hi, I updated the patch again, the utils.handleWindow returns the last opened window, it was just that we didn't waited for the last window to completely open, the thing that happened in WindowWrapper was that I delayed the retrieving of window, if we change the assertion to wait for the right number of windows in the right endurance iteration instead of controllers list the test passes. I ran the testrun with 20 entities and it passed: http://mozmill-crowd.blargon7.com/#/endurance/report/50df0bef5c9d0b802925c134be44d1c2 http://mozmill-crowd.blargon7.com/#/endurance/report/50df0bef5c9d0b802925c134be44e01f http://mozmill-crowd.blargon7.com/#/endurance/report/50df0bef5c9d0b802925c134be44caa6

Comment on attachment 8361125 [details] [diff] [review] patch_v9.0 Review of attachment 8361125 [details] [diff] [review]: ----------------------------------------------------------------- I took a long look at the original failure and what were were trying to fix here and I think you are right Cosmin. I also wasn't able to reproduce the initial issue. Now we are running mozmill 2.0 and we use a much longer delay between endurance loops (5s vs 0.1s). This patch adds more stability to the test, so I'd like to get this in. I've triple checked the order in which we open and then in which we close all windows and it is correct. Adding the check to wait until the controller is properly initialised should not add any notable overhead in time. Cosmin, this has a r+ from me, but it needs an update to the commit message/date/author/reviewer.

Comment on attachment 8361125 [details] [diff] [review] patch_v9.0 Review of attachment 8361125 [details] [diff] [review]: ----------------------------------------------------------------- ::: firefox/tests/endurance/testTabbedBrowsing_OpenNewWindow/test1.js @@ +40,5 @@ > + > + // Check each opened window is loaded > + assert.waitFor(function () { > + return newController.isLoaded(); > + }, "New window is loaded"); This assert is obsolete. It is already checked in MozMillController(). If it hasn't been loaded, it will raise an exception.

(In reply to Henrik Skupin (:whimboo) from comment #92) > This assert is obsolete. It is already checked in MozMillController(). If it > hasn't been loaded, it will raise an exception. I see this was fixed in June via bug 661408. So this test should actually work in mozmill 2.0 and not fail at all. We could also get away here with just unskipping the tests. I do like the change the check to the number of opened windows vs the current entity, so we might lave that in, but the rest of the code could be left as it was.

Cosmin, would you please update this patch. Take out the extra waitFor check that is already done in mozmill.

As you said Anndrei this has been fixed in mozmill already, so the here is the re-enable patch. I tested this a bit and it seems to work fine. Reports: http://mozmill-crowd.blargon7.com/#/endurance/report/a438ea29b921b2e8124749eda90dc97a http://mozmill-crowd.blargon7.com/#/endurance/report/a438ea29b921b2e8124749eda90dd43c http://mozmill-crowd.blargon7.com/#/endurance/report/a438ea29b921b2e8124749eda90dd2eb

Comment on attachment 8384738 [details] [diff] [review] patch_v10.0 Review of attachment 8384738 [details] [diff] [review]: ----------------------------------------------------------------- ::: firefox/tests/endurance/testTabbedBrowsing_OpenNewWindow/test1.js @@ +32,5 @@ > controller.mainMenu.click("#menu_newNavigator"); > assert.waitFor(function () { > var windows = mozmill.utils.getWindows("navigator:browser"); > > + return ((windows.length - 1) === enduranceManager.currentEntity); Not sure why you added those extra brackets. There's no need to wrap everything with them. Please remove the outer pair.

I removed the brackets. The patch applies cleanly on all branches.

Comment on attachment 8386848 [details] [diff] [review] patch_v10.1 Review of attachment 8386848 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. http://hg.mozilla.org/qa/mozmill-tests/rev/08631b121e79 (default) Let's see how this performs on CI, then backport it.

The test has not been enabled via the manifest.

This patch re-enables the test in manifest file. http://mozmill-crowd.blargon7.com/#/endurance/report/3ed2024184b13bf096824c0808319699

Comment on attachment 8387462 [details] [diff] [review] patch_v10.1[follow-up] Review of attachment 8387462 [details] [diff] [review]: ----------------------------------------------------------------- Landed followup: http://hg.mozilla.org/qa/mozmill-tests/rev/30c530ed2182 (default) Thanks Henrik for the keen eye. We shouldn't have missed this :( Cosmin, please fold both patches and prepare a backport patch. If all is good during the weekend we'll land them on Monday.

This is the patch for aurora, it applies cleanly on all other branches.

Comment on attachment 8389108 [details] [diff] [review] patch_v10.1[aurora].patch Review of attachment 8389108 [details] [diff] [review]: ----------------------------------------------------------------- Landed: http://hg.mozilla.org/qa/mozmill-tests/rev/2e62ef902b21 (mozilla-aurora) http://hg.mozilla.org/qa/mozmill-tests/rev/9e2f6b09a0c7 (mozilla-beta) http://hg.mozilla.org/qa/mozmill-tests/rev/953802ddfd62 (mozilla-release) http://hg.mozilla.org/qa/mozmill-tests/rev/0bf58ffc3710 (mozilla-esr24)