1408099 - browserSettings.homepageOverride.get throws an exception if the home page has never been overridden

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P1)

Description

[Bob Silverberg [:bsilverberg]]

There's a bug in the code for getLevelOfControl in the ExtensionPreferencesManager (EPM) that will throw an exception if it is called before a corresponding setting has been added to the EPM.

In the case of browserSettings.homepageOverride, an extension could call this method, but if no installed extensions ever touched the home page via chrome_settings_overrides.homepage, then the EPM won't know about the setting and it will throw an exception.

This is a simple fix to the getLevelOfControl function in the EPM, although based on a separate conversation in IRC it sounds like we might be able to entirely remove the getLevelOfControl function from the EPM as it's only there to support reporting about locked prefs, which I think we have decided we do not want to support.

This needs to be fixed, regardless of the implementation.

Comment 1

[Bob Silverberg [:bsilverberg]]

The feature containing this bug landed in 57. I'm not sure how critical a bug this is, and whether a fix qualifies for uplift, so flagging it for tracking 57.

Comment 2

[Bob Silverberg [:bsilverberg]]

Attachment: Bug 1408099 - Fix ExtensionPreferencesManager.getLevelOfControl to deal with undefined settings,

Prior to this, if getLevelOfControl was called before the setting was defined to the EPM, an
exception was thrown. This fixes it by returning a level of control of "not_controllable"
if the setting has not yet been defined.

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

Comment 3

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8918011 [details]
Bug 1408099 - Fix ExtensionPreferencesManager.getLevelOfControl to deal with undefined settings,

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

Comment 4

[Marcia Knous [:marcia]]

Bob: Who would be a good person to assess the criticality? Also if it just throws an exception it sounds like something we could live with for 57.

Comment 5

[Bob Silverberg [:bsilverberg]]

(In reply to Marcia Knous [:marcia - use ni] from comment #4)
> Bob: Who would be a good person to assess the criticality? Also if it just
> throws an exception it sounds like something we could live with for 57.

I think andym would be the person to decide. If we do not fix this then one particular API, browserSettings.homepageOverride.get() will throw an exception rather than returning the expected information, in certain situations. It doesn't seem that critical to me, but I wanted to raise it for someone to assess.

Comment 6

[Andy McKay]

Agree with Bob. This is a new API. I'd be surprised if there are any developers using it, and if they do they can catch the error. Let's not uplift.

Comment 7

[Andrew Swan [:aswan]]

Comment on attachment 8918011 [details]
Bug 1408099 - Fix ExtensionPreferencesManager.getLevelOfControl to deal with undefined settings,

https://reviewboard.mozilla.org/r/188892/#review194602

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/247bb54b615f
Fix ExtensionPreferencesManager.getLevelOfControl to deal with undefined settings, r=aswan

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/247bb54b615f

Comment 10

[magicp]

If wontfix in 57, The following compatibility should be changed.
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browserSettings/homepageOverride

Comment 12

[marius.santa]

Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.