Bug 1342397 - Properly reset stuff when failing to create LookupCache for update. draft
authorHenry Chang <hchang@mozilla.com>
Sat, 25 Feb 2017 00:22:39 +0800
changeset 490027 534d9fa4e418736bc81a12f893b9853dc2918e56
parent 489048 039475e399397ce1a79a0b31f2379a43eb52b894
child 547145 aad6d74ee9b0ed7ba4d5e35a4b9f82fb178a7fab
push id46974
push userhchang@mozilla.com
push dateMon, 27 Feb 2017 15:38:22 +0000
bugs1342397
milestone54.0a1
Bug 1342397 - Properly reset stuff when failing to create LookupCache for update. MozReview-Commit-ID: 1ecI2lFIj2U
toolkit/components/url-classifier/Classifier.cpp
toolkit/components/url-classifier/Classifier.h
--- a/toolkit/components/url-classifier/Classifier.cpp
+++ b/toolkit/components/url-classifier/Classifier.cpp
@@ -708,17 +708,21 @@ Classifier::ApplyUpdates(nsTArray<TableU
       // LookupCache::Open() and HashStore::Open() since those operations
       // might fail and cause the database to be removed. It'll be fine
       // until we move "apply update" code off the worker thread.
       rv = CopyInUseDirForUpdate(); // i.e. mUpdatingDirectory will be setup.
       if (NS_FAILED(rv)) {
         LOG(("Failed to copy in-use directory for update."));
         return rv;
       }
-      CopyInUseLookupCacheForUpdate(); // i.e. mNewLookupCaches will be setup.
+      rv = CopyInUseLookupCacheForUpdate(); // i.e. mNewLookupCaches will be setup.
+      if (NS_FAILED(rv)) {
+        LOG(("Failed to create lookup caches from copied files."));
+        return rv;
+      }
     }
 
     LOG(("Applying %" PRIuSIZE " table updates.", aUpdates->Length()));
 
     for (uint32_t i = 0; i < aUpdates->Length(); i++) {
       // Previous UpdateHashStore() may have consumed this update..
       if ((*aUpdates)[i]) {
         // Run all updates for one table
@@ -993,29 +997,33 @@ Classifier::CopyInUseDirForUpdate()
 
   // We moved some things to new places, so move the handles around, too.
   rv = SetupPathNames();
   NS_ENSURE_SUCCESS(rv, rv);
 
   return NS_OK;
 }
 
-void
+nsresult
 Classifier::CopyInUseLookupCacheForUpdate()
 {
   MOZ_ASSERT(mNewLookupCaches.IsEmpty(), "Update intermediaries is forgotten to "
                                          "be removed in the previous update.");
 
   // TODO: Bug 1341183 - Implement deep copy function or copy ctor.
   // For now we just re-open every table we have opened.
   // Another option is to NOT pre-open these LookupCaches and
   // do a "merge" instead of "swap" after update.
   for (auto c: mLookupCaches) {
-    Unused << GetLookupCacheForUpdate(c->TableName());
+    if (!GetLookupCacheForUpdate(c->TableName())) {
+      return NS_ERROR_FAILURE;
+    }
   }
+
+  return NS_OK;
 }
 
 nsresult
 Classifier::RecoverBackups()
 {
   bool backupExists;
   nsresult rv = mBackupDirectory->Exists(&backupExists);
   NS_ENSURE_SUCCESS(rv, rv);
@@ -1309,57 +1317,65 @@ Classifier::UpdateCache(TableUpdate* aUp
 #endif
 
   return NS_OK;
 }
 
 LookupCache *
 Classifier::GetLookupCache(const nsACString& aTable, bool aForUpdate)
 {
-  if (aForUpdate) {
-    return GetLookupCacheFrom(aTable, mNewLookupCaches, mUpdatingDirectory);
-  }
-  return GetLookupCacheFrom(aTable, mLookupCaches, mRootStoreDirectory);
-}
+  nsTArray<LookupCache*>& lookupCaches = aForUpdate ? mNewLookupCaches
+                                                    : mLookupCaches;
+  auto& rootStoreDirectory = aForUpdate ? mUpdatingDirectory
+                                        : mRootStoreDirectory;
 
-LookupCache *
-Classifier::GetLookupCacheFrom(const nsACString& aTable,
-                               nsTArray<LookupCache*>& aLookupCaches,
-                               nsIFile* aRootStoreDirectory)
-{
-  for (uint32_t i = 0; i < aLookupCaches.Length(); i++) {
-    if (aLookupCaches[i]->TableName().Equals(aTable)) {
-      return aLookupCaches[i];
+  for (auto c: lookupCaches) {
+    if (c->TableName().Equals(aTable)) {
+      return c;
     }
   }
 
   // TODO : Bug 1302600, It would be better if we have a more general non-main
   //        thread method to convert table name to protocol version. Currently
   //        we can only know this by checking if the table name ends with '-proto'.
   UniquePtr<LookupCache> cache;
   nsCString provider = GetProvider(aTable);
   if (StringEndsWith(aTable, NS_LITERAL_CSTRING("-proto"))) {
-    cache = MakeUnique<LookupCacheV4>(aTable, provider, aRootStoreDirectory);
+    cache = MakeUnique<LookupCacheV4>(aTable, provider, rootStoreDirectory);
   } else {
-    cache = MakeUnique<LookupCacheV2>(aTable, provider, aRootStoreDirectory);
+    cache = MakeUnique<LookupCacheV2>(aTable, provider, rootStoreDirectory);
   }
 
   nsresult rv = cache->Init();
   if (NS_FAILED(rv)) {
     return nullptr;
   }
   rv = cache->Open();
-  if (NS_FAILED(rv)) {
-    if (rv == NS_ERROR_FILE_CORRUPTED) {
-      Reset();
-    }
+  if (NS_SUCCEEDED(rv)) {
+    lookupCaches.AppendElement(cache.get());
+    return cache.release();
+  }
+
+  // At this point we failed to open LookupCache.
+  //
+  // GetLookupCache for update and for other usage will run on update thread
+  // and worker thread respectively (Bug 1339760). Removing stuff only in
+  // their own realms potentially increases the concurrency.
+
+  if (aForUpdate) {
+    // Remove intermediaries no matter if it's due to file corruption or not.
+    RemoveUpdateIntermediaries();
     return nullptr;
   }
-  aLookupCaches.AppendElement(cache.get());
-  return cache.release();
+
+  // Non-update case.
+  if (rv == NS_ERROR_FILE_CORRUPTED) {
+    Reset(); // Not including the update intermediaries.
+  }
+  return nullptr;
 }
 
 nsresult
 Classifier::ReadNoiseEntries(const Prefix& aPrefix,
                              const nsACString& aTableName,
                              uint32_t aCount,
                              PrefixArray* aNoiseEntries)
 {
--- a/toolkit/components/url-classifier/Classifier.h
+++ b/toolkit/components/url-classifier/Classifier.h
@@ -27,17 +27,17 @@ public:
   typedef nsClassHashtable<nsCStringHashKey, nsCString> ProviderDictType;
 
 public:
   Classifier();
   ~Classifier();
 
   nsresult Open(nsIFile& aCacheDirectory);
   void Close();
-  void Reset();
+  void Reset(); // Not including any intermediary for update.
 
   /**
    * Clear data for specific tables.
    * If ClearType is Clear_Cache, this function will only clear cache in lookup
    * cache, otherwise, it will clear data in lookup cache and data stored on disk.
    */
   enum ClearType {
     Clear_Cache,
@@ -118,17 +118,17 @@ private:
   void DeleteTables(nsIFile* aDirectory, const nsTArray<nsCString>& aTables);
   void AbortUpdateAndReset(const nsCString& aTable);
 
   nsresult CreateStoreDirectory();
   nsresult SetupPathNames();
   nsresult RecoverBackups();
   nsresult CleanToDelete();
   nsresult CopyInUseDirForUpdate();
-  void     CopyInUseLookupCacheForUpdate();
+  nsresult CopyInUseLookupCacheForUpdate();
   nsresult RegenActiveTables();
 
 
 
   // Remove any intermediary for update, including in-memory
   // and on-disk data.
   void RemoveUpdateIntermediaries();