Bug 1421766 - Make geckodriver read marionette port from MarionetteActivePort in profile, r=webdriver-reviewers,jdescottes,whimboo

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.

Differential Revision: https://phabricator.services.mozilla.com/D136740

Cargo.lock

@@ -1908,6 +1908,7 @@ dependencies = [
  "serde_derive",
  "serde_json",
  "serde_yaml",
+ "tempfile",
  "url",
  "uuid",
  "webdriver",

testing/geckodriver/Cargo.toml

@@ -31,5 +31,8 @@ uuid = { version = "0.8", features = ["v4"] }
 webdriver = { path = "../webdriver" }
 zip = { version = "0.4", default-features = false, features = ["deflate"] }
 
+[dev-dependencies]
+tempfile = "3"
+
 [[bin]]
 name = "geckodriver"

testing/geckodriver/src/browser.rs

@@ -10,7 +10,8 @@ use mozprofile::preferences::Pref;
 use mozprofile::profile::{PrefFile, Profile};
 use mozrunner::runner::{FirefoxProcess, FirefoxRunner, Runner, RunnerProcess};
 use std::fs;
-use std::path::PathBuf;
+use std::io::Read;
+use std::path::{Path, PathBuf};
 use std::time;
 
 use webdriver::error::{ErrorStatus, WebDriverError, WebDriverResult};
@@ -22,7 +23,7 @@ pub(crate) enum Browser {
     Remote(RemoteBrowser),
 
     /// An existing browser instance not controlled by GeckoDriver
-    Existing,
+    Existing(u16),
 }
 
 impl Browser {
@@ -30,7 +31,15 @@ impl Browser {
         match self {
             Browser::Local(x) => x.close(wait_for_shutdown),
             Browser::Remote(x) => x.close(),
-            Browser::Existing => Ok(()),
+            Browser::Existing(_) => Ok(()),
+        }
+    }
+
+    pub(crate) fn marionette_port(&mut self) -> Option<u16> {
+        match self {
+            Browser::Local(x) => x.marionette_port(),
+            Browser::Remote(x) => x.marionette_port(),
+            Browser::Existing(x) => Some(*x),
         }
     }
 }
@@ -38,8 +47,10 @@ impl Browser {
 #[derive(Debug)]
 /// A local Firefox process, running on this (host) device.
 pub(crate) struct LocalBrowser {
-    process: FirefoxProcess,
+    marionette_port: u16,
     prefs_backup: Option<PrefsBackup>,
+    process: FirefoxProcess,
+    profile_path: PathBuf,
 }
 
 impl LocalBrowser {
@@ -79,6 +90,7 @@ impl LocalBrowser {
             )
         })?;
 
+        let profile_path = profile.path.clone();
         let mut runner = FirefoxRunner::new(&binary, profile);
 
         runner.arg("--marionette");
@@ -109,8 +121,10 @@ impl LocalBrowser {
         };
 
         Ok(LocalBrowser {
-            process,
+            marionette_port,
             prefs_backup,
+            process,
+            profile_path,
         })
     }
 
@@ -132,6 +146,17 @@ impl LocalBrowser {
         Ok(())
     }
 
+    fn marionette_port(&mut self) -> Option<u16> {
+        if self.marionette_port != 0 {
+            return Some(self.marionette_port);
+        }
+        let port = read_marionette_port(&self.profile_path);
+        if let Some(port) = port {
+            self.marionette_port = port;
+        }
+        port
+    }
+
     pub(crate) fn check_status(&mut self) -> Option<String> {
         match self.process.try_wait() {
             Ok(Some(status)) => Some(
@@ -146,10 +171,33 @@ impl LocalBrowser {
     }
 }
 
+fn read_marionette_port(profile_path: &Path) -> Option<u16> {
+    let port_file = profile_path.join("MarionetteActivePort");
+    let mut port_str = String::with_capacity(6);
+    let mut file = match fs::File::open(&port_file) {
+        Ok(file) => file,
+        Err(_) => {
+            trace!("Failed to open {}", &port_file.to_string_lossy());
+            return None;
+        }
+    };
+    if let Err(e) = file.read_to_string(&mut port_str) {
+        trace!("Failed to read {}: {}", &port_file.to_string_lossy(), e);
+        return None;
+    };
+    println!("Read port: {}", port_str);
+    let port = port_str.parse::<u16>().ok();
+    if port.is_none() {
+        warn!("Failed fo convert {} to u16", &port_str);
+    }
+    port
+}
+
 #[derive(Debug)]
 /// A remote instance, running on a (target) Android device.
 pub(crate) struct RemoteBrowser {
     handler: AndroidHandler,
+    marionette_port: u16,
 }
 
 impl RemoteBrowser {
@@ -185,13 +233,20 @@ impl RemoteBrowser {
 
         handler.launch()?;
 
-        Ok(RemoteBrowser { handler })
+        Ok(RemoteBrowser {
+            handler,
+            marionette_port,
+        })
     }
 
     fn close(self) -> WebDriverResult<()> {
         self.handler.force_stop()?;
         Ok(())
     }
+
+    fn marionette_port(&mut self) -> Option<u16> {
+        Some(self.marionette_port)
+    }
 }
 
 fn set_prefs(
@@ -284,12 +339,15 @@ impl PrefsBackup {
 #[cfg(test)]
 mod tests {
     use super::set_prefs;
+    use crate::browser::read_marionette_port;
     use crate::capabilities::FirefoxOptions;
     use mozprofile::preferences::{Pref, PrefValue};
     use mozprofile::profile::Profile;
     use serde_json::{Map, Value};
     use std::fs::File;
     use std::io::{Read, Write};
+    use std::path::Path;
+    use tempfile::tempdir;
 
     fn example_profile() -> Value {
         let mut profile_data = Vec::with_capacity(1024);
@@ -420,4 +478,24 @@ mod tests {
             .unwrap();
         assert_eq!(final_prefs_data, initial_prefs_data);
     }
+
+    #[test]
+    fn test_local_marionette_port() {
+        fn create_port_file(profile_path: &Path, data: &[u8]) {
+            let port_path = profile_path.join("MarionetteActivePort");
+            let mut file = File::create(&port_path).unwrap();
+            file.write_all(data).unwrap();
+        }
+
+        let profile_dir = tempdir().unwrap();
+        let profile_path = profile_dir.path();
+        assert_eq!(read_marionette_port(&profile_path), None);
+        assert_eq!(read_marionette_port(&profile_path), None);
+        create_port_file(&profile_path, b"");
+        assert_eq!(read_marionette_port(&profile_path), None);
+        create_port_file(&profile_path, b"1234");
+        assert_eq!(read_marionette_port(&profile_path), Some(1234));
+        create_port_file(&profile_path, b"1234abc");
+        assert_eq!(read_marionette_port(&profile_path), None);
+    }
 }

testing/geckodriver/src/marionette.rs

@@ -37,6 +37,7 @@ use std::path::PathBuf;
 use std::sync::Mutex;
 use std::thread;
 use std::time;
+use webdriver::capabilities::BrowserCapabilities;
 use webdriver::command::WebDriverCommand::{
     AcceptAlert, AddCookie, CloseWindow, DeleteCookie, DeleteCookies, DeleteSession, DismissAlert,
     ElementClear, ElementClick, ElementSendKeys, ExecuteAsyncScript, ExecuteScript, Extension,
@@ -110,8 +111,8 @@ impl MarionetteHandler {
         session_id: Option<String>,
         new_session_parameters: &NewSessionParameters,
     ) -> WebDriverResult<MarionetteConnection> {
-        let (capabilities, options) = {
         let mut fx_capabilities = FirefoxCapabilities::new(self.settings.binary.as_ref());
+        let (capabilities, options) = {
             let mut capabilities = new_session_parameters
                 .match_browser(&mut fx_capabilities)?
                 .ok_or_else(|| {
@@ -122,7 +123,7 @@ impl MarionetteHandler {
                 })?;
 
             let options = FirefoxOptions::from_capabilities(
-                fx_capabilities.chosen_binary,
+                fx_capabilities.chosen_binary.clone(),
                 &self.settings,
                 &mut capabilities,
             )?;
@@ -134,14 +135,38 @@ impl MarionetteHandler {
         }
 
         let marionette_host = self.settings.host.to_owned();
-        let marionette_port = self
+        let marionette_port = match self.settings.port {
-            .settings
+            Some(port) => port,
-            .port
+            None => {
-            .unwrap_or(get_free_port(&marionette_host)?);
+                // If we're launching Firefox Desktop version 95 or later, and there's no port
+                // specified, we can pass 0 as the port and later read it back from
+                // the profile.
+                let can_use_profile: bool = options.android.is_none()
+                    && !self.settings.connect_existing
+                    && fx_capabilities
+                        .browser_version(&capabilities)
+                        .map(|opt_v| {
+                            opt_v
+                                .map(|v| {
+                                    fx_capabilities
+                                        .compare_browser_version(&v, ">=95")
+                                        .unwrap_or(false)
+                                })
+                                .unwrap_or(false)
+                        })
+                        .unwrap_or(false);
+                if can_use_profile {
+                    0
+                } else {
+                    get_free_port(&marionette_host)?
+                }
+            }
+        };
 
-        let websocket_port = match options.use_websocket {
+        let websocket_port = if options.use_websocket {
-            true => Some(self.settings.websocket_port),
+            Some(self.settings.websocket_port)
-            false => None,
+        } else {
+            None
         };
 
         let browser = if options.android.is_some() {
@@ -167,10 +192,10 @@ impl MarionetteHandler {
                 self.settings.jsdebugger,
             )?)
         } else {
-            Browser::Existing
+            Browser::Existing(marionette_port)
         };
         let session = MarionetteSession::new(session_id, capabilities);
-        MarionetteConnection::new(marionette_host, marionette_port, browser, session)
+        MarionetteConnection::new(marionette_host, browser, session)
     }
 
     fn close_connection(&mut self, wait_for_shutdown: bool) {
@@ -740,7 +765,7 @@ fn try_convert_to_marionette_message(
                     flags: vec![AppStatus::eForceQuit],
                 },
             )),
-            Browser::Existing => Some(Command::WebDriver(
+            Browser::Existing(_) => Some(Command::WebDriver(
                 MarionetteWebDriverCommand::DeleteSession,
             )),
         },
@@ -1101,11 +1126,10 @@ struct MarionetteConnection {
 impl MarionetteConnection {
     fn new(
         host: String,
-        port: u16,
         mut browser: Browser,
         session: MarionetteSession,
     ) -> WebDriverResult<MarionetteConnection> {
-        let stream = match MarionetteConnection::connect(&host, port, &mut browser) {
+        let stream = match MarionetteConnection::connect(&host, &mut browser) {
             Ok(stream) => stream,
             Err(e) => {
                 if let Err(e) = browser.close(true) {
@@ -1121,16 +1145,15 @@ impl MarionetteConnection {
         })
     }
 
-    fn connect(host: &str, port: u16, browser: &mut Browser) -> WebDriverResult<TcpStream> {
+    fn connect(host: &str, browser: &mut Browser) -> WebDriverResult<TcpStream> {
         let timeout = time::Duration::from_secs(60);
         let poll_interval = time::Duration::from_millis(100);
         let now = time::Instant::now();
 
         debug!(
-            "Waiting {}s to connect to browser on {}:{}",
+            "Waiting {}s to connect to browser on {}",
             timeout.as_secs(),
             host,
-            port
         );
 
         loop {
@@ -1144,19 +1167,30 @@ impl MarionetteConnection {
                 }
             }
 
+            let last_err;
+
+            if let Some(port) = browser.marionette_port() {
                 match MarionetteConnection::try_connect(host, port) {
                     Ok(stream) => {
                         debug!("Connection to Marionette established on {}:{}.", host, port);
                         return Ok(stream);
                     }
                     Err(e) => {
+                        let err_str = e.to_string();
+                        last_err = Some(err_str);
+                    }
+                }
+            } else {
+                last_err = Some("Failed to read marionette port".into());
+            }
             if now.elapsed() < timeout {
-                        trace!("{}. Retrying in {:?}", e.to_string(), poll_interval);
+                trace!("Retrying in {:?}", poll_interval);
                 thread::sleep(poll_interval);
             } else {
-                        return Err(WebDriverError::new(ErrorStatus::Timeout, e.to_string()));
+                return Err(WebDriverError::new(
-                    }
+                    ErrorStatus::Timeout,
-                }
+                    last_err.unwrap_or_else(|| "Unknown error".into()),
+                ));
             }
         }
     }