Bug 967941, part 2 - Convert nsZipReaderCache::mZips to use nsRefPtrHashtable. r=aklotz
authorAndrew McCreight <continuation@gmail.com>
Mon, 24 Feb 2014 17:16:11 -0800
changeset 170656 ef74212f2e25614500f731f83ccdf444396d9e3b
parent 170655 36e79221886df990d40b3865eec1a78f632e98c5
child 170657 68c714066b0cacd5602b8ff94a8bd0c44f080aef
push id270
push userpvanderbeken@mozilla.com
push dateThu, 06 Mar 2014 09:24:21 +0000
reviewersaklotz
bugs967941
milestone30.0a1
Bug 967941, part 2 - Convert nsZipReaderCache::mZips to use nsRefPtrHashtable. r=aklotz
modules/libjar/nsJAR.cpp
modules/libjar/nsJAR.h
--- a/modules/libjar/nsJAR.cpp
+++ b/modules/libjar/nsJAR.cpp
@@ -1049,27 +1049,26 @@ nsZipReaderCache::Init(uint32_t cacheSiz
     os->AddObserver(this, "chrome-flush-caches", true);
     os->AddObserver(this, "flush-cache-entry", true);
   }
 // ignore failure of the observer registration.
 
   return NS_OK;
 }
 
-static bool
-DropZipReaderCache(nsHashKey *aKey, void *aData, void* closure)
+static PLDHashOperator
+DropZipReaderCache(const nsACString &aKey, nsJAR* aZip, void*)
 {
-  nsJAR* zip = (nsJAR*)aData;
-  zip->SetZipReaderCache(nullptr);
-  return true;
+  aZip->SetZipReaderCache(nullptr);
+  return PL_DHASH_NEXT;
 }
 
 nsZipReaderCache::~nsZipReaderCache()
 {
-  mZips.Enumerate(DropZipReaderCache, nullptr);
+  mZips.EnumerateRead(DropZipReaderCache, nullptr);
 
 #ifdef ZIP_CACHE_HIT_RATE
   printf("nsZipReaderCache size=%d hits=%d lookups=%d rate=%f%% flushes=%d missed %d\n",
          mCacheSize, mZipCacheHits, mZipCacheLookups,
          (float)mZipCacheHits / mZipCacheLookups,
          mZipCacheFlushes, mZipSyncMisses);
 #endif
 }
@@ -1084,19 +1083,17 @@ nsZipReaderCache::IsCached(nsIFile* zipF
 
   nsAutoCString uri;
   rv = zipFile->GetNativePath(uri);
   if (NS_FAILED(rv))
     return rv;
 
   uri.Insert(NS_LITERAL_CSTRING("file:"), 0);
 
-  nsCStringKey key(uri);
-
-  *aResult = mZips.Exists(&key);
+  *aResult = mZips.Contains(uri);
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsZipReaderCache::GetZip(nsIFile* zipFile, nsIZipReader* *result)
 {
   NS_ENSURE_ARG_POINTER(zipFile);
   nsresult rv;
@@ -1108,42 +1105,36 @@ nsZipReaderCache::GetZip(nsIFile* zipFil
 #endif
 
   nsAutoCString uri;
   rv = zipFile->GetNativePath(uri);
   if (NS_FAILED(rv)) return rv;
 
   uri.Insert(NS_LITERAL_CSTRING("file:"), 0);
 
-  nsCStringKey key(uri);
-  nsJAR* zip = static_cast<nsJAR*>(static_cast<nsIZipReader*>(mZips.Get(&key))); // AddRefs
+  nsJAR* zip;
+  mZips.Get(uri, &zip);
   if (zip) {
 #ifdef ZIP_CACHE_HIT_RATE
     mZipCacheHits++;
 #endif
     zip->ClearReleaseTime();
-  }
-  else {
+  } else {
     zip = new nsJAR();
-    if (zip == nullptr)
-        return NS_ERROR_OUT_OF_MEMORY;
     NS_ADDREF(zip);
     zip->SetZipReaderCache(this);
 
     rv = zip->Open(zipFile);
     if (NS_FAILED(rv)) {
       NS_RELEASE(zip);
       return rv;
     }
 
-#ifdef DEBUG
-    bool collision =
-#endif
-      mZips.Put(&key, static_cast<nsIZipReader*>(zip)); // AddRefs to 2
-    NS_ASSERTION(!collision, "horked");
+    MOZ_ASSERT(!mZips.Contains(uri));
+    mZips.Put(uri, zip);
   }
   *result = zip;
   return rv;
 }
 
 NS_IMETHODIMP
 nsZipReaderCache::GetInnerZip(nsIFile* zipFile, const nsACString &entry,
                               nsIZipReader* *result)
@@ -1161,72 +1152,69 @@ nsZipReaderCache::GetInnerZip(nsIFile* z
   nsAutoCString uri;
   rv = zipFile->GetNativePath(uri);
   if (NS_FAILED(rv)) return rv;
 
   uri.Insert(NS_LITERAL_CSTRING("jar:"), 0);
   uri.AppendLiteral("!/");
   uri.Append(entry);
 
-  nsCStringKey key(uri);
-  nsJAR* zip = static_cast<nsJAR*>(static_cast<nsIZipReader*>(mZips.Get(&key))); // AddRefs
+  nsJAR* zip;
+  mZips.Get(uri, &zip);
   if (zip) {
 #ifdef ZIP_CACHE_HIT_RATE
     mZipCacheHits++;
 #endif
     zip->ClearReleaseTime();
-  }
-  else {
+  } else {
     zip = new nsJAR();
     NS_ADDREF(zip);
     zip->SetZipReaderCache(this);
 
     rv = zip->OpenInner(outerZipReader, entry);
     if (NS_FAILED(rv)) {
       NS_RELEASE(zip);
       return rv;
     }
-#ifdef DEBUG
-    bool collision =
-#endif
-    mZips.Put(&key, static_cast<nsIZipReader*>(zip)); // AddRefs to 2
-    NS_ASSERTION(!collision, "horked");
+
+    MOZ_ASSERT(!mZips.Contains(uri));
+    mZips.Put(uri, zip);
   }
   *result = zip;
   return rv;
 }
 
-static bool
-FindOldestZip(nsHashKey *aKey, void *aData, void* closure)
+static PLDHashOperator
+FindOldestZip(const nsACString &aKey, nsJAR* aZip, void* aClosure)
 {
-  nsJAR** oldestPtr = (nsJAR**)closure;
+  nsJAR** oldestPtr = static_cast<nsJAR**>(aClosure);
   nsJAR* oldest = *oldestPtr;
-  nsJAR* current = (nsJAR*)aData;
+  nsJAR* current = aZip;
   PRIntervalTime currentReleaseTime = current->GetReleaseTime();
   if (currentReleaseTime != PR_INTERVAL_NO_TIMEOUT) {
     if (oldest == nullptr ||
         currentReleaseTime < oldest->GetReleaseTime()) {
       *oldestPtr = current;
     }
   }
-  return true;
+  return PL_DHASH_NEXT;
 }
 
 struct ZipFindData {nsJAR* zip; bool found;};
 
-static bool
-FindZip(nsHashKey *aKey, void *aData, void* closure)
+static PLDHashOperator
+FindZip(const nsACString &aKey, nsJAR* aZip, void* aClosure)
 {
-  ZipFindData* find_data = (ZipFindData*)closure;
+  ZipFindData* find_data = static_cast<ZipFindData*>(aClosure);
 
-  if (find_data->zip == (nsJAR*)aData) {
+  if (find_data->zip == aZip) {
     find_data->found = true;
-    return false;
+    return PL_DHASH_STOP;
   }
-  return true;
+  return PL_DHASH_NEXT;
 }
 
 nsresult
 nsZipReaderCache::ReleaseZip(nsJAR* zip)
 {
   nsresult rv;
   MutexAutoLock lock(mLock);
 
@@ -1241,31 +1229,31 @@ nsZipReaderCache::ReleaseZip(nsJAR* zip)
   // deleting the zip. However, the second thread is still blocked at the
   // start of ReleaseZip, but the 'zip' param now hold a reference to a
   // deleted zip!
   //
   // So, we are going to try safeguarding here by searching our hashtable while
   // locked here for the zip. We return fast if it is not found.
 
   ZipFindData find_data = {zip, false};
-  mZips.Enumerate(FindZip, &find_data);
+  mZips.EnumerateRead(FindZip, &find_data);
   if (!find_data.found) {
 #ifdef ZIP_CACHE_HIT_RATE
     mZipSyncMisses++;
 #endif
     return NS_OK;
   }
 
   zip->SetReleaseTime();
 
   if (mZips.Count() <= mCacheSize)
     return NS_OK;
 
   nsJAR* oldest = nullptr;
-  mZips.Enumerate(FindOldestZip, &oldest);
+  mZips.EnumerateRead(FindOldestZip, &oldest);
 
   // Because of the craziness above it is possible that there is no zip that
   // needs removing.
   if (!oldest)
     return NS_OK;
 
 #ifdef ZIP_CACHE_HIT_RATE
     mZipCacheFlushes++;
@@ -1280,91 +1268,75 @@ nsZipReaderCache::ReleaseZip(nsJAR* zip)
   if (oldest->mOuterZipEntry.IsEmpty()) {
     uri.Insert(NS_LITERAL_CSTRING("file:"), 0);
   } else {
     uri.Insert(NS_LITERAL_CSTRING("jar:"), 0);
     uri.AppendLiteral("!/");
     uri.Append(oldest->mOuterZipEntry);
   }
 
-  nsCStringKey key(uri);
+  // Retrieving and removing the JAR must be done without an extra AddRef
+  // and Release, or we'll trigger nsJAR::Release's magic refcount 1 case
+  // an extra time and trigger a deadlock.
   nsRefPtr<nsJAR> removed;
-  mZips.Remove(&key, (nsISupports **)removed.StartAssignment());
+  mZips.Remove(uri, getter_AddRefs(removed));
   NS_ASSERTION(removed, "botched");
   NS_ASSERTION(oldest == removed, "removed wrong entry");
 
   if (removed)
     removed->SetZipReaderCache(nullptr);
 
   return NS_OK;
 }
 
-static bool
-FindFlushableZip(nsHashKey *aKey, void *aData, void* closure)
+static PLDHashOperator
+FindFlushableZip(const nsACString &aKey, nsRefPtr<nsJAR>& aCurrent, void*)
 {
-  nsHashKey** flushableKeyPtr = (nsHashKey**)closure;
-  nsJAR* current = (nsJAR*)aData;
-
-  if (current->GetReleaseTime() != PR_INTERVAL_NO_TIMEOUT) {
-    *flushableKeyPtr = aKey;
-    current->SetZipReaderCache(nullptr);
-    return false;
+  if (aCurrent->GetReleaseTime() != PR_INTERVAL_NO_TIMEOUT) {
+    aCurrent->SetZipReaderCache(nullptr);
+    return PL_DHASH_REMOVE;
   }
-  return true;
+  return PL_DHASH_NEXT;
 }
 
 NS_IMETHODIMP
 nsZipReaderCache::Observe(nsISupports *aSubject,
                           const char *aTopic,
                           const char16_t *aSomeData)
 {
   if (strcmp(aTopic, "memory-pressure") == 0) {
     MutexAutoLock lock(mLock);
-    while (true) {
-      nsHashKey* flushable = nullptr;
-      mZips.Enumerate(FindFlushableZip, &flushable);
-      if ( ! flushable )
-        break;
-#ifdef DEBUG
-      bool removed =
-#endif
-        mZips.Remove(flushable);   // Releases
-      NS_ASSERTION(removed, "botched");
-
-#ifdef xDEBUG_jband
-      printf("flushed something from the jar cache\n");
-#endif
-    }
+    mZips.Enumerate(FindFlushableZip, nullptr);
   }
   else if (strcmp(aTopic, "chrome-flush-caches") == 0) {
-    mZips.Enumerate(DropZipReaderCache, nullptr);
-    mZips.Reset();
+    mZips.EnumerateRead(DropZipReaderCache, nullptr);
+    mZips.Clear();
   }
   else if (strcmp(aTopic, "flush-cache-entry") == 0) {
     nsCOMPtr<nsIFile> file = do_QueryInterface(aSubject);
     if (!file)
       return NS_OK;
 
     nsAutoCString uri;
     if (NS_FAILED(file->GetNativePath(uri)))
       return NS_OK;
 
     uri.Insert(NS_LITERAL_CSTRING("file:"), 0);
-    nsCStringKey key(uri);
 
     MutexAutoLock lock(mLock);
-    nsJAR* zip = static_cast<nsJAR*>(static_cast<nsIZipReader*>(mZips.Get(&key)));
+    nsJAR* zip;
+    mZips.Get(uri, &zip);
     if (!zip)
       return NS_OK;
 
 #ifdef ZIP_CACHE_HIT_RATE
     mZipCacheFlushes++;
 #endif
 
     zip->SetZipReaderCache(nullptr);
 
-    mZips.Remove(&key);
+    mZips.Remove(uri);
     NS_RELEASE(zip);
   }
   return NS_OK;
 }
 
 ////////////////////////////////////////////////////////////////////////////////
--- a/modules/libjar/nsJAR.h
+++ b/modules/libjar/nsJAR.h
@@ -14,17 +14,20 @@
 #include "prinrval.h"
 
 #include "mozilla/Mutex.h"
 #include "nsIComponentManager.h"
 #include "nsCOMPtr.h"
 #include "nsString.h"
 #include "nsIFile.h"
 #include "nsStringEnumerator.h"
+#include "nsHashKeys.h"
 #include "nsHashtable.h"
+#include "nsRefPtrHashtable.h"
+#include "nsTHashtable.h"
 #include "nsIZipReader.h"
 #include "nsZipArchive.h"
 #include "nsICertificatePrincipal.h"
 #include "nsISignatureVerifier.h"
 #include "nsIObserverService.h"
 #include "nsWeakReference.h"
 #include "nsIObserver.h"
 #include "mozilla/Attributes.h"
@@ -184,20 +187,23 @@ public:
   NS_DECL_NSIZIPREADERCACHE
   NS_DECL_NSIOBSERVER
 
   nsZipReaderCache();
   virtual ~nsZipReaderCache();
 
   nsresult ReleaseZip(nsJAR* reader);
 
+  typedef nsRefPtrHashtable<nsCStringHashKey, nsJAR> ZipsHashtable;
+
 protected:
+
   mozilla::Mutex        mLock;
-  int32_t               mCacheSize;
-  nsSupportsHashtable   mZips;
+  uint32_t              mCacheSize;
+  ZipsHashtable         mZips;
 
 #ifdef ZIP_CACHE_HIT_RATE
   uint32_t              mZipCacheLookups;
   uint32_t              mZipCacheHits;
   uint32_t              mZipCacheFlushes;
   uint32_t              mZipSyncMisses;
 #endif