From 35b37316149108d53a02fb8498e250ea8a22ab5d Mon Sep 17 00:00:00 2001 From: Ryan VanderMeulen Date: Wed, 24 May 2017 09:11:04 -0400 Subject: [PATCH] Backed out changeset c0b940487708 (bug 1359299) for causing intermittent Windows safebrowsing crashes. --- .../components/url-classifier/Classifier.cpp | 20 --------- .../components/url-classifier/Classifier.h | 2 - toolkit/components/url-classifier/Entries.h | 10 ----- .../components/url-classifier/LookupCache.cpp | 32 +++++---------- .../components/url-classifier/LookupCache.h | 11 ++--- .../url-classifier/LookupCacheV4.cpp | 5 ++- .../url-classifier/tests/unit/test_partial.js | 41 +++++++++++++++++++ 7 files changed, 59 insertions(+), 62 deletions(-) diff --git a/toolkit/components/url-classifier/Classifier.cpp b/toolkit/components/url-classifier/Classifier.cpp index 941e6e1d2c1c..ef46edc5efff 100644 --- a/toolkit/components/url-classifier/Classifier.cpp +++ b/toolkit/components/url-classifier/Classifier.cpp @@ -627,19 +627,6 @@ Classifier::RemoveUpdateIntermediaries() } } -void -Classifier::CopyFullHashCacheToNewLookupCache(LookupCache* aNewLookupCache) -{ - MOZ_ASSERT(aNewLookupCache); - - for (auto c: mLookupCaches) { - if (c->TableName() == aNewLookupCache->TableName()) { - aNewLookupCache->CopyFullHashCache(c); - return; - } - } -} - void Classifier::MergeNewLookupCaches() { @@ -1478,13 +1465,6 @@ Classifier::GetLookupCache(const nsACString& aTable, bool aForUpdate) } rv = cache->Open(); if (NS_SUCCEEDED(rv)) { - if (aForUpdate) { - // Since update algorithm will invalidate expired cache entries, we need - // to copy fullhash cache in "old LookupCache" to "update Lookupcache" before - // applying an update. - CopyFullHashCacheToNewLookupCache(cache.get()); - } - lookupCaches.AppendElement(cache.get()); return cache.release(); } diff --git a/toolkit/components/url-classifier/Classifier.h b/toolkit/components/url-classifier/Classifier.h index 45d27d8624fe..a8c8a085450f 100644 --- a/toolkit/components/url-classifier/Classifier.h +++ b/toolkit/components/url-classifier/Classifier.h @@ -140,8 +140,6 @@ private: void MergeNewLookupCaches(); // Merge mNewLookupCaches into mLookupCaches. - void CopyFullHashCacheToNewLookupCache(LookupCache* aNewLookupCache); - // 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 bb32204db0f3..85532126b7de 100644 --- a/toolkit/components/url-classifier/Entries.h +++ b/toolkit/components/url-classifier/Entries.h @@ -356,16 +356,6 @@ 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 bbc4e6ae1fd2..cc20a54bb05a 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 = mFullHashCache.Get(prefix); + CachedFullHashResponse* fullHashResponse = mCache.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) { - mFullHashCache.Remove(prefix); + mCache.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) { - mFullHashCache.Remove(prefix); + mCache.Remove(prefix); } } @@ -209,7 +209,7 @@ LookupCache::InvalidateExpiredCacheEntries() { int64_t nowSec = PR_Now() / PR_USEC_PER_SEC; - for (auto iter = mFullHashCache.Iter(); !iter.Done(); iter.Next()) { + for (auto iter = mCache.Iter(); !iter.Done(); iter.Next()) { CachedFullHashResponse* response = iter.Data(); if (response->negativeCacheExpirySec < nowSec) { iter.Remove(); @@ -217,19 +217,10 @@ LookupCache::InvalidateExpiredCacheEntries() } } -void -LookupCache::CopyFullHashCache(const LookupCache* aSource) -{ - MOZ_ASSERT(aSource); - - CopyClassHashTable(aSource->mFullHashCache, - mFullHashCache); -} - void LookupCache::ClearCache() { - mFullHashCache.Clear(); + mCache.Clear(); } void @@ -248,7 +239,7 @@ LookupCache::GetCacheInfo(nsIUrlClassifierCacheInfo** aCache) RefPtr info = new nsUrlClassifierCacheInfo; info->table = mTableName; - for (auto iter = mFullHashCache.ConstIter(); !iter.Done(); iter.Next()) { + for (auto iter = mCache.ConstIter(); !iter.Done(); iter.Next()) { RefPtr entry = new nsUrlClassifierCacheEntry; // Set prefix of the cache entry. @@ -514,7 +505,7 @@ LookupCache::DumpCache() return; } - for (auto iter = mFullHashCache.ConstIter(); !iter.Done(); iter.Next()) { + for (auto iter = mCache.ConstIter(); !iter.Done(); iter.Next()) { CachedFullHashResponse* response = iter.Data(); nsAutoCString prefix; @@ -649,7 +640,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; } @@ -658,8 +649,7 @@ LookupCacheV2::AddGethashResultToCache(AddCompleteArray& aAddCompletes, nsDependentCSubstring fullhash( reinterpret_cast(add.CompleteHash().buf), COMPLETE_SIZE); - CachedFullHashResponse* response = - mFullHashCache.LookupOrAdd(add.ToUint32()); + CachedFullHashResponse* response = mCache.LookupOrAdd(add.ToUint32()); response->negativeCacheExpirySec = defaultExpirySec; FullHashExpiryCache& fullHashes = response->fullHashes; @@ -667,9 +657,7 @@ LookupCacheV2::AddGethashResultToCache(AddCompleteArray& aAddCompletes, } for (const Prefix& prefix : aMissPrefixes) { - CachedFullHashResponse* response = - mFullHashCache.LookupOrAdd(prefix.ToUint32()); - + CachedFullHashResponse* response = mCache.LookupOrAdd(prefix.ToUint32()); response->negativeCacheExpirySec = defaultExpirySec; } } diff --git a/toolkit/components/url-classifier/LookupCache.h b/toolkit/components/url-classifier/LookupCache.h index 2bad02c18f2f..ca0f0ac2ea0c 100644 --- a/toolkit/components/url-classifier/LookupCache.h +++ b/toolkit/components/url-classifier/LookupCache.h @@ -199,15 +199,12 @@ public: // Called when update to clear expired entries. void InvalidateExpiredCacheEntries(); - // Copy fullhash cache from another LookupCache. - void CopyFullHashCache(const LookupCache* aSource); - - // Clear fullhash cache from fullhash/gethash response. + // Clear completions retrieved from gethash request. void ClearCache(); // Check if completions can be found in cache. // Currently this is only used by testcase. - bool IsInCache(uint32_t key) { return mFullHashCache.Get(key); }; + bool IsInCache(uint32_t key) { return mCache.Get(key); }; #if DEBUG void DumpCache(); @@ -257,8 +254,8 @@ protected: // For gtest to inspect private members. friend class PerProviderDirectoryTestUtils; - // Cache stores fullhash response(V4)/gethash response(V2) - FullHashResponseMap mFullHashCache; + // Cache gethash result. + FullHashResponseMap mCache; }; class LookupCacheV2 final : public LookupCache diff --git a/toolkit/components/url-classifier/LookupCacheV4.cpp b/toolkit/components/url-classifier/LookupCacheV4.cpp index a96d417b7e15..b9e279884b05 100644 --- a/toolkit/components/url-classifier/LookupCacheV4.cpp +++ b/toolkit/components/url-classifier/LookupCacheV4.cpp @@ -330,7 +330,10 @@ LookupCacheV4::ApplyUpdate(TableUpdateV4* aTableUpdate, nsresult LookupCacheV4::AddFullHashResponseToCache(const FullHashResponseMap& aResponseMap) { - CopyClassHashTable(aResponseMap, mFullHashCache); + for (auto iter = aResponseMap.ConstIter(); !iter.Done(); iter.Next()) { + CachedFullHashResponse* response = mCache.LookupOrAdd(iter.Key()); + *response = *(iter.Data()); + } 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 4bb375e0031e..ecb5f2d3722d 100644 --- a/toolkit/components/url-classifier/tests/unit/test_partial.js +++ b/toolkit/components/url-classifier/tests/unit/test_partial.js @@ -558,6 +558,46 @@ 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"]; @@ -695,6 +735,7 @@ function run_test() testCachedResults, testCachedResultsWithSub, testCachedResultsWithExpire, + testCachedResultsUpdate, testCachedResultsFailure, testErrorList, testErrorListIndependent