Closed Bug 1208744 Opened 6 years ago Closed 4 years ago

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

Categories

(Core :: Preferences: Backend, defect)

defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1267868
Tracking Status
firefox44 --- affected

People

(Reporter: Gijs, Unassigned)

Details

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.
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?
Flags: needinfo?(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.
Flags: needinfo?(tnikkel)
How about adding "exact match" mode to pref callbacks?
Did bug 1267868 address this?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Kit Cambridge [:kitcambridge] from comment #4)
> Did bug 1267868 address this?

Looks like it, yes! \o/

Thanks for flagging that up.
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: needinfo?(gijskruitbosch+bugs)
Resolution: --- → DUPLICATE
Duplicate of bug: 1267868
Cool! I wasn't sure if that was the same issue at first, so that's why I didn't dupe it. :-)
You need to log in before you can comment on or make changes to this bug.