Closed Bug 1330309 Opened 7 years ago Closed 1 year ago

Investigate command modularisation for Marionette server

Categories

(Remote Protocol :: Marionette, defect, P3)

Version 3
defect

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: ato, Unassigned)

References

(Blocks 1 open bug)

Details

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.
Assignee: nobody → ato
Status: NEW → ASSIGNED
just setting priorities for this bug, we have a lot of other things that need doing before this bug is acted on.
Depends on: 1375425
Bug 1311041 will contain substantive changes to window tracking
that will be necessary before we can attempt modularisation of the
Marionette server.
Priority: -- → P3
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},
> };
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?
(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.
At least we could use it to validate data in the geckodriver/webdriver crates, right?
Blocks: 1399441
Assignee: ato → nobody
Status: ASSIGNED → NEW

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

Severity: normal → S3
Product: Testing → Remote Protocol

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

Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.