1189038 - When Firefox is the default browser you shouldn't be able to uncheck the check on startup setting

Status: RESOLVED FIXED

(Firefox :: Shell Integration, defect)

Description

[Verdi [:verdi] Best to slack me]

Bug 1041516 made it so that when you set Firefox as the default browser, checking for default status on startup also gets set. That's good but we didn't prevent the check on startup setting from being unchecked. So a user can sabotage themselves by opening the options tab and unchecking "Always check if Firefox is your default browser." This will put them in a state where another browser can be set as default without Firefox noticing. 

We should make this setting unable to be changed when Firefox is the default browser.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

In this test, I mocked the shellservice to instrument the preferences code to act as though the browser was not the default and then becoming the default browser.

We can use this approach to fix/circumvent bug 1180714 and fix browser_UITour_defaultBrowser.js[1].

[1] http://mxr.mozilla.org/mozilla-central/source/browser/components/uitour/test/browser_UITour_defaultBrowser.js?rev=cb217f7271c2#43

Comment 2

[:Gijs (out for now; he/him)]

Comment on attachment 8717172 [details] [diff] [review]
Patch

Review of attachment 8717172 [details] [diff] [review]:
-----------------------------------------------------------------

This is pretty neat, but I have a number of comments, so I think I'd like to see it again before r+.

::: browser/components/preferences/in-content/main.js
@@ +115,5 @@
>                .getService(Components.interfaces.nsIObserverService)
>                .notifyObservers(window, "main-pane-loaded", null);
>    },
>  
> +  setupDefaultBrowserUpdateInterval(interval=1000) {

Nit: spaces around =

@@ +122,5 @@
> +    }
> +    // In Windows 8 we launch the control panel since it's the only
> +    // way to get all file type association prefs. So we don't know
> +    // when the user will select the default.  We refresh here periodically
> +    // in case the default changes.  On other Windows OS's defaults can also

Nit: remove extra space before 'On'.

@@ +125,5 @@
> +    // when the user will select the default.  We refresh here periodically
> +    // in case the default changes.  On other Windows OS's defaults can also
> +    // be set while the prefs are open.
> +    this._defaultBrowserInterval =
> +      window.setInterval(this.updateSetDefaultBrowser, interval);

Nit: Not being a fan of footguns, can we make this:

this.updateSetDefaultBrowser.bind(this) ?

now that we're changing it anyway?

@@ +707,5 @@
>      let setDefaultPane = document.getElementById("setDefaultPane");
> +    let isDefault = shellSvc.isDefaultBrowser(false, true);
> +    setDefaultPane.selectedIndex = isDefault ? 1 : 0;
> +    let alwaysCheck = document.getElementById("alwaysCheckDefault");
> +    alwaysCheck.disabled = isDefault && alwaysCheck.checked;

This is going to interfere with locked prefs. Maybe use:

alwaysCheck.disabled = alwaysCheck.disabled || isDefault && alwaysCheck.checked;

? But please test that that actually works and that at this point locked prefs have already been disabled (which I'm not sure about).

@@ +716,5 @@
>     */
>    setDefaultBrowser: function()
>    {
> +    let alwaysCheck = document.getElementById("alwaysCheckDefault");
> +    alwaysCheck.checked = true;

Why are we doing this? Have you verified this actually changes the pref - I kind of thought it wouldn't... (you'd need a change event, right?)

::: browser/components/preferences/in-content/tests/browser_defaultbrowser_alwayscheck.js
@@ +1,1 @@
> +add_task(function* clicking_make_default_checks_alwaysCheck_checkbox() {

Nit: please start the file with "use strict";

@@ +10,5 @@
> +
> +    let setDefaultButton = content.document.getElementById("setDefaultButton");
> +    setDefaultButton.click();
> +
> +    yield new Promise(resolve => setTimeout(resolve, 100));

This is setting us up for intermittent timeouts. Please consider instead using:

yield BrowserTestUtils.waitForAttribute("checked", alwaysCheck);

which uses a mutation observer (which will fire either at the end of the tick or the next tick, I forget which - but after the code that changed the checked status has run to completion).

@@ +44,5 @@
> +    mockShellService._isDefault = options.isDefault;
> +    win.gMainPane.setupDefaultBrowserUpdateInterval(50);
> +  });
> +
> +  // The preferences page refreshes default-browser status every second.

This comment doesn't really make sense to me in this context? The timeout is for 100ms, not a second, and you just changed how often it refreshes to 50ms instead of 1 second... I'm also not entirely sure what the purpose of the 100ms wait here is - I guess we need to wait for the update to have run with the new shell service? Considering we're already changing the update interval, I'm tempted to suggest that you could just force-run win.gMainPane.updateSetDefaultBrowser() from the test.

@@ +54,5 @@
> +    let win = content.document.defaultView;
> +    win.gMainPane.setupDefaultBrowserUpdateInterval();
> +    win.getShellService = win.oldShellService;
> +    delete win.oldShellService;
> +  });

We're closing the tab straight afterwards, so I don't think we need to do this.

We *should* make sure the pref for checking gets reset to whatever the default is.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: MozReview Request: Bug 1189038 - Auto-check the 'Always Check if Firefox is Default' pref when setting Firefox as default. r?gijs

Review commit: https://reviewboard.mozilla.org/r/34249/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34249/

Comment 4

[:Gijs (out for now; he/him)]

Comment on attachment 8717644 [details]
MozReview Request: Bug 1189038 - Auto-check the 'Always Check if Firefox is Default' pref when setting Firefox as default. r?gijs

https://reviewboard.mozilla.org/r/34249/#review31029

::: browser/components/preferences/in-content/main.js:711
(Diff revision 1)
> +    if (!alwaysCheckPref.value) {
> +      alwaysCheckPref.value = true;
> +    }

Why is this conditional? Can't we just always set it to true?

::: browser/components/preferences/in-content/tests/browser_defaultbrowser_alwayscheck.js:53
(Diff revision 1)
> +       "alwaysCheck pref should ships with 'true' by default");

Nit: should ship

::: browser/components/preferences/in-content/tests/browser_defaultbrowser_alwayscheck.js:59
(Diff revision 1)
> +    yield ContentTaskUtils.waitForCondition(() => setDefaultPane.selectedIndex == "1",

waitForCondition still uses timeouts under the hood. Was there a reason waitForAttribute didn't work?

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

waitForAttribute is on BrowserTestUtils. CotnentTaskUtils only has waitForEvent and waitForCondition.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/cbd51059e3be43c0bf6d6cee6176fc547746cbae
Bug 1189038 - Auto-check the 'Always Check if Firefox is Default' pref when setting Firefox as default. r=gijs

Comment 7

[:Gijs (out for now; he/him)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #5)
> waitForAttribute is on BrowserTestUtils. CotnentTaskUtils only has
> waitForEvent and waitForCondition.

I see that I'm too late with this comment, but: hrm. Seems like we could copy it to ContentTaskUtils or move to TestUtils (assuming that's available) or even just Cu.import BrowserTestUtils in the content task we're running - there's nothing parent-process specific about that method. I'd really like to move away from using setTimeout in tests because it's such a mare's nest. :-(

Anyway, let's see how this goes now that it's landed and we can always update it if it does go intermittent.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cbd51059e3be