Closed Bug 1421766 Opened 7 years ago Closed 3 years ago

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

Categories

(Testing :: geckodriver, enhancement, P3)

enhancement

Tracking

(firefox98 fixed)

RESOLVED FIXED
98 Branch
Tracking Status
firefox98 --- fixed

People

(Reporter: whimboo, Assigned: jgraham)

References

(Blocks 3 open bugs, )

Details

Attachments

(1 file)

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.
Priority: -- → P1
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.
Blocks: 1401129
Priority: P1 → P2
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?
Oh, right, maybe not when it's passed in as an argument rather than as a zip file.
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.
Assignee: nobody → ato
Status: NEW → ASSIGNED
Andreas, will you fix that or should I take this bug? I would like to get it shipped with the next release soonish.
Flags: needinfo?(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.
Assignee: ato → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(ato)
Depends on: 1431459
Priority: P2 → P3
Version: 59 Branch → unspecified
Blocks: 1441204
No longer blocks: 1401129
Blocks: 1466818
No longer blocks: 1441204
No longer blocks: 1466818
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)>)
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?
Flags: needinfo?(avast)
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...
Flags: needinfo?(avast)
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
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.
Blocks: 1495062
No longer blocks: 1491288
Summary: No connection to Marionette can be made due to unknown port for custom profile → Read Marionette port from profile preference (was: No connection to Marionette can be made due to unknown port for custom profile)

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

Blocks: 1520585
No longer blocks: 1495062
No longer blocks: 1520585
No longer blocks: 1573798
Blocks: 1584911
Blocks: 1649094
No longer blocks: 1584911
Blocks: 1668243
No longer blocks: 1649094
No longer blocks: 1668243
Blocks: 1686110
Blocks: 1723202
No longer blocks: 1686110

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

Depends on: 1240830

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.

Blocks: 1240830
Depends on: 1735162
No longer depends on: 1240830
Summary: Read Marionette port from profile preference (was: No connection to Marionette can be made due to unknown port for custom profile) → Read Marionette port from MarionetteActivePort file within the Firefox profile directory

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.

Assignee: nobody → james
Status: NEW → ASSIGNED
Blocks: 1751844
Blocks: 1749673
No longer blocks: 1751844

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

Summary: Read Marionette port from MarionetteActivePort file within the Firefox profile directory → [desktop] Read Marionette port from MarionetteActivePort file within the Firefox profile directory
Blocks: 1752284
Pushed by james@hoppipolla.co.uk: https://hg.mozilla.org/integration/autoland/rev/2ef8122c2767 Make geckodriver read marionette port from MarionetteActivePort in profile, r=webdriver-reviewers,jdescottes,whimboo
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 98 Branch
Regressions: 1755261
Regressions: 1755312
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: