Improve firefox profile prefs merging:

[DrMarcII]

• Merge prefs::Default prefs into custom profile unless the custom
  profile explicitly sets that preference.
• Set the marionette.defaultPrefs.port preference last so users cannot
  accidentally overwrite its value by providing it in capabilities.

---

This change is 

[jgraham]

I'm not sure this is obviously the correct behaviour. If you set a custom profile you might reasonably expect to get the prefs you set, except any that are actually required to make marionette function correctly. If you want the default set you can always set those explicitly.

What's the specific problem here that you're trying to solve? (n.b. I hope that in the future we can enable more things that currently require an entire custom profile via more fine grained capabilities e.g. extension installation).

[andreastt]

I guess this might be a result of 2bfdc3e.

Many of the preferences in prefs::DEFAULT are actually required for Marionette to function correctly, and some of these we can’t set at runtime. We should probably move those that are absolute requirements over to prefs::REQUIRED, and we should set those we can set at runtime in Marionette. I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1323290 about the latter before Christmas.

As I read this code, the default prefs will only get written to the profile if a custom profile is not provided. The intention of prefs::DEFAULT is to say “we recommend these settings for running Marionette”, and triggering another code path that doesn’t set these if the user provides a profile a bit surprising in my opinion.

I’m also curious what the reason for this change is. I guess you are providing a profile with some preferences that get overridden by geckodriver?

[DrMarcII]

With the customers I support, the usual pattern is that they start by using the default settings, and at some point they realize that they need to install an extension or set up some certs that can (currently) most easily be accomplished by providing a custom profile. It is then surprising when a profile created with the sole purpose of doing this causes other behavior changes because DEFAULT prefs are no longer being set. My goal with the change is to make that transition from no profile to providing a profile more seamless by only changing things that the user has explicitly chosen to change when providing the profile.

[andreastt]

My thinking at the moment is to move those preferences that are absolutely required to meet WebDriver conformance from prefs::DEFAULT to prefs::REQUIRED and to accept this change. This would change the meaning of ‘default preferences’ towards ‘suggested preferences’, as they would not take effect if a custom profile with custom preferences is provided.

[jgraham]

But this part I agree is correct :)

[DrMarcII]

Any additional thoughts on this change?

[AutomatedTester]

@jgraham r?

[andreastt]

The long term goal is to make users less reliant on passing full profiles. The two most common reasons for passing profiles today is for (1) setting preferences and (2) installing extensions.

We already have the ability to pass a prefs object as part of moz:firefoxOptions which solves the first use case. Since this PR was filed, Marionette has exposed commands for installing (unsigned) extensions at runtime, which we plan to expose in the Mozilla vendor namespace in geckodriver in #211, which addresses the second.

Concurrently I’ve also done work on setting recommended automation preferences in the Marionette server at runtime. This currently duplicates most of the preferences set by geckodriver, but we will keep these around for backwards compatibility with earlier Firefoxen that doesn’t have the recommended automation prefs patch.

The required set of prefs in geckodriver I would probably say are not all required:

    pub static ref REQUIRED: [(&'static str, Pref); 5] = [
        // Do not warn on quitting Firefox
        ("browser.warnOnQuit", Pref::new(false)),

        // Do not warn on exit when multiple tabs are open
        ("browser.tabs.warnOnClose", Pref::new(false)),

        // Do not warn when quitting with multiple tabs
        ("browser.showQuitWarning", Pref::new(false)),

        // Until bug 1238095 is fixed, we have to disable safe CPOW checks
        ("dom.ipc.cpows.forbid-unsafe-from-browser", Pref::new(false)),

        // TODO(ato): Should not be needed, as Marionette is enabled by
        // passing the --marionette flag to the binary
        ("marionette.defaultPrefs.enabled", Pref::new(true)),
    ];

I’d argue that if you provide a profile that wants to override browser.warnOnQuit, browser.tabs.warnOnClose, or browser.showQuitWarning, you should probably be allowed to do so.
dom.ipc.cpows.forbid-unsafe-from-browser can now be removed completely since all targetted Firefoxen no longer use unsafe CPOWs in Marionette. Similarly, marionette.defaultPrefs.enabled is implied by passing the --marionette flag to the Firefox binary.

The only true and real requirement is to set marionette.defaultPrefs.port/marionette.port, which is done out of band of prefs.rs.

To resolve this PR, I suggest the following approach:

1. Merge this PR as-is: This will cause the DEFAULT prefs only to be inserted if there is no custom profile, or the custom profile does not contain the key. In other words, this will allow a profile to override any pref from prefs::DEFAULT.
2. Remove prefs::REQUIRED from geckodriver: The browser.* prefs can be moved over to prefs::DEFAULT and the remaining prefs can be removed as explained above.
3. Add forward-compatibility for marionette.port (marionette.defaultPrefs.port has been renamed).

[andreastt]

@DrMarcII My apologies this took a long time to land. )-: