1410412 - Implement BrowserSetting.onChange

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement, P2)

Description

[Frederik Braun [:freddyb]]

imho it's pretty hard to write an extension that changes a pref when it can't properly monitor for other changes and update its UI accordingly.

One could use setInterval to poll though.

Comment 1

[Shane Caraveo (:mixedpuppy)]

bumping priority here, secure-proxy would need this to get out of experimental api land.

Comment 2

[Liz Henry (:lizzard)]

Changing to corp confidential temporarily (till after Sept 10) on Tony and elan's request.

Comment 3

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Can we open this up again? Is this still needed for secure-proxy?

Comment 4

[Tony Cinotto [:tcinotto] (UTC-8)]

Baku thoughts here?

Comment 5

[Shane Caraveo (:mixedpuppy)]

@Fallen, This should have been done from the very start, any extension using any number of settings should be (or be able to be) aware of when those settings change underneath them.

Comment 6

[Andrea Marchesini [:baku]]

It would be nice to have browserSettings.onChanged listener. Currently, secure-proxy uses an experimental API to monitor these 2 prefs: network.proxy.type and network.proxy.no_proxies_on, but if we had browser.Settings.onChanged, we can simply listen for proxyType and passthrough changes. See: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/proxy/settings

Comment 7

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1410412 implement browser setting onChange event

Comment 8

[Shane Caraveo (:mixedpuppy)]

I have a patch that introduces the settings.onChange api. The problem that this presents is, for those settings that make changes to multiple preferences, multiple notifications may happen. As well, the setting data received in any one of those may not be the correct "final" value.

I don't see a good way around this issue, we have no way to coalesce the preference changes. So I've a couple thoughts on how to change this so we can be a little more compatible, and a little more flexible.

Idea 1:

By default we could only notify when the setting is changed via the settings api. This would allow us to be "correct" for the setting onChange notification. This however will not provide notifications if something in the system takes over those values.

We have a need for some extensions to actually know if the underlying pref itself has changed. We could have an extra param for onChange.addListener where an extension provides a flag to use the preference observers.

Idea 2:

Many settings are a single pref, in which case the above is not an issue. We could add a "completed" flag on the details sent to the onChange listener. If completed is true, they would know that it is a reliable change. We would then turn this into a documentation issue and leave extensions to deal with the problem somehow.

Idea 3:

Combine parts of 1 and 2, make the details flag a "system" flag. For events emitted for changes through the settings api, system is false. For changes emitted via a pref observer, system is true.

Idea 4:

Essentially the same as 3, but we have two event managers, one for onChange and one for onSystemChange. onChange only fires for changes done via the api, onSystemChange hooks into the preference observers.

Looking for feedback and thoughts.

Comment 9

[:Tomislav Jovanovic :zombie]

This doesn't look like a new problem, but something that's already present with reading the settings value if the corresponding prefs are changed outside of this api.

I don't think we should over-complicate this, so one of two simple options might be enough:

1. Don't include the new value with the event, in the hope that by the time the extension asks for it, the whole group of prefs is already changed
2. Just debounce onChange events for some arbitrary amount of time, say half a second.

Comment 10

[Luca Greco [:rpl] [:luca] [:lgreco]]

As Tomislav I've been also thinking that debounce the events may be a reasonable way to deal with it.

Nevertheless, I'm wondering if that approach could be problematic for some particular settings ([1]), when knowing sooner that the settings has been changed may actually make a difference (e.g. if the preference been set has an impact on some privacy-related behaviors, and the extension monitoring that pref needs to know about that change as soon as possible to change some of its behavior accordingly).

If the scenario describe above is actual realistic ([2]), I think it may be reasonable to treat those settings differently, e.g.

• fire those events without any debounce, even if the settings value that the extension would get isn't going to change
• or cache the last value fired and don't fire it again if the "settings computed value" is not going to be different

[1]: I think that for most of them it shouldn't make any big difference, e.g. a little latency in emitting an changed event for "newTabPosition" shouldn't be really a problem
[2]: and at a first glance it seems it is, given that one of the extensions that wants to use this API is the secure-proxy extension

Comment 11

[Luca Greco [:rpl] [:luca] [:lgreco]]

Not related to the needinfo, but still related to this bugzilla issue:

we should call the new API event browserSettings.onChanged (instead of onChange) to make it consistent with the naming conventions used by other similar events we already have (e.g. the sessions API has a session.onChanged API event).

Comment 12

[Shane Caraveo (:mixedpuppy)]

(In reply to :Tomislav Jovanovic :zombie from comment #9)

This doesn't look like a new problem, but something that's already present with reading the settings value if the corresponding prefs are changed outside of this api.

I don't think we should over-complicate this, so one of two simple options might be enough:

1. Don't include the new value with the event, in the hope that by the time the extension asks for it, the whole group of prefs is already changed

I suppose we could do that, it would be incompatible with chrome, but that may not be such a big deal, we haven't had this api all this time.

2. Just debounce onChange events for some arbitrary amount of time, say half a second.

I'm hoping to avoid that, I could see it causing weird problems that are hard to understand.

(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #11)

we should call the new API event browserSettings.onChanged

The chrome compatible api is setting.onChange.

https://developer.chrome.com/extensions/types#ChromeSetting

I'm now inclined to just document the behavior, and suggest that extensions rely on getting the value again rather than what is received via the event. This really only effects those settings that use more than one pref, which is in the minority.

Comment 13

[Luca Greco [:rpl] [:luca] [:lgreco]]

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

(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #11)

we should call the new API event browserSettings.onChanged

The chrome compatible api is setting.onChange.

https://developer.chrome.com/extensions/types#ChromeSetting

sigh :-(
yeah, we need to name it onChange then, we could have it as an alias (as we did for menus and contextMenus), but it is really not worth it.

Comment 14

[:Tomislav Jovanovic :zombie]

(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #10)

(e.g. if the preference been set has an impact on some privacy-related behaviors, and the extension monitoring that pref needs to know about that change as soon as possible to change some of its behavior accordingly).

We already sorta "debounce" all the webRequest.onX events by up to 250ms, so things (especially privacy-related things) can already get out of sync, and it hasn't been an issue.

• fire those events without any debounce, even if the settings value that the extension would get isn't going to change

I'm afraid that may end up in extensions wars over some settings, and debounce could at least slow that down somewhat. :shrug:

Comment 15

[Shane Caraveo (:mixedpuppy)]

(In reply to :Tomislav Jovanovic :zombie from comment #14)

(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #10)

(e.g. if the preference been set has an impact on some privacy-related behaviors, and the extension monitoring that pref needs to know about that change as soon as possible to change some of its behavior accordingly).

We already sorta "debounce" all the webRequest.onX events by up to 250ms, so things (especially privacy-related things) can already get out of sync, and it hasn't been an issue.

We would need to coalesce the onChange events, but the problem is that we don't know how many we'd get for a particular setting. That the reason behind Idea 1.

• fire those events without any debounce, even if the settings value that the extension would get isn't going to change

I'm afraid that may end up in extensions wars over some settings, and debounce could at least slow that down somewhat. :shrug:

I'm not sure I follow here. Precedence is by install time. So the potential for changing is minimal. Once a "newer" extension takes control of the setting, the "older" extension cannot take control back. The "newer" extension can clear the setting, which would remove itself from the precedence list and control is relinquished to the prior extension in the precedence list.

Comment 16

[:Tomislav Jovanovic :zombie]

We would need to coalesce the onChange events, but the problem is that we don't know how many we'd get for a particular setting. That the reason behind Idea 1.

We don't need to if we use a time-based debounce, just like we don't know how many webRequests events are coming.

I'm not sure I follow here. Precedence is by install time.

Ah, I missed that, much better.

Comment 17

[Rob Wu [:robwu]]

I suppose that onPrefsChanged cannot be used because it only detects changes via ExtensionPreferencesManager, right?

I hope that there is no need to debounce. onChange events should only fire if the extension-observable value changes (see my comment at https://phabricator.services.mozilla.com/D51324#1566719 ).
If the setting's value may be unstable due to expected changes in preference value, then it could be implemented in the callback themselves - https://searchfox.org/mozilla-central/rev/35873cfc312a6285f54aa5e4ec2d4ab911157522/toolkit/components/extensions/ExtensionPreferencesManager.jsm#441

Comment 18

[Shane Caraveo (:mixedpuppy)]

I've been thinking about this a bunch, and I think that the right path to go is to only have notifications happen when EPM does the change.

• about:preferences should be blocking changes to any prefs already, if it is extension controlled. So if the user "changes" it, they've disabled or uninstalled, situations where we would get the update. This alone will allow an extension to know a newer extension has taken control.
• I need to revive bug 1548700 which would allow for selecting a specific extension (or back to default/user values) to control a pref set, in which case it would also go through EPM.
• as part of that bug or another followup, we can add something to push a notification that the setting has changed if the user chooses/changes default/user-set values. That should catch extensions that are just watching for changes, but not using the setting.

That leaves about:config changes, which are already a non-starter.

So this patch can take care of the first bullet, the others will come later.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d9b0de6a3abc
implement browser setting onChange event r=zombie

Comment 20

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fdd07df83c87
Backed out changeset d9b0de6a3abc for failing at browser_extension_controlled.js on a CLOSED TREE.

Comment 21

[Raul Gurzau (:RaulG)]

Backed out changeset d9b0de6a3abc (bug 1410412) for failing at browser_extension_controlled.js on a CLOSED TREE.

Backout link: https://hg.mozilla.org/integration/autoland/rev/fdd07df83c87f12725f4b97c80e644fd11673977

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

Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=276868666&repo=autoland&lineNumber=2446

Log snippet:

[task 2019-11-18T23:17:55.449Z] 23:17:55 INFO - Console message: [JavaScript Error: "Error: An unexpected error occurred" {file: "moz-extension://fe719452-0f64-43ec-a56a-92cff3e4df5a/%7B442ee706-a772-4624-a2fa-bb7891fcf7a1%7D.js" line: 2}]
[task 2019-11-18T23:17:55.450Z] 23:17:55 INFO - Buffered messages finished
[task 2019-11-18T23:17:55.451Z] 23:17:55 INFO - TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content/tests/browser_extension_controlled.js | Test timed out -
[task 2019-11-18T23:17:55.451Z] 23:17:55 INFO - Not taking screenshot here: see the one that was previously logged
[task 2019-11-18T23:17:55.452Z] 23:17:55 INFO - TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content/tests/browser_extension_controlled.js | Extension left running at test shutdown -
[task 2019-11-18T23:17:55.452Z] 23:17:55 INFO - Stack trace:
[task 2019-11-18T23:17:55.452Z] 23:17:55 INFO - chrome://mochikit/content/browser-test.js:test_ok:1299
[task 2019-11-18T23:17:55.453Z] 23:17:55 INFO - chrome://mochikit/content/tests/SimpleTest/ExtensionTestUtils.js:ExtensionTestUtils.loadExtension/<:109
[task 2019-11-18T23:17:55.453Z] 23:17:55 INFO - chrome://mochikit/content/browser-test.js:nextTest:577
[task 2019-11-18T23:17:55.453Z] 23:17:55 INFO - chrome://mochikit/content/browser-test.js:timeoutFn:1190
[task 2019-11-18T23:17:55.454Z] 23:17:55 INFO - setTimeout handler*chrome://mochikit/content/browser-test.js:Tester_execTest:1137
[task 2019-11-18T23:17:55.454Z] 23:17:55 INFO - chrome://mochikit/content/browser-test.js:nextTest/<:932
[task 2019-11-18T23:17:55.455Z] 23:17:55 INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:805
[task 2019-11-18T23:17:55.456Z] 23:17:55 INFO - GECKO(2006) | MEMORY STAT | vsize 20975883MB | residentFast 1853MB
[task 2019-11-18T23:17:55.456Z] 23:17:55 INFO - TEST-OK | browser/components/preferences/in-content/tests/browser_extension_controlled.js | took 90299ms
[task 2019-11-18T23:17:55.456Z] 23:17:55 INFO - Not taking screenshot here: see the one that was previously logged
[task 2019-11-18T23:17:55.457Z] 23:17:55 INFO - TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content/tests/browser_extension_controlled.js | Found a tab after previous test timed out: about:preferences#privacy -
[task 2019-11-18T23:17:55.457Z] 23:17:55 INFO - checking window state

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4d11ccc12529
implement browser setting onChange event r=zombie

Comment 23

[Bogdan Tara[:bogdan_tara]]

Backed out changeset 4d11ccc12529 (Bug 1410412) on mixedpuppy's request

Backout link: https://hg.mozilla.org/integration/autoland/rev/fbf7d8f9ba38924f9a71fd5ffed8441ab97bcbf0

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cf30ec111af9
implement browser setting onChange event r=zombie

Comment 25

[Andreea Pavel [:apavel]]

Backed out for failing browser_ext_chrome_settings_overrides_home.js

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

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

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

Comment 26

[Shane Caraveo (:mixedpuppy)]

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

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4a8770b8e4b9
implement browser setting onChange event r=zombie

Comment 28

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/4a8770b8e4b9

Comment 29

[Shane Caraveo (:mixedpuppy)]

This is reported as unsupported, it just needs to update to being supported in 72.

https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/types/BrowserSetting/onChange

Comment 30

[Alex Cornestean]

Hello,

Will this fix require manual validation? If yes, please provide some steps to reproduce in order to correctly test it and also, please set the "qe-verify+" flag. Otherwise, could the "qe-verify-" flag be added? Thanks!