Bug 1755261 - Improve handling of named profiles, r=webdriver-reviewers,whimboo

With named profiles we can't expect to write prefs, or to read the port out
of the profile path. So don't do these things.

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

testing/geckodriver/src/browser.rs

@@ -3,7 +3,7 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 use crate::android::AndroidHandler;
-use crate::capabilities::FirefoxOptions;
+use crate::capabilities::{FirefoxOptions, ProfileType};
 use crate::logging;
 use crate::prefs;
 use mozprofile::preferences::Pref;
@@ -35,11 +35,11 @@ impl Browser {
         }
     }
 
-    pub(crate) fn marionette_port(&mut self) -> Option<u16> {
+    pub(crate) fn marionette_port(&mut self) -> WebDriverResult<Option<u16>> {
         match self {
             Browser::Local(x) => x.marionette_port(),
             Browser::Remote(x) => x.marionette_port(),
-            Browser::Existing(x) => Some(*x),
+            Browser::Existing(x) => Ok(Some(*x)),
         }
     }
 }
@@ -50,7 +50,7 @@ pub(crate) struct LocalBrowser {
     marionette_port: u16,
     prefs_backup: Option<PrefsBackup>,
     process: FirefoxProcess,
-    profile_path: PathBuf,
+    profile_path: Option<PathBuf>,
 }
 
 impl LocalBrowser {
@@ -71,30 +71,37 @@ impl LocalBrowser {
             )
         })?;
 
-        let is_custom_profile = options.profile.is_some();
+        let is_custom_profile = matches!(options.profile, ProfileType::Path(_));
 
         let mut profile = match options.profile {
-            Some(x) => x,
-            None => Profile::new()?,
+            ProfileType::Named => None,
+            ProfileType::Path(x) => Some(x),
+            ProfileType::Temporary => Some(Profile::new()?),
         };
 
-        let prefs_backup = set_prefs(
-            marionette_port,
-            &mut profile,
-            is_custom_profile,
-            options.prefs,
-            allow_hosts,
-            allow_origins,
-            jsdebugger,
-        )
-        .map_err(|e| {
-            WebDriverError::new(
-                ErrorStatus::SessionNotCreated,
-                format!("Failed to set preferences: {}", e),
+        let (profile_path, prefs_backup) = if let Some(ref mut profile) = profile {
+            let profile_path = profile.path.clone();
+            let prefs_backup = set_prefs(
+                marionette_port,
+                profile,
+                is_custom_profile,
+                options.prefs,
+                allow_hosts,
+                allow_origins,
+                jsdebugger,
             )
-        })?;
+            .map_err(|e| {
+                WebDriverError::new(
+                    ErrorStatus::SessionNotCreated,
+                    format!("Failed to set preferences: {}", e),
+                )
+            })?;
+            (Some(profile_path), prefs_backup)
+        } else {
+            warn!("Unable to set geckodriver prefs when using a named profile");
+            (None, None)
+        };
 
-        let profile_path = profile.path.clone();
         let mut runner = FirefoxRunner::new(&binary, profile);
 
         runner.arg("--marionette");
@@ -150,15 +157,22 @@ impl LocalBrowser {
         Ok(())
     }
 
-    fn marionette_port(&mut self) -> Option<u16> {
+    fn marionette_port(&mut self) -> WebDriverResult<Option<u16>> {
         if self.marionette_port != 0 {
-            return Some(self.marionette_port);
+            return Ok(Some(self.marionette_port));
         }
-        let port = read_marionette_port(&self.profile_path);
-        if let Some(port) = port {
-            self.marionette_port = port;
+        if let Some(profile_path) = self.profile_path.as_ref() {
+            let port = read_marionette_port(profile_path);
+            if let Some(port) = port {
+                self.marionette_port = port;
+            }
+            return Ok(port);
         }
-        port
+        // This should be impossible, but it isn't enforced
+        Err(WebDriverError::new(
+            ErrorStatus::SessionNotCreated,
+            "Port not known when using named profile",
+        ))
     }
 
     pub(crate) fn check_status(&mut self) -> Option<String> {
@@ -217,9 +231,16 @@ impl RemoteBrowser {
         let handler = AndroidHandler::new(&android_options, marionette_port, websocket_port)?;
 
         // Profile management.
-        let is_custom_profile = options.profile.is_some();
-
-        let mut profile = options.profile.unwrap_or(Profile::new()?);
+        let (mut profile, is_custom_profile) = match options.profile {
+            ProfileType::Named => {
+                return Err(WebDriverError::new(
+                    ErrorStatus::SessionNotCreated,
+                    "Cannot use a named profile on Android",
+                ));
+            }
+            ProfileType::Path(x) => (x, true),
+            ProfileType::Temporary => (Profile::new()?, false),
+        };
 
         set_prefs(
             handler.marionette_target_port,
@@ -252,8 +273,8 @@ impl RemoteBrowser {
         Ok(())
     }
 
-    fn marionette_port(&mut self) -> Option<u16> {
-        Some(self.marionette_port)
+    fn marionette_port(&mut self) -> WebDriverResult<Option<u16>> {
+        Ok(Some(self.marionette_port))
     }
 }
 
@@ -369,7 +390,7 @@ impl PrefsBackup {
 mod tests {
     use super::set_prefs;
     use crate::browser::read_marionette_port;
-    use crate::capabilities::FirefoxOptions;
+    use crate::capabilities::{FirefoxOptions, ProfileType};
     use mozprofile::preferences::{Pref, PrefValue};
     use mozprofile::profile::Profile;
     use serde_json::{Map, Value};
@@ -429,7 +450,10 @@ mod tests {
         let opts = FirefoxOptions::from_capabilities(None, &marionette_settings, &mut caps)
             .expect("Valid profile and prefs");
 
-        let mut profile = opts.profile.expect("valid firefox profile");
+        let mut profile = match opts.profile {
+            ProfileType::Path(profile) => profile,
+            _ => panic!("Expected ProfileType::Path"),
+        };
 
         set_prefs(2828, &mut profile, true, opts.prefs, vec![], vec![], false)
             .expect("set preferences");
@@ -519,13 +543,13 @@ mod tests {
 
         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);
+        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/capabilities.rs

@@ -367,6 +367,19 @@ impl AndroidOptions {
     }
 }
 
+#[derive(Debug, PartialEq)]
+pub enum ProfileType {
+    Path(Profile),
+    Named,
+    Temporary,
+}
+
+impl Default for ProfileType {
+    fn default() -> Self {
+        ProfileType::Temporary
+    }
+}
+
 /// Rust representation of `moz:firefoxOptions`.
 ///
 /// Calling `FirefoxOptions::from_capabilities(binary, capabilities)` causes
@@ -376,7 +389,7 @@ impl AndroidOptions {
 #[derive(Default, Debug)]
 pub struct FirefoxOptions {
     pub binary: Option<PathBuf>,
-    pub profile: Option<Profile>,
+    pub profile: ProfileType,
     pub args: Option<Vec<String>>,
     pub env: Option<Vec<(String, String)>>,
     pub log: LogOptions,
@@ -419,27 +432,36 @@ impl FirefoxOptions {
             rv.env = FirefoxOptions::load_env(options)?;
             rv.log = FirefoxOptions::load_log(options)?;
             rv.prefs = FirefoxOptions::load_prefs(options)?;
-            rv.profile = FirefoxOptions::load_profile(options)?;
+            if let Some(profile) = FirefoxOptions::load_profile(options)? {
+                rv.profile = ProfileType::Path(profile);
+            }
         }
 
         if let Some(args) = rv.args.as_ref() {
             let os_args = parse_args(args.iter().map(OsString::from).collect::<Vec<_>>().iter());
             if let Some(path) = get_arg_value(os_args.iter(), Arg::Profile) {
-                if rv.profile.is_some() {
+                if let ProfileType::Path(_) = rv.profile {
                     return Err(WebDriverError::new(
                         ErrorStatus::InvalidArgument,
                         "Can't provide both a --profile argument and a profile",
                     ));
                 }
                 let path_buf = PathBuf::from(path);
-                rv.profile = Some(Profile::new_from_path(&path_buf)?);
+                rv.profile = ProfileType::Path(Profile::new_from_path(&path_buf)?);
             }
 
-            if get_arg_value(os_args.iter(), Arg::NamedProfile).is_some() && rv.profile.is_some() {
-                return Err(WebDriverError::new(
-                    ErrorStatus::InvalidArgument,
-                    "Can't provide both a -P argument and a profile",
-                ));
+            if get_arg_value(os_args.iter(), Arg::NamedProfile).is_some() {
+                if let ProfileType::Path(_) = rv.profile {
+                    return Err(WebDriverError::new(
+                        ErrorStatus::InvalidArgument,
+                        "Can't provide both a -P argument and a profile",
+                    ));
+                }
+                // See bug 1757720
+                warn!("Firefox was configured to use a named profile (`-P <name>`). \
+                       Support for named profiles will be removed in a future geckodriver release. \
+                       Please instead use the `--profile <path>` Firefox argument to start with an existing profile");
+                rv.profile = ProfileType::Named;
             }
         }
 
@@ -1233,7 +1255,10 @@ mod tests {
         firefox_opts.insert("profile".into(), encoded_profile);
 
         let opts = make_options(firefox_opts, None).expect("valid firefox options");
-        let mut profile = opts.profile.expect("valid firefox profile");
+        let mut profile = match opts.profile {
+            ProfileType::Path(profile) => profile,
+            _ => panic!("Expected ProfileType::Path"),
+        };
         let prefs = profile.user_prefs().expect("valid preferences");
 
         println!("{:#?}", prefs.prefs);
@@ -1249,7 +1274,26 @@ mod tests {
         let mut firefox_opts = Capabilities::new();
         firefox_opts.insert("args".into(), json!(["--profile", "foo"]));
 
-        make_options(firefox_opts, None).expect("Valid args");
+        let options = make_options(firefox_opts, None).expect("Valid args");
+        assert!(matches!(options.profile, ProfileType::Path(_)));
+    }
+
+    #[test]
+    fn fx_options_args_named_profile() {
+        let mut firefox_opts = Capabilities::new();
+        firefox_opts.insert("args".into(), json!(["-P", "foo"]));
+
+        let options = make_options(firefox_opts, None).expect("Valid args");
+        assert!(matches!(options.profile, ProfileType::Named));
+    }
+
+    #[test]
+    fn fx_options_args_no_profile() {
+        let mut firefox_opts = Capabilities::new();
+        firefox_opts.insert("args".into(), json!(["--headless"]));
+
+        let options = make_options(firefox_opts, None).expect("Valid args");
+        assert!(matches!(options.profile, ProfileType::Temporary));
     }
 
     #[test]

testing/geckodriver/src/marionette.rs

@@ -4,7 +4,7 @@
 
 use crate::browser::{Browser, LocalBrowser, RemoteBrowser};
 use crate::build;
-use crate::capabilities::{FirefoxCapabilities, FirefoxOptions};
+use crate::capabilities::{FirefoxCapabilities, FirefoxOptions, ProfileType};
 use crate::command::{
     AddonInstallParameters, AddonUninstallParameters, GeckoContextParameters,
     GeckoExtensionCommand, GeckoExtensionRoute, CHROME_ELEMENT_KEY,
@@ -145,6 +145,7 @@ impl MarionetteHandler {
                 // 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()
+                    && options.profile != ProfileType::Named
                     && !self.settings.connect_existing
                     && fx_capabilities
                         .browser_version(&capabilities)
@@ -464,8 +465,7 @@ impl MarionetteSession {
                 };
                 let page_load = try_opt!(
                     try_opt!(
-                        resp.result
-                            .get("pageLoad"),
+                        resp.result.get("pageLoad"),
                         ErrorStatus::UnknownError,
                         "Missing field: pageLoad"
                     )
@@ -1173,7 +1173,7 @@ impl MarionetteConnection {
 
             let last_err;
 
-            if let Some(port) = browser.marionette_port() {
+            if let Some(port) = browser.marionette_port()? {
                 match MarionetteConnection::try_connect(host, port) {
                     Ok(stream) => {
                         debug!("Connection to Marionette established on {}:{}.", host, port);

testing/mozbase/rust/mozprofile/src/profile.rs

@@ -19,6 +19,12 @@ pub struct Profile {
     user_prefs: Option<PrefFile>,
 }
 
+impl PartialEq for Profile {
+    fn eq(&self, other: &Profile) -> bool {
+        self.path == other.path
+    }
+}
+
 impl Profile {
     pub fn new() -> IoResult<Profile> {
         let dir = Builder::new().prefix("rust_mozprofile").tempdir()?;

testing/mozbase/rust/mozrunner/src/runner.rs

@@ -130,7 +130,7 @@ pub struct FirefoxProcess {
     // The profile field is not directly used, but it is kept to avoid its
     // Drop removing the (temporary) profile directory.
     #[allow(dead_code)]
-    profile: Profile,
+    profile: Option<Profile>,
 }
 
 impl RunnerProcess for FirefoxProcess {
@@ -180,7 +180,7 @@ impl RunnerProcess for FirefoxProcess {
 #[derive(Debug)]
 pub struct FirefoxRunner {
     path: PathBuf,
-    profile: Profile,
+    profile: Option<Profile>,
     args: Vec<OsString>,
     envs: HashMap<OsString, OsString>,
     stdout: Option<Stdio>,
@@ -193,7 +193,7 @@ impl FirefoxRunner {
     /// On macOS, `path` can optionally point to an application bundle,
     /// i.e. _/Applications/Firefox.app_, as well as to an executable program
     /// such as _/Applications/Firefox.app/Content/MacOS/firefox-bin_.
-    pub fn new(path: &Path, profile: Profile) -> FirefoxRunner {
+    pub fn new(path: &Path, profile: Option<Profile>) -> FirefoxRunner {
         let mut envs: HashMap<OsString, OsString> = HashMap::new();
         envs.insert("MOZ_NO_REMOTE".into(), "1".into());
 
@@ -268,7 +268,9 @@ impl Runner for FirefoxRunner {
     }
 
     fn start(mut self) -> Result<FirefoxProcess, RunnerError> {
-        self.profile.user_prefs()?.write()?;
+        if let Some(ref mut profile) = self.profile {
+            profile.user_prefs()?.write()?;
+        }
 
         let stdout = self.stdout.unwrap_or_else(Stdio::inherit);
         let stderr = self.stderr.unwrap_or_else(Stdio::inherit);
@@ -299,8 +301,10 @@ impl Runner for FirefoxRunner {
         if !seen_no_remote {
             cmd.arg("-no-remote");
         }
-        if !seen_profile {
-            cmd.arg("-profile").arg(&self.profile.path);
+        if let Some(ref profile) = self.profile {
+            if !seen_profile {
+                cmd.arg("-profile").arg(&profile.path);
+            }
         }
 
         info!("Running command: {:?}", cmd);