1705770 - Refactor MarionetteHandler to avoid exposing impossible states

Status: RESOLVED FIXED

(Testing :: geckodriver, enhancement, P3)

Description

[James Graham [:jgraham]]

MarionetteHandler currently has a lot of mutable state in the class e.g. the browser instance. This seems like a flawed design; the lifetime of a marionette-owned browser instance isn't the lifetime of the handler, it's the lifetime of the connection. Similarly the MarionetteConnection object exposes a state where the stream is None; this is purely a transient state which we don't need to allow.

Cleaning this up makes it possible to make static guarantees like "we don't reuse the browser object after closing the connection".

Comment 1

[James Graham [:jgraham]]

Attachment: Bug 1705770 - Refactor MarionetteHandler to avoid exposing invalid states,

Currently the MarionetteHandler class owns some data like the browser
instance which conceptually should live as long as a specific session,
not for the whole lifetime of the handler (which is basically the
lifetime of the server). This is problematic because it means we need
to consider the possibility of accessing that data after the session
is complete.

MarionetteHandler and MarionetteConnetion are also written in a way
that exposes a partially constructed state (constructing an instance
returns an object with several optional fields that are filled in
later). This means we also need to consider the possibility of these
fields being None when in fact it would always be a bug to be in that
situation.

To fix both these issues, this patch refactors the code around the
Marionette connection. All state that lives for the length of the
connection/session is put onto the MarionetteConnection
object. MarionetteHandler itself contains only the static
configuration data from GeckoDriver startup, plus an optional
connection (it would be good to also refactor this so that once a
session was started the MarionetteHandler itself would change type,
such that it's statically verifyiable that we never call things that
require a session before the session is started or after it's closed,
but that would require more changes in the WebDriver library).

In addition, constructing the Connection instance now creates the
connection and fully populates the fields.

Handling of the different browser types (local owned by GeckoDriver,
remtoe owned by GeckoDriver, and local not owned by GeckoDriver) is
moved out into the Browser enum and variants instead of being
different methods in MarionetteHandler.

When closing a connection it's necessary to call the close() method on
the browser object to ensure that it's closed (if owned by
GeckoDriver) and all state is cleaned up.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/82852180707c
Refactor MarionetteHandler to avoid exposing invalid states, r=webdriver-reviewers,whimboo

Comment 3

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/82852180707c