Closed Bug 1407999 Opened 5 years ago Closed 5 years ago

Locking homepage prefs isn't working on Firefox 57

Categories

(Firefox :: Preferences, defect)

57 Branch
defect
Not set
critical

Tracking

()

VERIFIED FIXED
Firefox 58
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- unaffected
firefox57 + verified
firefox58 --- verified

People

(Reporter: mkaply, Assigned: mstriemer)

References

Details

Attachments

(2 files)

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.
This was broken by bug 1354344.
Blocks: 1354344
[Tracking Requested - why for this release]: This broke a major component for enterprise (locking the homepage and homepage buttons)
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.
I can confirm that the locking of the homepage entry field is broken because of this is well. The syncFromHomePref error is unrelated
Assignee: nobody → mstriemer
Status: NEW → ASSIGNED
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.
Attachment #8917927 - Flags: review?(bob.silverberg)
Tracking 57+ for this regression issue that affects enterprise.
(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/
(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 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.
Attachment #8917927 - Flags: review?(jaws) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6fd00865ed5d
Support locked prefs for homepage r=jaws
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6fd00865ed5d
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Please request uplift to beta when you get a chance.
Flags: needinfo?(mstriemer)
Version: 55 Branch → 57 Branch
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
Flags: needinfo?(mstriemer)
Attachment #8919422 - Flags: approval-mozilla-beta?
Comment on attachment 8919422 [details] [diff] [review]
handle-locked-homepage-pref-beta.patch

Must fix, recent regression, Beta57+
Attachment #8919422 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
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).
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.