Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Description • 7 years ago

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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 6 • 7 years ago

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

	James Graham [:jgraham]
	Comment 7 • 7 years ago
mozreview-review

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.

	Andreas Tolfsen ❲:ato❳
	Comment 8 • 7 years ago
mozreview-review

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

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

	Andreas Tolfsen ❲:ato❳
	Comment 9 • 7 years ago
mozreview-review

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?

	Andreas Tolfsen ❲:ato❳
	Comment 10 • 7 years ago
mozreview-review

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.

	Andreas Tolfsen ❲:ato❳
	Comment 11 • 7 years ago
mozreview-review

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

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

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 12 • 7 years ago
mozreview-review-reply

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+?

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 13 • 7 years ago
mozreview-review-reply

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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 14 • 7 years ago
mozreview-review-reply

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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 15 • 7 years ago
mozreview-review-reply

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.

	Andrew Halberstadt [:ahal]
	Comment 23 • 7 years ago
mozreview-review

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.

	Andreas Tolfsen ❲:ato❳
	Comment 24 • 7 years ago
mozreview-review

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

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

	Andreas Tolfsen ❲:ato❳
	Comment 25 • 7 years ago
mozreview-review

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.

	Andreas Tolfsen ❲:ato❳
	Comment 26 • 7 years ago
mozreview-review-reply

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.

	James Graham [:jgraham]
	Comment 27 • 7 years ago
mozreview-review

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
```

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 28 • 7 years ago
mozreview-review-reply

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.

	James Graham [:jgraham]
	Comment 34 • 7 years ago
mozreview-review

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

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

	Pulsebot
	Comment 35 • 7 years ago

Pushed by hskupin@mozilla.com:
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

	Ryan VanderMeulen [:RyanVM]
	Comment 36 • 7 years ago
bugherder

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