1448085 - Fix the enterprise policy to set homepage to use the correct mechanism for setting complex prefs

Status: RESOLVED FIXED

(Firefox :: Enterprise Policies, enhancement)

Description

[Robin Steuber (she/her) [:bytesized]]

(Copying from Mike Kaply [:mkaply], Bug 1447345 Comment #7)
> Actually we forgot something about this.
> 
> Because homepage is a complex pref, you have to set it as default
> differently.
> 
> See:
> 
> https://mike.kaply.com/2012/08/29/setting-the-default-firefox-homepage-with-
> autoconfig/
> 
> The policy is broken right now:
> 
> [Exception... "Component returned failure code: 0x80004004 (NS_ERROR_ABORT)
> [nsIPrefBranch.getComplexValue]"  nsresult: "0x80004004 (NS_ERROR_ABORT)" 
> location: "JS frame ::
> jar:file:///C:/Program%20Files/Nightly/64bits/testNightly/newer/coffee2/
> browser/omni.ja!/components/nsBrowserContentHandler.js :: get startPage ::
> line 592"  data: no]
> 
> this should be the only pref we have to do this weirdness with.

Comment 1

[Mike Kaply [:mkaply]]

Would it make more sense to just fix setDefaultPref to detect the complex value and do the right thing?

Comment 2

[Robin Steuber (she/her) [:bytesized]]

How should it detect the complex value?

Comment 3

[Mike Kaply [:mkaply]]

> How should it detect the complex value?

It looks like the only way is with a try catch if it is a string:

    switch (this._prefSvc.getPrefType(prefName)) {
      case Ci.nsIPrefBranch.PREF_STRING:
        try {
          return this._prefSvc.getComplexValue(prefName, Ci.nsISupportsString).data;
        } catch (ex) {
          if (this.isDefaultBranch)
            return defaultValue;
          else
            return this._prefSvc.getCharPref(prefName);
        }

Alternatively we could check for the specific pref name. There are only a handful.

Comment 4

[:Felipe Gomes (needinfo for replies!)]

I guess that will be too much wasted work for setDefaultPref to do every time, just for this exception. I think the best is to either make the whitelist, or make the Homepage policy not use setDefaultPref and do it the correct way as an exception.

Comment 5

[Robin Steuber (she/her) [:bytesized]]

I agree with Felipe. It seems to me that the list of prefs that need to be handled this way is just too short to justify checking this for every call.

Comment 6

[Mike Kaply [:mkaply]]

That's fair. It's sad there isn't an easy way to check

Comment 7

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1448085 - Fix the enterprise policy to set homepage to use the correct mechanism for setting complex prefs

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

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8961570 [details]
Bug 1448085 - Fix the enterprise policy to set homepage to use the correct mechanism for setting complex prefs

https://reviewboard.mozilla.org/r/230432/#review235966

Assuming that the setAndLockPref case works (for whatever reason), r+

Comment 9

[Mike Kaply [:mkaply]]

I want to do some local testing of this with the GPO and then I'll approve.

Comment 10

[Robin Steuber (she/her) [:bytesized]]

Attachment: Patch for beta uplift

Because the homepage UI has changed, beta will need a slightly different version of the test file so that it passes. The following changes had to be made from the version for mozilla-central:

- In beta, there is no "Home" button. The homepage preferences are directly in "about:preferences", so no button needs to be clicked.

- The ids of the buttons related to homepage are "useCurrent", "useBookmark", and "restoreDefaultHomePage" instead of "useCurrentBtn", "useBookmarkBtn", and "restoreDefaultHomePageBtn".

Comment 11

[Mike Kaply [:mkaply]]

Comment on attachment 8961570 [details]
Bug 1448085 - Fix the enterprise policy to set homepage to use the correct mechanism for setting complex prefs

Ship it

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c0537ee24668
Fix the enterprise policy to set homepage to use the correct mechanism for setting complex prefs r=Felipe

Comment 13

[:Felipe Gomes (needinfo for replies!)]

[Tracking Requested - why for this release]:
Enterprise Policies

Comment 14

[Raul Gurzau (:RaulG)]

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

Comment 15

[Robin Steuber (she/her) [:bytesized]]

Comment on attachment 8962424 [details] [diff] [review]
Patch for beta uplift

Approval Request Comment
[Feature/Bug causing the regression]: Enterprise Policy Engine
[User impact if declined]: Setting homepage via policy will set the page once, but resetting to default will reset to the Firefox default rather than the organization default.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: To be tested by Abe
[List of other uplifts needed for the feature/fix]: The patch from Bug 1447345 must be merged before this patch
[Is the change risky?]: Minimal Risk
[Why is the change risky/not risky?]: Makes a minor change to the way that the enterprise policy works. Should not affect users not using enterprise policies
[String changes made/needed]: None

Comment 16

[Julien Cristau [:jcristau]]

Comment on attachment 8962424 [details] [diff] [review]
Patch for beta uplift

enterprise policy followup for 1447345, approved for 60.0b8

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/45de343ccebd