This bug is similar to bug 1121698 for handling chrome windows. Marionette is only able to return a list of open tabs. But it is not clear in which state such a tab is. Especially when tests are opening or closing tabs, they have to wait until the tab has e.g. been completely loaded and transitioned (with animations turned on). This can be done by listening for the appropriate events. Currently we have this implemented in our mozmill-tests repository in the tabs module: http://hg.mozilla.org/qa/mozmill-tests/file/default/firefox/lib/tabs.js To be able to get started we need the feature to work with observers and events. This is being worked on in bug 1121691 and bug 1121702.

As part of the work I’m doing on https://bugzilla.mozilla.org/show_bug.cgi?id=marionette-window-tracking, content browsers will be inserted into the known window handle list only when they have fully initialised, unlike today when they are inserted as soon as the tab opens. This change makes Marionette more predictable, but also causes some regressions in certain Firefox UI tests that open/close tabs with shortcuts. The test_tabbar.py test for example uses a list of <tab> elements to determine if the tab was opened or closed, but that a <tab> exist does not necessarily mean the content browser has been initialised. This causes it to often run into a race condition where too many content browsers are reported open when proceeding to the next test. With the window tracking refactoring, Marionette will fire reliable system observer notifications when new content browsers are created and we can use this to implement a fail-safe handling of tabs in the Firefox UI tests. Unfortunately I will not be able to fix this before landing the refactoring, so I will address this bug as part of that.

On further investigation I might actually be able to fix this before the window tracking refactoring. It looks like the Firefox UI tests are looking at the window handle list to increment by 1 when opening new tabs, but at the length of <tab> elements when closing. It looks like it’s fairly reliable to use window handles there too.

Comment on attachment 8989787 [details] Bug 1121705 - Look at window handles decrement when closing tab. https://reviewboard.mozilla.org/r/254766/#review261828 ::: testing/marionette/puppeteer/firefox/firefox_puppeteer/ui/browser/tabbar.py:323 (Diff revision 1) > accel=True) > else: > raise ValueError('Unknown closing method: "%s"' % trigger) > > Wait(self.marionette).until( > - lambda _: len(self.window.tabbar.tabs) == len(start_handles) - 1, > + lambda mn: len(mn.window_handles) == len(start_handles) - 1, Good catch. So the Firefox-UI tests have been created to basically work with the UI elements, and not any data the Gecko API provides. As such we really should observe the open tabs too. I think it should be done the following way: 1) When opening a window we have to wait for the new window handle to appear via Marionette, and then that the tab is visually there (means present in tabbar.tabs). 2) When closing a window we have to wait that the tab is visually gone, and then that also the window handle is no longer in the list of open handles.

Comment on attachment 8989787 [details] Bug 1121705 - Look at window handles decrement when closing tab. https://reviewboard.mozilla.org/r/254766/#review261828 > Good catch. > > So the Firefox-UI tests have been created to basically work with the UI elements, and not any data the Gecko API provides. As such we really should observe the open tabs too. > > I think it should be done the following way: > > 1) When opening a window we have to wait for the new window handle to appear via Marionette, and then that the tab is visually there (means present in tabbar.tabs). > > 2) When closing a window we have to wait that the tab is visually gone, and then that also the window handle is no longer in the list of open handles. Thanks for the review. Sure, that sounds optimal despite the fact that it’s not what it’s doing at the moment. I’ve updated the patch to do <tab> element checks in conjunction with content browser checks. The fundamental problem I’m trying to fix here is that the presence of a <tab> in the tabbar does not necessarily mean that the content browser has been created. The new <tab> is created immediately, but with https://bugzilla.mozilla.org/show_bug.cgi?id=marionette-window-tracking it may take some more time before we consider the content browser created and fully initialised.

Comment on attachment 8989787 [details] Bug 1121705 - Look at window handles decrement when closing tab. https://reviewboard.mozilla.org/r/254766/#review261886 ::: testing/marionette/puppeteer/firefox/firefox_puppeteer/ui/browser/tabbar.py:122 (Diff revision 2) > automatically be performed. But if it opens in the background, the current > tab will keep its focus. > > + It will first verify that a new content browser has been > + created, and then that a new `<tab>` element has been > + introduced in the tabbar. It is the other way around. We can have a tab but it not necessarily has a content browser yet. But AFAIR the tabbar class relies on a valid window handle at the moment and as such we would have to wait for the window handle first. Maybe reword the sentence a bit. ::: testing/marionette/puppeteer/firefox/firefox_puppeteer/ui/browser/tabbar.py:157 (Diff revision 2) > handles = self.marionette.window_handles > [new_handle] = list(set(handles) - set(start_handles)) > + > + Wait(self.marionette).until( > + lambda _: len(self.window.tabbar.tabs) == len(start_tabs) + 1, > + message='Tab for "%s" not present in tabbar' % new_handle) Given my above comment this check for the new tab being opened should always pass immediately. But it is good to have here for safety.

Comment on attachment 8989787 [details] Bug 1121705 - Look at window handles decrement when closing tab. https://reviewboard.mozilla.org/r/254766/#review261886 > It is the other way around. We can have a tab but it not necessarily has a content browser yet. > > But AFAIR the tabbar class relies on a valid window handle at the moment and as such we would have to wait for the window handle first. > > Maybe reword the sentence a bit. This is the order you asked me to put it in your original issue? For the record, what order you run these checks in does not matter. The purpose is to ensure you don’t end up with a content browser without a tab, and the code as-is catches that. Moving the new tab check first has the side-effect that we can’t log which window handle we’re dealing with if the check fails, and also that we can’t assign new_tab before the second check has run. I assume this is fine.

Comment on attachment 8989787 [details] Bug 1121705 - Look at window handles decrement when closing tab. https://reviewboard.mozilla.org/r/254766/#review261886 > This is the order you asked me to put it in your original issue? > > For the record, what order you run these checks in does not matter. > The purpose is to ensure you don’t end up with a content browser > without a tab, and the code as-is catches that. > > Moving the new tab check first has the side-effect that we can’t > log which window handle we’re dealing with if the check fails, and > also that we can’t assign new_tab before the second check has run. > I assume this is fine. It doesn't matter yet. But there might be cases in the future when we cannot wait for a content browser to be ready. This comes especially with sessionrestore into play, when after a restart we have x-amount of tabs open, but only the currently selected tab has a valid content browser. For all the others the content browser will lazely created once switched to the tab. This means tab first, then window handle. But yes, for opening the tab it shouldn't make a difference to the caller.

(In reply to Henrik Skupin (:whimboo) [away 07/08 - 07/22] from comment #10) > It doesn't matter yet. But there might be cases in the future > when we cannot wait for a content browser to be ready. This comes > especially with sessionrestore into play, when after a restart > we have x-amount of tabs open, but only the currently selected > tab has a valid content browser. For all the others the content > browser will lazely created once switched to the tab. > > This means tab first, then window handle. But yes, for opening the > tab it shouldn't make a difference to the caller. It doesn’t matter at all. The functions will still fail with an exception regardless. The only difference will be in what order you throw the exceptions. If you have three tabs open (one activated, two dormant) after a session restore and only one content browser, the len(tabs) + 1 check would pass because you have four <tab> elements after opening a new one. The second content browser check would fail because you would only have two. Sorry to say, but I feel the two last iterations on this bug has been completely unwarranted. The first iteration of the patch passed all the tests, and in order to support the test scenario that you devised the functions would have be refactored to make out the difference between an active and a dormant content browser.

Pushed by atolfsen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/c5adc3b888b9 Look at window handles decrement when closing tab. r=whimboo