Closed Bug 1422649 Opened 2 years ago Closed 2 years ago

Remove a bogus exception from the early pref access checking

Categories

(Core :: Preferences: Backend, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: njn, Assigned: njn)

References

Details

Attachments

(2 files)

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.
Attachment #8934027 - Flags: review?(wmccloskey)
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.
Attachment #8934027 - Flags: review?(wmccloskey) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f87d8c908c39aed95ce68bc021e168fc69ebca2
Bug 1422649 - Remove a bogus exception from the early pref access checking. r=billm
https://hg.mozilla.org/mozilla-central/rev/0f87d8c908c3
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
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.
FWIW, here is the patch to fix the crash.
Pushed by dluca@mozilla.com:
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
Thank you for the fix, hiro.
You need to log in before you can comment on or make changes to this bug.