From e4d42d14a2916236ea6c38f61daed5bf696e0f1c Mon Sep 17 00:00:00 2001 From: DimiL Date: Tue, 6 Jun 2017 14:16:57 +0800 Subject: [PATCH] Bug 1359299 - V4 caches in LookupCache need to be copied around in copy constructor. r=hchang MozReview-Commit-ID: AjzUUmQKiPW --HG-- extra : rebase_source : 1c30ee5aa3548dce9b861a70660b1e60410f3654 --- .../components/url-classifier/Classifier.cpp | 40 ++++++++++++++---- .../components/url-classifier/Classifier.h | 2 + toolkit/components/url-classifier/Entries.h | 10 +++++ .../components/url-classifier/LookupCache.cpp | 34 ++++++++++----- .../components/url-classifier/LookupCache.h | 11 +++-- .../url-classifier/LookupCacheV4.cpp | 5 +-- .../url-classifier/tests/unit/test_partial.js | 41 ------------------- 7 files changed, 76 insertions(+), 67 deletions(-) diff --git a/toolkit/components/url-classifier/Classifier.cpp b/toolkit/components/url-classifier/Classifier.cpp index ef46edc5efff..72a013684595 100644 --- a/toolkit/components/url-classifier/Classifier.cpp +++ b/toolkit/components/url-classifier/Classifier.cpp @@ -627,6 +627,34 @@ Classifier::RemoveUpdateIntermediaries() } } +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() { @@ -860,6 +888,10 @@ Classifier::ApplyUpdatesForeground(nsresult aBackgroundRv, 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) { @@ -1223,9 +1255,6 @@ Classifier::UpdateHashStore(nsTArray* aUpdates, return NS_ERROR_UC_UPDATE_TABLE_NOT_FOUND; } - // Clear cache when update - lookupCache->InvalidateExpiredCacheEntries(); - FallibleTArray AddPrefixHashes; rv = lookupCache->GetPrefixes(AddPrefixHashes); NS_ENSURE_SUCCESS(rv, rv); @@ -1315,11 +1344,6 @@ Classifier::UpdateTableV4(nsTArray* aUpdates, 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 diff --git a/toolkit/components/url-classifier/Classifier.h b/toolkit/components/url-classifier/Classifier.h index a8c8a085450f..83cbcecf5ee8 100644 --- a/toolkit/components/url-classifier/Classifier.h +++ b/toolkit/components/url-classifier/Classifier.h @@ -140,6 +140,8 @@ private: void MergeNewLookupCaches(); // Merge mNewLookupCaches into mLookupCaches. + void CopyAndInvalidateFullHashCache(); + // Remove any intermediary for update, including in-memory // and on-disk data. void RemoveUpdateIntermediaries(); diff --git a/toolkit/components/url-classifier/Entries.h b/toolkit/components/url-classifier/Entries.h index 85532126b7de..bb32204db0f3 100644 --- a/toolkit/components/url-classifier/Entries.h +++ b/toolkit/components/url-classifier/Entries.h @@ -356,6 +356,16 @@ struct CachedFullHashResponse { typedef nsClassHashtable FullHashResponseMap; +template +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 diff --git a/toolkit/components/url-classifier/LookupCache.cpp b/toolkit/components/url-classifier/LookupCache.cpp index cc20a54bb05a..fd058de453e8 100644 --- a/toolkit/components/url-classifier/LookupCache.cpp +++ b/toolkit/components/url-classifier/LookupCache.cpp @@ -148,7 +148,7 @@ LookupCache::CheckCache(const Completion& aCompletion, uint32_t prefix = aCompletion.ToUint32(); - CachedFullHashResponse* fullHashResponse = mCache.Get(prefix); + CachedFullHashResponse* fullHashResponse = mFullHashCache.Get(prefix); if (!fullHashResponse) { return NS_OK; } @@ -178,7 +178,7 @@ LookupCache::CheckCache(const Completion& aCompletion, fullHashes.Remove(completion); if (fullHashes.Count() == 0 && fullHashResponse->negativeCacheExpirySec < nowSec) { - mCache.Remove(prefix); + mFullHashCache.Remove(prefix); } } } @@ -193,7 +193,7 @@ LookupCache::CheckCache(const Completion& aCompletion, } else { LOG(("Found an expired prefix in the negative cache")); if (fullHashes.Count() == 0) { - mCache.Remove(prefix); + mFullHashCache.Remove(prefix); } } @@ -209,7 +209,7 @@ 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(); @@ -217,10 +217,21 @@ LookupCache::InvalidateExpiredCacheEntries() } } +void +LookupCache::CopyFullHashCache(const LookupCache* aSource) +{ + if (!aSource) { + return; + } + + CopyClassHashTable(aSource->mFullHashCache, + mFullHashCache); +} + void LookupCache::ClearCache() { - mCache.Clear(); + mFullHashCache.Clear(); } void @@ -239,7 +250,7 @@ LookupCache::GetCacheInfo(nsIUrlClassifierCacheInfo** aCache) RefPtr 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 entry = new nsUrlClassifierCacheEntry; // Set prefix of the cache entry. @@ -505,7 +516,7 @@ LookupCache::DumpCache() 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; @@ -640,7 +651,7 @@ 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; } @@ -649,7 +660,8 @@ LookupCacheV2::AddGethashResultToCache(AddCompleteArray& aAddCompletes, nsDependentCSubstring fullhash( reinterpret_cast(add.CompleteHash().buf), COMPLETE_SIZE); - CachedFullHashResponse* response = mCache.LookupOrAdd(add.ToUint32()); + CachedFullHashResponse* response = + mFullHashCache.LookupOrAdd(add.ToUint32()); response->negativeCacheExpirySec = defaultExpirySec; FullHashExpiryCache& fullHashes = response->fullHashes; @@ -657,7 +669,9 @@ LookupCacheV2::AddGethashResultToCache(AddCompleteArray& aAddCompletes, } for (const Prefix& prefix : aMissPrefixes) { - CachedFullHashResponse* response = mCache.LookupOrAdd(prefix.ToUint32()); + CachedFullHashResponse* response = + mFullHashCache.LookupOrAdd(prefix.ToUint32()); + response->negativeCacheExpirySec = defaultExpirySec; } } diff --git a/toolkit/components/url-classifier/LookupCache.h b/toolkit/components/url-classifier/LookupCache.h index ca0f0ac2ea0c..2bad02c18f2f 100644 --- a/toolkit/components/url-classifier/LookupCache.h +++ b/toolkit/components/url-classifier/LookupCache.h @@ -199,12 +199,15 @@ public: // 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(); @@ -254,8 +257,8 @@ protected: // 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 diff --git a/toolkit/components/url-classifier/LookupCacheV4.cpp b/toolkit/components/url-classifier/LookupCacheV4.cpp index b9e279884b05..a96d417b7e15 100644 --- a/toolkit/components/url-classifier/LookupCacheV4.cpp +++ b/toolkit/components/url-classifier/LookupCacheV4.cpp @@ -330,10 +330,7 @@ LookupCacheV4::ApplyUpdate(TableUpdateV4* aTableUpdate, 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(aResponseMap, mFullHashCache); return NS_OK; } diff --git a/toolkit/components/url-classifier/tests/unit/test_partial.js b/toolkit/components/url-classifier/tests/unit/test_partial.js index ecb5f2d3722d..4bb375e0031e 100644 --- a/toolkit/components/url-classifier/tests/unit/test_partial.js +++ b/toolkit/components/url-classifier/tests/unit/test_partial.js @@ -558,46 +558,6 @@ function testCachedResultsWithExpire() { }); } -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"]; @@ -735,7 +695,6 @@ function run_test() testCachedResults, testCachedResultsWithSub, testCachedResultsWithExpire, - testCachedResultsUpdate, testCachedResultsFailure, testErrorList, testErrorListIndependent