Bug 1359299 - V4 caches in LookupCache need to be copied around in copy constructor. r?hchang draft
authorDimiL <dlee@mozilla.com>
Tue, 06 Jun 2017 14:16:57 +0800
changeset 590887 4caf50911e080c6f61d97337e8119b92188f066c
parent 589301 2c6289f56812c30254acfdddabcfec1e149c0336
child 632337 966ab02943f621099f9a4b403c1c750d9765824a
push id62864
push userbmo:dlee@mozilla.com
push dateThu, 08 Jun 2017 08:13:23 +0000
reviewershchang
bugs1359299
milestone55.0a1
Bug 1359299 - V4 caches in LookupCache need to be copied around in copy constructor. r?hchang MozReview-Commit-ID: AjzUUmQKiPW
toolkit/components/url-classifier/Classifier.cpp
toolkit/components/url-classifier/Classifier.h
toolkit/components/url-classifier/Entries.h
toolkit/components/url-classifier/LookupCache.cpp
toolkit/components/url-classifier/LookupCache.h
toolkit/components/url-classifier/LookupCacheV4.cpp
toolkit/components/url-classifier/tests/unit/test_partial.js
--- a/toolkit/components/url-classifier/Classifier.cpp
+++ b/toolkit/components/url-classifier/Classifier.cpp
@@ -623,16 +623,44 @@ Classifier::RemoveUpdateIntermediaries()
     // we will fail here and it doesn't matter until the next
     // update. (the next udpate will fail due to the removable
     // "safebrowsing-udpating" directory.)
     LOG(("Failed to remove updating directory."));
   }
 }
 
 void
+Classifier::CopyAndInvalidateFullHashCache()
+{
+  MOZ_ASSERT(NS_GetCurrentThread() != mUpdateThread,
+             "CopyAndInvalidateFullHashCache cannot be called on update thread "
+             "since it mutates mLookupCaches which is only safe on "
+             "worker thread.");
+
+  // New lookup caches are built from disk, data likes cache which is
+  // generated online won't exist. We have to manually copy cache from
+  // old LookupCache to new LookupCache.
+  for (auto& newCache: mNewLookupCaches) {
+    for (auto& oldCache: mLookupCaches) {
+      if (oldCache->TableName() == newCache->TableName()) {
+        newCache->CopyFullHashCache(oldCache);
+        break;
+      }
+    }
+  }
+
+  // Clear cache when update.
+  // Invalidate cache entries in CopyAndInvalidateFullHashCache because only
+  // at this point we will have cache data in LookupCache.
+  for (auto& newCache: mNewLookupCaches) {
+    newCache->InvalidateExpiredCacheEntries();
+  }
+}
+
+void
 Classifier::MergeNewLookupCaches()
 {
   MOZ_ASSERT(NS_GetCurrentThread() != mUpdateThread,
              "MergeNewLookupCaches cannot be called on update thread "
              "since it mutates mLookupCaches which is only safe on "
              "worker thread.");
 
   for (auto& newCache: mNewLookupCaches) {
@@ -855,16 +883,20 @@ Classifier::ApplyUpdatesForeground(nsres
                                    const nsACString& aFailedTableName)
 {
   if (mUpdateInterrupted) {
     LOG(("Update is interrupted! Just remove update intermediaries."));
     RemoveUpdateIntermediaries();
     return NS_OK;
   }
   if (NS_SUCCEEDED(aBackgroundRv)) {
+    // Copy and Invalidate fullhash cache here because this call requires
+    // mLookupCaches which is only available on work-thread
+    CopyAndInvalidateFullHashCache();
+
     return SwapInNewTablesAndCleanup();
   }
   if (NS_ERROR_OUT_OF_MEMORY != aBackgroundRv) {
     ResetTables(Clear_All, nsTArray<nsCString> { nsCString(aFailedTableName) });
   }
   return aBackgroundRv;
 }
 
@@ -1218,19 +1250,16 @@ Classifier::UpdateHashStore(nsTArray<Tab
 
   // Read the part of the store that is (only) in the cache
   LookupCacheV2* lookupCache =
     LookupCache::Cast<LookupCacheV2>(GetLookupCacheForUpdate(store.TableName()));
   if (!lookupCache) {
     return NS_ERROR_UC_UPDATE_TABLE_NOT_FOUND;
   }
 
-  // Clear cache when update
-  lookupCache->InvalidateExpiredCacheEntries();
-
   FallibleTArray<uint32_t> AddPrefixHashes;
   rv = lookupCache->GetPrefixes(AddPrefixHashes);
   NS_ENSURE_SUCCESS(rv, rv);
   rv = store.AugmentAdds(AddPrefixHashes);
   NS_ENSURE_SUCCESS(rv, rv);
   AddPrefixHashes.Clear();
 
   uint32_t applied = 0;
@@ -1310,21 +1339,16 @@ Classifier::UpdateTableV4(nsTArray<Table
   }
 
   LookupCacheV4* lookupCache =
     LookupCache::Cast<LookupCacheV4>(GetLookupCacheForUpdate(aTable));
   if (!lookupCache) {
     return NS_ERROR_UC_UPDATE_TABLE_NOT_FOUND;
   }
 
-  // Remove cache entries whose negative cache time is expired when update.
-  // We don't check if positive cache time is expired here because we want to
-  // keep the eviction rule simple when doing an update.
-  lookupCache->InvalidateExpiredCacheEntries();
-
   nsresult rv = NS_OK;
 
   // If there are multiple updates for the same table, prefixes1 & prefixes2
   // will act as input and output in turn to reduce memory copy overhead.
   PrefixStringMap prefixes1, prefixes2;
   PrefixStringMap* input = &prefixes1;
   PrefixStringMap* output = &prefixes2;
 
--- a/toolkit/components/url-classifier/Classifier.h
+++ b/toolkit/components/url-classifier/Classifier.h
@@ -135,16 +135,18 @@ private:
   nsresult SetupPathNames();
   nsresult RecoverBackups();
   nsresult CleanToDelete();
   nsresult CopyInUseDirForUpdate();
   nsresult RegenActiveTables();
 
   void MergeNewLookupCaches(); // Merge mNewLookupCaches into mLookupCaches.
 
+  void CopyAndInvalidateFullHashCache();
+
   // Remove any intermediary for update, including in-memory
   // and on-disk data.
   void RemoveUpdateIntermediaries();
 
 #ifdef MOZ_SAFEBROWSING_DUMP_FAILED_UPDATES
   already_AddRefed<nsIFile> GetFailedUpdateDirectroy();
   nsresult DumpFailedUpdate();
 #endif
--- a/toolkit/components/url-classifier/Entries.h
+++ b/toolkit/components/url-classifier/Entries.h
@@ -351,12 +351,22 @@ struct CachedFullHashResponse {
       }
     }
     return true;
   }
 };
 
 typedef nsClassHashtable<nsUint32HashKey, CachedFullHashResponse> FullHashResponseMap;
 
+template<class T>
+void
+CopyClassHashTable(const T& aSource, T& aDestination)
+{
+  for (auto iter = aSource.ConstIter(); !iter.Done(); iter.Next()) {
+    auto value = aDestination.LookupOrAdd(iter.Key());
+    *value = *(iter.Data());
+  }
+}
+
 } // namespace safebrowsing
 } // namespace mozilla
 
 #endif // SBEntries_h__
--- a/toolkit/components/url-classifier/LookupCache.cpp
+++ b/toolkit/components/url-classifier/LookupCache.cpp
@@ -143,17 +143,17 @@ LookupCache::CheckCache(const Completion
 {
   // Shouldn't call this function if prefix is not in the database.
   MOZ_ASSERT(*aHas);
 
   *aConfirmed = false;
 
   uint32_t prefix = aCompletion.ToUint32();
 
-  CachedFullHashResponse* fullHashResponse = mCache.Get(prefix);
+  CachedFullHashResponse* fullHashResponse = mFullHashCache.Get(prefix);
   if (!fullHashResponse) {
     return NS_OK;
   }
 
   int64_t nowSec = PR_Now() / PR_USEC_PER_SEC;
   int64_t expiryTimeSec;
 
   FullHashExpiryCache& fullHashes = fullHashResponse->fullHashes;
@@ -173,59 +173,70 @@ LookupCache::CheckCache(const Completion
       // Remove fullhash entry from the cache when the negative cache
       // is also expired because whether or not the fullhash is cached
       // locally, we will need to consult the server next time we
       // lookup this hash. We may as well remove it from our cache.
       if (fullHashResponse->negativeCacheExpirySec < expiryTimeSec) {
         fullHashes.Remove(completion);
         if (fullHashes.Count() == 0 &&
             fullHashResponse->negativeCacheExpirySec < nowSec) {
-          mCache.Remove(prefix);
+          mFullHashCache.Remove(prefix);
         }
       }
     }
     return NS_OK;
   }
 
   // Check negative cache.
   if (fullHashResponse->negativeCacheExpirySec >= nowSec) {
     // Url is safe.
     LOG(("Found a valid prefix in the negative cache"));
     *aHas = false;
   } else {
     LOG(("Found an expired prefix in the negative cache"));
     if (fullHashes.Count() == 0) {
-      mCache.Remove(prefix);
+      mFullHashCache.Remove(prefix);
     }
   }
 
   return NS_OK;
 }
 
 // This function remove cache entries whose negative cache time is expired.
 // It is possible that a cache entry whose positive cache time is not yet
 // expired but still being removed after calling this API. Right now we call
 // this on every update.
 void
 LookupCache::InvalidateExpiredCacheEntries()
 {
   int64_t nowSec = PR_Now() / PR_USEC_PER_SEC;
 
-  for (auto iter = mCache.Iter(); !iter.Done(); iter.Next()) {
+  for (auto iter = mFullHashCache.Iter(); !iter.Done(); iter.Next()) {
     CachedFullHashResponse* response = iter.Data();
     if (response->negativeCacheExpirySec < nowSec) {
       iter.Remove();
     }
   }
 }
 
 void
+LookupCache::CopyFullHashCache(const LookupCache* aSource)
+{
+  if (!aSource) {
+    return;
+  }
+
+  CopyClassHashTable<FullHashResponseMap>(aSource->mFullHashCache,
+                                          mFullHashCache);
+}
+
+void
 LookupCache::ClearCache()
 {
-  mCache.Clear();
+  mFullHashCache.Clear();
 }
 
 void
 LookupCache::ClearAll()
 {
   ClearCache();
   ClearPrefixes();
   mPrimed = false;
@@ -234,17 +245,17 @@ LookupCache::ClearAll()
 void
 LookupCache::GetCacheInfo(nsIUrlClassifierCacheInfo** aCache)
 {
   MOZ_ASSERT(aCache);
 
   RefPtr<nsUrlClassifierCacheInfo> info = new nsUrlClassifierCacheInfo;
   info->table = mTableName;
 
-  for (auto iter = mCache.ConstIter(); !iter.Done(); iter.Next()) {
+  for (auto iter = mFullHashCache.ConstIter(); !iter.Done(); iter.Next()) {
     RefPtr<nsUrlClassifierCacheEntry> entry = new nsUrlClassifierCacheEntry;
 
     // Set prefix of the cache entry.
     nsAutoCString prefix(reinterpret_cast<const char*>(&iter.Key()), PREFIX_SIZE);
     CStringToHexString(prefix, entry->prefix);
 
     // Set expiry of the cache entry.
     CachedFullHashResponse* response = iter.Data();
@@ -500,17 +511,17 @@ nsCString GetFormattedTimeString(int64_t
 
 void
 LookupCache::DumpCache()
 {
   if (!LOG_ENABLED()) {
     return;
   }
 
-  for (auto iter = mCache.ConstIter(); !iter.Done(); iter.Next()) {
+  for (auto iter = mFullHashCache.ConstIter(); !iter.Done(); iter.Next()) {
     CachedFullHashResponse* response = iter.Data();
 
     nsAutoCString prefix;
     CStringToHexString(
       nsCString(reinterpret_cast<const char*>(&iter.Key()), PREFIX_SIZE), prefix);
     LOG(("Cache prefix(%s): %s, Expiry: %s", mTableName.get(), prefix.get(),
           GetFormattedTimeString(response->negativeCacheExpirySec).get()));
 
@@ -635,34 +646,37 @@ LookupCacheV2::GetPrefixes(FallibleTArra
   return mPrefixSet->GetPrefixesNative(aAddPrefixes);
 }
 
 void
 LookupCacheV2::AddGethashResultToCache(AddCompleteArray& aAddCompletes,
                                        MissPrefixArray& aMissPrefixes,
                                        int64_t aExpirySec)
 {
-  int64_t defaultExpirySec  = PR_Now() / PR_USEC_PER_SEC + V2_CACHE_DURATION_SEC;
+  int64_t defaultExpirySec = PR_Now() / PR_USEC_PER_SEC + V2_CACHE_DURATION_SEC;
   if (aExpirySec != 0) {
     defaultExpirySec = aExpirySec;
   }
 
   for (const AddComplete& add : aAddCompletes) {
     nsDependentCSubstring fullhash(
       reinterpret_cast<const char*>(add.CompleteHash().buf), COMPLETE_SIZE);
 
-    CachedFullHashResponse* response = mCache.LookupOrAdd(add.ToUint32());
+    CachedFullHashResponse* response =
+      mFullHashCache.LookupOrAdd(add.ToUint32());
     response->negativeCacheExpirySec = defaultExpirySec;
 
     FullHashExpiryCache& fullHashes = response->fullHashes;
     fullHashes.Put(fullhash, defaultExpirySec);
   }
 
   for (const Prefix& prefix : aMissPrefixes) {
-    CachedFullHashResponse* response = mCache.LookupOrAdd(prefix.ToUint32());
+    CachedFullHashResponse* response =
+      mFullHashCache.LookupOrAdd(prefix.ToUint32());
+
     response->negativeCacheExpirySec = defaultExpirySec;
   }
 }
 
 nsresult
 LookupCacheV2::ReadCompletions()
 {
   HashStore store(mTableName, mProvider, mRootStoreDirectory);
--- a/toolkit/components/url-classifier/LookupCache.h
+++ b/toolkit/components/url-classifier/LookupCache.h
@@ -194,22 +194,25 @@ public:
   // Write data stored in lookup cache to disk.
   nsresult WriteFile();
 
   bool IsPrimed() const { return mPrimed; };
 
   // Called when update to clear expired entries.
   void InvalidateExpiredCacheEntries();
 
-  // Clear completions retrieved from gethash request.
+  // Copy fullhash cache from another LookupCache.
+  void CopyFullHashCache(const LookupCache* aSource);
+
+  // Clear fullhash cache from fullhash/gethash response.
   void ClearCache();
 
   // Check if completions can be found in cache.
   // Currently this is only used by testcase.
-  bool IsInCache(uint32_t key) { return mCache.Get(key); };
+  bool IsInCache(uint32_t key) { return mFullHashCache.Get(key); };
 
 #if DEBUG
   void DumpCache();
 #endif
 
   void GetCacheInfo(nsIUrlClassifierCacheInfo** aCache);
 
   virtual nsresult Open();
@@ -249,18 +252,18 @@ protected:
   nsCString mTableName;
   nsCString mProvider;
   nsCOMPtr<nsIFile> mRootStoreDirectory;
   nsCOMPtr<nsIFile> mStoreDirectory;
 
   // For gtest to inspect private members.
   friend class PerProviderDirectoryTestUtils;
 
-  // Cache gethash result.
-  FullHashResponseMap mCache;
+  // Cache stores fullhash response(V4)/gethash response(V2)
+  FullHashResponseMap mFullHashCache;
 };
 
 class LookupCacheV2 final : public LookupCache
 {
 public:
   explicit LookupCacheV2(const nsACString& aTableName,
                          const nsACString& aProvider,
                          nsIFile* aStoreFile)
--- a/toolkit/components/url-classifier/LookupCacheV4.cpp
+++ b/toolkit/components/url-classifier/LookupCacheV4.cpp
@@ -325,20 +325,17 @@ LookupCacheV4::ApplyUpdate(TableUpdateV4
   }
 
   return NS_OK;
 }
 
 nsresult
 LookupCacheV4::AddFullHashResponseToCache(const FullHashResponseMap& aResponseMap)
 {
-  for (auto iter = aResponseMap.ConstIter(); !iter.Done(); iter.Next()) {
-    CachedFullHashResponse* response = mCache.LookupOrAdd(iter.Key());
-    *response = *(iter.Data());
-  }
+  CopyClassHashTable<FullHashResponseMap>(aResponseMap, mFullHashCache);
 
   return NS_OK;
 }
 
 nsresult
 LookupCacheV4::InitCrypto(nsCOMPtr<nsICryptoHash>& aCrypto)
 {
   nsresult rv;
--- a/toolkit/components/url-classifier/tests/unit/test_partial.js
+++ b/toolkit/components/url-classifier/tests/unit/test_partial.js
@@ -553,56 +553,16 @@ function testCachedResultsWithExpire() {
       var assertions = {
         "urlsDontExist" : ["foo.com/a"],
         "completerQueried" : [newCompleter, []]
       }
       doTest([expireUpdate], assertions);
     });
 }
 
-function testCachedResultsUpdate()
-{
-  var existUrls = ["foo.com/a"];
-  setupCachedResults(existUrls, function() {
-    // This is called after setupCachedResults().  Verify that
-    // checking the url again does not cause a completer request.
-
-    // install a new completer, this one should never be queried.
-    var newCompleter = installCompleter('test-phish-simple', [[1, []]], []);
-
-    var assertions = {
-      "urlsExist" : existUrls,
-      "completerQueried" : [newCompleter, []]
-    };
-
-    var addUrls = ["foobar.org/a"];
-
-    var update2 = buildPhishingUpdate(
-        [
-          { "chunkNum" : 2,
-            "urls" : addUrls
-          }],
-        4);
-
-    checkAssertions(assertions, function () {
-      // Apply the update. The cached completes should be gone.
-      doStreamUpdate(update2, function() {
-        // Now the completer gets queried again.
-        var newCompleter2 = installCompleter('test-phish-simple', [[1, existUrls]], []);
-        var assertions2 = {
-          "tableData" : "test-phish-simple;a:1-2",
-          "urlsExist" : existUrls,
-          "completerQueried" : [newCompleter2, existUrls]
-        };
-        checkAssertions(assertions2, runNextTest);
-      }, updateError);
-    });
-  });
-}
-
 function testCachedResultsFailure()
 {
   var existUrls = ["foo.com/a"];
   setupCachedResults(existUrls, function() {
     // This is called after setupCachedResults().  Verify that
     // checking the url again does not cause a completer request.
 
     // install a new completer, this one should never be queried.
@@ -730,16 +690,15 @@ function run_test()
       testCompleterFailure,
       testMixedSizesSameDomain,
       testMixedSizesDifferentDomains,
       testInvalidHashSize,
       testWrongTable,
       testCachedResults,
       testCachedResultsWithSub,
       testCachedResultsWithExpire,
-      testCachedResultsUpdate,
       testCachedResultsFailure,
       testErrorList,
       testErrorListIndependent
   ]);
 }
 
 do_test_pending();