Implement fail-safe handling of tabs

RESOLVED FIXED in Firefox 63

Status

P1
normal
RESOLVED FIXED
4 years ago
8 months ago

People

(Reporter: whimboo, Assigned: ato)

Tracking

Version 2
mozilla63
Points:
---

Firefox Tracking Flags

(firefox63 fixed)

Details

(Whiteboard: [lib][blocked])

Attachments

(1 attachment)

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.
Blocks: 1121725
Depends on: 1127831
No longer blocks: 1119715, 1121725
Product: Mozilla QA → Testing
(Assignee)

Comment 1

8 months ago
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.
Depends on: 1311041
Priority: -- → P3
QA Contact: hskupin
(Assignee)

Updated

8 months ago
No longer depends on: 1121691, 1121702, 1127831
(Assignee)

Comment 2

8 months ago
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.
Assignee: nobody → ato
Blocks: 1311041
Status: NEW → ASSIGNED
No longer depends on: 1311041
Priority: P3 → P1
Comment hidden (mozreview-request)
(Reporter)

Comment 4

8 months ago
mozreview-review
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.
Attachment #8989787 - Flags: review?(hskupin)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

8 months ago
mozreview-review-reply
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.
(Reporter)

Comment 7

8 months ago
mozreview-review
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.
Attachment #8989787 - Flags: review?(hskupin) → review+
(Assignee)

Comment 8

8 months ago
mozreview-review-reply
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 hidden (mozreview-request)
(Assignee)

Updated

8 months ago
Attachment #8989787 - Flags: review+ → review?(hskupin)
(Reporter)

Comment 10

8 months ago
mozreview-review-reply
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.
Attachment #8989787 - Flags: review?(hskupin) → review+
(Assignee)

Comment 11

8 months ago
(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.

Comment 12

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

Comment 13

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c5adc3b888b9
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.