Closed Bug 1520302 Opened 5 years ago Closed 2 years ago

Decide how to handle window manipulation methods with xvfb and no window manager present

Categories

(Remote Protocol :: Marionette, enhancement, P3)

Product:
Remote Protocol
Component:
Marionette
Type:
enhancement

Tracking

(firefox65 wontfix, firefox66 wontfix, firefox67 wontfix, firefox68 affected)

Status:
RESOLVED WORKSFORME
Tracking Flags:
Tracking Status
firefox65 --- wontfix
firefox66 --- wontfix
firefox67 --- wontfix
firefox68 --- affected

People

(Reporter: whimboo, Assigned: whimboo)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

Bug 1520302 - [marionette] Add missing unit tests for fullscreen and minimize.
47 bytes, text/x-phabricator-request
Details | Review

While working on bug 1519777 I noticed that when using xvfb the windowState property doesn't reflect the current state of the window.

When the test gets started it reports 1 which means that the browser window is started in maximized mode. Going into fullscreen and checking win.fullScreen which reports true, the state is still 1. Only a call to SetWindowRect actually sets it to 3. But then it stays at this value, and going into fullscreen doesn't change it again.

As we know xvfb isn't the best way to run tests without a display those days, but people still do it. As such we need a reliable solution.

I wonder if there is the possibility to remove WindowState completely from our code, and use other properties like fullScreen to determine the window state.

See also bug 1510305 which shows that for minimize there is another inconsistency.

Blocks: webdriver
Severity: normal → major
Flags: needinfo?(ato)
Priority: -- → P2

If you look at when the the window state is mutated in the C++ GTK
widget layer code, it happens when we receive signals from X11
telling us the winodw has entered a different state. We receive
no such signals when there isn’t a WM to handle putting the window
into the requested mode, but the display server still attempts its
best by extending the bondaries of the window.

The Fullscreen API that WebDriver mandates using is different to
the fullscreen capability of the browser application window (e.g.
Shift-Command-F). Without having checked the implementation my
guess is that ChromeWindow/WindowProxy.fullScreen gets linked with
this API rather than the window state, which would explain why it
is true when windowState remains unchanged, as the latter is linked
to the X11 signals.

Flags: needinfo?(ato)

Oh, if it is that unreliable we should most likely not make use of it at all? I will apply the changes for fullscreen in my patch on bug 1519777.

But I will experiment a bit more if there is a way to get it working. Checking other tests especially under mochitest (plain, chrome) doesn't help because all those tests don't work under xvfb.

Maybe this bug should block bug 1519777.

Blocks: 1519777

Given that I haven't really used xvfb before, I had to dig into it for a while. Now Andreas' comment 2 also makes sense. By default and just using xvfb-run no window manager will be present. That is the mode I used for testing my patches on bug 1519777, and no wonder why the window state (maximized, minimized, fullscreen) hasn't been changed. There is simply no way.

But installing fluxbox and using it for running the Marionette tests under xvfb, it works like a charm.

Andreas, I wonder if we should simply ignore the case when no window manager has been given, and make that mode unsupported. Means people would have to use a simple window manager like fluxbox or better switch to headless mode.

From my perspective that would be the best choice. Otherwise our code would contain a lot of spaghetti code trying to workaround those problems for a mode we don't really want to support anyway. Maybe it makes sense to just hang in case any of the window manipulation commands don't get into the requested state.

So if we want to do it, I would suggest listing for the resize event, which is always the last event being fired. Additionally for safety we could also add specific events for eg fullscreen. Otherwise we could also use a PollPromise to check for the expected window state.

Flags: needinfo?(ato)
Summary: windowState values are wrongly reported when using xvfb under Linux → Decide how to handle window manipulation methods with xvfb and no window manager present
Blocks: 1515867

Alternatively we could also use a longer timeout of eg. 30s to make it obvious, and really raise a timeout error without hiding it.

Also I wonder if we should allow the window manipulation commands to raise Timeout errors per WebDriver spec, if it's not successful. It's not only Firefox which affected by that issue but any browser.

If there is no window manager then I am happy for us to error out. People have the choice of headless if they really dont want to see the browser but if they really want XVFB then they need to pick a window manager

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #3)

I wonder if we should simply ignore the case when no window
manager has been given, and make that mode unsupported. Means
people would have to use a simple window manager like fluxbox or
better switch to headless mode.

From my perspective that would be the best choice. Otherwise our
code would contain a lot of spaghetti code trying to workaround
those problems for a mode we don't really want to support
anyway. Maybe it makes sense to just hang in case any of the
window manipulation commands don't get into the requested state.

I have reservations about making WebDriver with Xvfb an unsupported
use case, because this is what a lot of users are using in the wild
and has (maybe still even is) the recommended way by Selenium to
run tests.

I think you overestimate the technical competence of WebDriver users
if you suggest to them to run Xvfb with a window manager. My concern
is that not handling the no-WM will be detrimental to Marionette’s
performance and will look Firefox look worse than Chrome. I’m right
here in assuming that changing the code the way you suggest will
incur a performance regression in this case?

So if we want to do it, I would suggest listing for the
resize event, which is always the last event being
fired. Additionally for safety we could also add specific events
for eg fullscreen. Otherwise we could also use a PollPromise
to check for the expected window state.

That sounds like an interesting idea. Have you tried it out in
practice?

Flags: needinfo?(ato)

We continued the discussion on IRC and decided to raise an unsupported operation error if one of the minimize, maximize, fullscreen, or restore commands fail with a timeout. All other commands will be unaffected, also if a window manager is used for xvfb.

What we need is a sane default timeout for all those commands. On bug 1519777 I collected some stats for fullscreen, which seem to take longest especially on MacOS due to the animation. There you can expect a duration up to 1.5s for entering and exiting the fullscreen mode. On Windows and Linux the command is nearly instantaneous and durations might go up to 300ms. Minimize and Maximize usually take around 1s on MacOS with some spikes up to 2s. As such the minimal timeout value should be 5s, which twice that duration I have seen so far, and not long enough to raise the above error.

What's puzzling are the inconsistent initial values of windowState when using different screen resolutions with xvfb. For large screens I get 1 (normal window) as expected. But for small resolutions like 2014x768 I see an initial state of 3 (maximized window), which is wrong because the window doesn't take up all the space.

IMO we should get a patch on this bug landed for Firefox 65, to not cause timeout errors for people using slower machines.

Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Priority: P2 → P1
If Marionette is run via xvfb but without using a window manager
certain events will not be fired, which would cause a hang in
Marionette. A timeout of 5s will be used after which an
unsupported operation error will be raised.
Blocks: 1520284
Depends on: 1510305

I just noticed when testing the current patch that there is also a timeout for minimizing a window when running the tests in headless mode. The expected visibilitychange event on the content window is not fired at all. I asked Brendan on bug 1510305 which seems to be related to this problem.

There are two options now... by default we would raise an unsupported operation error because the command will hang, or we change the TimedPromise to a PollPromise and checking the windowState for being 2. The latter would still cause problems for our unit and wdspec tests, but won't change the behavior.

To not break headless mode I will add a temporary workaround by checking if headless mode is active, and then wait until the windowState change. All other modes will continue to use the visibilitychange event.

When running Marionette tests in headless mode the command
"WebDriver:MaximizeWindow" times out because no `sizemodechange`
event is getting fired. Instead we have to wait for a single
`resize event`.

Depends on D16803

With bug 1520791 fixed we could use the docShellIsActive flag similar to Mochitests to determine if the window has been minimized. This would allow us to move to a PollPromise instead of waiting for events which might never occur. I will have a look at it today.

Depends on: 1520791
Depends on: 1521028

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment
#12)

To not break headless mode I will add a temporary workaround by
checking if headless mode is active, and then wait until the
windowState change. All other modes will continue to use the
visibilitychange event.

If I understand correctly, this makes sense.

One more note... when I run the updated wdspec tests on Linux I noticed that "WebDriver:MaximizeWindow" returns too early and as such doesn't return the maximal available size. This is due to listening to the "sizemodechange" event, which is fired too early.

I'm currently not working on this bug which is also blocked at the moment.

Assignee: hskupin → nobody
Status: ASSIGNED → NEW
Priority: P1 → P3

We had an issue with Selenium's GitHub actions where tests were failing because no window manager was installed. By using the following steps these tests are working now:

      - name: Install Fluxbox
        run: sudo apt-get -y install fluxbox
[..]
      - name: Start Xvfb
        run: Xvfb :99 &
      - name: Start Fluxbox
        run: fluxbox -display :99 &

I assume that I should write a blog post explaining why a window manager is needed and that no-one should run headful browser tests with just a plain xvfb environment.

See Also: → 1789823
Assignee: nobody → hskupin
Status: NEW → ASSIGNED

Emilio, with your latest changes to Marionette on bug 1780212 I assume that we do not need any of the first 3 attached patches anymore and that we should mark them as abandoned? I'll still have a look what's needed for a better test coverage though.

Flags: needinfo?(emilio)

Yeah I don't think those should be needed.

Flags: needinfo?(emilio)
Attachment #9037176 - Attachment is obsolete: true
Attachment #9037049 - Attachment is obsolete: true
Attachment #9037175 - Attachment is obsolete: true

(In reply to Emilio Cobos Álvarez (:emilio) from comment #23)

Yeah I don't think those should be needed.

Great. I abandoned all of them and will check the test improvements later this or next week.

Blocks: 1521028
No longer depends on: 1521028
Severity: major → S2

As we decided to always require a Window Manager to be running with xvfb I don't think that there is much left to do for this bug.

I had a look at the Marionette unit tests and I don't think it makes sense anymore to get these added given that we have appropriate WebDriver tests available now.

That means I'm going to close this bug now but might still write a blog post with some instructions as well. I'll add a link once that has been done.

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → WORKSFORME
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: