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

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P1)

Description

[Drew Willcoxon :adw]

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

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1576266 fix handling of non-default prefs used with ExtensionPreferencesManager

Comment 2

[Shane Caraveo (:mixedpuppy)]

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

Comment 3

[Shane Caraveo (:mixedpuppy)]

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

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/281856fd4019
fix handling of non-default prefs used with ExtensionPreferencesManager r=robwu

Comment 5

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/281856fd4019

Comment 6

[Miruna Curtean]

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.

Comment 7

[Shane Caraveo (:mixedpuppy)]

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

Comment 8

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 9

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

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

Comment 10

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1ee62510b8eb

Comment 11

[Shane Caraveo (: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.

Comment 12

[Miruna Curtean]

Attachment: eventTelemetry not enabled.gif

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.

Comment 13

[Shane Caraveo (: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.

Comment 14

[Shane Caraveo (:mixedpuppy)]

Attachment: simple.xpi

An extension that modifies a couple settings for testing.

Comment 15

[Miruna Curtean]

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.