1723451 - Slow network connection cause Marionette handshake to fail over and over again

Status: RESOLVED FIXED

(Testing :: geckodriver, defect, P3)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

As noticed on https://github.com/mozilla/geckodriver/issues/1903 there is a recursion in the Marionette handshake when slow network setups are present. Especially for BrowserTime scenarios there is a need for it to test eg. on 3G and such.

I had a look and as it looks like it's a regression from bug 1525126 where I reduced the socket timeout to just 100ms for the handshake only. That seems to be a very short timeout and clearly needs to be increased or even be removed so that the normal timeout of the socket connection, which should be 60s, gets used.

Downside of the removal would be that it would take a certain amount of time longer to detect if there is still a browser process running in case of startup crashes.

It would be good to see this bug fixed for the geckodriver 0.30.0 release.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

To add more details:

• Setting shorter timeout: https://searchfox.org/mozilla-central/source/testing/geckodriver/src/marionette.rs#1126).
• Loop to detect browser startup crashes: https://searchfox.org/mozilla-central/source/testing/geckodriver/src/marionette.rs#1087-1111

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Also as discussed in the triage meeting moving to needinfo from James.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

This is actually not a recursion but repeatedly trying to pass the handshake with Marionette. But given that we always make use of the same 100ms timeout there is no difference until it ends-up in the 60s timeout. As such we should better increase the poll interval / timeout for each try until the maximum timeout has been reached.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Attachment: Bug 1723451 - [geckodriver] Use larger Marionette handshake timeout to not fail for slow connections.

Comment 5

[James Graham [:jgraham]]

Attachment: Bug 1723451 - Don't reconnect if we fail to read marionette handshake,

If we don't read the WebDriver handshake within the socket timeout,
don't reconnect, but reuse the same connection for subsequent attempts
at the read. This means that once the data is sent, OS-level buffering
should ensure we can read it on subsequent attempts, even if the
network is too slow to send the data inside the initial timeout.

Implementation wise, this turns the connection into an explicit state
machine. Implementation wise care is taken to ensure that only a
single state is ever in scope; in particular we can't have multiple
references to the stream.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

As it turned out the issues that were remaining have been caused by a bug in throttle, which got fixed in the 3.0 release recently. It caused also a slowdown of the network for localhost on MacOS, including any connection via adb. With the mentioned fix there is no longer a problem for folks at Browsertime, so we are fine to get the current patch landed.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d82efb06794c
[geckodriver] Use larger Marionette handshake timeout to not fail for slow connections. r=webdriver-reviewers,jgraham,jdescottes

Comment 8

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/d82efb06794c

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Is this something we'd want to backport? Not sure how geckodriver releases work :)

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

No, we always release geckodriver from mozilla-central. So we can mark any other branch as wontfix.