Open Bug 1448219 Opened 2 years ago Updated 29 days ago

[meta] Convert all VarCache prefs to use StaticPrefs

Categories

(Core :: General, task)

task
Not set

Tracking

()

Tracking Status
firefox61 --- affected

People

(Reporter: njn, Unassigned)

References

(Depends on 3 open bugs)

Details

(Keywords: meta)

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.
Depends on: 1448220
Depends on: 1448222
Summary: [meta] Convert all VarCache prefs to the new StaticPrefList form → [meta] Convert all VarCache prefs to the new StaticPrefList mechanism
Summary: [meta] Convert all VarCache prefs to the new StaticPrefList mechanism → [meta] Convert all VarCache prefs to use StaticPrefs
Depends on: 1448225
Depends on: 1449064
Depends on: 1449833
Duplicate of this bug: 1453149
Depends on: 1476820
Depends on: 1485063
Depends on: 1563996

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.

Flags: needinfo?(kwright)
Depends on: 1569004

(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.

Flags: needinfo?(kwright)
Depends on: 1570082
Depends on: 1570084

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
Depends on: 1570212
Type: enhancement → task

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.

Depends on: 1571544
Depends on: 1571621
Depends on: 1572505
Depends on: 1572508
Depends on: 1572529
Depends on: 1572534
Depends on: 1573268
Depends on: 1573364
Depends on: 1573720
Depends on: 1573968
Depends on: 1573992
Depends on: 1591226
You need to log in before you can comment on or make changes to this bug.