The default bug view has changed. See this FAQ.

Support for Marionette protocol level 3 in Marionette Python client

RESOLVED FIXED in Firefox 44

Status

Testing
Marionette
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: ato, Assigned: ato)

Tracking

({ateam-marionette-client})

unspecified
mozilla44
ateam-marionette-client
Points:
---

Firefox Tracking Flags

(firefox44 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
To future-proof the Marionette Python client it should have support for level 3 of the Marionette protocol outlined in bug 1211489.

The changes required on the client side should this time be minimal as the command result data structures are not changing.  However, the client should expect to receive "Response" messages and send "Command" messages as outlined in the message format description in bug 1211489.
(Assignee)

Updated

2 years ago
Blocks: 1211489
Keywords: ateam-marionette-client
(Assignee)

Comment 1

2 years ago
try against level 2 server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=07608ba1a982
(Assignee)

Comment 2

2 years ago
try with fixes against level 2 server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=636f6b920561
(Assignee)

Comment 3

2 years ago
Created attachment 8674276 [details]
MozReview Request: Bug 1211503: Support for Marionette protocol level 3 in the Python client

Bug 1211503: Support for Marionette protocol level 3 in the Python client

Futures-proofs the Marionette Python client to have support for level
3 of the Marionette protocol outlined in bug 1211489.

This patch changes the marionette-transport API most notably by renaming
the MarionetteTransport class to TcpTransport and by splitting the
receive-capabilities of TcpTransport.send into a new function called
request.

Furthermore it introduces a message data structure for dealing with
incoming responses and commands, and for marshaling messages to send in
order to support all three protocol levels.

r=dburns
r=jgriffin
Attachment #8674276 - Flags: review?(jgriffin)
Attachment #8674276 - Flags: review?(dburns)
Comment on attachment 8674276 [details]
MozReview Request: Bug 1211503: Support for Marionette protocol level 3 in the Python client

https://reviewboard.mozilla.org/r/22207/#review19891

::: testing/marionette/driver/marionette_driver/marionette.py:770
(Diff revision 1)
> -            error = resp["error"]
> +            error = obj["error"]

are we expecting these to be here when the code hits the point? If it's not do we want the unhandled exception?

::: testing/marionette/transport/marionette_transport/transport.py:143
(Diff revision 1)
> -        is prepended by len(message) + ":".
> +        # protocol 3 and above

nit: comment doesnt match code. if we want `and above` it should be `>= 3`
Attachment #8674276 - Flags: review?(dburns) → review+
(Assignee)

Comment 5

2 years ago
https://reviewboard.mozilla.org/r/22207/#review19903

::: testing/marionette/transport/marionette_transport/transport.py:274
(Diff revision 1)
> +        to come back.  Allowed input is a ``Message`` instance or a JSON
> +        serialisable object.

Docs are wrong
(Assignee)

Comment 6

a year ago
Comment on attachment 8674276 [details]
MozReview Request: Bug 1211503: Support for Marionette protocol level 3 in the Python client

Bug 1211503: Support for Marionette protocol level 3 in the Python client

Futures-proofs the Marionette Python client to have support for level
3 of the Marionette protocol outlined in bug 1211489.

This patch changes the marionette-transport API most notably by renaming
the MarionetteTransport class to TcpTransport and by splitting the
receive-capabilities of TcpTransport.send into a new function called
request.

Furthermore it introduces a message data structure for dealing with
incoming responses and commands, and for marshaling messages to send in
order to support all three protocol levels.

r=dburns
r=jgriffin
(Assignee)

Comment 7

a year ago
https://reviewboard.mozilla.org/r/22207/#review19891

> nit: comment doesnt match code. if we want `and above` it should be `>= 3`

Well spotted, sir.
Attachment #8674276 - Flags: review?(jgriffin) → review+
Comment on attachment 8674276 [details]
MozReview Request: Bug 1211503: Support for Marionette protocol level 3 in the Python client

https://reviewboard.mozilla.org/r/22207/#review20001
(Assignee)

Updated

a year ago
Assignee: nobody → ato
Status: NEW → ASSIGNED
(Assignee)

Comment 9

a year ago
Comment on attachment 8674276 [details]
MozReview Request: Bug 1211503: Support for Marionette protocol level 3 in the Python client

Bug 1211503: Support for Marionette protocol level 3 in the Python client

Futures-proofs the Marionette Python client to have support for level
3 of the Marionette protocol outlined in bug 1211489.

This patch changes the marionette-transport API most notably by renaming
the MarionetteTransport class to TcpTransport and by splitting the
receive-capabilities of TcpTransport.send into a new function called
request.

Furthermore it introduces a message data structure for dealing with
incoming responses and commands, and for marshaling messages to send in
order to support all three protocol levels.

r=dburns
r=jgriffin

Comment 10

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cf388b1cf3a
https://hg.mozilla.org/mozilla-central/rev/4cf388b1cf3a
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44

Comment 12

a year ago
Created attachment 8678780 [details] [diff] [review]
A quick fix

Hi Andreas,

After the patches for this bug landed, marionette-webapi doesn't work anymore, and here is a short explanation. The first response we get after an “executeJSScript" or an “executeAsyncScript” is sent might be an emulator command instead of the actual response to the original request. Now, after marionette-client handle the emulator command received, it still takes the command as the actual response to the original request at the end of |_send_message(…)|, and thus an exception  is raised due to missing both |msg.result| and |msg.error| [1].

With the help from Edgar and John, I did some modification as this patch shows. Since both "executeJSScript" and "executeAsyncScript" start a new sub-session for running the testcase passed in, which means that multiple commands might be recieved since then. Thus, a loop is needed here to handle those commands and to wait for the actual response. To make things explicit, now the loop can only be entered when the origin request is either "executeJSScript" or “executeAsyncScript".

[1] https://dxr.mozilla.org/mozilla-central/source/testing/marionette/driver/marionette_driver/marionette.py#711
Flags: needinfo?(ato)
Hi Andreas, Jonathan and David,
Sorry to flag you all as I have no idea who is available to check this issue at the moment. Could any of you please check Ben's comment 12? This bug causes regression that marionette-webapi doesn't work anymore that needs to be fixed to not blocking tests. Thank you very much.
Flags: needinfo?(jgriffin)
Flags: needinfo?(dburns)
This is something ato will need to look at; he's traveling this week but should be able to get to it early next week.
Flags: needinfo?(jgriffin)
(In reply to Jonathan Griffin (:jgriffin) from comment #14)
> This is something ato will need to look at; he's traveling this week but
> should be able to get to it early next week.

Thank you Jonathan for the kind information.
Flags: needinfo?(dburns)
Hi Andreas,
Sorry to pin you again. We are now managing to have a workaround in our working environments to not be blocked by this bug. However, ideally we need marionette-webapi getting back to work as soon as possible. Would you please take a look?
(Assignee)

Comment 17

a year ago
bhsu, I don't really understand what you're trying to say.

From the patch you attached I understand that my patch removed the ability to run multiple emulator callbacks in a single invocation of executeScript/executeJSScript/executeAsyncScript.  Is this the only defect?

Basically we need the ability to run code such as:

    marionette.execute_async_script("""
        runEmulatorCmd("foo");
        runEmulatorCmd("bar");
        marionetteScriptFinished();
        """)

This doesn't work now because the second runEmulatorCmd will make the server return a command request, when the client is _actually_ expecting a response message with the "error" or "result" key.

I've filed bug 1223028 to follow up on this.  Please see the code I've attached there to see if this will resolve your problem.  My patch is essentially the same as the one you suggest, but I'm doubtful we need to move the loop into the try…catch block and test explicitly for the requested command name.
Flags: needinfo?(ato) → needinfo?(bhsu)

Comment 18

a year ago
Hi Andreas,

Thanks for your reply!

> From the patch you attached I understand that my patch removed the ability
> to run multiple emulator callbacks in a single invocation of
> executeScript/executeJSScript/executeAsyncScript.  Is this the only defect?

Yes, you're absolute right :)

> Basically we need the ability to run code such as:
> 
>     marionette.execute_async_script("""
>         runEmulatorCmd("foo");
>         runEmulatorCmd("bar");
>         marionetteScriptFinished();
>         """)
> 
> This doesn't work now because the second runEmulatorCmd will make the server
> return a command request, when the client is _actually_ expecting a response
> message with the "error" or "result" key.

Though it's not that important, in fact, it's the first |runEmulatorCmd(...)| that crashes marionette-client now, since it's mistaken as the response of the request command sent by |execute_async_script(...)| to start the test.

> I've filed bug 1223028 to follow up on this.  Please see the code I've
> attached there to see if this will resolve your problem.  My patch is
> essentially the same as the one you suggest, but I'm doubtful we need to
> move the loop into the try…catch block and test explicitly for the requested
> command name.

First, I think the loop should be put into the try...catch block, since marionette-client communicates with the the emulator via telent whose methods might raise certain errors according to [1]. Besides, both |_emulator_cmd(...)| and |_emulator_shell(...)| might raise an excption if some checks fail. Second, I don't have a strong preference for whether to place an explicit check if it's well documented. However, you listed three request commands which are invoked by |execute_script(...)|, |execute_js_script(...)| and |execute_async_script(...)| respectively, while there are only two of them listed in the comment in your patch. Do you mind checking it again? Thanks

[1] https://docs.python.org/2/library/telnetlib.html
Flags: needinfo?(bhsu)
(Assignee)

Comment 19

a year ago
(In reply to Ben Hsu [:bhsu] from comment #18)
> Though it's not that important, in fact, it's the first
> |runEmulatorCmd(...)| that crashes marionette-client now, since it's
> mistaken as the response of the request command sent by
> |execute_async_script(...)| to start the test.

Hm, okay.  Would you mind testing the patch locally?

> However, you listed three
> request commands which are invoked by |execute_script(...)|,
> |execute_js_script(...)| and |execute_async_script(...)| respectively, while
> there are only two of them listed in the comment in your patch. Do you mind
> checking it again? Thanks

Will add it, thanks.

Comment 20

a year ago
> Hm, okay.  Would you mind testing the patch locally?

Sure, tests under telephony/test/marionette/ passed perfectly :)

Though some other tests for other components are still failed, I believe those failures have nothing to do with this issue anymore.
(Assignee)

Comment 21

a year ago
(In reply to Ben Hsu [:bhsu] from comment #20)
> > Hm, okay.  Would you mind testing the patch locally?
> 
> Sure, tests under telephony/test/marionette/ passed perfectly :)
> 
> Though some other tests for other components are still failed, I believe
> those failures have nothing to do with this issue anymore.

Perfect, thank you so much for testing that!
You need to log in before you can comment on or make changes to this bug.