Bug 1373250: encapsulate all of the memory management in a single structure so that we don't have to do any manual freeing. r=milan

MozReview-Commit-ID: 8hIm53rDUI3

--HG--
extra : rebase_source : 270e89dc05e378f5ae560a0beb719fe18bf671bf

modules/libpref/Preferences.cpp

@@ -229,14 +229,6 @@ ValueObserver::Observe(nsISupports     *aSubject,
   return NS_OK;
 }
 
-// We want to have an atomic pointer to the preference data that should be
-// written.  Make a struct so that we can capture the array and the length.
-struct PrefWriteData
-{
-  UniquePtr<char*[]> mData;
-  uint32_t mCount;
-};
-
 // Write the preference data to a file.
 //
 class PreferencesWriter final
@@ -246,10 +238,8 @@ public:
   {
   }
 
-  // Note that this method will free the contents of aPrefs, but not
-  // the aPrefs itself.
   static
-  nsresult Write(nsIFile* aFile, char* aPrefs[], uint32_t aPrefCount)
+  nsresult Write(nsIFile* aFile, PrefSaveData& aPrefs)
   {
     nsCOMPtr<nsIOutputStream> outStreamSink;
     nsCOMPtr<nsIOutputStream> outStream;
@@ -270,20 +260,31 @@ public:
       return rv;
     }
 
+    struct CharComparator
+    {
+      bool LessThan(const mozilla::UniqueFreePtr<char>& a,
+                    const mozilla::UniqueFreePtr<char>& b) const
+      {
+        return strcmp(a.get(), b.get()) < 0;
+      }
+      bool Equals(const mozilla::UniqueFreePtr<char>& a,
+                  const mozilla::UniqueFreePtr<char>& b) const
+      {
+        return strcmp(a.get(), b.get()) == 0;
+      }
+    };
+
     /* Sort the preferences to make a readable file on disk */
-    NS_QuickSort(aPrefs, aPrefCount, sizeof(char *),
+    aPrefs.Sort(CharComparator());
-                 pref_CompareStrings, nullptr);
 
     // write out the file header
     outStream->Write(kPrefFileHeader, sizeof(kPrefFileHeader) - 1, &writeAmount);
 
-    for (uint32_t valueIdx = 0; valueIdx < aPrefCount; valueIdx++) {
+    for (auto& prefptr : aPrefs) {
-      char*& pref = aPrefs[valueIdx];
+      char* pref = prefptr.get();
       MOZ_ASSERT(pref);
       outStream->Write(pref, strlen(pref), &writeAmount);
       outStream->Write(NS_LINEBREAK, NS_LINEBREAK_LEN, &writeAmount);
-      free(pref);
-      pref = nullptr;
     }
 
     // tell the safe output stream to overwrite the real prefs file
@@ -322,9 +323,9 @@ public:
   // This is the data that all of the runnables (see below) will attempt
   // to write.  It will always have the most up to date version, or be
   // null, if the up to date information has already been written out.
-  static Atomic<PrefWriteData*> sPendingWriteData;
+  static Atomic<PrefSaveData*> sPendingWriteData;
 };
-Atomic<PrefWriteData*> PreferencesWriter::sPendingWriteData((PrefWriteData*)0);
+Atomic<PrefSaveData*> PreferencesWriter::sPendingWriteData(nullptr);
 
 class PWRunnable : public Runnable
 {
@@ -333,16 +334,13 @@ public:
 
   NS_IMETHOD Run() override
   {
-    PrefWriteData* prefs = PreferencesWriter::sPendingWriteData.exchange(nullptr);
+    mozilla::UniquePtr<PrefSaveData> prefs(PreferencesWriter::sPendingWriteData.exchange(nullptr));
     // If we get a nullptr on the exchange, it means that somebody
     // else has already processed the request, and we can just return.
 
     nsresult rv = NS_OK;
     if (prefs) {
-      // The ::Write call will zero out the prefs->mData array, but not free it.
+      rv = PreferencesWriter::Write(mFile, *prefs);
-      // The UniquePtr going out of scope will do that when we free prefs.
-      rv = PreferencesWriter::Write(mFile, prefs->mData.get(), prefs->mCount);
-      delete prefs;
 
       // Make a copy of these so we can have them in runnable lambda.
       // nsIFile is only there so that we would never release the
@@ -1216,16 +1214,15 @@ Preferences::WritePrefFile(nsIFile* aFile, SaveMethod aSaveMethod)
   if (AllowOffMainThreadSave()) {
 
     nsresult rv = NS_OK;
-    PrefWriteData* prefs = new PrefWriteData;
+    mozilla::UniquePtr<PrefSaveData> prefs =
-    prefs->mData = pref_savePrefs(gHashTable, &prefs->mCount);
+      MakeUnique<PrefSaveData>(pref_savePrefs(gHashTable));
 
     // Put the newly constructed preference data into sPendingWriteData
     // for the next request to pick up
-    prefs = PreferencesWriter::sPendingWriteData.exchange(prefs);
+    prefs.reset(PreferencesWriter::sPendingWriteData.exchange(prefs.release()));
     if (prefs) {
       // There was a previous request that hasn't been processed,
-      // and this is the data it had.  Delete the old data and return.
+      // and this is the data it had.
-      delete prefs;
       return rv;
     } else {
       // There were no previous requests, dispatch one since
@@ -1249,12 +1246,8 @@ Preferences::WritePrefFile(nsIFile* aFile, SaveMethod aSaveMethod)
   // This will do a main thread write.  It is safe to do it this way
   // as AllowOffMainThreadSave() returns a consistent value for the
   // lifetime of the parent process.
-  uint32_t prefCount = 0;
+  PrefSaveData prefsData = pref_savePrefs(gHashTable);
-  UniquePtr<char*[]> prefsData = pref_savePrefs(gHashTable, &prefCount);
+  return PreferencesWriter::Write(aFile, prefsData);
-
-  // The ::Write call will zero out the prefsData array, but not free it.
-  // The UniquePtr going out of scope will do that.
-  return PreferencesWriter::Write(aFile, prefsData.get(), prefCount);
 }
 
 static nsresult openPrefFile(nsIFile* aFile)

modules/libpref/prefapi.cpp

@@ -312,20 +312,11 @@ pref_SetPref(const dom::PrefSetting& aPref)
     return rv;
 }
 
-UniquePtr<char*[]>
+PrefSaveData
-pref_savePrefs(PLDHashTable* aTable, uint32_t* aPrefCount)
+pref_savePrefs(PLDHashTable* aTable)
 {
-    // This function allocates the entries in the savedPrefs array it returns.
+    PrefSaveData savedPrefs(aTable->EntryCount());
-    // It is the callers responsibility to go through the array and free
-    // all of them.  The aPrefCount entries will be non-null.  Any end padding
-    // is an implementation detail and may change.
-    MOZ_ASSERT(aPrefCount);
-    auto savedPrefs = MakeUnique<char*[]>(aTable->EntryCount());
 
-    // This is not necessary, but leaving it in for now
-    memset(savedPrefs.get(), 0, aTable->EntryCount() * sizeof(char*));
-
-    int32_t j = 0;
     for (auto iter = aTable->Iter(); !iter.Done(); iter.Next()) {
         auto pref = static_cast<PrefHashEntry*>(iter.Get());
 
@@ -364,14 +355,13 @@ pref_savePrefs(PLDHashTable* aTable, uint32_t* aPrefCount)
         nsAutoCString prefName;
         str_escape(pref->key, prefName);
 
-        savedPrefs[j++] = ToNewCString(prefPrefix +
+        savedPrefs.AppendElement()->
-                                       prefName +
+            reset(ToNewCString(prefPrefix +
-                                       NS_LITERAL_CSTRING("\", ") +
+                               prefName +
-                                       prefValue +
+                               NS_LITERAL_CSTRING("\", ") +
-                                       NS_LITERAL_CSTRING(");"));
+                               prefValue +
+                               NS_LITERAL_CSTRING(");")));
     }
-    *aPrefCount = j;
-
     return savedPrefs;
 }
 
@@ -452,24 +442,6 @@ pref_GetPrefFromEntry(PrefHashEntry *aHashEntry, dom::PrefSetting* aPref)
                 aPref->userValue().get_PrefValue().type()));
 }
 
-
-int
-pref_CompareStrings(const void *v1, const void *v2, void *unused)
-{
-    char *s1 = *(char**) v1;
-    char *s2 = *(char**) v2;
-
-    if (!s1)
-    {
-        if (!s2)
-            return 0;
-        return -1;
-    }
-    if (!s2)
-        return 1;
-    return strcmp(s1, s2);
-}
-
 bool PREF_HasUserPref(const char *pref_name)
 {
     if (!gHashTable)

modules/libpref/prefapi_private_data.h

@@ -20,8 +20,11 @@ class PrefSetting;
 } // namespace dom
 } // namespace mozilla
 
-mozilla::UniquePtr<char*[]>
+
-pref_savePrefs(PLDHashTable* aTable, uint32_t* aPrefCount);
+typedef nsTArray<mozilla::UniqueFreePtr<char> > PrefSaveData;
+
+PrefSaveData
+pref_savePrefs(PLDHashTable* aTable);
 
 nsresult
 pref_SetPref(const mozilla::dom::PrefSetting& aPref);
@@ -37,7 +40,6 @@ void
 pref_SetWatchingPref(bool watching);
 #endif
 
-int pref_CompareStrings(const void *v1, const void *v2, void* unused);
 PrefHashEntry* pref_HashTableLookup(const char *key);
 
 bool