Specific values for browser.cache.disk.capacity do break the cache
Categories
(Core :: Networking: Cache, defect, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox66 | --- | fixed |
People
(Reporter: sworddragon2, Assigned: michal)
References
Details
(Whiteboard: [necko-triaged], [qa-66b-p2])
Attachments
(1 file, 2 obsolete files)
18.17 KB,
patch
|
michal
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/60.0 Build ID: 20100101 Steps to reproduce: 1. Go to about:config. 2. Set browser.cache.disk.smart_size.enabled to false. 3. Set browser.cache.disk.capacity to some specific values (like 8388608 but not 6291456 or 9000000). 4. Surf on some websites to store cache entries and watch your cache (in my case on Linux on watching ~/.cache/.mozilla/firefox/_profile_name_). Actual results: The cache appears to create some initial entries but then stalls.
Assignee | ||
Updated•6 years ago
|
Updated•6 years ago
|
Assignee | ||
Comment 1•6 years ago
|
||
(In reply to sworddragon2 from comment #0) > 3. Set browser.cache.disk.capacity to some specific values (like 8388608 but > not 6291456 or 9000000). CacheObserver::DiskCacheCapacity() returns capacity in bytes and it uses uint32_t, so values over 0x3FFFFF will overflow. We could: 1) Limit CacheObserver::sDiskCacheCapacity to 0x3FFFFF so it won't overflow 2) Use int64_t/uint64_t. 3) Change getters and setters in CacheObserver to use values in kiB. Honza, what do you prefer? I slightly prefer 3) because internally we store the value in kiB and most callers also use the value in kiB, so we unnecessarily shift the bits left and then immediately right.
Comment 2•6 years ago
|
||
(In reply to Michal Novotny [:michal] from comment #1) > Honza, what do you prefer? I slightly prefer 3) because internally we store > the value in kiB and most callers also use the value in kiB, so we > unnecessarily shift the bits left and then immediately right. Thanks. Makes sense (option 3). Are you going to work on it?
Assignee | ||
Comment 3•6 years ago
|
||
Yes, I'll do it.
Assignee | ||
Comment 4•6 years ago
|
||
Assignee | ||
Comment 5•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=01c53624cdbdf41bd11a87d05e63e089afa27845
Comment 6•6 years ago
|
||
Comment on attachment 9021841 [details] [diff] [review] fix Review of attachment 9021841 [details] [diff] [review]: ----------------------------------------------------------------- thanks for doing this! ::: netwerk/cache2/CacheFileChunk.cpp @@ +900,5 @@ > + if (limit > UINT32_MAX) { > + limit = UINT32_MAX; > + } > + > + int64_t usage = ChunksMemoryUsage(); maybe convert all to kB or do you need B granularity? ::: netwerk/cache2/CacheFileIOManager.cpp @@ +4196,5 @@ > > // Returns default ("smart") size (in KB) of cache, given available disk space > // (also in KB) > static uint32_t > +SmartCacheSize(const int64_t availKB) the line |uint32_t avail10MBs = availKB / (1024*10);| may yell a warning about truncating, or not? just curious why you pass int64 but return int32 (both kb) and if clipping may occur, uncontrolled. ::: netwerk/cache2/CacheObserver.cpp @@ +398,5 @@ > // Otherwise (or when in the custom limit), check limit based on the global > + // limit. It's 1/8 of the respective capacity. > + int64_t derivedLimit = aUsingDisk ? DiskCacheCapacity() > + : MemoryCacheCapacity(); > + derivedLimit <<= 7; // kilobytes to bytes and divided by 8 derivedLimit <<= (10 - 3) ? :D
Assignee | ||
Comment 7•5 years ago
|
||
(In reply to Honza Bambas (:mayhemer) from comment #6)
- int64_t usage = ChunksMemoryUsage();
maybe convert all to kB or do you need B granularity?
CacheFileChunk::CanAllocate() parameter is in bytes (actual buffer size), so a conversion must be done somewhere and this seems to me to be the best place.
// Returns default ("smart") size (in KB) of cache, given available disk space
// (also in KB)
static uint32_t
+SmartCacheSize(const int64_t availKB)the line |uint32_t avail10MBs = availKB / (1024*10);| may yell a warning
about truncating, or not? just curious why you pass int64 but return int32
(both kb) and if clipping may occur, uncontrolled.
The reason why availKB is int64_t is that GetDiskSpaceAvailable() returns int64_t and even after converting to kB it could be larger than uint32_t (it's just 4TB of free space). We don't compute anything from availKB if it's larger than 2510241024, so there is no possibility of overflowing the return value.
Assignee | ||
Comment 8•5 years ago
|
||
(In reply to Honza Bambas (:mayhemer) from comment #6)
- int64_t usage = ChunksMemoryUsage();
maybe convert all to kB or do you need B granularity?
CacheFileChunk::CanAllocate() parameter is in bytes (actual buffer size), so a conversion must be done somewhere and this seems to me to be the best place.
// Returns default ("smart") size (in KB) of cache, given available disk space
// (also in KB)
static uint32_t
+SmartCacheSize(const int64_t availKB)the line |uint32_t avail10MBs = availKB / (1024*10);| may yell a warning
about truncating, or not? just curious why you pass int64 but return int32
(both kb) and if clipping may occur, uncontrolled.
The reason why availKB is int64_t is that GetDiskSpaceAvailable() returns int64_t and even after converting to kB it could be larger than uint32_t (it's just 4TB of free space). We don't compute anything from availKB if it's larger than 2510241024, so there is no possibility of overflowing the return value.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e68d3938a114
Specific values for browser.cache.disk.capacity do break the cache, r=mayhemer
Comment 10•5 years ago
|
||
bugherder |
Updated•5 years ago
|
Description
•