1448219 - [meta] Convert all VarCache prefs to use StaticPrefs

Status: RESOLVED FIXED

(Core :: General, task)

Description

[Nicholas Nethercote [inactive]]

Bug 1436655 will introduce a new mechanism for specifying VarCache prefs, by compiling them into the binary. This has a bunch of advantages, described in that bug.

This bug is about tracking the conversion of all the existing VarCache preferences to the new mechanism.

Comment 1

[Nicholas Nethercote [inactive]]

The Servo PR for this bug is at https://github.com/servo/servo/pull/20508.

Comment 3

[Nicholas Nethercote [inactive]]

KrisWright: If you are interested in doing some conversions, a good place to start would be TimoutManager::Initialize() in dom/base/TimeoutManager.cpp. There are 13 VarCache prefs there that look straightforward.

The following steps should work for most (but not all) VarCache prefs.

• Find an existing VarCache pref to convert, by searching for Add*VarCache calls.
• Look for the pref's entry in a prefs data file, usually modules/libpref/init/all.js. (You won't always find one.)
• Add a new entry to modules/libpref/init/StaticPrefList.yaml, based on what you found in the above two steps.
  • Note that sometimes the default value given in those two locations won't agree, in which case you probably should use the all.js one.
• Remove the entry from all.js, if present.
• Remove the Add*VarCache call, along with the mirror variable it used, and any accompanying stuff such as:
  • any constant for the default value;
  • any static bool being used to prevent Add*VarCache from being called more than once.
• Replace uses of the mirror variable with StaticPrefs::foo_bar_baz().
  • If there are no such uses, give it a mirror value of never.
  • Unless the pref is never used at all, in which case remove it!
• Also look for any entries in other prefs files, particularly mobile/android/app/mobile.js or browser/app/profile/firefox.js. If they are equivalent to the StaticPrefList.yaml entry, then remove them. (This is surprisingly common.) Or if they can be merged into the StaticPrefList.yaml entry (e.g. with #ifdef ANDROID), then do that.

It's usually best to do one pref per commit, because it makes reviewing and bisecting any regressions easier. Sometimes you might have two or three tightly-coupled prefs, in which case doing them in a single commit might be better.

You can look at commits in bugs blocking this bug for examples of previous conversions. They all predate the introduction of StaticPrefList.yaml, and therefore add the new static pref to StaticPrefList.h instead, but otherwise they'll match the steps above.

Comment 4

[Kris Wright :KrisWright]

(In reply to Nicholas Nethercote [:njn] from comment #3)

KrisWright: If you are interested in doing some conversions, a good place to start would be TimoutManager::Initialize() in dom/base/TimeoutManager.cpp. There are 13 VarCache prefs there that look straightforward.

Thanks for all of the info! I filed bug 1569004 to get started on this.

Comment 5

[Nicholas Nethercote [inactive]]

The following is a list of prefs for which we call Add*VarCache() when the pref doesn't even exist yet. (This is just during browser startup; there may be additional cases when running different workloads.) In that case, the VarCache variable takes the default value, and if the pref is created later on then the VarCache variable will be updated. It's a pretty gross pattern, and the kind of thing I want to outlaw, so these ones should be considered higher priority cases to convert to static prefs. I will convert privacy.firstparty.isolate.block_post_message myself because it's causing problems for me in bug 1569526.

 BAD browser.cache.disk.telemetry_report_ID
 BAD browser.tabs.remote.force-paint
 BAD content.cors.disable
 BAD content.cors.no_private_data
 BAD content.notify.backoffcount
 BAD content.notify.interval
 BAD content.notify.ontimer
 BAD content.sink.enable_perf_mode
 BAD content.sink.event_probe_rate
 BAD content.sink.initial_perf_time
 BAD content.sink.interactive_deflect_count
 BAD content.sink.interactive_parse_time
 BAD content.sink.interactive_time
 BAD content.sink.perf_deflect_count
 BAD content.sink.perf_parse_time
 BAD dom.allow_XUL_XBL_for_file
 BAD dom.disable_window_print
 BAD dom.ipc.processPrelaunch.delayMs
 BAD dom.ipc.processPriorityManager.backgroundGracePeriodMS
 BAD dom.ipc.processPriorityManager.backgroundPerceivableGracePeriodMS
 BAD dom.ipc.processPriorityManager.enabled
 BAD dom.ipc.processPriorityManager.testMode
 BAD dom.ipc.tabs.disabled
 BAD dom.largeAllocation.testing.allHttpLoads
 BAD dom.quotaManager.temporaryStorage.chunkSize
 BAD dom.quotaManager.temporaryStorage.fixedLimit
 BAD dom.securecontext.whitelist_onions
 BAD dom.testing.sync-content-blocking-notifications
 BAD layout.framevisibility.amountscrollbeforeupdatehorizontal
 BAD layout.framevisibility.amountscrollbeforeupdatevertical
 BAD layout.reflow.synthMouseMove
 BAD media.cloneElementVisually.testing
 BAD mozilla.widget.disable-native-theme
 BAD network.dns.disablePrefetchFromHTTPS
 BAD privacy.firstparty.isolate.block_post_message
 BAD privacy.fuzzyfox.clockgrainus
 BAD privacy.resistFingerprinting.target_video_res
 BAD privacy.resistFingerprinting.video_dropped_ratio
 BAD privacy.resistFingerprinting.video_frames_per_sec
 BAD security.all_resource_uri_content_accessible
 BAD security.turn_off_all_security_so_that_viruses_can_take_over_this_computer
 BAD toolkit.telemetry.ipcBatchTimeout
 BAD ui.mouse.radius.reposition
 BAD vsync.parentProcess.highPriority

Comment 6

[Gabriele Svelto [:gsvelto] (PTO)]

I've encountered some crashes related to the changes here that weren't reported because of a bug in the crash reporting machinery (see bug 1566855). The top frames of the stack look like this:

0 libxul.so!mozilla::StaticPrefs::InitStaticPrefsFromShared() [StaticPrefList_dom.h:b0124f06562982dce60b820d95aad23afd5cec90 : 1261 + 0x11]
1 libxul.so!mozilla::ipc::SharedPreferenceDeserializer::DeserializeFromSharedMemory(char*, char*, char*, char*)
2 libxul.so!mozilla::dom::ContentProcess::Init(int, char**) [ContentProcess.cpp:b0124f06562982dce60b820d95aad23afd5cec90 : 174 + 0x17]
3 libxul.so!XRE_InitChildProcess(int, char**, XREChildData const*) [nsEmbedFunctions.cpp:b0124f06562982dce60b820d95aad23afd5cec90 : 724 + 0xf]

The crash is NULL-pointer dereference. The entry that seems to correspond to the crash is dom.webdriver.enabled.

Comment 7

[Nicholas Nethercote [inactive]]

Kris, are we officially done here?

Comment 8

[Kris Wright :KrisWright]

(In reply to Nicholas Nethercote [:njn] from comment #7)

Kris, are we officially done here?

Yes, it looks like we have removed all of them! All that's left is deleting VarCache itself. Since there are no more prefs left to convert, I am going to go ahead and mark this bug as fixed.

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Bravo!