WebDriver/Meetings/2019-01-21

Agenda

• How to improve handling of window manipulation commands to make them more reliable across platforms and by using headless/xvfb
  • Adding Telemetry to commands (follows on from the above discussion)

Minutes

Making window manipulation reliable, especially headless/Xvfb

whimboo

Handling window sizes is really hard across platforms.

We landed a big patch in 65 where we got rid of a couple of hangs in Marionette.

We tried going into fullscreen when we were in fullscreen, causing us not to return from the command.

Different platforms: macOS, Windows, Linux.

All three support headless.

Additionally we have Xvfb on Linux, which allows the tests hidden by using a different display.

By default, when using Xvfb, it does not use a WM.

This means you are not able to maximise, minimise, or fullscreen a window.

This is a bit hard now, because our window manipulation commands depend on events.

If you don’t get these events, you hang. [Or timeout, as we do currently.]

Fortunately the events *do* get fired when using a WM manager inside Xvfb.

But unfortunately we don’t typically use a WM.

All in all it means we have lots of different modes to take care about.

Other harnesses don’t have these cases to take care about.

The hardest problem is that we don’t know what the user is running.

I have spent a lot of time on this and it has been hard.

The test coverage we have in wdpsec is not complete yet, so I’m not able to just run those tests to check if the implementation works.

I have been writing new tests, but not all browsers support all features like screen.availLeft so I have to find a way to make the available screen rect helper cross-browser compatible

This makes it hard to validate if the commands are working correctly.

Events are really hard to work with, and as we have seen, we have used different types of events.

visibilitychange for Minimize Window, which is not fired for headless mode and as such we run into a timeout and not being able to detect via document.hidden if the window is minimized (1510305).

Maximize Window returns to early due to break out when sizemodechange event is received, and the window rect is not the final window rect.

What if we get rid of events altogether, and instead check for states only?

Problem is, with the change that landed in 65, we have at least two cases where we don’t raise a timeout error. And one where we do.

Problem is, the current timeout value of 1.5s is too low, causing our tests to fail eg on MacOS because the animations take already more than 1s, and the debounce callback timeout will be added on-top

But first question first: What do you think of getting rid of events?

ato

I have one reservation about polling the window state.

But just to be clear, you also mean the window rect, right?

whimboo

Fullscreen is fine but maximise is problematic.

For Minimize Window, instead of relying on document.hidden or the visibilitychange event, we could check if the docshell is active.

ato

We should talk about each command separately, because they all need unique approaches.

The problem I had in the past relying on state, especially when checking the window rect, was that if you query it too often it stops updating.

There is a problem with data moving from child to parent process: either it can return the wrong values or it stops propagating.

I have a question: you were able to get these working reliably except for Xvfb?

whimboo

That is correct.

ato

Asking maximise doesnt do anything in XVFB

So I think we talked about this last week, but you mentioned that window.maximize() didn’t actually *do anything* in Xvfb, without a WM?

Because originally I thought X would extend the window boundaries to the fullest extent of the screen.

But apparently that is *not* the case?

whimboo

That is right.

David also did a poll on Twitter to see how people are using Xvfb and there is a large number who use the defaults only.

That is, Xvfb without a WM.

And no WM means no events.

Headless also has some of these issues.

ato

Yes, but with headless these are bugs that we can fix.

AutomatedTester

I clearly had the wrong memory here.

I thought a lot of the maximise/fullscreen/minimise was done at a best-attempt, and that there was no guarantee that we could get into those states.

ato

I think David is right, the spec doesn’t make any guarantees about the success of any of the window manipulation commands.

It sort of doesn’t make the guarantees implicitly, as there are no internal assertions in the algorithms about whether they actually reached the state they were trying to reach.

AutomatedTester

Maybe we need to add a note to the spec.

For example mobile, fullscreen is acceptable but there are things that could block it.

In GeckoView, the application it is embedded in could block it.

This could cause it to report that it is in fullscreen, but it might not be.

ato

Another concern I have about WPT tests for this is what lowest common denominator we could reach for test assertions.

I imagine across all driver implementations, platforms, configurations, and modes there will be a fairly low number of absolute truths we can rely on.

In other words, I imagine the test assertions would have to be rather weak.

whimboo

I want to propose increasing the timeout for those promises which are currently set to 1.5 seconds, to let’s say, 5 seconds.

Going fullscreen on macOS takes longer than 1.5 seconds.

Having a larger timeout for the existing commands would maybe let the WM get enough time.

We have two commands where we ignore the timeout, and two where we do not.

ato

I think that sounds uncontroversial.

I have concerns about the testability of these commands because what we will be able to

ACTION whimboo: Disable returning timeout errors for window manipulation commands.

whimboo

What about using the window.fullScreen attribute and checking if the docshell is active?

This is what the other harnesses are doing.

ato

Using window.fullScreen is a good idea which ties us to the platform Fullscreen API.

So far we have relied on events tied to the WM.

Adding Telemetry to commands

AutomatedTester

One of the things we don’t do, is Telemetry.

Historically people have been against it, for example Google and Salesforce.

We need some telemetry data of some sort, about how people are using [our product].

If we need to go look at a certain command, if there is a higher-than-normal failure rate for one commands.

We can see what commands people are using.

We can make more data-driven decisions.

Instead of asking people, this cuts of the middle man.

ato

Telemetry is enabled for Marionette but has a specific flag to separate out data

There shouldn't be a technical reason why we cannot add this

AutomatedTester

Is there a bug?

ato

Yes there is a meta bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1401172

Caching should be handled by Telemetry and we shouldn’t have need to take care about this.

whimboo

We use a lot of restarts and creating new profiles so we should not lose data.

ato

Maybe Telemetry handles this during shutdown.

Or probably some way of flushing the data before shutdown.

Firefox Puppeteer and marionette_driver deprecation

Raphael

I read your announcement about the deprecation of marionette_driver.

For the Telemetry tests we use Firefox Puppeteer, which wraps marionette_driver, and I wonder what this announcement means for me.

whimboo

I wrote this library.

It was used a lot for opening new windows and interacting with UI elements.

But with the new, New Window command added to Marionette, I don’t think you actually need to use Puppeteer.

Previously we had code for reliably working with opening tabs and windows in Firefox Puppeteer, but we now have something built in to the Marionette server to open new browsing contexts.

I can also help you move your code over to use use marionette_driver directly.

ato

Yes, we want to deprecate it but for now only the package on PyPI.

The in-tree the client will continue to live for the forseeable future.

We have a long-term vision to replace marionette_driver with something better, because we realise that marionette_driver is not the best thing we can provide Mozilla developers with.

But we don’t have that right now, and we don’t have any immediate plans for removing marionette_driver in-tree.

Eventually we want to get rid of the client and get users to convert to geckodriver and a stock WebDriver client.

I reviewed the code in-tree that used marionette_driver, and the Telemetry test code did not concern me.

Raphael

Yes, that’s the answer I wanted to hear.

If I summarise again, in the mean time it makes sense to move away from Firefox Puppeteer to just use Marionette.

And then in the long-term away from marionette_driver altogether to something more standard.

whimboo

That is correct.

Intermittents update (whimboo)

• Regressions from window manipulation changes
  • [fixed] "WebDriver:SetWindowRect" throws invalid TimeoutError if window is resized to a not reachable dimension (Bug 1478358)
  • [investigation] Under some system setups (like headless or xvfb) "WebDriver:FullscreenWindow" (Bug 1519777) and "WebDriver:Maximize" (bug 1520284) are timing out
  • [needs patch] Refactoring/Adding more Wdspec window manipulation tests ()
• [fixed] Perma-failures on Android for Mn jobs caused by reftest changes was fixed by temporarily disabling the newly added unittest
• [needs fix] We are using different screen sizes, and still have the screensaver, taskbar, and notifications enabled for Mn jobs (Bug 1520175)
• [needs investigation] The marionette client hangs 1000s due to logged channel errors from Firefox (Bug 1521447)

Status updates

(Spoken status updates in bold.)

• ato
  • Corrected a reference to Marionette in the geckodriver documentation (1519869)

• whimboo
  • [wdspec] Enhance/complete window manipulation tests (Bug 1521028)
  • [geckodriver] Fix trace log documentation for options in Selenium Python binding (Bug 151992)
  • [geckodriver] Add support for the "New Window" command (Bug 1509513)
  • [geckodriver] Fix trace log documentation for options in Selenium Python binding (Bug 1519925)
  • [marionette] Bump Mn test jobs on Android to Tier1 (Bug 1519850)
  • [marionette] TimedPromise timed out after 1500 ms in "WebDriver:FullscreenWindow" (Bug 1519777)
  • [marionette] Decide how to handle window manipulation methods with xvfb and no window manager present (Bug 1520302)
  • [marionette] Use "preflight_run_cmd_suites" for Marionette for unique screen size and to disable various Windows notifications (Bug 1520175)

PTO/travel (⛄️)

• ato away Thursday 24 January