Bug 784657 - Fix potential problem in nsDIskCacheMap timer handling. r=michal
authorBrian R. Bondy <netzen@gmail.com>
Thu, 23 Aug 2012 22:23:10 -0400
changeset 105307 7d83016f15625673de05422434e577560c16d2d1
parent 105306 281b36542acd08890101493a1bac38af711e42db
child 105308 b9d4600374dcf7c99d4b56d7fa3aa33a8eef709b
push id55
push usershu@rfrn.org
push dateThu, 30 Aug 2012 01:33:09 +0000
reviewersmichal
bugs784657
milestone17.0a1
Bug 784657 - Fix potential problem in nsDIskCacheMap timer handling. r=michal
netwerk/cache/nsDiskCacheDevice.h
netwerk/cache/nsDiskCacheMap.cpp
toolkit/components/telemetry/TelemetryHistograms.h
--- a/netwerk/cache/nsDiskCacheDevice.h
+++ b/netwerk/cache/nsDiskCacheDevice.h
@@ -68,16 +68,17 @@ public:
     uint32_t                getCacheSize();
     uint32_t                getEntryCount();
     
     nsDiskCacheMap *        CacheMap()    { return &mCacheMap; }
     
 private:    
     friend class nsDiskCacheDeviceDeactivateEntryEvent;
     friend class nsEvictDiskCacheEntriesEvent;
+    friend class nsDiskCacheMap;
     /**
      *  Private methods
      */
 
     inline bool IsValidBinding(nsDiskCacheBinding *binding)
     {
         NS_ASSERTION(binding, "  binding == nullptr");
         NS_ASSERTION(binding->mDeactivateEvent == nullptr,
--- a/netwerk/cache/nsDiskCacheMap.cpp
+++ b/netwerk/cache/nsDiskCacheMap.cpp
@@ -2,16 +2,17 @@
 /* vim:set ts=4 sw=4 sts=4 cin et: */
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "nsDiskCacheMap.h"
 #include "nsDiskCacheBinding.h"
 #include "nsDiskCacheEntry.h"
+#include "nsDiskCacheDevice.h"
 #include "nsCacheService.h"
 
 #include "nsCache.h"
 
 #include <string.h>
 #include "nsPrintfCString.h"
 
 #include "nsISerializable.h"
@@ -1212,17 +1213,17 @@ nsDiskCacheMap::NotifyCapacityChange(uin
 nsresult
 nsDiskCacheMap::InitCacheClean(nsIFile *  cacheDirectory,
                                nsDiskCache::CorruptCacheInfo *  corruptInfo)
 {
     // The _CACHE_CLEAN_ file will be used in the future to determine
     // if the cache is clean or not. 
     bool cacheCleanFileExists = false;
     nsCOMPtr<nsIFile> cacheCleanFile;
-    nsresult rv = cacheDirectory->Clone(getter_AddRefs(cacheCleanFile));
+    nsresult rv = cacheDirectory->GetParent(getter_AddRefs(cacheCleanFile));
     if (NS_SUCCEEDED(rv)) {
         rv = cacheCleanFile->AppendNative(
                  NS_LITERAL_CSTRING("_CACHE_CLEAN_"));
         if (NS_SUCCEEDED(rv)) {
             // Check if the file already exists, if it does, we will later read the
             // value and report it to telemetry.
             cacheCleanFile->Exists(&cacheCleanFileExists);
         }
@@ -1322,50 +1323,52 @@ nsDiskCacheMap::InvalidateCache()
 }
 
 nsresult
 nsDiskCacheMap::ResetCacheTimer(int32_t timeout)
 {
     mCleanCacheTimer->Cancel();
     nsresult rv =
       mCleanCacheTimer->InitWithFuncCallback(RevalidateTimerCallback,
-                                             this, timeout,
+                                             nullptr, timeout,
                                              nsITimer::TYPE_ONE_SHOT);
     NS_ENSURE_SUCCESS(rv, rv);
     mLastInvalidateTime = PR_IntervalNow();
 
     return rv;
 }
 
 void
 nsDiskCacheMap::RevalidateTimerCallback(nsITimer *aTimer, void *arg)
 {
-    nsDiskCacheMap *diskCacheMap = reinterpret_cast<nsDiskCacheMap *>(arg);
-    nsresult rv;
-
-    // Intentional braces to scope mutex to only what is needed
-    {
-        nsCacheServiceAutoLock lock(LOCK_TELEM(NSDISKCACHEMAP_REVALIDATION));
-        // If we have less than kLastInvalidateTime since the last timer was
-        // issued then another thread called InvalidateCache.  This won't catch
-        // all cases where we wanted to cancel the timer, but under the lock it
-        // is always OK to revalidate as long as IsCacheInSafeState() returns
-        // true.  We just want to avoid revalidating when we can to reduce IO
-        // and this check will do that.
-        uint32_t delta =
-            PR_IntervalToMilliseconds(PR_IntervalNow() -
-                                      diskCacheMap->mLastInvalidateTime) +
-            kRevalidateCacheTimeoutTolerance;
-        if (delta < kRevalidateCacheTimeout) {
-            diskCacheMap->ResetCacheTimer();
-            return;
-        }
-        rv = diskCacheMap->RevalidateCache();
+    nsCacheServiceAutoLock lock(LOCK_TELEM(NSDISKCACHEMAP_REVALIDATION));
+    if (!nsCacheService::gService->mDiskDevice ||
+        !nsCacheService::gService->mDiskDevice->Initialized()) {
+        return;
     }
 
+    nsDiskCacheMap *diskCacheMap =
+        &nsCacheService::gService->mDiskDevice->mCacheMap;
+
+    // If we have less than kRevalidateCacheTimeout since the last timer was
+    // issued then another thread called InvalidateCache.  This won't catch
+    // all cases where we wanted to cancel the timer, but under the lock it
+    // is always OK to revalidate as long as IsCacheInSafeState() returns
+    // true.  We just want to avoid revalidating when we can to reduce IO
+    // and this check will do that.
+    uint32_t delta =
+        PR_IntervalToMilliseconds(PR_IntervalNow() -
+                                  diskCacheMap->mLastInvalidateTime) +
+        kRevalidateCacheTimeoutTolerance;
+    if (delta < kRevalidateCacheTimeout) {
+        diskCacheMap->ResetCacheTimer();
+        return;
+    }
+
+    nsresult rv = diskCacheMap->RevalidateCache();
     if (NS_FAILED(rv)) {
         diskCacheMap->ResetCacheTimer(kRevalidateCacheErrorTimeout);
     }
 }
 
 bool
 nsDiskCacheMap::IsCacheInSafeState()
 {
--- a/toolkit/components/telemetry/TelemetryHistograms.h
+++ b/toolkit/components/telemetry/TelemetryHistograms.h
@@ -196,17 +196,17 @@ HISTOGRAM(SPDY_SETTINGS_MAX_STREAMS, 1, 
 HISTOGRAM(SPDY_SETTINGS_CWND, 1, 500, 50, EXPONENTIAL,  "SPDY: Settings CWND (packets)")
 HISTOGRAM(SPDY_SETTINGS_RETRANS, 1, 100, 50, EXPONENTIAL,  "SPDY: Retransmission Rate")
 HISTOGRAM(SPDY_SETTINGS_IW, 1, 1000, 50, EXPONENTIAL,  "SPDY: Settings IW (rounded to KB)")
 
 #undef _HTTP_HIST
 #undef HTTP_HISTOGRAMS
 
 HISTOGRAM_ENUMERATED_VALUES(DISK_CACHE_CORRUPT_DETAILS, 50, "Why the HTTP disk cache was corrupted at startup")
-HISTOGRAM_BOOLEAN(DISK_CACHE_REDUCTION_TRIAL, "Stores 1 if the cache would be corrupted with the disk cache corruption plan of Bug 105843")
+HISTOGRAM_BOOLEAN(DISK_CACHE_REDUCTION_TRIAL, "Stores 1 if the cache would be clean with the disk cache corruption plan of Bug 105843")
 HISTOGRAM_ENUMERATED_VALUES(HTTP_CACHE_DISPOSITION_2,         5, "HTTP Cache Hit, Reval, Failed-Reval, Miss")
 HISTOGRAM_ENUMERATED_VALUES(HTTP_DISK_CACHE_DISPOSITION_2,    5, "HTTP Disk Cache Hit, Reval, Failed-Reval, Miss")
 HISTOGRAM_ENUMERATED_VALUES(HTTP_MEMORY_CACHE_DISPOSITION_2,  5, "HTTP Memory Cache Hit, Reval, Failed-Reval, Miss")
 HISTOGRAM_ENUMERATED_VALUES(HTTP_OFFLINE_CACHE_DISPOSITION_2, 5, "HTTP Offline Cache Hit, Reval, Failed-Reval, Miss")
 HISTOGRAM(CACHE_DEVICE_SEARCH_2, 1, 10000, 50, EXPONENTIAL, "Time to search cache (ms)")
 HISTOGRAM(CACHE_MEMORY_SEARCH_2, 1, 10000, 50, EXPONENTIAL, "Time to search memory cache (ms)")
 HISTOGRAM(CACHE_DISK_SEARCH_2, 1, 10000, 50, EXPONENTIAL, "Time to search disk cache (ms)")
 HISTOGRAM(CACHE_OFFLINE_SEARCH_2, 1, 10000, 50, EXPONENTIAL, "Time to search offline cache (ms)")