Closed Bug 1479357 Opened 6 years ago Closed 5 years ago

Specific values for browser.cache.disk.capacity do break the cache

Categories

(Core :: Networking: Cache, defect, P3)

60 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla66
Tracking Status
firefox66 --- fixed

People

(Reporter: sworddragon2, Assigned: michal)

References

Details

(Whiteboard: [necko-triaged], [qa-66b-p2])

Attachments

(1 file, 2 obsolete files)

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.
Blocks: 1479356
Component: Untriaged → Networking: Cache
Product: Firefox → Core
Assignee: nobody → michal.novotny
Priority: -- → P3
Whiteboard: [necko-triaged]
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(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.
Flags: needinfo?(honzab.moz)
(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?
Flags: needinfo?(honzab.moz)
Yes, I'll do it.
Attached patch fix (obsolete) — Splinter Review
Attachment #9021841 - Flags: review?(honzab.moz)
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
Attachment #9021841 - Flags: review?(honzab.moz) → review+

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

Attachment #9021841 - Attachment is obsolete: true
Attachment #9021841 - Attachment is obsolete: true
Attachment #9035188 - Flags: review+
Attachment #9035189 - Flags: review+
Attached patch patch v2 - reformatted (obsolete) — Splinter Review

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

Attachment #9035189 - Attachment is obsolete: true
Keywords: checkin-needed

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

Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla66
Whiteboard: [necko-triaged] → [necko-triaged], [qa-66b-p2]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: