1330309 - Investigate command modularisation for Marionette server

Status: RESOLVED WONTFIX

(Remote Protocol :: Marionette, defect, P3)

Description

[Andreas Tolfsen ❲:ato❳]

In the last three years various pieces of the Marionette server have moved in the direction of increased compartmentalisation to make different parts of the server less entangled and tightly coupled.  In addition we have introduced some new interfaces that are not WebDriver specific, such as i11n and addon management features.

To prevent testing/marionette/driver.js from becoming a God object, it would be prudent to investigate splitting the file up so that the different interfaces are defined separately, and so that command dispatching does not need to go through driver.js.

I believe we have reached sufficient maturity in the Marionette code that this is a good time to investigate a modular plugin system for interfaces.  The interface that implements WebDriver could live in its own file, the addon management already does live in its own file, but still has hooks in driver.js since that is where the dispatcher sends all command requests.

Comment 1

[David Burns :automatedtester]

just setting priorities for this bug, we have a lot of other things that need doing before this bug is acted on.

Comment 2

[Andreas Tolfsen ❲:ato❳]

Bug 1311041 will contain substantive changes to window tracking
that will be necessary before we can attempt modularisation of the
Marionette server.

Comment 3

[Andreas Tolfsen ❲:ato❳]

One of the big painpoints with Marionette is validation of untrusted
JSON input.  I’ve been trying to figure out a solution for how to
automatically register commands and do validation against some form
of schema.

My current thinking is that it would be possible to associate some
dumbed-down version of a JSON schema to a command handler class
that could do the assertions for us.  It would be possible for the
assertions to pick up the input field name for the error message
using an ES6 technique:

> class ExecuteScript extends Marionette.Command {
>   handle({script, args, timeout, sandbox, newSandbox, file, line, debug} = {}) {
>     …
>   }
> }
> ExecuteScript.endpoint = "WebDriver:ExecuteScript";
> ExecuteScript.schema = {
>   script: {type: "string"},
>   args: {type: "string"},
>   timeout: {type: "number", optional: true},
>   sandbox: {type: "string", optional: true},
>   newSandbox: {type: "boolean", optional: true},
>   file: {from: "filename", type: "string", optional: true},
>   line: {type: "number", optional: true},
>   debug: {from: "debug_script", type: "boolean", optional: true},
> };

Comment 4

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

Maybe we could even have a generic JSON/Toml or whatever schema which could be used by any browser vendor to run argument validation steps for all the defined commands?

Comment 5

[Andreas Tolfsen ❲:ato❳]

(In reply to Henrik Skupin (:whimboo) from comment #4)

> Maybe we could even have a generic JSON/Toml or whatever schema
> which could be used by any browser vendor to run argument
> validation steps for all the defined commands?

That is what CDP does, I think.

I’m not sure it would be possible to share such a file with other
vendors since Marionette is an entirely custom protocol, i.e.
different to WebDriver.

Comment 6

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

At least we could use it to validate data in the geckodriver/webdriver crates, right?

Comment 7

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

Should this block the Marionette Fission bug 1518468? It might be good to have a plan before starting this work.

Comment 8

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

We are not planning to invest time in refactoring Marionette when it eventually gets replaced by WebDriver BiDi.