WebDriver/Meetings/2019-01-07

From MozillaWiki
Jump to: navigation, search

Agenda

Intermittents update (whimboo)

  • Marionette jobs with mozprocess timeouts of 1000s
    • [fixed] When running Marionette tests Firefox downloads and installs updates even when turned off via prefs (Bug 1508726)
    • There is a random IPC crash which isn't clear right now and needs feedback from KanRu
  • Switch to window doesn't wait long enough until target window is active and has the focus (Bug 1335085)
    • [disabled on Linux] TestScreenCaptureChrome.test_formats failed a lot for beta simulations for a XUL dialog (Bug 1504201)
    • Also some WPT tests seem to be affected by that which rely on a correctly initialized window
    • To fix this the patch series for the new window command has to be landed first
  • Regressions from Window manipulation patch
    • Delays in minimizing a window because 'visibilitychange' event is registered to the chrome but not content window.
    • Various Set Window Rect failures because the position is wrongly reported, which a directly following Get Window Rect call doesn't show
    • And some more related to inappropriate timeout errors
    • We want more trace logging for Marionette here but it would need some commits from the New Window command
  • [backed out] Regressions from the New Window command patch
    • Undefined outer window id when there is a delay in making the outerWindowId available to the parent process. Happens because PollPromise uses a timeout of 2s by default. We are going to fix that by not using a timeout for the PollPromise by default at all, which will prevent those intermittent failures
    • On Android 7.0 a couple of WPT tests under html/browsers fail loading the test page. As investigated so far it seems to be related to the changes of handling the observer notification.
    • If newly opened window shouldn't be focused, there is a hang on Linux when refocusing the original window due to missing focus and activate events

Minutes

Action item followup

ato
Both done.
Create deprecation document for Marionette client: https://lists.mozilla.org/pipermail/tools-marionette/2019-January/000060.html
File bug for document.hidden in headless mode: https://bugzilla.mozilla.org/show_bug.cgi?id=1510305

CDP/Fission

Resources:

ato
[Summarises the CDP meeting last week.]
AutomatedTester
There are also notes from bgrins, hopefully these are available to everyone.
https://groups.google.com/a/mozilla.com/d/msg/puppeteer-review/JxiETwKiKQI/-Vlze9M0EAAJ
whimboo
How much time will you have to work on Marionette intermittents?
ato
I'm currently tasked with cleaning up my CDP MVP and see how much work is needed to make it use the child actor infrastructure used in Firefox frontend code.
My impression is that the child actor system is very toolkit-centric and I'm not sure how easy it is to reuse it outside of toolkit.
After I've done some work on this, I will spend time on finishing up these patches and land a CDP MVP in central.
whimboo
OK, so I will try to keep on top of all of the intermittents for now.
AutomatedTester
We mustn't determine our own work at the expense of helping other teams.
We also have work to do that we need to accomplish.
whimboo
WebDriver:MinimizeWindow is broken and adds a few seconds to our test runs, but no one really seem to use this API for testing. It’s more about our wdspec and Mn unit tests
WebDriver:MaximizeWindow is widely used and has some lower intermittents but real tests might not be affected by that.
With WebDriver:SetWindowRect we see some hangs but no reports about that yet.
ato
Maybe that’s a good entry-point here: prioritise which command it is important to fix.
whimboo
Adding more logging would also be useful for debugging these problems. It should be easy to add once my new window patch lands.

Fission related

whimboo
For navigation, especially, we store a lot of state inside the frame script.
We then set up the state again when the frame script is reloaded.
We could get rid of the pending command queue if this stuff was encapsulated better.
ato
Yes, Marionette pretends that each request to the listener is atomic by automatically re-issuing a new message from the pending queue before returning control to the user.
CDP solves this problem by triggering navigation in one request, then emitting the events for loading separately of that.
To make Marionette Fission compatible we would have to do something similar, only internally.
Think of WebDriver as an internal CDP client.
We would have to trigger navigation, then *wait* for these events in Marionette chrome space.
There’s some overlap here I think between the work we need to do for CDP and Marionette.
Perhaps we could port the Marionette or WebDriver API over to CDP and re-use the same primitives we use there, if those are already Fission-compatible.
whimboo
For now we could register all necessary event listeners globally in the frame script, and emit Marionette specific events to the parent process.
With that all state handling could be done in driver.js.
ato
That would be a good step on the way of turning the navigation code into a child actor, but we will also have to implement navigation and DOM event emitting for CDP.
At the moment we handle remoteness changes on page navigation in Marionette fairly well.
There are other examples of Gecko components that don’t handle it at all.
Fission is probably over a year into the future, so there’s no immediate pressing need to do this.
But I *am* concerned that we don’t duplicate work with our limited engineering resources (two engineers).

geckodriver regressions

whimboo
We also have some geckodriver regressions, for example the Switch To Frame argument returning the no such frame error instead of invalid argument.
It's not high priority for me, as I want to land the New Window command first.
But at some point we will need to fix this.
I have asked Google if they are interested in changing chromedriver to invalid argument for the wrong JSON type.
If they do, we can change the WebDriver specification and not have to do anything special with serde.
ato
Yeah, with serde you will have to do some kind of custom coercion, and inside there you send back no such frame for the wrong type check.
whimboo
Or we can just accept a string and get Marionette to return the correct error which would be my preferred way to go if no spec changes are wanted.
ato
In any case, we should try that new code we are going to land is Fission compatible.
whimboo
It would be good to know what the Fission pre-requisits and no-goes are so that we can try to not violate that and not land incompatible code
ato
I can think of two examples.
Generally, keeping state in frame scripts is not a good idea.
More specifically, code should be compartmentalised because you don’t want one big God-actor that listens for every event and handles everything.
Examples we need to fix in Marionette are how capabilities are queried in the frame script.
Currently we use the parent MM’s initialProcessData map to copy everything to the child process.
But after that we never update it, which means capability values can be old, e.g. when the WebDriver:SetTimeouts command is used.
It is a problem similar to what we have with preferences in Firefox, except that is slightly more complicated.
We can’t copy every preference to every child process as it would have a substantial memory footprint.
But then, how do you deal with keeping preferences up-to-date?
I think eventually they landed on some complicated Round Robin-algorithm for this, but I’m not too sure.
Suffice to say, data synchronisation in a message passing IPC architecture is hard.
That said, we can get away with being slightly naïve with capabilities as it’s unlikely to cause any performance penalty to do things in a slightly non-performant way.

Status updates

(Spoken status updates in bold.)

  • ato
    • Added some references to wpt.fyi to the WebDriver README (WebDriver PR 1386)
    • Addressed some test failures with the patch to remove the scriptTimeout parameter to WebDriver:ExecuteScript and WebDriver:ExecuteAsyncScript (1510929)
      • This unblocks rmu’s patch to support indefinite script durations over in 1128997
    • Re-enabled window maximisation test when window covers screen (1496489)
    • Started looking at writing better screenshot tests, but need some information from jimevans (1494208)
    • Released webdriver crate 0.38.1, unblocking some of eijebong’s work in Servo
    • CDP work
      • Spent a considerable amount of time thinking about CDP and drawing diagrams
      • Started refactoring my prototype to use the child actor model now popular in Firefox frontend code, and to be a plain CDP implementation instead of a mix of Juggler and CDP
  • whimboo
    • [wpt] MarionetteTestharnessProtocolPart._close_windows() doesn't close any user prompts (Bug 1517796)
    • [marionette] Implement New Window command (Bug 1504756)
    • [marionette] Updated release notes for Firefox 64 on MDN
    • [marionette] "WebDriver:SetWindowRect" should not return window state (Bug 1517587)
    • [fxui] Fix update tests to set "app.update.disabledForTesting" to False (Bug 1511312)

PTO/travel (⛄️)

  • ato away Tuesday 8 January
Retrieved from "https://wiki.mozilla.org/index.php?title=WebDriver/Meetings/2019-01-07&oldid=1205862"