Closed Bug 1562305 Opened 4 months ago Closed 3 months ago

CacheObserver should use StaticPrefs instead of Add{Type}VarCache

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox70 --- fixed

People

(Reporter: keeler, Assigned: njn)

References

(Blocks 1 open bug)

Details

(Whiteboard: [necko-triaged])

Attachments

(14 files, 3 obsolete files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

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

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

Assignee: nobody → n.nethercote

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.

Type: defect → enhancement

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

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

It's unused.

Depends on D37190

Priority: -- → P2
Whiteboard: [necko-triaged]

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.

Attachment #9076434 - Attachment is obsolete: true
Attachment #9076433 - Attachment is obsolete: true
Attachment #9076422 - Attachment is obsolete: true
Pushed by nnethercote@mozilla.com:
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
Pushed by nnethercote@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.