Closed Bug 1576266 Opened 2 months ago Closed Last month

BrowserSetting prefs not defined by default are not cleared after restarting Firefox and unloading the controlling extension

Categories

(WebExtensions :: General, defect, P1)

defect

Tracking

(firefox69 wontfix, firefox70 verified, firefox71 verified)

VERIFIED FIXED
mozilla71
Tracking Status
firefox69 --- wontfix
firefox70 --- verified
firefox71 --- verified

People

(Reporter: adw, Assigned: mixedpuppy)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

STR:

  1. Install an extension that calls browser.urlbar.engagementTelemetry.set({ value: true }) [1]
  2. Restart Firefox
  3. Uninstall or disable the extension
  4. Open about:config and observe that browser.urlbar.eventTelemetry.enabled remains true when it should have been cleared

If you skip step 2 -- i.e., don't restart Firefox -- the pref is cleared, as expected.

The browser.urlbar.engagementTelemetry browser setting maps to the browser.urlbar.eventTelemetry.enabled pref, which does not exist by default. See https://searchfox.org/mozilla-central/rev/a650fac7b069ded3d63eba530271ce80e6cafb1b/browser/components/extensions/parent/ext-urlbar.js#201

I'm not sure whether this is a bug in the browser.urlbar implementation or something deeper in the webextensions framework. I did some debugging and it looks like the problem is in or around ExtensionPreferencesManager.

When I restart Firefox and then unload the extension, setPrefs is called, and prefs is an empty object here: https://searchfox.org/mozilla-central/source/toolkit/components/extensions/ExtensionPreferencesManager.jsm#99

In contrast, when I don't restart Firefox before unloading the extension, prefs is { "browser.urlbar.eventTelemetry.enabled": undefined }.

And when I give browser.urlbar.eventTelemetry.enabled a default value of false by adding it to browser/app/profile/firefox.js, prefs is { "browser.urlbar.eventTelemetry.enabled": false }.

Here's an excerpt from a chat about it with Shane:

mixedpuppy: ok, so what happens is that there is an initialvalue that is set via Preferences.get(prefname)
mixedpuppy: if that initialvalue is undefined, then Preferences.reset(prefname) shoudl be called
mixedpuppy: https://searchfox.org/mozilla-central/rev/a650fac7b069ded3d63eba530271ce80e6cafb1b/toolkit/components/extensions/ExtensionPreferencesManager.jsm#79-115

[1] e.g. https://github.com/0c0w3/urlbar-top-sites-experiment -- xpi in bug 1547670

Flags: needinfo?(mixedpuppy)
See Also: → 1576997
Assignee: nobody → mixedpuppy
Flags: needinfo?(mixedpuppy)
Priority: -- → P1
Blocks: 1578508
Pushed by scaraveo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/281856fd4019
fix handling of non-default prefs used with ExtensionPreferencesManager r=robwu
Status: NEW → RESOLVED
Closed: Last month
Resolution: --- → FIXED
Target Milestone: --- → mozilla71

Verified fixed on Windows 10 64-bit with latest Nightly 71.0a1 (201909122154120) and Ubtunu 18.04.3 LTS FF 71.0a1 (20190913092859).
Due to not being able to successfully install the .xpi file, the workaround used was to temporarily load the extension manifest in about:debugging and checking that the browser.urlbar.eventTelemetry.enabled had been set to true.
After restarting the browser the extension was removed due to only being temporarily removed (which is about the same as being uninstalled) and the browser.urlbar.eventTelemetry.enabled preference was reset to false.

Status: RESOLVED → VERIFIED

Comment on attachment 9089236 [details]
Bug 1576266 fix handling of non-default prefs used with ExtensionPreferencesManager

Beta/Release Uplift Approval Request

  • User impact if declined: Some experiments are using prefs that are not in the default set but still have side effects (e.g. turning on telemetry pings as in this bug). This patch supports unsetting those prefs if they are not in the default set.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): low. small patch with automated tests.
  • String changes made/needed: none
Attachment #9089236 - Flags: approval-mozilla-beta?

Comment on attachment 9089236 [details]
Bug 1576266 fix handling of non-default prefs used with ExtensionPreferencesManager

Fix for pref settings used by experiments. OK for uplift for beta 9.

Attachment #9089236 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Shane - when did this issue start? Which versions are affected?

Flags: needinfo?(mixedpuppy)

(In reply to Liz Henry (:lizzard) from comment #9)

Shane - when did this issue start? Which versions are affected?

Current release and later at least, but given what I've reviewed and started to address, I'd assume the issue goes further back than that.

Flags: needinfo?(mixedpuppy)

I've tried to test this on Firefox Beta 70.0b9 (20190923154733) after the uplift, as part of the QA procedure through the same method as in comment 6 but this time when the extension manifest is uploaded, the extension pref is not toggled to True.See GIF.
Also attempted to test this on the unbranded version 70.0b9 (20190923062057) with the same results.
Was this step for testing the bug invalid in the first place or is this an issue that started occurring on Beta?
Also noticed in the Beta/Release Uplift Approval Request that no manual QE is needed and there are automated testes so if there was no need for testing starting with Nightly and the method of verifying the manual fix was also flawed, please set the "qe verify -" flag.
Thank you.

Flags: needinfo?(mixedpuppy)

I don't think you can test using urlbar on beta since that is a privileged api and the extension would require mozilla signing. I'll attach an extension that uses both urlbar and browserSettings. You'll see that the urlbar pref will change on nightly (given your str) but not on beta. The browserSettings pref it modifies (permissions.default.desktop-notification) will change in both because browserSettings is not a privileged api.

Flags: needinfo?(mixedpuppy) → needinfo?(mcurtean)
Attached file simple.xpi

An extension that modifies a couple settings for testing.

Thank you so much for the extension and the explanation.
I've been able to perform the test in Nightly 71.0a1 (Build ID: 20190926213542) in order to see that both urlbar and browserSettings prefs were modified.
After that I've verified on Windows 64-bit and macOS High Sierra 10.13.6 on Beta Firefox 70.0b9 (Build ID: 20190923154733) that the browserSettings (permissions.default.desktop-notification) pref had changed value as expected.
As it was proven in the Nightly test that the extension modifies both urlbar (browser.urlbar.eventTelemetry.enabled) and browserSettings prefs, I am setting this as verified fixed.

Flags: needinfo?(mcurtean)
You need to log in before you can comment on or make changes to this bug.