1562305 - CacheObserver should use StaticPrefs instead of Add{Type}VarCache

Status: RESOLVED FIXED

(Core :: Networking: Cache, enhancement, P2)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler]]

See bug 1559108 and bug 1448219. Basically, CacheObserver doesn't protect against calling Add{Type}VarCache multiple times and should be using StaticPrefs anyways.

Comment 1

[Nicholas Nethercote [inactive]]

I will do this, but after I've finished bug 1563139, which won't be until next week.

Comment 2

[Nicholas Nethercote [inactive]]

Actually, I will do this before bug 1563139. In fact, I've already done, just finishing it up now. One thing I found was that most of these prefs need to be atomic because they're accessed off the main thread. The conversion to StaticPrefs makes this obvious because it will assert if a non-atomic static pref is accessed off the main thread.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: Bug 1562305 - Make browser.cache.disk.enable a static pref. r=michal

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: Bug 1562305 - Make browser.cache.memory.enable a static pref. r=michal

Depends on D37177

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: Bug 1562305 - Make browser.cache.disk.metadata_memory_limit a static pref. r=michal

Depends on D37178

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: Bug 1562305 - Make browser.cache.disk.capacity a static pref. r=michal

In SetDiskCacheCapacity(), the old code set sDiskCacheCapacity and then updated
the browser.cache.disk.capacity pref value. But because sDiskCacheCapacity was
a VarCache variable, libpref would immediately overwrite sDiskCacheCapacity
again with the same value. With the new code, this double-setting is avoided.

Depends on D37179

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: Bug 1562305 - Make browser.cache.disk.smart_size.enabled a static pref. r=michal

Depends on D37180

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: Bug 1562305 - Make browser.cache.memory.capacity a static pref. r=michal

Depends on D37181

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: Bug 1562305 - Make browser.cache.disk.free_space_soft_limit a static pref. r=michal

Note that the checking done by StaticPrefs identified that this variable is
accessed off the main thread, and thus should be atomic.

Depends on D37182

Comment 10

[Nicholas Nethercote [inactive]]

Attachment: Bug 1562305 - Make browser.cache.disk.free_space_hard_limit a static pref. r=michal

Depends on D37183

Comment 11

[Nicholas Nethercote [inactive]]

Attachment: Bug 1562305 - Make browser.cache.disk.preload_chunk_count a static pref. r=michal

Depends on D37185

Comment 12

[Nicholas Nethercote [inactive]]

Attachment: Bug 1562305 - Make browser.cache.disk.max_entry_size a static pref. r=michal

Depends on D37186

Comment 13

[Nicholas Nethercote [inactive]]

Attachment: Bug 1562305 - Make browser.cache.memory.max_entry_size a static pref. r=michal

Depends on D37187

Comment 14

[Nicholas Nethercote [inactive]]

Attachment: Bug 1562305 - Make browser.cache.disk.max_{,priority_}chunks_memory_usage static prefs. r=michal

Depends on D37189

Comment 15

[Nicholas Nethercote [inactive]]

Attachment: Bug 1562305 - Remove browser.cache.compression_level pref. r=michal

It's unused.

Depends on D37190

Comment 16

[Nicholas Nethercote [inactive]]

Attachment: Bug 1562305 - Make browser.cache.max_shutdown_io_lag a static pref. r=michal

Depends on D37191

Comment 17

[Nicholas Nethercote [inactive]]

Attachment: Bug 1562305 - Make browser.cache.disk.telemetry_report_ID a static pref. r=michal

Depends on D37192

Comment 18

[Nicholas Nethercote [inactive]]

Attachment: Bug 1562305 - Make browser.cache.disk.amount_written a static pref. r=michal

Depends on D37193

Comment 19

[Nicholas Nethercote [inactive]]

Attachment: Bug 1562305 - Make privacy.sanitize.sanitizeOnShutdown and privacy.clearOnShutdown.cache static prefs. r=michal

Depends on D37194

Comment 20

[Nicholas Nethercote [inactive]]

For the three prefs that involve setting (browser.cache.disk.capacity, browser.cache.disk.telemetry_report_ID, and browser.cache.disk.amount_written) I am going to remove the patches converting them. The way they work -- setting of the cached value off the main thread -- fundamentally doesn't work with StaticPrefs, and I'm not comfortable with the solution we came up with. It's conceivable that the wrong value could be read at some point, particularly if the surrounding code changes in the future. I will deal with those in a different way in a follow-up.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8d2643e05ffc
Make browser.cache.disk.enable a static pref. r=michal
https://hg.mozilla.org/integration/autoland/rev/c8c20e815ff7
Make browser.cache.memory.enable a static pref. r=michal
https://hg.mozilla.org/integration/autoland/rev/78143d8f7a06
Make browser.cache.disk.metadata_memory_limit a static pref. r=michal
https://hg.mozilla.org/integration/autoland/rev/944e841cc149
Make browser.cache.disk.smart_size.enabled a static pref. r=michal
https://hg.mozilla.org/integration/autoland/rev/35cfe718aabe
Make browser.cache.memory.capacity a static pref. r=michal
https://hg.mozilla.org/integration/autoland/rev/2823970d440d
Make browser.cache.disk.free_space_soft_limit a static pref. r=michal
https://hg.mozilla.org/integration/autoland/rev/39c2b2e8a991
Make browser.cache.disk.free_space_hard_limit a static pref. r=michal

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/efe4557cf735
Make browser.cache.disk.preload_chunk_count a static pref. r=michal
https://hg.mozilla.org/integration/autoland/rev/f4b486b1c11a
Make browser.cache.disk.max_entry_size a static pref. r=michal
https://hg.mozilla.org/integration/autoland/rev/f398da2e54cf
Make browser.cache.memory.max_entry_size a static pref. r=michal
https://hg.mozilla.org/integration/autoland/rev/4bea26835a12
Make browser.cache.disk.max_{,priority_}chunks_memory_usage static prefs. r=michal
https://hg.mozilla.org/integration/autoland/rev/f3554de19dd5
Remove browser.cache.compression_level pref. r=michal
https://hg.mozilla.org/integration/autoland/rev/99c8e1e34517
Make browser.cache.max_shutdown_io_lag a static pref. r=michal
https://hg.mozilla.org/integration/autoland/rev/097603980f72
Make privacy.sanitize.sanitizeOnShutdown and privacy.clearOnShutdown.cache static prefs. r=michal

Comment 23

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/8d2643e05ffc
https://hg.mozilla.org/mozilla-central/rev/c8c20e815ff7
https://hg.mozilla.org/mozilla-central/rev/78143d8f7a06
https://hg.mozilla.org/mozilla-central/rev/944e841cc149
https://hg.mozilla.org/mozilla-central/rev/35cfe718aabe
https://hg.mozilla.org/mozilla-central/rev/2823970d440d
https://hg.mozilla.org/mozilla-central/rev/39c2b2e8a991
https://hg.mozilla.org/mozilla-central/rev/efe4557cf735
https://hg.mozilla.org/mozilla-central/rev/f4b486b1c11a
https://hg.mozilla.org/mozilla-central/rev/f398da2e54cf
https://hg.mozilla.org/mozilla-central/rev/4bea26835a12
https://hg.mozilla.org/mozilla-central/rev/f3554de19dd5
https://hg.mozilla.org/mozilla-central/rev/99c8e1e34517
https://hg.mozilla.org/mozilla-central/rev/097603980f72