Bug 1479357 - Specific values for browser.cache.disk.capacity do break the cache, r=mayhemer

This patch changes all size limits in CacheObserver to kilobytes. The same unit is used at most places when checking these limits. This avoids uint32_t overflow when converting to bytes and back.

netwerk/cache2/CacheFileChunk.cpp

@@ -792,12 +792,17 @@ bool CacheFileChunk::CanAllocate(uint32_t aSize) const {
 
   LOG(("CacheFileChunk::CanAllocate() [this=%p, size=%u]", this, aSize));
 
-  uint32_t limit = CacheObserver::MaxDiskChunksMemoryUsage(mIsPriority);
+  int64_t limit = CacheObserver::MaxDiskChunksMemoryUsage(mIsPriority);
   if (limit == 0) {
     return true;
   }
 
-  uint32_t usage = ChunksMemoryUsage();
+  limit <<= 10;
+  if (limit > UINT32_MAX) {
+    limit = UINT32_MAX;
+  }
+
+  int64_t usage = ChunksMemoryUsage();
   if (usage + aSize > limit) {
     LOG(("CacheFileChunk::CanAllocate() - Returning false. [this=%p]", this));
     return false;

netwerk/cache2/CacheFileIOManager.cpp

@@ -2010,11 +2010,12 @@ nsresult CacheFileIOManager::WriteInternal(CacheFileHandle *aHandle,
            "failed! [rv=0x%08" PRIx32 "]",
            static_cast<uint32_t>(rv)));
     } else {
+      freeSpace >>= 10;  // bytes to kilobytes
       uint32_t limit = CacheObserver::DiskFreeSpaceHardLimit();
       if (freeSpace - aOffset - aCount + aHandle->mFileSize < limit) {
         LOG(
             ("CacheFileIOManager::WriteInternal() - Low free space, refusing "
-             "to write! [freeSpace=%" PRId64 ", limit=%u]",
+             "to write! [freeSpace=%" PRId64 "kB, limit=%ukB]",
              freeSpace, limit));
         return NS_ERROR_FILE_DISK_FULL;
       }
@@ -2547,11 +2548,12 @@ nsresult CacheFileIOManager::TruncateSeekSetEOFInternal(
            "GetDiskSpaceAvailable() failed! [rv=0x%08" PRIx32 "]",
            static_cast<uint32_t>(rv)));
     } else {
+      freeSpace >>= 10;  // bytes to kilobytes
       uint32_t limit = CacheObserver::DiskFreeSpaceHardLimit();
       if (freeSpace - aEOFPos + aHandle->mFileSize < limit) {
         LOG(
             ("CacheFileIOManager::TruncateSeekSetEOFInternal() - Low free space"
-             ", refusing to write! [freeSpace=%" PRId64 ", limit=%u]",
+             ", refusing to write! [freeSpace=%" PRId64 "kB, limit=%ukB]",
              freeSpace, limit));
         return NS_ERROR_FILE_DISK_FULL;
       }
@@ -2727,6 +2729,7 @@ nsresult CacheFileIOManager::EvictIfOverLimitInternal() {
          "GetDiskSpaceAvailable() failed! [rv=0x%08" PRIx32 "]",
          static_cast<uint32_t>(rv)));
   } else {
+    freeSpace >>= 10;  // bytes to kilobytes
     UpdateSmartCacheSize(freeSpace);
   }
 
@@ -2734,7 +2737,7 @@ nsresult CacheFileIOManager::EvictIfOverLimitInternal() {
   rv = CacheIndex::GetCacheSize(&cacheUsage);
   NS_ENSURE_SUCCESS(rv, rv);
 
-  uint32_t cacheLimit = CacheObserver::DiskCacheCapacity() >> 10;
+  uint32_t cacheLimit = CacheObserver::DiskCacheCapacity();
   uint32_t freeSpaceLimit = CacheObserver::DiskFreeSpaceSoftLimit();
 
   if (cacheUsage <= cacheLimit &&
@@ -2742,14 +2745,14 @@ nsresult CacheFileIOManager::EvictIfOverLimitInternal() {
     LOG(
         ("CacheFileIOManager::EvictIfOverLimitInternal() - Cache size and free "
          "space in limits. [cacheSize=%ukB, cacheSizeLimit=%ukB, "
-         "freeSpace=%" PRId64 ", freeSpaceLimit=%u]",
+         "freeSpace=%" PRId64 "kB, freeSpaceLimit=%ukB]",
          cacheUsage, cacheLimit, freeSpace, freeSpaceLimit));
     return NS_OK;
   }
 
   LOG(
       ("CacheFileIOManager::EvictIfOverLimitInternal() - Cache size exceeded "
-       "limit. Starting overlimit eviction. [cacheSize=%u, limit=%u]",
+       "limit. Starting overlimit eviction. [cacheSize=%ukB, limit=%ukB]",
        cacheUsage, cacheLimit));
 
   nsCOMPtr<nsIRunnable> ev;
@@ -2791,6 +2794,7 @@ nsresult CacheFileIOManager::OverLimitEvictionInternal() {
            "GetDiskSpaceAvailable() failed! [rv=0x%08" PRIx32 "]",
            static_cast<uint32_t>(rv)));
     } else {
+      freeSpace >>= 10;  // bytes to kilobytes
       UpdateSmartCacheSize(freeSpace);
     }
 
@@ -2798,13 +2802,13 @@ nsresult CacheFileIOManager::OverLimitEvictionInternal() {
     rv = CacheIndex::GetCacheSize(&cacheUsage);
     NS_ENSURE_SUCCESS(rv, rv);
 
-    uint32_t cacheLimit = CacheObserver::DiskCacheCapacity() >> 10;
+    uint32_t cacheLimit = CacheObserver::DiskCacheCapacity();
     uint32_t freeSpaceLimit = CacheObserver::DiskFreeSpaceSoftLimit();
 
     if (cacheUsage > cacheLimit) {
       LOG(
           ("CacheFileIOManager::OverLimitEvictionInternal() - Cache size over "
-           "limit. [cacheSize=%u, limit=%u]",
+           "limit. [cacheSize=%ukB, limit=%ukB]",
            cacheUsage, cacheLimit));
 
       // We allow cache size to go over the specified limit. Eviction should
@@ -2824,13 +2828,13 @@ nsresult CacheFileIOManager::OverLimitEvictionInternal() {
     } else if (freeSpace != 1 && freeSpace < freeSpaceLimit) {
       LOG(
           ("CacheFileIOManager::OverLimitEvictionInternal() - Free space under "
-           "limit. [freeSpace=%" PRId64 ", freeSpaceLimit=%u]",
+           "limit. [freeSpace=%" PRId64 "kB, freeSpaceLimit=%ukB]",
            freeSpace, freeSpaceLimit));
     } else {
       LOG(
           ("CacheFileIOManager::OverLimitEvictionInternal() - Cache size and "
            "free space in limits. [cacheSize=%ukB, cacheSizeLimit=%ukB, "
-           "freeSpace=%" PRId64 ", freeSpaceLimit=%u]",
+           "freeSpace=%" PRId64 "kB, freeSpaceLimit=%ukB]",
            cacheUsage, cacheLimit, freeSpace, freeSpaceLimit));
 
       mCacheSizeOnHardLimit = false;
@@ -4064,7 +4068,7 @@ void CacheFileIOManager::SyncRemoveAllCacheFiles() {
 
 // Returns default ("smart") size (in KB) of cache, given available disk space
 // (also in KB)
-static uint32_t SmartCacheSize(const uint32_t availKB) {
+static uint32_t SmartCacheSize(const int64_t availKB) {
   uint32_t maxSize;
 
   if (CacheObserver::ClearCacheOnShutdown()) {
@@ -4145,15 +4149,14 @@ nsresult CacheFileIOManager::UpdateSmartCacheSize(int64_t aFreeSpace) {
 
   mLastSmartSizeTime = TimeStamp::NowLoRes();
 
-  uint32_t smartSize =
+  uint32_t smartSize = SmartCacheSize(aFreeSpace + cacheUsage);
-      SmartCacheSize(static_cast<uint32_t>(aFreeSpace / 1024) + cacheUsage);
 
-  if (smartSize == (CacheObserver::DiskCacheCapacity() >> 10)) {
+  if (smartSize == CacheObserver::DiskCacheCapacity()) {
     // Smart size has not changed.
     return NS_OK;
   }
 
-  CacheObserver::SetDiskCacheCapacity(smartSize << 10);
+  CacheObserver::SetDiskCacheCapacity(smartSize);
 
   return NS_OK;
 }

netwerk/cache2/CacheObserver.cpp

@@ -37,7 +37,7 @@ uint32_t CacheObserver::sMetadataMemoryLimit = kDefaultMetadataMemoryLimit;
 
 static int32_t const kDefaultMemoryCacheCapacity = -1;  // autodetect
 int32_t CacheObserver::sMemoryCacheCapacity = kDefaultMemoryCacheCapacity;
-// Cache of the calculated memory capacity based on the system memory size
+// Cache of the calculated memory capacity based on the system memory size in KB
 int32_t CacheObserver::sAutoMemoryCacheCapacity = -1;
 
 static uint32_t const kDefaultDiskCacheCapacity = 250 * 1024;  // 250 MB
@@ -211,7 +211,7 @@ void CacheObserver::AttachToPreferences() {
 
 // static
 uint32_t CacheObserver::MemoryCacheCapacity() {
-  if (sMemoryCacheCapacity >= 0) return sMemoryCacheCapacity << 10;
+  if (sMemoryCacheCapacity >= 0) return sMemoryCacheCapacity;
 
   if (sAutoMemoryCacheCapacity != -1) return sAutoMemoryCacheCapacity;
 
@@ -234,16 +234,16 @@ uint32_t CacheObserver::MemoryCacheCapacity() {
   if (x > 0) {
     capacity = (int32_t)(x * x / 3.0 + x + 2.0 / 3 + 0.1);  // 0.1 for rounding
     if (capacity > 32) capacity = 32;
-    capacity <<= 20;
+    capacity <<= 10;
   }
 
-  // Result is in bytes.
+  // Result is in kilobytes.
   return sAutoMemoryCacheCapacity = capacity;
 }
 
 // static
 void CacheObserver::SetDiskCacheCapacity(uint32_t aCapacity) {
-  sDiskCacheCapacity = aCapacity >> 10;
+  sDiskCacheCapacity = aCapacity;
 
   if (!sSelf) {
     return;
@@ -383,10 +383,10 @@ bool CacheObserver::EntryIsTooBig(int64_t aSize, bool aUsingDisk) {
   if (preferredLimit != -1 && aSize > preferredLimit) return true;
 
   // Otherwise (or when in the custom limit), check limit based on the global
-  // limit.  It's 1/8 (>> 3) of the respective capacity.
+  // limit. It's 1/8 of the respective capacity.
   int64_t derivedLimit =
-      aUsingDisk ? (static_cast<int64_t>(DiskCacheCapacity() >> 3))
+      aUsingDisk ? DiskCacheCapacity() : MemoryCacheCapacity();
-                 : (static_cast<int64_t>(MemoryCacheCapacity() >> 3));
+  derivedLimit <<= (10 - 3);
 
   if (aSize > derivedLimit) return true;
 

netwerk/cache2/CacheObserver.h

@@ -27,38 +27,39 @@ class CacheObserver : public nsIObserver, public nsSupportsWeakReference {
   // Access to preferences
   static bool UseDiskCache() { return sUseDiskCache; }
   static bool UseMemoryCache() { return sUseMemoryCache; }
-  static uint32_t MetadataMemoryLimit()  // result in bytes.
+  static uint32_t MetadataMemoryLimit()  // result in kilobytes.
   {
-    return sMetadataMemoryLimit << 10;
+    return sMetadataMemoryLimit;
   }
-  static uint32_t MemoryCacheCapacity();  // result in bytes.
+  static uint32_t MemoryCacheCapacity();  // result in kilobytes.
-  static uint32_t DiskCacheCapacity()     // result in bytes.
+  static uint32_t DiskCacheCapacity()     // result in kilobytes.
   {
-    return sDiskCacheCapacity << 10;
+    return sDiskCacheCapacity;
   }
-  static void SetDiskCacheCapacity(uint32_t);  // parameter in bytes.
+  static void SetDiskCacheCapacity(uint32_t);  // parameter in kilobytes.
-  static uint32_t DiskFreeSpaceSoftLimit()     // result in bytes.
+  static uint32_t DiskFreeSpaceSoftLimit()     // result in kilobytes.
   {
-    return sDiskFreeSpaceSoftLimit << 10;
+    return sDiskFreeSpaceSoftLimit;
   }
-  static uint32_t DiskFreeSpaceHardLimit()  // result in bytes.
+  static uint32_t DiskFreeSpaceHardLimit()  // result in kilobytes.
   {
-    return sDiskFreeSpaceHardLimit << 10;
+    return sDiskFreeSpaceHardLimit;
   }
   static bool SmartCacheSizeEnabled() { return sSmartCacheSizeEnabled; }
   static uint32_t PreloadChunkCount() { return sPreloadChunkCount; }
-  static uint32_t MaxMemoryEntrySize()  // result in bytes.
+  static uint32_t MaxMemoryEntrySize()  // result in kilobytes.
   {
-    return sMaxMemoryEntrySize << 10;
+    return sMaxMemoryEntrySize;
   }
-  static uint32_t MaxDiskEntrySize()  // result in bytes.
+  static uint32_t MaxDiskEntrySize()  // result in kilobytes.
   {
-    return sMaxDiskEntrySize << 10;
+    return sMaxDiskEntrySize;
   }
-  static uint32_t MaxDiskChunksMemoryUsage(bool aPriority)  // result in bytes.
+  static uint32_t MaxDiskChunksMemoryUsage(
+      bool aPriority)  // result in kilobytes.
   {
-    return aPriority ? sMaxDiskPriorityChunksMemoryUsage << 10
+    return aPriority ? sMaxDiskPriorityChunksMemoryUsage
-                     : sMaxDiskChunksMemoryUsage << 10;
+                     : sMaxDiskChunksMemoryUsage;
   }
   static uint32_t CompressionLevel() { return sCompressionLevel; }
   static uint32_t HalfLifeSeconds() { return sHalfLifeHours * 60.0F * 60.0F; }

netwerk/cache2/CacheStorageService.cpp

@@ -83,15 +83,27 @@ CacheStorageService::MemoryPool::~MemoryPool() {
 }
 
 uint32_t CacheStorageService::MemoryPool::Limit() const {
+  uint32_t limit = 0;
+
   switch (mType) {
     case DISK:
-      return CacheObserver::MetadataMemoryLimit();
+      limit = CacheObserver::MetadataMemoryLimit();
+      break;
     case MEMORY:
-      return CacheObserver::MemoryCacheCapacity();
+      limit = CacheObserver::MemoryCacheCapacity();
+      break;
+    default:
+      MOZ_CRASH("Bad pool type");
   }
 
-  MOZ_CRASH("Bad pool type");
+  static const uint32_t kMaxLimit = 0x3FFFFF;
-  return 0;
+  if (limit > kMaxLimit) {
+    LOG(("  a memory limit (%u) is unexpectedly high, clipping to %u", limit,
+         kMaxLimit));
+    limit = kMaxLimit;
+  }
+
+  return limit << 10;
 }
 
 NS_IMPL_ISUPPORTS(CacheStorageService, nsICacheStorageService,
@@ -258,9 +270,11 @@ class WalkMemoryCacheRunnable : public WalkCacheRunnable {
       if (mNotifyStorage) {
         LOG(("  storage"));
 
+        uint64_t capacity = CacheObserver::MemoryCacheCapacity();
+        capacity <<= 10;  // kilobytes to bytes
+
         // Second, notify overall storage info
-        mCallback->OnCacheStorageInfo(mEntryArray.Length(), mSize,
+        mCallback->OnCacheStorageInfo(mEntryArray.Length(), mSize, capacity,
-                                      CacheObserver::MemoryCacheCapacity(),
                                       nullptr);
         if (!mVisitEntries) return NS_OK;  // done
 
@@ -458,8 +472,9 @@ class WalkDiskCacheRunnable : public WalkCacheRunnable {
       if (mNotifyStorage) {
         nsCOMPtr<nsIFile> dir;
         CacheFileIOManager::GetCacheDirectory(getter_AddRefs(dir));
-        mCallback->OnCacheStorageInfo(mCount, mSize,
+        uint64_t capacity = CacheObserver::DiskCacheCapacity();
-                                      CacheObserver::DiskCacheCapacity(), dir);
+        capacity <<= 10;  // kilobytes to bytes
+        mCallback->OnCacheStorageInfo(mCount, mSize, capacity, dir);
         mNotifyStorage = false;
       } else {
         mCallback->OnCacheEntryVisitCompleted();