1208744 - PrefCache observer doesn't check pref name, but prefix-matches the pref, causing brokenness if one pref is a strict prefix of another pref

Status: RESOLVED DUPLICATE of bug 1267868

(Core :: Preferences: Backend, defect)

Description

[:Gijs (he/him)]

STR:

1. add two BoolPrefCaches, for "pref.foo" and "pref.foobar", respectively.
2. change "pref.foobar" to a different value
3. inspect the cache for pref.foo

ER:
hasn't changed

AR:
has changed (!) because the observer doesn't actually check that the notification it's getting is for the correct pref.

This caught me out in bug 636905 (where I tried to add dom.disable_beforeunload_without_user_interaction or something, when dom.disable_beforeunload already existed), and it was really somewhat lucky that I caught it at all.

I think we should fix this gotcha, not least because the result isn't too bad if the 2 prefs are of the same type, but I expect assigning the wrong kind of data into the pref cache is going to make for very sad results.

Comment 1

[:Gijs (he/him)]

These are all the prefs that have prefixes in the current combination of firefox.js + all.js, followed by their prefixes:

accessibility.browsewithcaret_shortcut.enabled	accessibility.browsewithcaret
accessibility.tabfocus_applies_to_xul	accessibility.tabfocus
accessibility.typeaheadfind.autostart	accessibility.typeaheadfind
app.update.url.override	app.update.url
browser.cache.use_new_backend_temp	browser.cache.use_new_backend
browser.gesture.pinch.in.shift	browser.gesture.pinch.in
browser.gesture.pinch.out.shift	browser.gesture.pinch.out
browser.link.open_newwindow.disabled_in_fullscreen	browser.link.open_newwindow
browser.pocket.enabledLocales	browser.pocket.enabled
browser.safebrowsing.provider.mozilla.lists.mozfull.description	browser.safebrowsing.provider.mozilla.lists
browser.search.defaultenginename.US	browser.search.defaultenginename
browser.search.geoSpecificDefaults.url	browser.search.geoSpecificDefaults
browser.search.update.interval	browser.search.update
browser.send_pings.max_per_link	browser.send_pings
browser.sessionstore.privacy_level_deferred	browser.sessionstore.privacy_level
browser.tabs.crashReporting.emailMe	browser.tabs.crashReporting.email
browser.tabs.remote.autostart.1	browser.tabs.remote.autostart
browser.tabs.warnOnCloseOtherTabs	browser.tabs.warnOnClose
browser.urlbar.autoFill.typed	browser.urlbar.autoFill
browser.urlbar.suggest.history.onlyTyped	browser.urlbar.suggest.history
browser.usedOnWindows10.introURL	browser.usedOnWindows10
devtools.browserconsole.filter.networkinfo	devtools.browserconsole.filter.network
devtools.debugger.log.verbose	devtools.debugger.log
devtools.netmonitor.har.jsonpCallback	devtools.netmonitor.har.jsonp
devtools.performance.ui.enable-memory-flame	devtools.performance.ui.enable-memory
devtools.webconsole.filter.networkinfo	devtools.webconsole.filter.network
dom.push.adaptive.lastGoodPingInterval.mobile	dom.push.adaptive.lastGoodPingInterval
dom.push.pingInterval.default	dom.push.pingInterval
font.weight-override.Avenir-BlackOblique	font.weight-override.Avenir-Black
font.weight-override.Avenir-BookOblique	font.weight-override.Avenir-Book
font.weight-override.HelveticaNeue-LightItalic	font.weight-override.HelveticaNeue-Light
general.smoothScroll.durationToIntervalRatio	general.smoothScroll
general.smoothScroll.lines.durationMaxMS	general.smoothScroll.lines
general.smoothScroll.mouseWheel.durationMaxMS	general.smoothScroll.mouseWheel
general.smoothScroll.other.durationMaxMS	general.smoothScroll.other
general.smoothScroll.pages.durationMaxMS	general.smoothScroll.pages
general.smoothScroll.pixels.durationMaxMS	general.smoothScroll.pixels
general.smoothScroll.scrollbars.durationMaxMS	general.smoothScroll.scrollbars
intl.hyphenation-alias.bs-*	intl.hyphenation-alias.bs
intl.hyphenation-alias.de-*	intl.hyphenation-alias.de
intl.hyphenation-alias.en-*	intl.hyphenation-alias.en
intl.hyphenation-alias.no-*	intl.hyphenation-alias.no
intl.hyphenation-alias.sr-*	intl.hyphenation-alias.sr
intl.imm.composition_font.japanist_2003	intl.imm.composition_font
javascript.options.compact_on_user_inactive_delay	javascript.options.compact_on_user_inactive
javascript.options.ion.offthread_compilation	javascript.options.ion
javascript.options.mem.gc_incremental_slice_ms	javascript.options.mem.gc_incremental
javascript.options.strict.debug	javascript.options.strict
layers.dump-client-layers	layers.dump
layout.frame_rate.precise	layout.frame_rate
layout.imagevisibility.enabled_for_browser_elements_only	layout.imagevisibility.enabled
media.getusermedia.aec_enabled	media.getusermedia.aec
media.getusermedia.agc_enabled	media.getusermedia.agc
media.getusermedia.noise_enabled	media.getusermedia.noise
media.gmp-manager.url.override	media.gmp-manager.url
media.navigator.load_adapt.avg_seconds	media.navigator.load_adapt
media.peerconnection.ice.tcp_so_sock_count	media.peerconnection.ice.tcp
mousewheel.default.action.override_x	mousewheel.default.action
mousewheel.with_alt.action.override_x	mousewheel.with_alt.action
mousewheel.with_control.action.override_x	mousewheel.with_control.action
mousewheel.with_meta.action.override_x	mousewheel.with_meta.action
mousewheel.with_shift.action.override_x	mousewheel.with_shift.action
mousewheel.with_win.action.override_x	mousewheel.with_win.action
network.IDN.whitelist.cat	network.IDN.whitelist.ca
network.auth.force-generic-ntlm-v1	network.auth.force-generic-ntlm
network.dnsCacheExpirationGracePeriod	network.dnsCacheExpiration
network.http.accept-encoding.secure	network.http.accept-encoding
network.http.pipelining.abtest	network.http.pipelining
network.http.spdy.enabled.deps	network.http.spdy.enabled
network.protocol-handler.external.disks	network.protocol-handler.external.disk
network.protocol-handler.external.ttps	network.protocol-handler.external.ttp
network.proxy.ftp_port	network.proxy.ftp
network.proxy.http_port	network.proxy.http
network.proxy.socks_port	network.proxy.socks
network.proxy.ssl_port	network.proxy.ssl
nglayout.debug.paint_flashing_chrome	nglayout.debug.paint_flashing
plugin.state.nprobloxproxy	plugin.state.nproblox
plugin.state.playerplugin.charter	plugin.state.playerplugin
reader.color_scheme.values	reader.color_scheme
security.sandbox.windows.log.stackTraceDepth	security.sandbox.windows.log
services.sync.prefs.sync.accessibility.typeaheadfind.linksonly	services.sync.prefs.sync.accessibility.typeaheadfind
signon.rememberSignons.visibilityToggle	signon.rememberSignons
toolkit.telemetry.server_owner	toolkit.telemetry.server
ui.key.menuAccessKeyFocuses	ui.key.menuAccessKey


I haven't audited which of these, if any, use prefcaches, but it seems only a matter of time until this bites us again, also because it seems we create these in the DOM bindings code, amongst other places, purely from codegen.


In fact, a casual look through:

https://dxr.mozilla.org/mozilla-central/search?q=regexp%3A%3A%3Aadd\S%2Bcache&redirect=false&case=false

shows that we are already having this problem in practice:

https://dxr.mozilla.org/mozilla-central/rev/b68eab795f9de072bee12821b0f09422e5aa0da9/layout/base/nsPresShell.cpp#5858

which was added in bug 930535.

:tn, do you have time to investigate fixing this?

Comment 2

[Timothy Nikkel (:tnikkel)]

(In reply to :Gijs Kruitbosch from comment #1)
> layout.imagevisibility.enabled_for_browser_elements_only
> layout.imagevisibility.enabled
>
> shows that we are already having this problem in practice:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> b68eab795f9de072bee12821b0f09422e5aa0da9/layout/base/nsPresShell.cpp#5858
> 
> which was added in bug 930535.
> 
> :tn, do you have time to investigate fixing this?

layout.imagevisibility.enabled_for_browser_elements_only is obsolete, so I'll just remove it: bug 1213953.

Comment 3

[Masatoshi Kimura [:emk]]

How about adding "exact match" mode to pref callbacks?

Comment 4

[Lina Butler [:lina]]

Did bug 1267868 address this?

Comment 5

[:Gijs (he/him)]

(In reply to Kit Cambridge [:kitcambridge] from comment #4)
> Did bug 1267868 address this?

Looks like it, yes! \o/

Thanks for flagging that up.

Comment 6

[Lina Butler [:lina]]

Cool! I wasn't sure if that was the same issue at first, so that's why I didn't dupe it. :-)