1541317 - Adding chrome_settings_overrides keys in manifest file of extension during update of version, makes the extension unusable

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P1)

Description

[Arun Puri]

Attachment: Zip with 2 versions of a sample extension to reproduce problem. TBEV002.xpi is older version, and TBEV003.xpi is updated version with chrome_settings_overrides keys

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:66.0) Gecko/20100101 Firefox/66.0

Steps to reproduce:

1. Install an extension without chrome_settings_overrides keys
2. Upgrade extension with chrome_settings_overrides keys (Hompage and Search) via auto-upgrade or manual drag and drop.

Actual results:

1. Extension stops working either immediately or upon restart of browser, e.g Popup does not appear if you click on Extension Button in Toolbar. Tested in FF and FF Developer Edition. The settings in manifest keys are not applied as well.
2. The extension works correctly if upgraded version is applied directly (Fresh Install) and not upgraded.

Expected results:

1. Extension should continue to work and new values should be applied.

Comment 1

[Rares Doghi, Desktop QA]

Hi, We tried loading those extensions in about:debugging and then updating them with drag and drop but I'm getting some error that its not been verified, Are there any other official Extensions that might have those Chrome_settings_overrides ? so we can download an older version and then just try to update them ?

I'll set the component for this issue, maybe our Devs will understand a bit more why this issue occurs for you.

Comment 2

[Arun Puri]

Attachment: Signed Extensions.

Thanks for the reply Rares.

I had added a simplified version deliberately so that the dev team would be able to concentrate on only the relevant parts. The full extension is quite complex and would be difficult to isolate out individual parts.

I have uploaded the simplified extension and got it signed. I have added Signed Attachment. Let me know if you have any further assistance or questions.

Comment 3

[Andrew Swan [:aswan]]

Looks like upgrades were just not considered here:
https://searchfox.org/mozilla-central/rev/dd7e27f4a805e4115d0dbee70e1220b23b23c567/browser/components/extensions/parent/ext-chrome-settings-overrides.js#178-179

Better tests would be good too, including upgrades that add a new homepage setting, remove an old one, and change an existing one.

Mark, you were the last one to touch this code so tag, you're it :) Do you have the time/interest to address this?

Comment 4

[Shane Caraveo (:mixedpuppy)]

(In reply to Andrew Swan [:aswan] from comment #3)

Looks like upgrades were just not considered here:

On purpose. No settings are supported during an upgrade of an extension to avoid allowing an extension to, essentially, silently steal that setting. Search works this way as well. In fact, the only time we should be changing the prefs is on install.

I am however asking mconca if we desire a change in behavior here.

Comment 5

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1541317 support homepage setting on upgrade

Comment 6

[Andrew Swan [:aswan]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #4)

Looks like upgrades were just not considered here:

On purpose. No settings are supported during an upgrade of an extension to avoid allowing an extension to, essentially, silently steal that setting. Search works this way as well. In fact, the only time we should be changing the prefs is on install.

That seems totally reasonable as the desired behavior. But adding a search engine shouldn't brick the extension. And we should have tests that verify that the desired behavior is implemented correctly.

Comment 7

[Shane Caraveo (:mixedpuppy)]

(In reply to Andrew Swan [:aswan] from comment #6)

But adding a search engine shouldn't brick the extension. And we should have tests that verify that the desired behavior is implemented correctly.

That's what the patch should be doing.

Comment 8

[Mike Conca [:mconca]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #4)

(In reply to Andrew Swan [:aswan] from comment #3)

Looks like upgrades were just not considered here:

On purpose. No settings are supported during an upgrade of an extension to avoid allowing an extension to, essentially, silently steal that setting. Search works this way as well. In fact, the only time we should be changing the prefs is on install.

I am however asking mconca if we desire a change in behavior here.

We don't want extensions silently stealing settings, so this works. That said, there is probably room for improvement in the UX of this behavior. Alerting the user, and giving them options on how to proceed, would be nice, but are outside the scope of this bug.

Comment 9

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6671e51b20473d13fd3ac311d2ec07adf768c59

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f706ae697922
support homepage setting on upgrade r=rpl,mkaply

Comment 11

[Noemi Erli[:noemi_erli]]

Backed out changeset f706ae697922 (Bug 1541317) for failures in test_ext_settings_overrides_search.js

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=f706ae6979228b41360809a5f5debb65a00b403f&selectedJob=243009292

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=243009292&repo=autoland&lineNumber=2202

Backout: https://hg.mozilla.org/integration/autoland/rev/5e69a320a5a1d56962c8928c650da450f9dbbc31

Comment 12

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1541317 fix extension failures when setting homepage on upgrade

Comment 13

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d01be691bcffd2f37cc73c7b2e84220fc762835a

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b1d829e6fe2f
fix extension failures when setting homepage on upgrade r=aswan

Comment 15

[Daniel Varga [:dvarga]]

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

Comment 16

[Shane Caraveo (:mixedpuppy)]

Attachment: browserAction failure

after replaying the STR a couple times, I'm able to end up in a state where the browser action in the extensions are broker per this image. I'm unable to produce this with the patch.

Comment 17

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 9062630 [details]
Bug 1541317 fix extension failures when setting homepage on upgrade

Beta/Release Uplift Approval Request

• User impact if declined: There is a potential for various broken side effects from an extension adding a homepage setting via upgrade. An easily visible side effect (illustration of broken extension after upgrade) of the problem is in the image I attached in comment 16.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: An STR is provided in comment 0.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The actual code change is very minimal.
• String changes made/needed: none

Comment 18

[Pascal Chevrel:pascalc]

Comment on attachment 9062630 [details]
Bug 1541317 fix extension failures when setting homepage on upgrade

Low risk, approved for our beta branch.

Comment 19

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6124d4a75c1f

Comment 20

[CosminB]

Attachment: Bug1541317.zip

I was able to reproduce this issue on Firefox 68.0a1(20190402083512) and Firefox 67.0b18(20190506235559) under Win 7 64-bit.

This issue is verified as fixed on Firefox 68.0a1 (20190509214305) and Firefox 67.0b19(20190509185224) under Win 7 64-bit and Mac OS X 10.14.1.

Please see the attached videos.