1414882 - Add handshake to Marionette client

Status: RESOLVED FIXED

(Testing :: Marionette Client and Harness, enhancement)

Description

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

Currently Marionette client just retrieves 16 byte of data from the hello string as sent by the socket server in `wait_for_port`, which is `50:{"application`. As such this data can not be used to really identify the application to be used. Here we might not have to care about the protocol level.

In `start_session` and especially `TcpTransport.connect` the application type, and protocol are at least retrieved from the hello string. But both are actually optional. As such we do not really recognize if the socket endpoint is allowed to talk to us.

We should fix the client to do a real handshake similar to what geckodriver is doing for the protocol. Both actually do not check for the application type, which should always be `gecko`.

As such I propose that we update `wait_for_port` to make use of the `TcpTransport` class for the connection attempts, and only return if the socket endpoint is a valid application, and matches the minimum requirements for the socket.

Comment 1

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

Attachment: Bug 1414882 - Remove unused Device.wait_for_port() in mozrunner.

This method has not a single caller and as such doesn't seem to
be necessary anymore.

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

Comment 2

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

Attachment: Bug 1414882 - Remove wait_for_port in favor of raise_for_port.

wait_for_port() is defined to return a boolean value, which
makes it impossible to return more detailed information in
case of a connection cannot be established. Further the
raise_for_port() method is a simple wrapper, without any
additional usage.

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

Comment 3

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

Attachment: Bug 1414882 - Marionette executor has to use raise_for_port.

To allow more fine-grained failure details when waiting for
a port, raise_for_port has to be used.

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

Comment 4

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

Attachment: Bug 1414882 - Add handshake to Marionette client.

A connection to Marionette server should only be accepted
if the application type equals to 'gecko', and the protocol
version as returned is supported by the client.

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

Comment 5

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

Attachment: Bug 1414882 - Require successful handshake in raise_for_port.

In case another application is still using the port, and the
handshake with Marionette server is not successful, the
function should return immediately. Otherwise it has to continue
to wait until a successful connection has been made..

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

Comment 6

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

A try push for the current state of the code can be found at:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d1c81556167c97084086fb5d713d6d271c4fd13

Comment 7

[James Graham [:jgraham]]

Comment on attachment 8926569 [details]
Bug 1414882 - Marionette executor has to use raise_for_port.

https://reviewboard.mozilla.org/r/197802/#review203026

This breaks the case where we're in a debugger and we want to wait ~forever for a port because the browser won't start until the developer specifically asks it to start.

Comment 8

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8926569 [details]
Bug 1414882 - Marionette executor has to use raise_for_port.

https://reviewboard.mozilla.org/r/197802/#review203038

Comment 9

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8926570 [details]
Bug 1414882 - Add handshake to Marionette client.

https://reviewboard.mozilla.org/r/197804/#review203044

::: testing/marionette/client/marionette_driver/transport.py:215
(Diff revision 1)
> +        if self.application_type != "gecko":
> +            raise IOError("Invalid application '{}' detected".format(self.application_type))
> +
> +        if self.protocol < self.min_protocol_level:
> +            raise IOError("Protocol level {} not supported".format(self.protocol))

I guess we probably don’t want to set self.application_type and
self.protocol if they don’t match, right?  Perhaps the checks
should be reversed so that the if-conditions and exceptions are
raised before we save the state.  Is this something we care about?

Comment 10

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8926571 [details]
Bug 1414882 - Require successful handshake in raise_for_port.

https://reviewboard.mozilla.org/r/197806/#review203054

This patch in particular seems like a big improvement.

::: testing/marionette/client/marionette_driver/transport.py:215
(Diff revision 1)
>          if self.application_type != "gecko":
> -            raise IOError("Invalid application '{}' detected".format(self.application_type))
> +            raise ValueError("Application type '{}' is not supported".format(
> +                self.application_type))
>  
>          if self.protocol < self.min_protocol_level:
> -            raise IOError("Protocol level {} not supported".format(self.protocol))
> +            raise ValueError("Protocol level {} is not supported".format(self.protocol))

I guess technically you should not introduce them as IOErrors in
the previous patch and change it again here; they could just be
introduced as ValueErrors to begin with, but I’m not terribly
fussed if you fix it or not.

Comment 11

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8926568 [details]
Bug 1414882 - Remove wait_for_port in favor of raise_for_port.

https://reviewboard.mozilla.org/r/197800/#review203056

Comment 12

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

Comment on attachment 8926568 [details]
Bug 1414882 - Remove wait_for_port in favor of raise_for_port.

https://reviewboard.mozilla.org/r/197800/#review203056

What is the reason for the r- here? Did you maybe hit r- but wanted to set r+?

Comment 13

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

Comment on attachment 8926569 [details]
Bug 1414882 - Marionette executor has to use raise_for_port.

https://reviewboard.mozilla.org/r/197802/#review203026

Oh, I see. That may also be a problem of Marionette itself given that we do not support the debugger argument yet. I will have a look how to get this fixed as best as possible.

Comment 14

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

Comment on attachment 8926570 [details]
Bug 1414882 - Add handshake to Marionette client.

https://reviewboard.mozilla.org/r/197804/#review203044

> I guess we probably don’t want to set self.application_type and
> self.protocol if they don’t match, right?  Perhaps the checks
> should be reversed so that the if-conditions and exceptions are
> raised before we save the state.  Is this something we care about?

I can change that for sure. Also we should add another check for the protocol level to guard against None, and other types beside int. Please check this patch again after I updated it.

Comment 15

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

Comment on attachment 8926571 [details]
Bug 1414882 - Require successful handshake in raise_for_port.

https://reviewboard.mozilla.org/r/197806/#review203054

> I guess technically you should not introduce them as IOErrors in
> the previous patch and change it again here; they could just be
> introduced as ValueErrors to begin with, but I’m not terribly
> fussed if you fix it or not.

Sorry, but those got applied to the wrong commit. It will be fixed with the next update.

Comment 16

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

Attachment: asdf

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

Comment 17

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

Comment on attachment 8926569 [details]
Bug 1414882 - Marionette executor has to use raise_for_port.

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

Comment 18

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

Comment on attachment 8926570 [details]
Bug 1414882 - Add handshake to Marionette client.

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

Comment 19

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

Comment on attachment 8926571 [details]
Bug 1414882 - Require successful handshake in raise_for_port.

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

Comment 20

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

Comment on attachment 8926569 [details]
Bug 1414882 - Marionette executor has to use raise_for_port.

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

Comment 21

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

Comment on attachment 8926570 [details]
Bug 1414882 - Add handshake to Marionette client.

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

Comment 22

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

Comment on attachment 8926571 [details]
Bug 1414882 - Require successful handshake in raise_for_port.

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

Comment 23

[Andrew Halberstadt [:ahal]]

Comment on attachment 8926567 [details]
Bug 1414882 - Remove unused Device.wait_for_port() in mozrunner.

https://reviewboard.mozilla.org/r/197798/#review203326

Lgtm. Probably a b2g leftover.

Comment 24

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8926568 [details]
Bug 1414882 - Remove wait_for_port in favor of raise_for_port.

https://reviewboard.mozilla.org/r/197800/#review203432

Comment 25

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8926569 [details]
Bug 1414882 - Marionette executor has to use raise_for_port.

https://reviewboard.mozilla.org/r/197802/#review203436

Deferring the rest of this patch to jgraham.

::: testing/web-platform/tests/tools/wptrunner/wptrunner/executors/executormarionette.py:67
(Diff revision 3)
> -        # XXX Move this timeout somewhere
> +        try:
> -        self.logger.debug("Waiting for Marionette connection")
> +            self.logger.debug("Waiting for Marionette connection")
> -        while True:
> +            while True:
> -            success = self.marionette.wait_for_port(startup_timeout)
> -            #When running in a debugger wait indefinitely for firefox to start
> +                try:
> +                    self.marionette.raise_for_port()
> -            if success or self.executor.debug_info is None:
> -                break
> +                    break
> +                except IOError:
> +                    # When running in a debugger wait indefinitely for Firefox to start
> +                    if self.executor.debug_info:
> +                        pass

I liked the wait_for_port approach better instead of relying on
exceptions for controlling code flow, but I’ll leave this up for
jgraham to decide.

Comment 26

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8926569 [details]
Bug 1414882 - Marionette executor has to use raise_for_port.

https://reviewboard.mozilla.org/r/197802/#review203436

> I liked the wait_for_port approach better instead of relying on
> exceptions for controlling code flow, but I’ll leave this up for
> jgraham to decide.

Sorry, did not intend to make this an issue.

Comment 27

[James Graham [:jgraham]]

Comment on attachment 8926569 [details]
Bug 1414882 - Marionette executor has to use raise_for_port.

https://reviewboard.mozilla.org/r/197802/#review203440

::: testing/web-platform/tests/tools/wptrunner/wptrunner/executors/executormarionette.py:75
(Diff revision 3)
> -            #When running in a debugger wait indefinitely for firefox to start
> +                    self.marionette.raise_for_port()
> -            if success or self.executor.debug_info is None:
> -                break
> +                    break
> +                except IOError:
> +                    # When running in a debugger wait indefinitely for Firefox to start
> +                    if self.executor.debug_info:

This doesn't work; it will ignore the exception in all cases. You either need to write `raise` or change it to

```
except IOError:
    if self.executor.debug_info is None:
        raise
```

Comment 28

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

Comment on attachment 8926569 [details]
Bug 1414882 - Marionette executor has to use raise_for_port.

https://reviewboard.mozilla.org/r/197802/#review203440

> This doesn't work; it will ignore the exception in all cases. You either need to write `raise` or change it to
> 
> ```
> except IOError:
>     if self.executor.debug_info is None:
>         raise
> ```

Ouch, you are right. Will be fixed with the next update.

Comment 29

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

Comment on attachment 8926568 [details]
Bug 1414882 - Remove wait_for_port in favor of raise_for_port.

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

Comment 30

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

Comment on attachment 8926567 [details]
Bug 1414882 - Remove unused Device.wait_for_port() in mozrunner.

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

Comment 31

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

Comment on attachment 8926569 [details]
Bug 1414882 - Marionette executor has to use raise_for_port.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/197802/diff/3-4/

Comment 32

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

Comment on attachment 8926570 [details]
Bug 1414882 - Add handshake to Marionette client.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/197804/diff/3-4/

Comment 33

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

Comment on attachment 8926571 [details]
Bug 1414882 - Require successful handshake in raise_for_port.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/197806/diff/3-4/

Comment 34

[James Graham [:jgraham]]

Comment on attachment 8926569 [details]
Bug 1414882 - Marionette executor has to use raise_for_port.

https://reviewboard.mozilla.org/r/197802/#review203774

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/48e08f18761e
Remove wait_for_port in favor of raise_for_port. r=ato
https://hg.mozilla.org/integration/autoland/rev/96cbaf1268ad
Remove unused Device.wait_for_port() in mozrunner. r=ahal
https://hg.mozilla.org/integration/autoland/rev/065dbcb0a21d
Marionette executor has to use raise_for_port. r=ato,jgraham
https://hg.mozilla.org/integration/autoland/rev/1843a786310a
Add handshake to Marionette client. r=ato
https://hg.mozilla.org/integration/autoland/rev/b453363d6968
Require successful handshake in raise_for_port. r=ato

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/48e08f18761e
https://hg.mozilla.org/mozilla-central/rev/96cbaf1268ad
https://hg.mozilla.org/mozilla-central/rev/065dbcb0a21d
https://hg.mozilla.org/mozilla-central/rev/1843a786310a
https://hg.mozilla.org/mozilla-central/rev/b453363d6968

Comment 37

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

Moving bugs for Marionette client due to component changes.