1418227 - Socket timeout is lost in TcpTransport.connect()

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P1)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

The changes in bug 1413852 caused a major regression in Marionette clients socket handling code. Hereby the specified socket timeout gets lost completely, and always `None` is used. As result a long running command never gets aborted, and Marionette hangs forever.

Btw this bug is most likely the reason for all the 1000s timeout problems for test jobs with Marionette involved.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1418227 - Indicate that sock instance is private.

The sock instance in TcpTransport should not be modified by code
outside of the class. As such indicate it least as private.

Review commit: https://reviewboard.mozilla.org/r/200700/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/200700/

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1418227 - Socket timeout is lost in TcpTransport.connect().

The socket_timeout getter should not return the timeout directly
from the socket instance, because right after socket.socket() it
is set to "None" internally, which will cause the original value
as requested by Marionette to be lost.

Review commit: https://reviewboard.mozilla.org/r/200702/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/200702/

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1418227 - TcpTransport.sock should not be used by consumers.

The socket is managed by TcpTransport and as such the
latter should be used to retrieve the socket timeout.

Review commit: https://reviewboard.mozilla.org/r/200710/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/200710/

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8929375 [details]
Bug 1418227 - Indicate that sock instance is private.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200700/diff/1-2/

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8929376 [details]
Bug 1418227 - Socket timeout is lost in TcpTransport.connect().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/200702/diff/1-2/

Comment 6

[David Burns :automatedtester]

Comment on attachment 8929375 [details]
Bug 1418227 - Indicate that sock instance is private.

https://reviewboard.mozilla.org/r/200700/#review205956

Comment 7

[David Burns :automatedtester]

Comment on attachment 8929383 [details]
Bug 1418227 - TcpTransport.sock should not be used by consumers.

https://reviewboard.mozilla.org/r/200710/#review205958

Comment 8

[David Burns :automatedtester]

Comment on attachment 8929376 [details]
Bug 1418227 - Socket timeout is lost in TcpTransport.connect().

https://reviewboard.mozilla.org/r/200702/#review205962

Comment 9

[Gabriele Svelto [:gsvelto]]

Since you've put it as a dependency, in the case of bug 1412035 there was an I/O error happening in the test harness code that was silently ignored and would hang the harness so I'm not sure if it's also related to this. The root cause for that bug should have been fixed in bug 1416028.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

This bug only happens if there is no response being sent from Firefox via the server socket to the Marionette client. It means that this can only happen in case of hangs in Firefox, or for a broken message manager in GeckoDriver and our framescript.

Maybe adding the other bug as dependency was wrong, and I will remove it. Thanks.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f75b0e5beca2
TcpTransport.sock should not be used by consumers. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/fad877b10735
Indicate that sock instance is private. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/3be73de9db10
Socket timeout is lost in TcpTransport.connect(). r=automatedtester

Comment 12

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/f75b0e5beca2
https://hg.mozilla.org/mozilla-central/rev/fad877b10735
https://hg.mozilla.org/mozilla-central/rev/3be73de9db10

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Please uplift those patches to mozilla-beta given that it fixes a major regression. Thanks.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Not sure why regression got removed from this bug, and why the uplift request doesn't work with checkin-needed (even Aryx told me lately to request it that way). I will from now on use the [checkin-needed-beta] whiteboard entry again.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c0da1f29b298
https://hg.mozilla.org/releases/mozilla-beta/rev/4eb0dd155ef2
https://hg.mozilla.org/releases/mozilla-beta/rev/bf5501f121c8