1421766 - [desktop] Read Marionette port from MarionetteActivePort file within the Firefox profile directory

Status: RESOLVED FIXED

(Testing :: geckodriver, enhancement, P3)

Description

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

As isolated today from https://github.com/mozilla/geckodriver/issues/1058 there is a problem with the Marionette connection because the port marionette creates the server socket on is not known to geckodriver. Instead geckodriver tries to connect to a port it automatically got when checking for a free port. The result is a connection timeout after those 600 tries.

To allow geckodriver to connect to Marionette for the given instance of Firefox, several ways would be possible. After a team discussion today, we agreed on the following steps:

1. geckodriver continuous to find a free port by querying via port 0

2. If a custom profile has been specified via `args` inside the `moz:firefoxOptions` capability, the path of the profile has to be extracted. 

2.1. Inside the profile open the file `prefs.js` and replace the value of the preference `marionette.port` with the above retrieved port number. Store the changes afterward.

3. Continue to start Firefox with the profile

With step 2 Marionette will know which port to use, when starting the server socket. Once that is done, also geckodriver can connect to the known port.

Comment 1

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

For a temporary workaround see https://github.com/mozilla/geckodriver/issues/1058#issuecomment-347952270.

Comment 2

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

Given that there is a workaround for the problem we can lower the priority to P2. But it would be still great to have this fixed for the next geckodriver release.

Comment 3

[James Graham [:jgraham]]

I feel like the way this was supposed to work was by adding the preference to the profile passed in. Do we not do that?

Comment 4

[James Graham [:jgraham]]

Oh, right, maybe not when it's passed in as an argument rather than as a zip file.

Comment 5

[Andreas Tolfsen ❲:ato❳]

Yes, I believe we skip setting marionette.port when the profile
path is given.  We will still want to set the “override” prefs,
marionette.port and marionette.log.level.

Comment 6

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

Andreas, will you fix that or should I take this bug? I would like to get it shipped with the next release soonish.

Comment 7

[Andreas Tolfsen ❲:ato❳]

This is quite a hard bug to fix because we pass off ownership of the
profile to rust_mozrunner.  We somehow need to pass the ownership back
to geckodriver so it can mutate it, then back to rust_mozrunner again.
I would much prefer changing rust_mozrunner’s API altogether,
but that requires upstream changes because it does not live in tree.

In any case I’m happy for you to try to figure this out.

Comment 8

[Georg Horn]

I have made a patch that fixes the problem like described in the first post.

This was my first time of programming in rust, so i dont know if my code is very elegant but at least it works for me...

diff -c -r geckodriver-0.21.0/src/marionette.rs geckodriver-0.21.0-patched/src/marionette.rs
*** geckodriver-0.21.0/src/marionette.rs        Fri Jun 15 22:46:25 2018
--- geckodriver-0.21.0-patched/src/marionette.rs        Fri Sep 14 18:28:47 2018
***************
*** 53,58 ****
--- 53,61 ----
  use capabilities::{FirefoxCapabilities, FirefoxOptions};
  use logging;
  use prefs;
+ use regex::Regex;
+ use regex::NoExpand;
+ use std::fs;

  // localhost may be routed to the IPv6 stack on certain systems,
  // and nsIServerSocket in Marionette only supports IPv4
***************
*** 471,476 ****
--- 474,489 ----
              runner.arg("-jsdebugger");
          }
          if let Some(args) = options.args.as_ref() {
+           let mut found = false;
+           for i in 0..args.len() {
+               if found {
+                   try!(self.update_port_in_profile(port, &args[i]));
+                   break;
+               }
+               if args[i] == "-profile" {
+                   found = true;
+               }
+           }
              runner.args(args);
          }

***************
*** 484,489 ****
--- 497,516 ----

          Ok(())
      }
+
+     fn update_port_in_profile(&mut self, port: u16, profile_dir: &String) -> WebDriverResult<()>
+     {
+       let mut filename: String = profile_dir.to_owned();
+       filename.push_str("/prefs.js");
+       let content = fs::read_to_string(&filename).expect("Unable to read prefs.js");
+       let re = Regex::new(r#"marionette.port", [0-9]*"#).unwrap();
+       let tmp = format!(r#"marionette.port", {}"#, port);
+       let result = re.replace(&content, NoExpand(&tmp));
+       let mut file = File::create(&filename).expect("Unable to open prefs.js");
+       file.write_all(result.as_bytes()).expect("Unable to write to prefs.js");
+
+         Ok(())
+     }

      pub fn set_prefs(&self, port: u16, profile: &mut Profile, custom_profile: bool,
                       extra_prefs: Vec<(String, Pref)>)

Comment 9

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

Hi Avast. Thank you for the proposal in how to get this bug fixed. Generally we only accept real patch files, and as such you would have to create the patch based on the latest state in mozilla-central. Please see the following link for details:

https://firefox-source-docs.mozilla.org/testing/geckodriver/geckodriver/index.html#for-developers

Otherwise I want to refer to comment 7 of this bug, which outlines how a proper solution to fix the underlying issue should be done.

So would you have a bit of time to work with Andreas and get a patch completed?

Comment 10

[Georg Horn]

Hello Hendrik, sorry for my stupid name, i have entered the real name in my profile now.

I possibly do not understand how geckodriver and firefox work together, but as far as i understand it is useless to modify the profile that is created by geckodriver and passed to firefox, because firefox is started with a completely other profile.

I am using the perl bindings and the following script:

$driver = Selenium::Remote::Driver->new(
    'extra_capabilities' => {
        'moz:firefoxOptions' => {
            'args' => [ "-profile", "$ENV{'HOME'}/.mozilla/firefox/default" ],
        },
    }
);

creates a folder /tmp/rust_mozprofile.... which contains a user.js which in turn contains a line "user_pref("marionette.port", 33573);" but firefox is started with "/usr/bin/firefox" "-marionette" "-profile" "/home/horn/.mozilla/firefox/default" and the prefs.js in /home/horn/.mozilla/firefox/default contains the line "user_pref("marionette.port", 2828);"

So i personally think the solution described in the first post of this bug is correct here and i tried to implement that.

of course one can discuss if this way of passing the marionette port-number is optimal at all. I personally believe that firefox should determine a free port number and open a socket on that port and then pass the portnumber to geckodriver who then can connect to the port. possibly this can simply be done by making firefox print the portnumber to stdout and geckodriver read that portnumber from a pipe. But this is all speculation from my side...

Comment 11

[Andreas Tolfsen ❲:ato❳]

From what you describe, I would not expect the temporary profile
to be created, although maybe we are creating that unconditionally?
That would seem like a separate bug.

It then correctly recognises the /home/horn/.mozilla/firefox/default
profile you pass by moz:firefoxOptions.  The problem with this
approach is that geckodriver does not know how to write the Marionette
port number to this profile.  2828 is the default port number, but
geckodriver assigns a random port number so you can operate on
multiple Firefoxen simultaneously.  It is obviously a bug that we
don’t support custom profiles yet.

You may have greater luck launching geckodriver like this: "geckodriver
--marionette-port 2828" as that forces the port to be the default
port already defined in /home/horn/.mozilla/firefox/default, and
as geckodriver passes the -marionette flag to Firefox, this should
let you connect and use the custom profile.

IHTH

Comment 12

[Georg Horn]

Yes, the temporary profile is created also if "-profile" is given in args.

> It then correctly recognises the /home/horn/.mozilla/firefox/default
> profile you pass by moz:firefoxOptions. The problem with this
> approach is that geckodriver does not know how to write the Marionette
> port number to this profile.

That's what my patch does and what was proposed in the description of this bug, it opens the file prefs.js in /home/horn/.mozilla/firefox/default and replaces the port number with the current random port number.

This also works with multiple firefox instances at one time if your test script provides a separate copy of the original profile folder for each instance it starts. I do that like this:

$moz_prof_dir = "$ENV{'HOME'}/.mozilla/firefox/profile.$$";
system("cp -a $ENV{'HOME'}/.mozilla/firefox/default $moz_prof_dir");
$driver = Selenium::Remote::Driver->new(
  extra_capabilities => {
    'moz:firefoxOptions' => {
        args => [ "-profile", $moz_prof_dir ],
    }
  }
);

Works perfectly for me. If geckodriver creates the profile folder, it too creates one for each firefox session and writes the port number to the file user.js. If you provide the profile by yourself, you tell geckodriver where this profile lives and it just has to insert the proper port number into that profile instead of creating it completely.

> You may have greater luck launching geckodriver like this: "geckodriver
> --marionette-port 2828" as that forces the port to be the default
> port already defined in /home/horn/.mozilla/firefox/default

That's the workaround that was proposed above and it works indeed, but it does NOT work with multiple firefoxes simultaneously.

Comment 14

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

We won't be able to fix that for the 0.24 release. Moving out to 0.25.

Comment 15

[parthibdutta69]

This problem persists. When will 0.25 release happen?

Comment 16

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

The work that we wanna do on bug 1240830 should help to get rid of this problem.

Comment 17

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

Bug 1240830 is now a meta bug and as such should be blocked by this work.

Since bug 1735162 Marionette writes the MarionetteActivePort file to the profile directory. It only contains the port Marionette is running on. And it's way easier to implement as having to read and parse the prefs.js file.

Comment 18

[James Graham [:jgraham]]

Attachment: Bug 1421766 - Make geckodriver read marionette port from MarionetteActivePort in profile,

When no marionette port is explicitly specified, and the Firefox
version is >= 95, pass in a port number of 0 from geckodriver to
firefox, so that marionette binds on a free port. Then read the port
it used from the profile. This avoids the possibility of races between
geckodriver picking a free port and marionette binding to that port.

This could also be used on Android, but isn't implemented as part of
this patch.

Comment 19

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

Note that with the attached patch the support only gets added to desktop. I'll file a follow-up for Android support.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2ef8122c2767
Make geckodriver read marionette port from MarionetteActivePort in profile, r=webdriver-reviewers,jdescottes,whimboo

Comment 21

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/2ef8122c2767