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 84230 a31949628a7e971ecef6d25aca4bc633f3d4cfd4
parent 84229 ccc5f03e6dd2c1f70fa8f643dc8af53bc8692253
child 84231 8e345e9d93e9b2cd2d3cce19f76e33ca38dbcff8
push id114
push userffxbld
push dateFri, 09 Mar 2012 01:01:18 +0000
treeherdermozilla-release@c081ebf13261 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmwu
bugs695843
milestone11.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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.
  */