Closed Bug 1541317 Opened 5 months ago Closed 4 months ago

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

Categories

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

66 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla68
Tracking Status
firefox67 --- verified
firefox68 --- verified

People

(Reporter: arunpuri, Assigned: mixedpuppy)

References

Details

Attachments

(5 files, 1 obsolete file)

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.

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.

Component: Untriaged → Add-ons Manager
Product: Firefox → Toolkit
Attached file 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.

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?

Flags: needinfo?(mstriemer)
Assignee: nobody → mixedpuppy
Priority: -- → P2
Priority: P2 → P1
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

(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.

Flags: needinfo?(mstriemer) → needinfo?(mconca)

(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.

(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.

(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.

Flags: needinfo?(mconca)
Pushed by scaraveo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f706ae697922
support homepage setting on upgrade r=rpl,mkaply
See Also: → 1548700
Attachment #9059620 - Attachment is obsolete: true
Pushed by scaraveo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b1d829e6fe2f
fix extension failures when setting homepage on upgrade r=aswan
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Attached image 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 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
Attachment #9062630 - Flags: approval-mozilla-beta?

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

Low risk, approved for our beta branch.

Attachment #9062630 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attached file 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.

Status: RESOLVED → VERIFIED
Duplicate of this bug: 1494933
You need to log in before you can comment on or make changes to this bug.