Currently VarCache prefs are setup in two parts:
- The vanilla pref part, installed via a data file such as all.js, or via an
API call.
- The VarCache variable part, setup by an Add*VarCache() call.
Both parts are needed for the pref to actually operate as a proper VarCache
pref. (There are various prefs for which we do one but not the other unless a
certain condition is met.)
This patch introduces a new way of doing things. There is a new file,
modules/libpref/init/StaticPrefList.h, which defines prefs like this:
> VARCACHE_PREF(
> "layout.accessiblecaret.width",
> layout_accessiblecaret_width,
> float, 34.0
> )
This replaces both the existing parts.
The preprocessor is used to generate multiple things from this single
definition:
- A global variable (the VarCache itself).
- A getter for that global variable.
- A call to an init function that unconditionally installs the pref in the
prefs hash table at startup.
C++ files can include the new StaticPrefs.h file to access the getter.
Rust code cannot use the getter, but can access the global variable directly
via structs.rs. This is similar to how things currently work for Rust code.
Non-VarCache prefs can also be declared in StaticPrefList.h, using PREF instead
of the VARCACHE_PREF.
The new approach has the following advantages.
+ It eliminates the duplication (in all.js and the Add*VarCache() call) of the
pref name and default value, preventing potential mismatches. (This is a real
problem in practice!)
+ There is now a single initialization point for these VarCache prefs.
+ This avoids need to find a place to insert the Add*VarCache() calls, which
are currently spread all over the place.
+ It also eliminates the common pattern whereby these calls are wrapped in a
execute-once block protected by a static boolean (see bug 1346224).
+ It's no longer possible to have a VarCache pref for which only one of the
pieces has been setup.
+ It encapsulates the VarCache global variable, so there is no need to declare
it separately.
+ VarCache reads are done via a getter (e.g. StaticPrefs::foo_bar_baz())
instead of a raw global variable read.
+ This makes it clearer that you're reading a pref value, and easier to
search for uses.
+ This prevents accidental writes to the global variable.
+ This prevents accidental mistyping of the pref name.
+ This provides a single chokepoint in the code for such accesses, which make
adding checking and instrumentation feasible.
+ It subsumes MediaPrefs, and will allow that class to be removed. (gfxPrefs is
a harder lift, unfortunately.)
+ Once all VarCache prefs are migrated to the new approach, the VarCache
mechanism will be better encapsulated, with fewer details publicly visible.
+ (Future work) This will allow the pref names to be stored statically, saving
memory in every process.
The main downside of the new approach is that all of these prefs are in a
single header that is included in quite a few places, so any changes to this
header will cause a fair amount of recompilation.
Another minor downside is that all VarCache prefs are defined and visible from
start-up. For test-only prefs like network.predictor.doing-tests, having them
show in about:config isn't particularly useful.
The patch also moves three network VarCache prefs to the new mechanism as a
basic demonstration. (And note the inconsistencies in the multiple initial
values that were provided for
network.auth.subresource-img-cross-origin-http-auth-allow!) There will be
numerous follow-up bugs to convert the remaining VarCache prefs.
MozReview-Commit-ID: 9ABNpOR16uW
* * *
[mq]: fixup
MozReview-Commit-ID: 6ToT9dQjIAq
All prefs that need to be sent to a new content process are now put into the
shared memory segment, and they are identified by the pref name instead of an
index into a list. The old IPC used at process startup (in XPCOMInitData) is
removed.
Benefits:
- It removes the need for the early prefs list
(dom/ipc/ContentProcesses.{h,cpp}) and the associated checking, which is ugly
and often trips people up (e.g. bug 1432979, bug 1439406).
- Using prefnames instead of indices fixes some fragility (fixing bug 1419432).
- It fixes the problem of early prefs being installed as unlocked default
values even if they are locked and/or have user values.
MozReview-Commit-ID: FRIzHF8Tjd
This patch doesn't change the functionality, it just splits out the code into
separate functions that are easier to read.
MozReview-Commit-ID: Gx05YCxGgve
This patch replaces the large -intPrefs/-boolPrefs/-stringPrefs flags with
a short-lived, anonymous, shared memory segment that is used to pass the early
prefs.
Removing the bloat from the command line is nice, but more important is the
fact that this will let us pass more prefs at content process start-up, which
will allow us to remove the early/late prefs split (bug 1436911).
Although this mechanism is only used for prefs, it's conceivable that it could
be used for other data that must be received very early by children, and for
which the command line isn't ideal.
Notable details:
- Much of the patch deals with the various platform-specific ways of passing
handles/fds to children.
- Linux and Mac: we use a fixed fd (8) in combination with the new
GeckoChildProcessHost::AddFdToRemap() function (which ensures the child
won't close the fd).
- Android: like Linux and Mac, but the handles get passed via "parcels" and
we use the new SetPrefsFd() function instead of the fixed fd.
- Windows: there is no need to duplicate the handle because Windows handles
are system-wide. But we do use the new
GeckoChildProcessHost::AddHandleToShare() function to add it to the list of
inheritable handles. We also ensure that list is processed on all paths
(MOZ_SANDBOX with sandbox, MOZ_SANDBOX without sandbox, non-MOZ_SANDBOX) so
that the handles are marked as inheritable. The handle is passed via the
-prefsHandle flag.
The -prefsLen flag is used on all platforms to indicate the size of the
shared memory segment.
- The patch also moves the serialization/deserialization of the prefs in/out of
the shared memory into libpref, which is a better spot for it. (This means
Preferences::MustSendToContentProcesses() can be removed.)
MozReview-Commit-ID: 8fREEBiYFvc
The meaning of "possibly-changed" is provided by the big comment above
MustSendToContentProcesses.
On a new profile this reduces the number of prefs sent like so:
- Command-line: 222 --> 3
- IPC: 3129 --> 130
On an older profile:
- Command-line: 222 --> 3
- IPC: 3165 --> 180
MozReview-Commit-ID: DcgedhXhZd8
They currently fail to pass on `aKind`, always getting the user value (when
possible). There are three callsites that are affected:
- nsSHistory::Startup, docshell/shistory/nsSHistory.cpp.
- FeatureState::SetDefaultFromPref(), in gfx/config/gfxFeature.cpp.
- gfxPlatform::InitOMTPConfig(), in gfx/thebes/gfxPlatform.cpp.
The patch also adds a gtest that would have failed prior to the fix.
MozReview-Commit-ID: L0U1XQmPUFc
This patch rearranges these functions so that nsPrefBranch::GetPrefType() calls
into Preferences::GetType(), because all other nsPrefBranch methods depend on
Preferences methods.
The patch also removes the `aKind` argument from GetType(), because it has no
effect -- a pref only has one type, regardless of whether it has a default
value, a user value, or both.
MozReview-Commit-ID: J3vxFPaP8S3
The prefs parser has two significant problems.
- It doesn't separate tokenizing from parsing.
- It is implemented as a loop around a big switch on a "current state"
variable.
As a result, it is hard to understand and modify, slower than it could be, and
in obscure cases (involving comments and whitespace) it fails to parse what
should be valid input.
This patch replaces it with a recursive descent parser (albeit one without any
recursion!) that has separate tokenization. The new parser is easier to
understand and modify, more correct, and has better error messages. It doesn't
do error recovery, but that would be much easier to add than in the old parser.
The new parser also runs about 1.9x faster than the existing parser. (As
measured by parsing greprefs.js's contents from memory 1000 times in
succession, omitting the prefs hash table construction. If the table
construction is included, it's about 1.6x faster.)
The new parser is slightly stricter than the old parser in a few ways.
- Disconcertingly, the old parser allowed arbitrary junk between prefs
(including at the start and end of the prefs file) so long as that junk
didn't include any of the following chars: '/', '#', 'u', 's', 'p'. I.e.
lines like these:
!foo@bar&pref("prefname", true);
ticky_pref("prefname", true); // missing 's' at start
User_pref("prefname", true); // should be 'u' at start
would all be treated the same as this:
pref("prefname", true);
The new parser disallows such junk because it isn't necessary and seems like
an unintentional botch by the old parser.
- The old parser allowed character 0x1a (SUB) between tokens and treated it
like '\n'.
The new parser does not allow this character. SUB was used to indicate
end-of-file (*not* end-of-line) in some old operating systems such as MS-DOS,
but this doesn't seem necessary today.
- The old parser tolerated (with a warning) invalid escape sequences within
string literals -- such as "\q" (not a valid escape) and "\x1" and "\u12"
(both of which have insufficient hex digits) -- accepting them literally.
The new parser does not tolerate invalid escape sequences because it doesn't
seem necessary and would complicate things.
- The old parser tolerated character 0x00 (NUL) within string literals; this is
dangerous because C++ code that manipulates string values with embedded NULs
will almost certainly consider those chars as end-of-string markers.
The new parser treats NUL chars as end-of-file, to avoid this danger and
because it facilitates a significant optimization (described within the
code).
- The old parser allowed integer literals to overflow, silently wrapping them.
The new parser treats integer overflow as a parse error. This seems better,
and it caught existing overflows of places.database.lastMaintenance, in
testing/profiles/prefs_general.js (bug 1424030) and
testing/talos/talos/config.py (bug 1434813).
The first of these changes meant that a couple of existing prefs with ";;" at
the end had to be changed (done in the preceding patch).
The minor increase in strictness shouldn't be a problem for default pref files
such as greprefs.js within the application (which we can modify), nor for
app-written prefs files such as prefs.js. It could affect user-written prefs
files such as user.js; the experience above suggests that integer overflow and
";;" are the most likely problems in practice. In my opinion, the risk here is
acceptable.
The new parser also does a better job of tracking line numbers because it (a)
treats "\r\n" sequences as a single end-of-line marker, and (a) pays attention
to end-of-line sequences within string literals.
Finally, the patch adds thorough tests of both valid and invalid syntax.
MozReview-Commit-ID: JD3beOQl4AJ
The prefs parser has two significant problems.
- It doesn't separate tokenizing from parsing.
- It is implemented as a loop around a big switch on a "current state"
variable.
As a result, it is hard to understand and modify, slower than it could be, and
in obscure cases (involving comments and whitespace) it fails to parse what
should be valid input.
This patch replaces it with a recursive descent parser (albeit one without any
recursion!) that has separate tokenization. The new parser is easier to
understand and modify, more correct, and has better error messages. It doesn't
do error recovery, but that would be much easier to add than in the old parser.
The new parser also runs about 1.9x faster than the existing parser. (As
measured by parsing greprefs.js's contents from memory 1000 times in
succession, omitting the prefs hash table construction. If the table
construction is included, it's about 1.6x faster.)
The new parser is slightly stricter than the old parser in a few ways.
- Disconcertingly, the old parser allowed arbitrary junk between prefs
(including at the start and end of the prefs file) so long as that junk
didn't include any of the following chars: '/', '#', 'u', 's', 'p'. I.e.
lines like these:
!foo@bar&pref("prefname", true);
ticky_pref("prefname", true); // missing 's' at start
User_pref("prefname", true); // should be 'u' at start
would all be treated the same as this:
pref("prefname", true);
The new parser disallows such junk because it isn't necessary and seems like
an unintentional botch by the old parser.
- The old parser allowed character 0x1a (SUB) between tokens and treated it
like '\n'.
The new parser does not allow this character. SUB was used to indicate
end-of-file (*not* end-of-line) in some old operating systems such as MS-DOS,
but this doesn't seem necessary today.
- The old parser tolerated (with a warning) invalid escape sequences within
string literals -- such as "\q" (not a valid escape) and "\x1" and "\u12"
(both of which have insufficient hex digits) -- accepting them literally.
The new parser does not tolerate invalid escape sequences because it doesn't
seem necessary and would complicate things.
- The old parser tolerated character 0x00 (NUL) within string literals; this is
dangerous because C++ code that manipulates string values with embedded NULs
will almost certainly consider those chars as end-of-string markers.
The new parser treats NUL chars as end-of-file, to avoid this danger and
because it facilitates a significant optimization (described within the
code).
- The old parser allowed integer literals to overflow, silently wrapping them.
The new parser treats integer overflow as a parse error. This seems better,
and it caught an existing overflow in testing/profiles/prefs_general.js, for
places.database.lastMaintenance (see bug 1424030).
The first of these changes meant that a couple of existing prefs with ";;" at
the end had to be changed (done in the preceding patch).
The minor increase in strictness shouldn't be a problem for default pref files
such as greprefs.js within the application (which we can modify), nor for
app-written prefs files such as prefs.js. It could affect user-written prefs
files such as user.js; the experience above suggests that ";;" is the most
likely problem in practice. In my opinion, the risk here is acceptable.
The new parser also does a better job of tracking line numbers because it (a)
treats "\r\n" sequences as a single end-of-line marker, and (a) pays attention
to end-of-line sequences within string literals.
Finally, the patch adds thorough tests of both valid and invalid syntax.
MozReview-Commit-ID: 8EYWH7KxGG
* * *
[mq]: win-fix
MozReview-Commit-ID: 91Bxjfghqfw
This code is used to detect too-early accesses of prefs in content processes.
The patch makes the following changes.
- New terminology: "early" prefs are those sent via the command line; "late"
prefs are those sent via IPC. Previously the former were "init" prefs and the
latter didn't have a clear name.
- The phase tracking and checking is now almost completely encapsulated within
Preferences.cpp. The only exposure to outside code is via the
AreAllPrefsSetInContentProcess() method, which has a single use.
- The number of states tracked drops from 5 to 3. There's no need to track the
beginning of the pref-setting operations, because we only need to know if
they've finished. (This also avoids the weirdness where we could transition
from END_INIT_PREFS back to BEGIN_INIT_PREFS because of the way -intPrefs,
-boolPrefs and -stringPrefs were parsed separately.)
MozReview-Commit-ID: IVJWiDxdsDV
Preferences::SetPreference() is used when setting prefs in the content process
from dom::Pref values that are passed from the parent process. Currently we
use the high-level Set*InAnyProcess() methods to do this -- basically the same
code path as sets done via the API -- but this has several problems.
- It is subtly broken. If the content process already has a pref of type T with
a default value and then we get a SetPreference() call that tries to change
it to type U, it will erroneously fail. In practice this never(?) happens,
but it shows that things aren't arranged very well.
- SetPreference() also looks up the hashtable twice to get the same pref if
both a default value and a user value are present in the dom::Pref.
- This happens in content processes, while all other pref set operations occur
in the parent process. This is the main reason we have the Set*InAnyProcess()
functions.
In short, the setting of prefs via IPC is quite different to the setting of
prefs via other means -- because it happens in content processes, and it's more
of a clobber that can set both values at once -- and so should be handled
differently.
The solution is to make Preferences::SetPreference() use lower-level operations
to do the update. It now does the hash table lookup/add itself, and then calls
the new Pref::FromDomPref() method. That method then possibly calls the new
PrefValue::FromDomPrefValue() method for both kinds of value, and overwrites
them as necessary. SetValueFromDom() is no longer used and the patch removes it.
MozReview-Commit-ID: 2Rg8VMOc0Cl
It represents a pref, so `Pref` is a better name. Within Preferences.cpp the
patch uses domPref/aDomPref to distinguish it from PrefHashEntry values.
MozReview-Commit-ID: HXTl0GX4BtO
This requires adding an aPriority argument (defaulting to false) to
Preferences::RegisterCallback(). And RegisterVarCacheCallback() is no longer
necessary.
MozReview-Commit-ID: BMDk3HuaQVV
This splits the measurements into several buckets, like so:
> 718,528 B (00.40%) -- preferences
> ├──262,176 B (00.14%) ── hash-table
> ├──181,952 B (00.10%) ── callbacks
> ├──122,880 B (00.07%) ── pref-name-arena
> ├───91,872 B (00.05%) ── root-branches
> ├───38,296 B (00.02%) ── string-values
> ├───21,272 B (00.01%) ── cache-data
> └───────80 B (00.00%) ── misc
The patch also measures some things that were previously overlooked.
- String pref values. (The old code had a comment that incorrectly claimed they
were allocated out of an arena.)
- The PrefCallback objects pointed to by entries in nsPrefBranch::mObservers.
And it makes the code more like typical reporters.
- It removes the "AndOtherStuff" from Preferences' measuring method, and
measuring those global structures in
PreferenceServiceReporter::CollectReports().
- It adds `const` where appropriate.
MozReview-Commit-ID: dyNg7ldQdh
This patch also adds some Set*InAnyProcess() methods, and makes nsPrefBranch a
friend of Preferences so it can call those methods.
And it moves the thin Set*() wrapper functions to Preferences.h, alongside
SetUint().
MozReview-Commit-ID: 88HhmcTFZNc
This will allow other functions to be moved into Preferences and be marked as
`private` in subsequent patches.
The patch also renames SetPrefValue() as SetValueFromDom(), because that's a
clearer name.
MozReview-Commit-ID: CB1xmPSmac6
This is unused for now, but will be necessary for nsPrefBranch::Set*() to call
into Preferences::Set*().
The patch also renames some arguments from aPref to aPrefName, because that's a
better name.
MozReview-Commit-ID: 2OPB7CHOgpw
This is nicer than a bool for tracking the Default vs. User distinction, and it
replaces the Preferences.cpp-only WhichValue type.
MozReview-Commit-ID: 8CrdDN2vBJQ
The various getters and setters are in a confusing order. This patch puts them
in a more sensible order. It also streamlines the comments, which were
generally low-value and in some cases incorrect.
MozReview-Commit-ID: 3ngzZDSt0JI
The notable part of this change is Shutdown(). I've made it just null out
sPreferences, contrary to the old comment, which was strange for a couple of
reasons:
- ~Preferences() used to null out sPreference, which is backwards compared to
how this sort of thing normally works.
- In both the before and after cases, as far as I can tell,
Preferences::Shutdown() is called but ~Preferences() is never called;
something keeps the singleton Preferences instance alive until process
termination.
MozReview-Commit-ID: Ab0ui31rVcI
sRootBranch and sDefaultRootBranch have the same lifetime as sPreferences, so
this patch makes them non-static nsCOMPtr<> members of Preferences.
MozReview-Commit-ID: 1TLhh13ZpBI
It's unnecessarily general, because we only ever use
Preferences::DirtyCallback() as the callback.
And because it's no longer a callback, the patch renames DirtyCallback() as
HandleDirty().
MozReview-Commit-ID: Hl50dcxfVQq
Right now, NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR expects singleton
constructors to return already-addrefed raw pointers, and while it accepts
constructors that return already_AddRefed, most existing don't do so.
Meanwhile, the convention elsewhere is that a raw pointer return value is
owned by the callee, and that the caller needs to addref it if it wants to
keep its own reference to it.
The difference in convention makes it easy to leak (I've definitely caused
more than one shutdown leak this way), so it would be better if we required
the singleton getters to return an explicit already_AddRefed, which would
behave the same for all callers.
This also cleans up several singleton constructors that left a dangling
pointer to their singletons when their initialization methods failed, when
they released their references without clearing their global raw pointers.
MozReview-Commit-ID: 9peyG4pRYcr
It's unnecessarily general, because we only ever use
Preferences::DirtyCallback() as the callback.
And because it's no longer a callback, the patch renames DirtyCallback() as
HandleDirty().
MozReview-Commit-ID: Hl50dcxfVQq
This is detritus from old changes that can be cleaned up now.
The patch removes the declaration of PrefChangedFunc from Preferences.cpp
because it's also in Preferences.h, which is included by Preferences.cpp.
The patch also removes the part of the comment about passing a non-zero result
because it's clearly false -- the callback has no return value.
MozReview-Commit-ID: 72cdauYsRUt
