Bug 695843 part 1 - Add Refcounting on nsZipArchives. r=mwu
authorMike Hommey <mh+mozilla@glandium.org>
Thu, 08 Dec 2011 11:03:36 +0100
changeset 83042 a31949628a7e971ecef6d25aca4bc633f3d4cfd4
parent 83041 ccc5f03e6dd2c1f70fa8f643dc8af53bc8692253
child 83043 8e345e9d93e9b2cd2d3cce19f76e33ca38dbcff8
push id628
push userclegnitto@mozilla.com
push dateWed, 21 Dec 2011 14:41:57 +0000
treeherdermozilla-aurora@24a61ad789e8 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmwu
bugs695843
milestone11.0a1
Bug 695843 part 1 - Add Refcounting on nsZipArchives. r=mwu
intl/hyphenation/src/nsHyphenationManager.cpp
modules/libjar/nsJAR.cpp
modules/libjar/nsJAR.h
modules/libjar/nsZipArchive.cpp
modules/libjar/nsZipArchive.h
modules/libpref/src/Preferences.cpp
startupcache/StartupCache.cpp
startupcache/StartupCache.h
xpcom/build/Omnijar.cpp
xpcom/build/Omnijar.h
--- a/intl/hyphenation/src/nsHyphenationManager.cpp
+++ b/intl/hyphenation/src/nsHyphenationManager.cpp
@@ -178,17 +178,17 @@ void
 nsHyphenationManager::LoadPatternListFromOmnijar(Omnijar::Type aType)
 {
   nsCString base;
   nsresult rv = Omnijar::GetURIString(aType, base);
   if (NS_FAILED(rv)) {
     return;
   }
 
-  nsZipArchive *zip = Omnijar::GetReader(aType);
+  nsRefPtr<nsZipArchive> zip = Omnijar::GetReader(aType);
   if (!zip) {
     return;
   }
 
   nsZipFind *find;
   zip->FindInit("hyphenation/hyph_*.dic", &find);
   if (!find) {
     return;
--- a/modules/libjar/nsJAR.cpp
+++ b/modules/libjar/nsJAR.cpp
@@ -174,17 +174,17 @@ nsJAR::Open(nsIFile* zipFile)
   if (mOpened) return NS_ERROR_FAILURE; // Already open!
 
   mZipFile = zipFile;
   mOuterZipEntry.Truncate();
   mOpened = true;
   
   // 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;
   }
   return mZip->OpenArchive(zipFile);
 }
 
 NS_IMETHODIMP
@@ -226,19 +226,20 @@ NS_IMETHODIMP
 nsJAR::Close()
 {
   mOpened = false;
   mParsedManifest = false;
   mManifestData.Reset();
   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;
   }
   return mZip->CloseArchive();
 }
 
 NS_IMETHODIMP
 nsJAR::Test(const nsACString &aEntryName)
@@ -384,18 +385,20 @@ nsJAR::GetCertificatePrincipal(const nsA
 {
   //-- Parameter check
   if (!aPrincipal)
     return NS_ERROR_NULL_POINTER;
   *aPrincipal = nsnull;
 
   // 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
   nsresult rv = ParseManifest();
   if (NS_FAILED(rv)) return rv;
   if (mGlobalStatus == JAR_NO_MANIFEST)
     return NS_OK;
 
--- a/modules/libjar/nsJAR.h
+++ b/modules/libjar/nsJAR.h
@@ -128,17 +128,17 @@ class nsJAR : public nsIZipReader
     void SetZipReaderCache(nsZipReaderCache* cache) {
       mCache = cache;
     }
 
   protected:
     //-- 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
     PRInt16                  mGlobalStatus;   // Global signature verification status
     PRIntervalTime           mReleaseTime;    // used by nsZipReaderCache for flushing entries
     nsZipReaderCache*        mCache;          // if cached, this points to the cache it's contained in
     mozilla::Mutex           mLock;	
     PRInt64                  mMtime;
--- a/modules/libjar/nsZipArchive.cpp
+++ b/modules/libjar/nsZipArchive.cpp
@@ -787,25 +787,29 @@ PRInt64 nsZipArchive::SizeOfMapping()
 {
     return mFd ? mFd->SizeOfMapping() : 0;
 }
 
 //------------------------------------------
 // nsZipArchive constructor and destructor
 //------------------------------------------
 
-nsZipArchive::nsZipArchive() :
-  mBuiltSynthetics(false)
+nsZipArchive::nsZipArchive()
+  : mRefCnt(0)
+  , mBuiltSynthetics(false)
 {
   MOZ_COUNT_CTOR(nsZipArchive);
 
   // initialize the table to NULL
   memset(mFiles, 0, sizeof(mFiles));
 }
 
+NS_IMPL_THREADSAFE_ADDREF(nsZipArchive)
+NS_IMPL_THREADSAFE_RELEASE(nsZipArchive)
+
 nsZipArchive::~nsZipArchive()
 {
   CloseArchive();
 
   MOZ_COUNT_DTOR(nsZipArchive);
 }
 
 
--- a/modules/libjar/nsZipArchive.h
+++ b/modules/libjar/nsZipArchive.h
@@ -222,18 +222,25 @@ public:
   const PRUint8* GetData(nsZipItem* aItem);
 
   /**
    * Gets the amount of memory taken up by the archive's mapping.
    * @return the size
    */
   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;
 
   // Whether we synthesized the directory entries
   bool          mBuiltSynthetics;
 
   // file handle
@@ -261,17 +268,17 @@ class nsZipFind
 {
 public:
   nsZipFind(nsZipArchive* aZip, char* aPattern, bool regExp);
   ~nsZipFind();
 
   nsresult      FindNext(const char** aResult, PRUint16* aNameLen);
 
 private:
-  nsZipArchive* mArchive;
+  nsRefPtr<nsZipArchive> mArchive;
   char*         mPattern;
   nsZipItem*    mItem;
   PRUint16      mSlot;
   bool          mRegExp;
 
   //-- prevent copies and assignments
   nsZipFind& operator=(const nsZipFind& rhs);
   nsZipFind(const nsZipFind& rhs);
--- a/modules/libpref/src/Preferences.cpp
+++ b/modules/libpref/src/Preferences.cpp
@@ -1002,17 +1002,17 @@ static nsresult pref_InitInitialObjects(
   // - $app/defaults/preferences/*.js
 
   nsZipFind *findPtr;
   nsAutoPtr<nsZipFind> find;
   nsTArray<nsCString> prefEntries;
   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");
     NS_ENSURE_SUCCESS(rv, rv);
 
     // Load jar:$gre/omni.jar!/defaults/pref/*.js
     rv = jarReader->FindInit("defaults/pref/*.js$", &findPtr);
     NS_ENSURE_SUCCESS(rv, rv);
@@ -1068,17 +1068,17 @@ static nsresult pref_InitInitialObjects(
 #endif
   };
 
   rv = pref_LoadPrefsInDir(defaultPrefDir, specialFiles, ArrayLength(specialFiles));
   if (NS_FAILED(rv))
     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);
     find = findPtr;
     prefEntries.Clear();
     while (NS_SUCCEEDED(find->FindNext(&entryName, &entryNameLen))) {
       prefEntries.AppendElement(Substring(entryName, entryName + entryNameLen));
     }
--- a/startupcache/StartupCache.cpp
+++ b/startupcache/StartupCache.cpp
@@ -244,16 +244,36 @@ StartupCache::LoadArchive()
   nsresult rv = mFile->Exists(&exists);
   if (NS_FAILED(rv) || !exists)
     return NS_ERROR_FILE_NOT_FOUND;
   
   mArchive = new nsZipArchive();
   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
 StartupCache::GetBuffer(const char* id, char** outbuf, PRUint32* length) 
 {
   NS_ASSERTION(NS_IsMainThread(), "Startup cache only available on main thread");
   WaitOnWriteThread();
   if (!mStartupWriteInitiated) {
@@ -263,46 +283,29 @@ StartupCache::GetBuffer(const char* id, 
     if (entry) {
       *outbuf = new char[entry->size];
       memcpy(*outbuf, entry->data, entry->size);
       *length = entry->size;
       return NS_OK;
     }
   }
 
-  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.
 nsresult
 StartupCache::PutBuffer(const char* id, const char* inbuf, PRUint32 len) 
 {
   NS_ASSERTION(NS_IsMainThread(), "Startup cache only available on main thread");
   WaitOnWriteThread();
--- a/startupcache/StartupCache.h
+++ b/startupcache/StartupCache.h
@@ -162,17 +162,17 @@ private:
   nsresult ResetStartupWriteTimer();
   void WaitOnWriteThread();
 
   static nsresult InitSingleton();
   static void WriteTimeout(nsITimer *aTimer, void *aClosure);
   static void ThreadedWrite(void *aClosure);
 
   nsClassHashtable<nsCStringHashKey, CacheEntry> mTable;
-  nsAutoPtr<nsZipArchive> mArchive;
+  nsRefPtr<nsZipArchive> mArchive;
   nsCOMPtr<nsILocalFile> mFile;
   
   nsCOMPtr<nsIObserverService> mObserverService;
   nsRefPtr<StartupCacheListener> mListener;
   nsCOMPtr<nsITimer> mTimer;
 
   bool mStartupWriteInitiated;
 
--- a/xpcom/build/Omnijar.cpp
+++ b/xpcom/build/Omnijar.cpp
@@ -57,17 +57,17 @@ static const char *sProp[2] =
 
 #define SPROP(Type) ((Type == mozilla::Omnijar::GRE) ? sProp[GRE] : sProp[APP])
 
 void
 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]);
 }
 
 void
 Omnijar::InitOne(nsIFile *aPath, Type aType)
 {
@@ -101,27 +101,24 @@ Omnijar::InitOne(nsIFile *aPath, Type aT
     if ((aType == APP) && (sPath[GRE]) &&
         NS_SUCCEEDED(sPath[GRE]->Equals(file, &equals)) && equals) {
         // If we're using omni.jar on both GRE and APP and their path
         // is the same, we're in the unified case.
         sIsUnified = true;
         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]);
 }
 
 void
 Omnijar::Init(nsIFile *aGrePath, nsIFile *aAppPath)
 {
     InitOne(aGrePath, GRE);
@@ -132,33 +129,33 @@ Omnijar::Init(nsIFile *aGrePath, nsIFile
 void
 Omnijar::CleanUp()
 {
     CleanUpOne(GRE);
     CleanUpOne(APP);
     sInitialized = false;
 }
 
-nsZipArchive *
+already_AddRefed<nsZipArchive>
 Omnijar::GetReader(nsIFile *aPath)
 {
     NS_ABORT_IF_FALSE(IsInitialized(), "Omnijar not initialized");
 
     bool equals;
     nsresult rv;
 
     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;
 }
 
 nsresult
 Omnijar::GetURIString(Type aType, nsACString &result)
 {
     NS_ABORT_IF_FALSE(IsInitialized(), "Omnijar not initialized");
--- a/xpcom/build/Omnijar.h
+++ b/xpcom/build/Omnijar.h
@@ -39,18 +39,18 @@
 
 #ifndef mozilla_Omnijar_h
 #define mozilla_Omnijar_h
 
 #include "nscore.h"
 #include "nsCOMPtr.h"
 #include "nsString.h"
 #include "nsIFile.h"
+#include "nsZipArchive.h"
 
-class nsZipArchive;
 class nsIURI;
 
 namespace mozilla {
 
 class Omnijar {
 private:
 /**
  * Store an nsIFile for an omni.jar. We can store two paths here, one
@@ -104,18 +104,18 @@ static void CleanUp();
  * Returns an nsIFile pointing to the omni.jar file for GRE or APP.
  * Returns nsnull when there is no corresponding omni.jar.
  * Also returns nsnull for APP in the unified case.
  */
 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();
 }
 
 /**
  * Returns whether GRE or APP use an omni.jar. Returns PR_False for
  * APP when using an omni.jar in the unified case.
  */
 static inline bool
 HasOmnijar(Type aType)
@@ -123,28 +123,29 @@ HasOmnijar(Type aType)
     NS_ABORT_IF_FALSE(IsInitialized(), "Omnijar not initialized");
     return !!sPath[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
  * for GRE or APP. i.e. jar:/path/to/omni.jar!/ for omni.jar and
  * /path/to/base/dir/ otherwise. Returns an empty string for APP in
  * the unified case.
  * The returned URI is guaranteed to end with a slash.
  */