Skip to content

Commit

Permalink
prefs, marionette: remove or move remaining required prefs
Browse files Browse the repository at this point in the history
When a user provides a profile that wnats to override browser.warnOnQuit,
browser.tabs.warnOnClose, or browser.showQuitWarning, we should
allow users to do stupid things.  We should not prevent the profile's
preferences from being applied.

dom.ipc.cpows.forbid-unsafe-from-browser is being removed because all
targetted Firefoxen are not using any unsafe CPOWs in Marionette code.

marionette.defaultPrefs.enabled is superfluous for as long as the
--marionette flag is being passed to the Firefox binary.

Remaining relevant prefs from prefs::REQUIRED have been merged into
prefs::DEFAULT.

This is a follow-up to the discussion around
#423.
  • Loading branch information
andreastt authored and jgraham committed Apr 4, 2017
1 parent 95ef3b4 commit 2e0054b
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 21 deletions.
2 changes: 0 additions & 2 deletions src/marionette.rs
Original file line number Diff line number Diff line change
Expand Up @@ -351,8 +351,6 @@ impl MarionetteHandler {

prefs.insert_slice(&extra_prefs[..]);

prefs.insert_slice(&prefs::REQUIRED[..]);

if let Some(ref level) = self.current_log_level {
prefs.insert("marionette.logging", Pref::new(level.to_string()));
};
Expand Down
29 changes: 10 additions & 19 deletions src/prefs.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use mozprofile::preferences::Pref;

lazy_static! {
pub static ref DEFAULT: [(&'static str, Pref); 75] = [
pub static ref DEFAULT: [(&'static str, Pref); 78] = [
// Disable automatic downloading of new releases
("app.update.auto", Pref::new(false)),

Expand Down Expand Up @@ -65,6 +65,9 @@ lazy_static! {
// Skip check for default browser on startup
("browser.shell.checkDefaultBrowser", Pref::new(false)),

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

// Disable Android snippets
("browser.snippets.enabled", Pref::new(false)),
("browser.snippets.syncPromo.enabled", Pref::new(false)),
Expand All @@ -88,6 +91,9 @@ lazy_static! {
// might get unloaded
("browser.tabs.disableBackgroundZombification", Pref::new(false)),

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

// Do not warn when closing all other open tabs
("browser.tabs.warnOnCloseOtherTabs", Pref::new(false)),

Expand All @@ -100,6 +106,9 @@ lazy_static! {
// Disable the UI tour
("browser.uitour.enabled", Pref::new(false)),

// Do not warn on quitting Firefox
("browser.warnOnQuit", Pref::new(false)),

// Do not show datareporting policy notifications which can
// interfere with tests
("datareporting.healthreport.about.reportUrl", Pref::new("http://%(server)s/dummy/abouthealthreport/")),
Expand Down Expand Up @@ -214,22 +223,4 @@ lazy_static! {
// We want to collect telemetry, but we don't want to send in the results
("toolkit.telemetry.server", Pref::new("https://%(server)s/dummy/telemetry/")),
];

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)),
];
}

9 comments on commit 2e0054b

@juangj
Copy link
Contributor

@juangj juangj commented on 2e0054b Apr 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this commit broke Marionette for me on Windows in Firefoxen 52.0 and 53.0b9, though I don't really understand why. It looks like Marionette does not get enabled, so Geckodriver just hangs while waiting to connect to Marionette.

Interestingly, in the Browser Console, there's a Warning: unrecognized command line flag --marionette; that warning appears even when using older Geckodriver versions (say, 0.14.0). So perhaps that's related, considering the comment on line 231?

(I tried passing the "marionette.defaultPrefs.enabled": true pref in my moz:firefoxOptions but that didn't seem to work, surprisingly. That could be my fault somewhere else in our stack, though.)

@andreastt
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for alerting us! That --marionette is unrecognised is very surprising to me. Normally, when you pass an unknown flag Firefox accepts it without warnings, and I’ve never seen the “unrecognized command lined flag” warning before.

What happens if you start Firefox 52.0 or 53.0b9 manually with firefox --marionette? I would expect it to display some message in stdout saying that it is listening on port 2828.

If you pass marionette.defaultPrefs.enabled as part of prefs in moz:firefoxOptions, that should be exactly equivalent to the old behaviour, before I removed prefs::REQUIRED in this patch.

@andreastt
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(FWIW I apologise for these constant regressions, but I expect this to improve somewhat once https://bugzilla.mozilla.org/show_bug.cgi?id=1340637 and the follow-up bug to hook geckodriver up to WPT in-tree lands.)

@juangj
Copy link
Contributor

@juangj juangj commented on 2e0054b Apr 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Windows, the same thing happens -- no log messages from Marionette are printed, and the Browser Console complains about the unknown flag.

If I do it on Linux, I do get the 1491951219139 Marionette INFO Listening on port 2828 message I expect.

(Just from reading the code, I would definitely think that passing the pref through moz:firefoxOptions is supposed to work, so I probably have a problem elsewhere in our code that's supposed to plumb that capability through.)

@andreastt
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means Firefox appears to have different behaviour for dealing with command line flags on Windows and Linux systems. This is… worrying. I will investigate further in the morning.

I recently landed https://hg.mozilla.org/mozilla-central/rev/16ae5cf72cd6 on mozilla-central, and which is available in Firefox 53 onwards, that lists the --marionette flag in --help. I don’t know if this should have any impact on Windows.

But judging from the information you have given me, it looks like we need to set the marionette.enabled pref irregardless of passing --marionette to Firefox, if only for backwards compatibility.

@andreastt
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe Windows systems only accept X11-style single-dashed flags, like -marionette, and not GNU style --marionette?

@juangj
Copy link
Contributor

@juangj juangj commented on 2e0054b Apr 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that seems to be the case. If I change mozrunner to pass -marionette, then everything works. Fascinating.

@juangj
Copy link
Contributor

@juangj juangj commented on 2e0054b Apr 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Geckodriver is ignoring the prefs that I'm setting in the caps. Will file a separate issue.

@andreastt
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have filed #640 to fix the -marionette flag issue.

Please sign in to comment.