1422649 - Remove a bogus exception from the early pref access checking

Status: RESOLVED FIXED

(Core :: Preferences: Backend, enhancement)

Description

[Nicholas Nethercote [inactive]]

New content processes get prefs in three ways.

- They read them from greprefs.js, prefs.js and other such files.

- They get sent "early prefs" from the parent process via the command line
  (-intPrefs/-boolPrefs/-stringPrefs).

- They get sent "late prefs" from the parent process via IPC message.

(The latter two are necessary for communicating prefs that have been added or
modified in the parent process since the file reading occurred at startup.)

We have some machinery that detects if a late pref is accessed before the late
prefs are set, which is good. But it has a big exception in it; late pref
accesses that occur early via Add*VarCache() and RegisterCallbackAndCall() are
allowed.

This exception was added in bug 1341414. The description of that bug says "We
should change AddBoolVarCache so that it doesn't look at the pref in the
content process until prefs have been received from the parent." Unfortunately,
the patch in that bug added the exception to the checking without changing
Add*VarCache() in the suggested way!

This means it's possible for late prefs to be read early via VarCaches (or
RegisterCallbackAndCall()) when their values are incorrect, which is bad.

Changing Add*VarCache() to delay the reading as bug 1341414 originally
suggested seems difficult. A simpler fix is to just remove the exception in the
checking and extend the early prefs list as necessary. This patch does that,
lengthening the early prefs list from ~210 to ~300. Fortunately, most (all?) of
the added prefs are ints or bools rather than strings, so it doesn't increase
the size of the command line arguments for content processes by too much.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Remove a bogus exception from the early pref access checking

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8934027 [details] [diff] [review]
Remove a bogus exception from the early pref access checking

Review of attachment 8934027 [details] [diff] [review]:
-----------------------------------------------------------------

Just FYI, the patch that we discussed that would make this all a lot cleaner (but that fails on try) is here:
https://hg.mozilla.org/try/rev/ed528dd18f5505f08eee120b583275f61283a1f6

It probably doesn't apply anymore, but hopefully the idea of the patch should be clear.

Comment 3

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0f87d8c908c39aed95ce68bc021e168fc69ebca2
Bug 1422649 - Remove a bogus exception from the early pref access checking. r=billm

Comment 4

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/0f87d8c908c3

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Merging this change caused a crash on m-c.

 Hit MOZ_CRASH(accessing non-early pref layout.css.emulate-moz-box-with-flex before late prefs are set) at /home/ikezoe/central/modules/libpref/Preferences.cpp:789

Unfortunately this change landed in inbound and bug 1398963 which introduced layout.css.emulate-moz-box-with-flex landed in autoland.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Add layout.css.emulate-moz-box-with-flex

FWIW, here is the patch to fix the crash.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/f1329009bf0d
Add layout.css.emulate-moz-box-with-flex in gEarlyPrefs. r=bustage-fix a=bustage-fix on a CLOSED TREE

Comment 8

[Nicholas Nethercote [inactive]]

Thank you for the fix, hiro.