Closed Bug 1477904 Opened Last year Closed Last year

Correctly handle static varcache prefs whose default values change after initialization

Categories

(Core :: Preferences: Backend, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

Details

Attachments

(1 file)

We currently don't correctly handle static var cache preferences in the content process when their default values differ from the statically defined defaults. This is a weird corner case, but the fact that we don't handle it consistently leads to subtle and hard to diagnose problems, so we should fix it.
Duplicate of this bug: 1478065
Comment on attachment 8994401 [details]
Bug 1477904: Correctly handle static var caches with changed default values.

https://reviewboard.mozilla.org/r/258966/#review266224

::: modules/libpref/Preferences.cpp:4848
(Diff revision 1)
> -    // While it is technically also possible for the default values to have
> -    // changed at runtime, and therefore not match the static defaults, we don't
> -    // support that for static preferences in this configuration, and therefore
> -    // ignore the possibility.
>      for (auto& pref : gSharedMap->Iter()) {
> -      if (pref.HasUserValue() || pref.IsLocked()) {
> +      if (pref.HasUserValue() || pref.DefaultChanged()) {

Why remove the IsLocked() call? Presumably because mHasDefaultValue can't be set on a locked pref? But locking can be done at runtime...

::: modules/libpref/init/StaticPrefList.h:153
(Diff revision 1)
>    bool, PREF_VALUE
>  )
>  #undef PREF_VALUE
>  
> +// NOTE: This preference is used in unit tests. If it is removed or its default
> +// value changes, please update test_sharedMap_var_caches.js accordingly.

Good comment.

::: modules/libpref/test/unit_ipc/test_sharedMap_var_caches.js:6
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +"use strict";
> +
> +// Tests that static preference variable caches in the content process

Use "varcache" instead of "variable cache"?

::: modules/libpref/test/unit_ipc/test_sharedMap_var_caches.js:10
(Diff revision 1)
> +
> +// Tests that static preference variable caches in the content process
> +// correctly handle values which are different from their
> +// statically-defined defaults.
> +//
> +// Since we can't access variable cache values from JS, this tests

ditto
Attachment #8994401 - Flags: review?(n.nethercote) → review+
Comment on attachment 8994401 [details]
Bug 1477904: Correctly handle static var caches with changed default values.

https://reviewboard.mozilla.org/r/258966/#review266224

> Why remove the IsLocked() call? Presumably because mHasDefaultValue can't be set on a locked pref? But locking can be done at runtime...

This condition handles all of the cases that the old one did. If we're handling all changed default values, we don't need to special case just the locked ones.
https://hg.mozilla.org/mozilla-central/rev/d6c5e93d33a1
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.