Originally reported as: https://github.com/mozilla/geckodriver/issues/1837

Whenever there is an active session for an already established connection, there is the possibility that other connection attempts from clients could happen. Those would get a new session by creating a new GeckoDriver instance from the driver factory. When these extra sessions get closed eg due to a client disconnect, the session will be closed and also the JSWindowActors unregistered.

Under such a situation the original session won't work anymore because the appropriate JSWindowActors cannot be found anymore.

Given that we actually only support a single session we should enforce a single connection only, and bail out early for new connection attempts.

As the WebDriver spec says, there should be only a single active session:

A remote end that is not an intermediary node has at most one active session at a given time.

As such we should restrict to a single session, and deny any further connection attempts while an active session exists.

This would be great to have fixed for bug 1691399.

Lets wait until bug 1691402 has been fixed.

So right now Marionette is able to handle multiple connections:
https://searchfox.org/mozilla-central/rev/31b583bbcc9cfaacd93e785c05dda0e87d7cca4e/testing/marionette/server.js#60

I'm not sure when and why this was added in the past. Maybe there was a specific reason for it? But that raises the question in how we actually want to have Marionette to behave. It's clear that we want to only support a single WebDriver session, but shall different client be able to share this session once they are aware of the session id? If that is the case whenever a connection is closed we should not destroy the GeckoDriver instance as long as it is the very last active connection. Otherwise we should just get rid of handling multiple connections, by only allowing a single one.

As it looks like this has been introduced when moving away from the DevTools DebuggerServer on bug 797529. Given that this server clearly has to work with multiple connections, the Marionette model wouldn't actually need it.

The benefit of having this support right now is that other clients could make a connection and re-use the current session as long as they have the session id. Such a behavior would help users of Selenium when running tests on remote machines, and where a connection could die. Tests could try to re-connect to the existing session also for sanely shutting down the browser. Note that terminating geckodriver would keep the instance of Firefox running until bug 1430064 is fixed.

As such I might propose to keep multiple connections (also to minimize risks for regressions), and restrict to a single instance of the GeckoDriver class in Marionette.

James and Julian, what's your take on that?

Do we have a use case for multiple connections? Reconnecting to the same websocket server in case an existing connection is dropped makes some sense, but I think it's going to require care to actually make that work (e.g. it affects how we handle failure to send/recv and raises questions about whether it's OK for the client to just miss events we can't send or should be buffering them).

I assume the one/many distinction is in practice going to be quite small, so I'm happy with whatever seems easy here.

It would be easier to just drop the multiple connections support and restrict it to a single one. That way we can already abort early when a client wants to connect, while an already open connection is present. Downside would be if something might change in the future in regards of how many sessions a remote end could have open. Then we might have to revert the code. I see less likelihood for that for the WebDriver HTTP connection, so my feeling is that this would be fine.

We never had a use-case for it but noticed that recently by that geckodriver issue:
https://github.com/mozilla/geckodriver/issues/1837

I did some investigation of the underlying code and I would like to keep the multiple connections implementation because it will then closely match what we have in mind for WebDriver BiDi. Here is what I have seen...

Given that we do not have the session layer in between the connection and the driver, everything gets passed directly to the driver instance. And here each accepted connection actually has it's very own instance of the driver. That means that we will have to check all known connections, and filter out those driver instances that do not have an active session. If there is one connection with an active session no more client connection requests should be accepted, and the code in TCPListener.onSocketAccepted has to immediately deny the connection and return before a new TCPConnection gets created.

So what happens when a client deletes the session? The Marionette client closes the socket connection, which results in the connection to be removed from the connection list. As such a new connection attempt could create a new WebDriver session. Instead geckodriver immediately closes the browser, and as such there is nothing we would have to care about.

Accidental connection drops due to network outages shouldn't be a problem given that we run the server socket for connections from localhost only. There might still be some way to kill active connections by the user/system. In such a case the connection will close, and an active session via the driver instance is gone.

Nevertheless I wonder why we allow --marionette-host for geckodriver, which won't work anyway since bug 1344748 has been landed and we no longer accept remote connections. James, shall we remove the option for geckodriver?

I think the question is "are there any use cases for specifying different names for local interfaces?" (e.g. 127.0.0.1 vs localhost). I'm not sure what the answer to that is.

If we want to remove it it's mechaniscally pretty easy (ignoreing docs changes, etc.):

diff --git a/testing/geckodriver/src/main.rs b/testing/geckodriver/src/main.rs
index ca894e7c2e2fa..bd850b4681a26 100644
--- a/testing/geckodriver/src/main.rs
+++ b/testing/geckodriver/src/main.rs
@@ -165,7 +165,6 @@ fn parse_args(app: &mut App) -> ProgramResult<Operation> {
 
     let binary = matches.value_of("binary").map(PathBuf::from);
 
-    let marionette_host = matches.value_of("marionette_host").unwrap();
     let marionette_port = match matches.value_of("marionette_port") {
         Some(s) => match u16::from_str(s) {
             Ok(n) => Some(n),
@@ -180,7 +179,6 @@ fn parse_args(app: &mut App) -> ProgramResult<Operation> {
         Operation::Version
     } else {
         let settings = MarionetteSettings {
-            host: marionette_host.to_string(),
             port: marionette_port,
             binary,
             connect_existing: matches.is_present("connect_existing"),
@@ -276,14 +274,6 @@ fn make_app<'a, 'b>() -> App<'a, 'b> {
                 .value_name("BINARY")
                 .help("Path to the Firefox binary"),
         )
-        .arg(
-            Arg::with_name("marionette_host")
-                .long("marionette-host")
-                .takes_value(true)
-                .value_name("HOST")
-                .default_value("127.0.0.1")
-                .help("Host to use to connect to Gecko"),
-        )
         .arg(
             Arg::with_name("marionette_port")
                 .long("marionette-port")
diff --git a/testing/geckodriver/src/marionette.rs b/testing/geckodriver/src/marionette.rs
index 80c74f4866bad..c48badc9e17a3 100644
--- a/testing/geckodriver/src/marionette.rs
+++ b/testing/geckodriver/src/marionette.rs
@@ -93,7 +93,6 @@ pub struct MarionetteHandshake {
 
 #[derive(Default)]
 pub struct MarionetteSettings {
-    pub host: String,
     pub port: Option<u16>,
     pub binary: Option<PathBuf>,
     pub connect_existing: bool,
@@ -151,7 +150,7 @@ impl MarionetteHandler {
             logging::set_max_level(l);
         }
 
-        let host = self.settings.host.to_owned();
+        let host = "127.0.0.1".to_string();
         let port = self.settings.port.unwrap_or(get_free_port(&host)?);
 
         match options.android {

I cannot actually say. As such I filed bug 1711451 for the geckodriver part.

https://hg.mozilla.org/mozilla-central/rev/3c7d0220d187