1407999 - Locking homepage prefs isn't working on Firefox 57

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect)

Description

[Mike Kaply [:mkaply]]

Locking the homepage doesn't work anymore. Doing this on a chrome scratch pad

var prefs = Services.prefs.getDefaultBranch(null); 
prefs.setCharPref("browser.startup.homepage", "http://www.yahoo.com");
prefs.lockPref("browser.startup.homepage");

shows

[Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getComplexValue]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: chrome://browser/content/preferences/in-content/main.js :: syncFromHomePref :: line 693"  data: no]

when opening prefs. This definitely used to work.

In addition, locking these three prefs to true:

pref.browser.homepage.disable_button.current_page
pref.browser.homepage.disable_button.bookmark_page
pref.browser.homepage.disable_button.restore_default

Is supposed to disable the three homepage buttons in preferences. It is not.

You can use the chrome scratchpad to test this:

var prefs = Services.prefs.getDefaultBranch(null); 
prefs.setBoolPref("pref.browser.homepage.disable_button.current_page", true);
prefs.lockPref("pref.browser.homepage.disable_button.current_page");
prefs.setBoolPref("pref.browser.homepage.disable_button.bookmark_page", true);
prefs.lockPref("pref.browser.homepage.disable_button.bookmark_page");
prefs.setBoolPref("pref.browser.homepage.disable_button.restore_default", true);
prefs.lockPref("pref.browser.homepage.disable_button.restore_default");

I'll see if I can get a regression window.

Comment 1

[Mike Kaply [:mkaply]]

This was broken by bug 1354344.

Comment 2

[Mike Kaply [:mkaply]]

[Tracking Requested - why for this release]: This broke a major component for enterprise (locking the homepage and homepage buttons)

Comment 3

[Mike Kaply [:mkaply]]

The problem is this line:

http://searchfox.org/mozilla-central/source/browser/components/preferences/in-content/main.js#698

            button.disabled = isControlled;

This is setting the button disabled state ignoring whether or not the button was disabled because it was locked.

Comment 4

[Mike Kaply [:mkaply]]

I can confirm that the locking of the homepage entry field is broken because of this is well. The syncFromHomePref error is unrelated

Comment 5

[Mark Striemer [:mstriemer]]

Attachment: Bug 1407999 - Support locked prefs for homepage

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

Comment 6

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8917927 [details]
Bug 1407999 - Support locked prefs for homepage

https://reviewboard.mozilla.org/r/188832/#review194162

I'm not sure that I'm the one to review this, as it doesn't actually touch anything directly related to WebExtensions. The code change seems okay to me, but I did add a comment to the test.

::: browser/components/preferences/in-content/tests/browser_extension_controlled.js:133
(Diff revision 1)
> +  });
> +
> +  // Lock all of the prefs.
> +  prefs.setCharPref(homePagePref, "http://www.yahoo.com");
> +  prefs.lockPref(homePagePref);
> +  buttonPrefs.forEach(pref => {

I'm a bit confused about how the value of these button prefs and whether they are locked or not interact. Does it matter if these are locked, or just that they are set to `true`? It seems like it's only the `homePagePref` that matters if it is locked or not.

Comment 7

[Marcia Knous [:marcia]]

Tracking 57+ for this regression issue that affects enterprise.

Comment 8

[Mark Striemer [:mstriemer]]

(In reply to Bob Silverberg [:bsilverberg] from comment #6)
> Comment on attachment 8917927 [details]
> Bug 1407999 - Support locked prefs for homepage

> :::
> browser/components/preferences/in-content/tests/browser_extension_controlled.
> js:133
> (Diff revision 1)
> > +  });
> > +
> > +  // Lock all of the prefs.
> > +  prefs.setCharPref(homePagePref, "http://www.yahoo.com");
> > +  prefs.lockPref(homePagePref);
> > +  buttonPrefs.forEach(pref => {
> 
> I'm a bit confused about how the value of these button prefs and whether
> they are locked or not interact. Does it matter if these are locked, or just
> that they are set to `true`? It seems like it's only the `homePagePref` that
> matters if it is locked or not.

According to the post [1] mkaply sent me to they need to be locked.

[1] https://mike.kaply.com/2014/10/22/disabling-buttons-in-preferences/

Comment 9

[Bob Silverberg [:bsilverberg]]

(In reply to Mark Striemer [:mstriemer] from comment #8)
> (In reply to Bob Silverberg [:bsilverberg] from comment #6)
> 
> According to the post [1] mkaply sent me to they need to be locked.
> 
> [1] https://mike.kaply.com/2014/10/22/disabling-buttons-in-preferences/

So it does. That's odd.

Comment 10

[Mark Striemer [:mstriemer]]

Comment on attachment 8917927 [details]
Bug 1407999 - Support locked prefs for homepage

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188832/diff/1-2/

Comment 11

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

Comment on attachment 8917927 [details]
Bug 1407999 - Support locked prefs for homepage

https://reviewboard.mozilla.org/r/188832/#review194490

::: browser/components/preferences/in-content/main.js:690
(Diff revision 2)
>     *   option is preserved.
>     */
>  
>    syncFromHomePref() {
>      let homePref = document.getElementById("browser.startup.homepage");
> +    let defaultBranch = Services.prefs.getDefaultBranch("");

Please remove usage of getDefaultBranch here.

::: browser/components/preferences/in-content/main.js:699
(Diff revision 2)
> -    handleControllingExtension("prefs", "homepage_override")
> -      .then((isControlled) => {
> -        // Disable or enable the inputs based on if this is controlled by an extension.
> +      // Disable or enable the inputs based on if this is controlled by an extension.
> -        document.querySelectorAll("#browserHomePage, .homepage-button")
> +      document.querySelectorAll("#browserHomePage, .homepage-button")
> -          .forEach((button) => {
> -            button.disabled = isControlled;
> +        .forEach((element) => {
> +          let isLocked = defaultBranch.prefIsLocked(element.getAttribute("preference"));

To check if the preference is locked, you should be able to get a reference to the <preference> element and then can check the .locked property of the preference.

`document.getElementById(element.getAttribute("preference")).locked`

or you can use `Services.prefs.prefIsLocked()`

::: browser/components/preferences/in-content/main.js:717
(Diff revision 2)
>      let defaultValue = defaultBranch.getComplexValue("browser.startup.homepage",
>        Ci.nsIPrefLocalizedString).data;

This should be replaced with `Services.prefs.getComplexValue("browser.startup.homepage");`

::: browser/components/preferences/in-content/tests/browser_extension_controlled.js:123
(Diff revision 2)
> +    "pref.browser.homepage.disable_button.current_page",
> +    "pref.browser.homepage.disable_button.bookmark_page",
> +    "pref.browser.homepage.disable_button.restore_default",
> +  ];
> +  let homePageInput = doc.getElementById("browserHomePage");
> +  let prefs = Services.prefs.getDefaultBranch(null);

This isn't needed.

::: browser/components/preferences/in-content/tests/browser_extension_controlled.js:135
(Diff revision 2)
> +      prefs.setBoolPref(pref, true);
> +      prefs.lockPref(pref);

Services.prefs.setBoolPref(pref, true);
Services.prefs.lockPref(pref);

etc. for the other calls too.

Comment 12

[Mark Striemer [:mstriemer]]

Comment on attachment 8917927 [details]
Bug 1407999 - Support locked prefs for homepage

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188832/diff/2-3/

Comment 13

[Mark Striemer [:mstriemer]]

Comment on attachment 8917927 [details]
Bug 1407999 - Support locked prefs for homepage

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188832/diff/3-4/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6fd00865ed5d
Support locked prefs for homepage r=jaws

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/6fd00865ed5d

Comment 16

[Julien Cristau [:jcristau]]

Please request uplift to beta when you get a chance.

Comment 17

[Mark Striemer [:mstriemer]]

Attachment: handle-locked-homepage-pref-beta.patch

Approval Request Comment
[Feature/Bug causing the regression]: bug 1354344
[User impact if declined]: Enterprise users cannot lock down homepage preference on installations
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: If possible, STR in comment 0
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Low
[Why is the change risky/not risky?]: It only affects the homepage button controls which are already regressed and is covered by automated tests
[String changes made/needed]: None

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8919422 [details] [diff] [review]
handle-locked-homepage-pref-beta.patch

Must fix, recent regression, Beta57+

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8bf87cac5783

Comment 20

[Bogdan Maris, Desktop QA]

I reproduced the initial issue using old Nightly build from 2017-10-12. I verified that running the code from comment 0 now successfully locks those three prefs and disables the homepage buttons. I tested on Firefox 57 beta 10 and latest Nightly 58.0a1 across platforms (Windows 10 64bit, macOS 10.12.6 and Ubuntu 16.04 32bit).