Bug 695843 part 1 - Add Refcounting on nsZipArchives. r=mwu

intl/hyphenation/src/nsHyphenationManager.cpp

@@ -183,7 +183,7 @@ nsHyphenationManager::LoadPatternListFromOmnijar(Omnijar::Type aType)
     return;
   }
 
-  nsZipArchive *zip = Omnijar::GetReader(aType);
+  nsRefPtr<nsZipArchive> zip = Omnijar::GetReader(aType);
   if (!zip) {
     return;
   }

modules/libjar/nsJAR.cpp

@@ -179,7 +179,7 @@ nsJAR::Open(nsIFile* zipFile)
   
   // The omnijar is special, it is opened early on and closed late
   // this avoids reopening it
-  nsZipArchive *zip = mozilla::Omnijar::GetReader(zipFile);
+  nsRefPtr<nsZipArchive> zip = mozilla::Omnijar::GetReader(zipFile);
   if (zip) {
     mZip = zip;
     return NS_OK;
@@ -231,9 +231,10 @@ nsJAR::Close()
   mGlobalStatus = JAR_MANIFEST_NOT_PARSED;
   mTotalItemsInManifest = 0;
 
-  if ((mZip == mozilla::Omnijar::GetReader(mozilla::Omnijar::GRE)) ||
-      (mZip == mozilla::Omnijar::GetReader(mozilla::Omnijar::APP))) {
-    mZip.forget();
+  nsRefPtr<nsZipArchive> greOmni = mozilla::Omnijar::GetReader(mozilla::Omnijar::GRE);
+  nsRefPtr<nsZipArchive> appOmni = mozilla::Omnijar::GetReader(mozilla::Omnijar::APP);
+
+  if (mZip == greOmni || mZip == appOmni) {
     mZip = new nsZipArchive();
     return NS_OK;
   }
@@ -389,8 +390,10 @@ nsJAR::GetCertificatePrincipal(const nsACString &aFilename, nsIPrincipal** aPrin
 
   // Don't check signatures in the omnijar - this is only
   // interesting for extensions/XPIs.
-  if ((mZip == mozilla::Omnijar::GetReader(mozilla::Omnijar::GRE)) ||
-      (mZip == mozilla::Omnijar::GetReader(mozilla::Omnijar::APP)))
+  nsRefPtr<nsZipArchive> greOmni = mozilla::Omnijar::GetReader(mozilla::Omnijar::GRE);
+  nsRefPtr<nsZipArchive> appOmni = mozilla::Omnijar::GetReader(mozilla::Omnijar::APP);
+
+  if (mZip == greOmni || mZip == appOmni)
     return NS_OK;
 
   //-- Parse the manifest

modules/libjar/nsJAR.h

@@ -133,7 +133,7 @@ class nsJAR : public nsIZipReader
     //-- Private data members
     nsCOMPtr<nsIFile>        mZipFile;        // The zip/jar file on disk
     nsCString                mOuterZipEntry;  // The entry in the zip this zip is reading from
-    nsAutoPtr<nsZipArchive>  mZip;            // The underlying zip archive
+    nsRefPtr<nsZipArchive>   mZip;            // The underlying zip archive
     nsObjectHashtable        mManifestData;   // Stores metadata for each entry
     bool                     mParsedManifest; // True if manifest has been parsed
     nsCOMPtr<nsIPrincipal>   mPrincipal;      // The entity which signed this file

modules/libjar/nsZipArchive.cpp

@@ -792,8 +792,9 @@ PRInt64 nsZipArchive::SizeOfMapping()
 // nsZipArchive constructor and destructor
 //------------------------------------------
 
-nsZipArchive::nsZipArchive() :
-  mBuiltSynthetics(false)
+nsZipArchive::nsZipArchive()
+  : mRefCnt(0)
+  , mBuiltSynthetics(false)
 {
   MOZ_COUNT_CTOR(nsZipArchive);
 
@@ -801,6 +802,9 @@ nsZipArchive::nsZipArchive() :
   memset(mFiles, 0, sizeof(mFiles));
 }
 
+NS_IMPL_THREADSAFE_ADDREF(nsZipArchive)
+NS_IMPL_THREADSAFE_RELEASE(nsZipArchive)
+
 nsZipArchive::~nsZipArchive()
 {
   CloseArchive();

modules/libjar/nsZipArchive.h

@@ -227,8 +227,15 @@ public:
    */
   PRInt64 SizeOfMapping();
 
+  /*
+   * Refcounting
+   */
+  NS_METHOD_(nsrefcnt) AddRef(void);
+  NS_METHOD_(nsrefcnt) Release(void);
+
 private:
   //--- private members ---
+  nsrefcnt      mRefCnt; /* ref count */
 
   nsZipItem*    mFiles[ZIP_TABSIZE];
   PLArenaPool   mArena;
@@ -266,7 +273,7 @@ public:
   nsresult      FindNext(const char** aResult, PRUint16* aNameLen);
 
 private:
-  nsZipArchive* mArchive;
+  nsRefPtr<nsZipArchive> mArchive;
   char*         mPattern;
   nsZipItem*    mItem;
   PRUint16      mSlot;

modules/libpref/src/Preferences.cpp

@@ -1007,7 +1007,7 @@ static nsresult pref_InitInitialObjects()
   const char *entryName;
   PRUint16 entryNameLen;
 
-  nsZipArchive* jarReader = mozilla::Omnijar::GetReader(mozilla::Omnijar::GRE);
+  nsRefPtr<nsZipArchive> jarReader = mozilla::Omnijar::GetReader(mozilla::Omnijar::GRE);
   if (jarReader) {
     // Load jar:$gre/omni.jar!/greprefs.js
     rv = pref_ReadPrefFromJar(jarReader, "greprefs.js");
@@ -1073,7 +1073,7 @@ static nsresult pref_InitInitialObjects()
     NS_WARNING("Error parsing application default preferences.");
 
   // Load jar:$app/omni.jar!/defaults/preferences/*.js
-  nsZipArchive *appJarReader = mozilla::Omnijar::GetReader(mozilla::Omnijar::APP);
+  nsRefPtr<nsZipArchive> appJarReader = mozilla::Omnijar::GetReader(mozilla::Omnijar::APP);
   if (appJarReader) {
     rv = appJarReader->FindInit("defaults/preferences/*.js$", &findPtr);
     NS_ENSURE_SUCCESS(rv, rv);

startupcache/StartupCache.cpp

@@ -249,6 +249,26 @@ StartupCache::LoadArchive()
   return mArchive->OpenArchive(mFile);
 }
 
+namespace {
+
+nsresult
+GetBufferFromZipArchive(nsZipArchive *zip, bool doCRC, const char* id,
+                        char** outbuf, PRUint32* length)
+{
+  if (!zip)
+    return NS_ERROR_NOT_AVAILABLE;
+
+  nsZipItemPtr<char> zipItem(zip, id, doCRC);
+  if (!zipItem)
+    return NS_ERROR_NOT_AVAILABLE;
+
+  *outbuf = zipItem.Forget();
+  *length = zipItem.Length();
+  return NS_OK;
+}
+
+} /* anonymous namespace */
+
 // NOTE: this will not find a new entry until it has been written to disk!
 // Consumer should take ownership of the resulting buffer.
 nsresult
@@ -268,36 +288,19 @@ StartupCache::GetBuffer(const char* id, char** outbuf, PRUint32* length)
     }
   }
 
-  if (mArchive) {
-    nsZipItemPtr<char> zipItem(mArchive, id, true);
-    if (zipItem) {
-      *outbuf = zipItem.Forget();
-      *length = zipItem.Length();
-      return NS_OK;
-    } 
-  }
+  nsresult rv = GetBufferFromZipArchive(mArchive, true, id, outbuf, length);
+  if (NS_SUCCEEDED(rv))
+    return rv;
 
-  if (mozilla::Omnijar::GetReader(mozilla::Omnijar::APP)) {
-    // no need to checksum omnijarred entries
-    nsZipItemPtr<char> zipItem(mozilla::Omnijar::GetReader(mozilla::Omnijar::APP), id);
-    if (zipItem) {
-      *outbuf = zipItem.Forget();
-      *length = zipItem.Length();
-      return NS_OK;
-    } 
-  }
+  nsRefPtr<nsZipArchive> omnijar = mozilla::Omnijar::GetReader(mozilla::Omnijar::APP);
+  // no need to checksum omnijarred entries
+  rv = GetBufferFromZipArchive(omnijar, false, id, outbuf, length);
+  if (NS_SUCCEEDED(rv))
+    return rv;
 
-  if (mozilla::Omnijar::GetReader(mozilla::Omnijar::GRE)) {
-    // no need to checksum omnijarred entries
-    nsZipItemPtr<char> zipItem(mozilla::Omnijar::GetReader(mozilla::Omnijar::GRE), id);
-    if (zipItem) {
-      *outbuf = zipItem.Forget();
-      *length = zipItem.Length();
-      return NS_OK;
-    } 
-  }
-
-  return NS_ERROR_NOT_AVAILABLE;
+  omnijar = mozilla::Omnijar::GetReader(mozilla::Omnijar::GRE);
+  // no need to checksum omnijarred entries
+  return GetBufferFromZipArchive(omnijar, false, id, outbuf, length);
 }
 
 // Makes a copy of the buffer, client retains ownership of inbuf.

startupcache/StartupCache.h

@@ -167,7 +167,7 @@ private:
   static void ThreadedWrite(void *aClosure);
 
   nsClassHashtable<nsCStringHashKey, CacheEntry> mTable;
-  nsAutoPtr<nsZipArchive> mArchive;
+  nsRefPtr<nsZipArchive> mArchive;
   nsCOMPtr<nsILocalFile> mFile;
   
   nsCOMPtr<nsIObserverService> mObserverService;

xpcom/build/Omnijar.cpp

@@ -62,7 +62,7 @@ Omnijar::CleanUpOne(Type aType)
 {
     if (sReader[aType]) {
         sReader[aType]->CloseArchive();
-        delete sReader[aType];
+        NS_IF_RELEASE(sReader[aType]);
     }
     sReader[aType] = nsnull;
     NS_IF_RELEASE(sPath[aType]);
@@ -106,17 +106,14 @@ Omnijar::InitOne(nsIFile *aPath, Type aType)
         return;
     }
 
-    nsZipArchive* zipReader = new nsZipArchive();
-    if (!zipReader)
-        return;
-
+    nsRefPtr<nsZipArchive> zipReader = new nsZipArchive();
     if (NS_FAILED(zipReader->OpenArchive(file))) {
-        delete zipReader;
         return;
     }
 
     CleanUpOne(aType);
     sReader[aType] = zipReader;
+    NS_IF_ADDREF(sReader[aType]);
     sPath[aType] = file;
     NS_IF_ADDREF(sPath[aType]);
 }
@@ -137,7 +134,7 @@ Omnijar::CleanUp()
     sInitialized = false;
 }
 
-nsZipArchive *
+already_AddRefed<nsZipArchive>
 Omnijar::GetReader(nsIFile *aPath)
 {
     NS_ABORT_IF_FALSE(IsInitialized(), "Omnijar not initialized");
@@ -148,12 +145,12 @@ Omnijar::GetReader(nsIFile *aPath)
     if (sPath[GRE]) {
         rv = sPath[GRE]->Equals(aPath, &equals);
         if (NS_SUCCEEDED(rv) && equals)
-            return sReader[GRE];
+            return GetReader(GRE);
     }
     if (sPath[APP]) {
         rv = sPath[APP]->Equals(aPath, &equals);
         if (NS_SUCCEEDED(rv) && equals)
-            return sReader[APP];
+            return GetReader(APP);
     }
     return nsnull;
 }

xpcom/build/Omnijar.h

@@ -44,8 +44,8 @@
 #include "nsCOMPtr.h"
 #include "nsString.h"
 #include "nsIFile.h"
+#include "nsZipArchive.h"
 
-class nsZipArchive;
 class nsIURI;
 
 namespace mozilla {
@@ -109,8 +109,8 @@ static inline already_AddRefed<nsIFile>
 GetPath(Type aType)
 {
     NS_ABORT_IF_FALSE(IsInitialized(), "Omnijar not initialized");
-    NS_IF_ADDREF(sPath[aType]);
-    return sPath[aType];
+    nsCOMPtr<nsIFile> path = sPath[aType];
+    return path.forget();
 }
 
 /**
@@ -128,18 +128,19 @@ HasOmnijar(Type aType)
  * Returns a nsZipArchive pointer for the omni.jar file for GRE or
  * APP. Returns nsnull in the same cases GetPath() would.
  */
-static inline nsZipArchive *
+static inline already_AddRefed<nsZipArchive>
 GetReader(Type aType)
 {
     NS_ABORT_IF_FALSE(IsInitialized(), "Omnijar not initialized");
-    return sReader[aType];
+    nsRefPtr<nsZipArchive> reader = sReader[aType];
+    return reader.forget();
 }
 
 /**
  * Returns a nsZipArchive pointer for the given path IAOI the given
  * path is the omni.jar for either GRE or APP.
  */
-static nsZipArchive *GetReader(nsIFile *aPath);
+static already_AddRefed<nsZipArchive> GetReader(nsIFile *aPath);
 
 /**
  * Returns the URI string corresponding to the omni.jar or directory