Closed Bug 1404584 Opened 7 years ago Closed 7 years ago

Overriding Home page using chrome_settings_overrides cannot be un-done

Categories

(WebExtensions :: General, defect, P5)

Product:
WebExtensions
Component:
General
Version:
57 Branch
Type:
defect

Tracking

(firefox57 wontfix, firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox59 --- fixed

People

(Reporter: juraj.masiar, Assigned: bsilverberg)

References

Details

(Whiteboard: investigate)

Attachments

(3 files, 3 obsolete files)

Bug 1404584 Part 1: Use extensionId in ExtensionSettingsStore and ExtensionPreferencesManager methods,
59 bytes, text/x-review-board-request
aswan
: review+
gchang
: approval-mozilla-beta-
Details
Bug 1404584 Part 2: Convert ext-chrome-settings-overrides to use update and uninstall events
59 bytes, text/x-review-board-request
aswan
: review+
gchang
: approval-mozilla-beta-
Details
Bug 1404584 Part 3: Convert ExtensionPreferencesManager to use update and uninstall events
59 bytes, text/x-review-board-request
aswan
: review+
jkt
: review+
gchang
: approval-mozilla-beta-
Details 
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170925150345

Steps to reproduce:

I've introduced homepage override in my GroupSpeedDial 4.6 and then removed in 4.7 and people still cannot change their homepage.

Steps:
1) specify overriding homepage using chrome_settings_overrides in manifest
2) homepage is changed
3) remove chrome_settings_overrides from manifest


Actual results:

Homepage is still overridden by extension and cannot be changed! 
Add-on needs to be completely uninstalled to restore homepage.


Expected results:

Homepage should be editable once add-on no longer overrides it (and I would say homepage should be editable even when add-on is overriding it).
Bob can you take this one?
Flags: needinfo?(bob.silverberg)
Assignee: nobody → bob.silverberg
Flags: needinfo?(bob.silverberg)
Whiteboard: investigate
The problem here is that there is currently no code to deal with the situation where the manifest key has been removed. There are also no tests for this scenario which is why this wasn't caught sooner. I've written a test to prove the bug, but I'm not sure how to fix it.

We cannot deal with it in onManifestEntry because the key no longer exists and therefore onManifestEntry doesn't fire. It seems like we need to deal with it in onStartup, but I know that we've been trying to avoid doing things in onStartup.

Kris, do you have any ideas of the best approach to this?
Flags: needinfo?(kmaglione+bmo)
We should probably handle it at the same time we handle removing unused permissions:

http://searchfox.org/mozilla-central/rev/e62604a97286f49993eb29c493672c17c7398da9/toolkit/components/extensions/Extension.jsm#1339-1378
Flags: needinfo?(kmaglione+bmo)
status-firefox57: --- → wontfix
Priority: -- → P5
Note that this also affects chrome_settings_overrides.search_provider.is_default, which I have also addressed in the patch.
Comment on attachment 8918306 [details]
Bug 1404584 - Respond to manifest keys being removed for homepage and default search engine

Redirecting review to aswan from kmag as I believe the latter is super busy. Kris has already commented on how he would approach this bug, so his input may be unneeded at this time.
Attachment #8918306 - Flags: review?(kmaglione+bmo) → review?(aswan)
Comment on attachment 8918306 [details]
Bug 1404584 - Respond to manifest keys being removed for homepage and default search engine

https://reviewboard.mozilla.org/r/189134/#review195118

I don't like this at all, this is spreading the implementation of chrome_settings_overrides around too much.  I don't know exactly what Kris proposed but I doubt it is what is in this patch...
Attachment #8918306 - Flags: review?(aswan) → review-
Comment on attachment 8918306 [details]
Bug 1404584 - Respond to manifest keys being removed for homepage and default search engine

https://reviewboard.mozilla.org/r/189134/#review195118

I'm not a big fan of spreading the logic around either, which is why I was inclined to use onStartup in ext-chrome-settings-overrides.js, and you were the one who pointed out that Kris might have a problem with that.

What Kris specifically suggested is that we deal with this in `startup` in Extension.jsm in a similar way to how we're dealing with resetting permissions. He even suggested I incorporate it into the existing `updatePermissions` method as opposed to creating a new one, so that's what I did. If you think it's so off the mark perhaps you could suggest something different.
I think the ideal fix is a modest amount of work, filed bug 1409245 for it.
Depends on: 1409245
(In reply to Andrew Swan [:aswan] from comment #10)
> I think the ideal fix is a modest amount of work, filed bug 1409245 for it.

Thanks Andrew. So you suggest that as a fix for this bug? Is it something you see landing for 58? I think this is a fairly serious issue that needs to be addressed at the earliest possible opportunity (which is now 58). If not, can you suggest another workaround for the interim?
Flags: needinfo?(aswan)
(In reply to Bob Silverberg [:bsilverberg] from comment #11)
> (In reply to Andrew Swan [:aswan] from comment #10)
> > I think the ideal fix is a modest amount of work, filed bug 1409245 for it.
> 
> Thanks Andrew. So you suggest that as a fix for this bug? Is it something
> you see landing for 58? I think this is a fairly serious issue that needs to
> be addressed at the earliest possible opportunity (which is now 58). If not,
> can you suggest another workaround for the interim?

I'll take a shot at this for 58, if we get to the end of this week and it looks unlikely then we can reconsider.
Flags: needinfo?(aswan)
Attachment #8918306 - Attachment is obsolete: true
Attachment #8918306 - Flags: review?(mozilla)
Attachment #8922433 - Attachment is obsolete: true
Attachment #8922434 - Attachment is obsolete: true
Comment on attachment 8924293 [details]
Bug 1404584 Part 1: Use extensionId in ExtensionSettingsStore and ExtensionPreferencesManager methods,

https://reviewboard.mozilla.org/r/195528/#review200822
Attachment #8924293 - Flags: review?(aswan) → review+
Comment on attachment 8924294 [details]
Bug 1404584 Part 2: Convert ext-chrome-settings-overrides to use update and uninstall events

https://reviewboard.mozilla.org/r/195530/#review200832

::: browser/components/extensions/ext-chrome-settings-overrides.js:77
(Diff revision 1)
> +    this.processDefaultSearchSetting("removeSetting", id);
> +    this.removeEngines(id);

Since you always call these two together (here, in onUpdate, and in close()), can you just add the removeSetting call to removeEngines() (and perhaps change its name to match)

::: browser/components/extensions/test/browser/browser_ext_settings_overrides_default_search.js:34
(Diff revision 1)
>  
> -/* This tests setting a default engine. */
> -add_task(async function test_extension_setting_default_engine() {
> -  let defaultEngineName = Services.search.currentEngine.name;
> +let defaultEngineName = Services.search.currentEngine.name;
>  
> +function cleanup() {

can you call this `restoreDefaultEngine()` or something more descriptive

::: browser/components/extensions/test/xpcshell/test_ext_chrome_settings_overrides_update.js:18
(Diff revision 1)
> +AddonTestUtils.init(this);
> +AddonTestUtils.overrideCertDB();
> +
> +createAppInfo("xpcshell@tests.mozilla.org", "XPCShell", "1", "42");
> +
> +add_task(async function test_overrides_homepage_update() {

please add a comment explaining that this tests the scenario where these settings are removed in an updated version.
Attachment #8924294 - Flags: review?(aswan) → review+
Comment on attachment 8924295 [details]
Bug 1404584 Part 3: Convert ExtensionPreferencesManager to use update and uninstall events

https://reviewboard.mozilla.org/r/195532/#review200834

Nice, its not a huge change but I think the code here is a lot more straightforward to read now...

::: browser/components/extensions/test/browser/head.js:477
(Diff revision 1)
> +    let listener = (_eventName, ...args) => {
> +      let extension = args[0];
> +      if (_eventName === eventName &&
> +          extension.id == id) {
> +        Management.off(eventName, listener);
> +        resolve(...args);

I realize you just moved this code, but I don't think it makes any sense to call `resolve()` with more than 1 argument?  If you want to pass the whole args array you need to pass it as an array, ie drop the ...
Attachment #8924295 - Flags: review?(aswan) → review+
Comment on attachment 8924294 [details]
Bug 1404584 Part 2: Convert ext-chrome-settings-overrides to use update and uninstall events

https://reviewboard.mozilla.org/r/195530/#review200998

Codewise, this looks good to me. The tests I wrote should cover all the interesting scenarios.
Attachment #8924294 - Flags: review?(mozilla) → review+
Comment on attachment 8924294 [details]
Bug 1404584 Part 2: Convert ext-chrome-settings-overrides to use update and uninstall events

I made some changes to record the fact that we've added a search engine in the Settings Store, so flagging both of you to please take another look. This does fix the failure of test_TelemetryEnvironment.js.
Attachment #8924294 - Flags: review?(mozilla)
Attachment #8924294 - Flags: review?(aswan)
Attachment #8924294 - Flags: review+
Comment on attachment 8924294 [details]
Bug 1404584 Part 2: Convert ext-chrome-settings-overrides to use update and uninstall events

https://reviewboard.mozilla.org/r/195530/#review201972

::: browser/components/extensions/ext-chrome-settings-overrides.js:70
(Diff revision 3)
> +  static async removeEngines(id) {
> +    await ExtensionSettingsStore.initialize();
> +    if (ExtensionSettingsStore.hasSetting(id, DEFAULT_SEARCH_STORE_TYPE, ENGINE_ADDED_SETTING_NAME)) {
> +      ExtensionSettingsStore.removeSetting(id, DEFAULT_SEARCH_STORE_TYPE, ENGINE_ADDED_SETTING_NAME);
> +      await searchInitialized();
> +      let engines = Services.search.getEnginesByExtensionID(id);

Can't you just get this from the settings store?

::: browser/components/extensions/ext-chrome-settings-overrides.js:81
(Diff revision 3)
> +        }
> +      }
> +    }
> +  }
> +
> +  static removeSetting(id) {

This name confuses me, what setting is removed?  This class handles the whole chrome_settings_overrides API but this method only handles search engine related things?

::: browser/components/extensions/ext-chrome-settings-overrides.js:87
(Diff revision 3)
> +    this.processDefaultSearchSetting("removeSetting", id);
> +    this.removeEngines(id);
> +  }
> +
> +  static onUninstall(id) {
> +    this.removeSetting(id);

doesn't homepage_override need to be cleared here too?

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:260
(Diff revision 3)
>     *                          just added, or null if the item that was just
>     *                          added does not need to be set because it is not
>     *                          at the top of the precedence list.
>     */
>    async addSetting(id, type, key, value, initialValueCallback, callbackArgument = key) {
> +    initialValueCallback = initialValueCallback || function() {};

please use `() => undefined` instead of `function ...`.  you can also make it the default value on the previous line.
Attachment #8924294 - Flags: review?(aswan) → review-
Comment on attachment 8924295 [details]
Bug 1404584 Part 3: Convert ExtensionPreferencesManager to use update and uninstall events

https://reviewboard.mozilla.org/r/195532/#review202034

Looks like these changes should be tested in "toolkit/components/extensions/test/xpcshell/test_ext_contextual_identities.js" as we mentioned this was just a hack before the downgrade/upgrade behavior was removed which is my understanding of this patch.

I see there are a few test fails on the try like: toolkit/components/extensions/test/xpcshell/test_ext_extensionSettingsStore.js so r+ based on those passing.
Attachment #8924295 - Flags: review?(jkt) → review+
(In reply to Jonathan Kingston [:jkt] from comment #29)
> Comment on attachment 8924295 [details]
> Bug 1404584 Part 3: Convert ExtensionPreferencesManager to use update and
> uninstall events
>

Thanks Jonathan.
 
> https://reviewboard.mozilla.org/r/195532/#review202034
> 
> Looks like these changes should be tested in
> "toolkit/components/extensions/test/xpcshell/test_ext_contextual_identities.
> js" as we mentioned this was just a hack before the downgrade/upgrade
> behavior was removed which is my understanding of this patch.
> 

When you say "changes should be tested in ..." do you mean we need to add tests for this, or that the existing tests should be enough to cover this? I am assuming the latter, but I wanted to double check.

> I see there are a few test fails on the try like:
> toolkit/components/extensions/test/xpcshell/test_ext_extensionSettingsStore.
> js so r+ based on those passing.

Yes, I plan to address those.
Comment on attachment 8924294 [details]
Bug 1404584 Part 2: Convert ext-chrome-settings-overrides to use update and uninstall events

https://reviewboard.mozilla.org/r/195530/#review201972

> doesn't homepage_override need to be cleared here too?

No, because it uses the ExtensionPreferencesManager, which deals with uninstall itself.
Comment on attachment 8924294 [details]
Bug 1404584 Part 2: Convert ext-chrome-settings-overrides to use update and uninstall events

https://reviewboard.mozilla.org/r/195530/#review201972

> Can't you just get this from the settings store?

You marked this issue resolved but didn't change anything or explain why you can't just use `item`
Comment on attachment 8924294 [details]
Bug 1404584 Part 2: Convert ext-chrome-settings-overrides to use update and uninstall events

https://reviewboard.mozilla.org/r/195530/#review202986

Getting close, a few nits, I think the disabling/re-enabling scenario is the biggest outstanding issue.

::: browser/components/extensions/ext-chrome-settings-overrides.js:88
(Diff revision 4)
> +    this.processDefaultSearchSetting("removeSetting", id);
> +    this.removeEngine(id);
> +  }
> +
> +  static onUninstall(id) {
> +    this.removeSearchSettings(id);

please add a comment explaining that homepage is handled inside the PrefencesStore

::: browser/components/extensions/ext-chrome-settings-overrides.js:119
(Diff revision 4)
> -              extension.shutdownReason == "ADDON_UNINSTALL") {
> -            switch (extension.shutdownReason) {
> +            chrome_settings_overrides.processDefaultSearchSetting("disable", extension.id);
> +            chrome_settings_overrides.removeEngine(extension.id);

It looks like this might not work if an extension that adds a non-default search engine and sets it as the default is disabled and then re-enabled?

::: browser/components/extensions/ext-chrome-settings-overrides.js:188
(Diff revision 4)
>      } else if (extension.startupReason === "ADDON_ENABLE") {
> -      this.processDefaultSearchSetting("enable");
> +      chrome_settings_overrides.processDefaultSearchSetting("enable", extension.id);
>      }
>    }
>  
> -  addSearchEngine(searchProvider) {
> +  async addSearchEngine(id, searchProvider) {

why are you passing `id` here?  you can just get it from `this.extension.id`

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:124
(Diff revision 4)
> + * @param {string} type
> + *        The type of setting to be altered.
> + * @param {string} key
> + *        A string that uniquely identifies the setting.
> + * @param {string} id
> + *        The id of the extension for which a setting is being removed/disabled.

This doesn't match the actual meaning of this parameter.

::: toolkit/components/extensions/ExtensionSettingsStore.jsm:140
(Diff revision 4)
>      return null;
>    }
>  
> +  if (id) {
> +    // Return the item that corresponds to the extension with id of id.
> +    let item = keyInfo.precedenceList.filter(item => item.id === id)[0];

I think `.find()` would be better than `.filter()` here
Attachment #8924294 - Flags: review?(aswan) → review-
Comment on attachment 8924294 [details]
Bug 1404584 Part 2: Convert ext-chrome-settings-overrides to use update and uninstall events

https://reviewboard.mozilla.org/r/195530/#review202986

> It looks like this might not work if an extension that adds a non-default search engine and sets it as the default is disabled and then re-enabled?

Why do you think this is the case? I tried adding a test for the case where an extension sets a non-built-in engine to the default, then disables and re-enables it, and it worked as expected. The test has an issue with needing to interact with the popup, which I discussed with Mike in IRC and he opened bug 1418084 about adding some tests, so I haven't included the test in this patch, but I'm not sure if you are correct about this.
Comment on attachment 8924294 [details]
Bug 1404584 Part 2: Convert ext-chrome-settings-overrides to use update and uninstall events

https://reviewboard.mozilla.org/r/195530/#review202986

> Why do you think this is the case? I tried adding a test for the case where an extension sets a non-built-in engine to the default, then disables and re-enables it, and it worked as expected. The test has an issue with needing to interact with the popup, which I discussed with Mike in IRC and he opened bug 1418084 about adding some tests, so I haven't included the test in this patch, but I'm not sure if you are correct about this.

Hm, not sure, probably I missed the `if (reason == ENABLE)` branch in `setDefault()`.  In general I'm finding the whole implementation of search very difficult to follow with logic spread out all over the place :(
Comment on attachment 8924294 [details]
Bug 1404584 Part 2: Convert ext-chrome-settings-overrides to use update and uninstall events

https://reviewboard.mozilla.org/r/195530/#review206062
Attachment #8924294 - Flags: review?(aswan) → review+
Pushed by bsilverberg@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/18e5e565af57
Part 1: Use extensionId in ExtensionSettingsStore and ExtensionPreferencesManager methods, r=aswan
https://hg.mozilla.org/integration/autoland/rev/91aec40e17a3
Part 2: Convert ext-chrome-settings-overrides to use update and uninstall events, r=aswan,mkaply
https://hg.mozilla.org/integration/autoland/rev/ec69c7fcd213
Part 3: Convert ExtensionPreferencesManager to use update and uninstall events, r=aswan,jkt
https://hg.mozilla.org/mozilla-central/rev/18e5e565af57
https://hg.mozilla.org/mozilla-central/rev/91aec40e17a3
https://hg.mozilla.org/mozilla-central/rev/ec69c7fcd213
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Comment on attachment 8924293 [details]
Bug 1404584 Part 1: Use extensionId in ExtensionSettingsStore and ExtensionPreferencesManager methods,

Approval Request Comment
[Feature/Bug causing the regression]: 1341458
[User impact if declined]: If an extension overrides the home page and then releases an update which no longer overrides the home page, a user can be prevented from being able to change their home page via about:preferences.
[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]: No 
[List of other uplifts needed for the feature/fix]: Only the two other patches attached to this bug
[Is the change risky?]: Not really
[Why is the change risky/not risky?]: The change does touch a number of files, but all of the changes are covered by automated tests. In addition a try run against beta was completed at https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cde193906f42bde5ed524ce067f07036af2eaaf with no failures.
[String changes made/needed]: None
Attachment #8924293 - Flags: approval-mozilla-beta?
Comment on attachment 8924294 [details]
Bug 1404584 Part 2: Convert ext-chrome-settings-overrides to use update and uninstall events

Approval Request Comment
[Feature/Bug causing the regression]: 1341458
[User impact if declined]: If an extension overrides the home page and then releases an update which no longer overrides the home page, a user can be prevented from being able to change their home page via about:preferences.
[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]: No 
[List of other uplifts needed for the feature/fix]: Only the two other patches attached to this bug
[Is the change risky?]: Not really
[Why is the change risky/not risky?]: The change does touch a number of files, but all of the changes are covered by automated tests. In addition a try run against beta was completed at https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cde193906f42bde5ed524ce067f07036af2eaaf with no failures.
[String changes made/needed]: None
Attachment #8924294 - Flags: approval-mozilla-beta?
Comment on attachment 8924295 [details]
Bug 1404584 Part 3: Convert ExtensionPreferencesManager to use update and uninstall events

Approval Request Comment
[Feature/Bug causing the regression]: 1341458
[User impact if declined]: If an extension overrides the home page and then releases an update which no longer overrides the home page, a user can be prevented from being able to change their home page via about:preferences.
[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]: No 
[List of other uplifts needed for the feature/fix]: Only the two other patches attached to this bug
[Is the change risky?]: Not really
[Why is the change risky/not risky?]: The change does touch a number of files, but all of the changes are covered by automated tests. In addition a try run against beta was completed at https://treeherder.mozilla.org/#/jobs?repo=try&revision=2cde193906f42bde5ed524ce067f07036af2eaaf with no failures.
[String changes made/needed]: None
Attachment #8924295 - Flags: approval-mozilla-beta?
Hi :bsilverberg,
Given bug 1341458 was fixed in 55, it's been there for several versions. 
So, if we let this ride the train on 59 only and won't fix in 58, will this be a big impact for user for 58 because these patches are way too big to me and I would like to get the extra bake time in nightly.
Flags: needinfo?(bob.silverberg)
(In reply to Gerry Chang [:gchang] from comment #49)
> Hi :bsilverberg,
> Given bug 1341458 was fixed in 55, it's been there for several versions. 
> So, if we let this ride the train on 59 only and won't fix in 58, will this
> be a big impact for user for 58 because these patches are way too big to me
> and I would like to get the extra bake time in nightly.

This seems like a reasonable assessment. The only thing I want to add is that although the API landed originally in 55, with legacy add-ons being removed in 57 and everything moving to WebExtensions it is likely that this API will see much more use in 57+. If you still feel the risk outweighs the benefits I have no problem with that.
Flags: needinfo?(bob.silverberg)
Hi :bsilverberg,
Thanks for the information. Let's let it ride the train 59 and bake more.
Attachment #8924293 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8924294 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Attachment #8924295 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Flags: qe-verify-
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: