Bug 1434206 - Keep CacheResult objects in smart pointers. r?gcp draft
authorFrancois Marier <francois@mozilla.com>
Fri, 01 Jun 2018 15:49:14 -0700
changeset 805428 cdb199da28f9a33a7bcf817288db2935b58967f4
parent 805427 34741e20c984f304c073a9feb3146b29d2f3ec30
child 805429 eb9b0b6473ecb719674b191d18391f64d12ec8c0
push id112654
push userfmarier@mozilla.com
push dateThu, 07 Jun 2018 20:10:46 +0000
reviewersgcp
bugs1434206
milestone62.0a1
Bug 1434206 - Keep CacheResult objects in smart pointers. r?gcp Some of the objects were kept in UniquePtr and nsAutoPtr but that seemed unnecessary complexity given that we can simply use RefPtr everywhere. It's also possible to make all of the CacheResult arrays const since we don't ever modify the elements once they are added. MozReview-Commit-ID: 5OlcbkQLrGb
toolkit/components/url-classifier/LookupCache.h
toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
toolkit/components/url-classifier/nsUrlClassifierDBService.h
toolkit/components/url-classifier/nsUrlClassifierProxies.cpp
toolkit/components/url-classifier/nsUrlClassifierProxies.h
--- a/toolkit/components/url-classifier/LookupCache.h
+++ b/toolkit/components/url-classifier/LookupCache.h
@@ -84,31 +84,34 @@ public:
 
   bool mProtocolV2;
 };
 
 typedef nsTArray<LookupResult> LookupResultArray;
 
 class CacheResult {
 public:
+  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(CacheResult);
+
   enum { V2, V4 };
 
   virtual int Ver() const = 0;
   virtual bool findCompletion(const Completion& aCompletion) const = 0;
 
-  virtual ~CacheResult() {}
-
   template<typename T>
   static const T* Cast(const CacheResult* aThat) {
     return ((aThat && T::VER == aThat->Ver()) ?
       reinterpret_cast<const T*>(aThat) : nullptr);
   }
 
   nsCString table;
   Prefix prefix;
+
+protected:
+  virtual ~CacheResult() {}
 };
 
 class CacheResultV2 final : public CacheResult
 {
 public:
   static const int VER;
 
   // True when 'prefix' in CacheResult indicates a prefix that
@@ -156,17 +159,17 @@ public:
     nsDependentCSubstring completion(
       reinterpret_cast<const char*>(aCompletion.buf), COMPLETE_SIZE);
     return response.fullHashes.Contains(completion);
   }
 
   virtual int Ver() const override { return VER; }
 };
 
-typedef nsTArray<UniquePtr<CacheResult>> CacheResultArray;
+typedef nsTArray<RefPtr<const CacheResult>> ConstCacheResultArray;
 
 class LookupCache {
 public:
   // Check for a canonicalized IP address.
   static bool IsCanonicalizedIP(const nsACString& aHost);
 
   // take a lookup string (www.hostname.com/path/to/resource.html) and
   // expand it into the set of fragments that should be searched for in an
--- a/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
+++ b/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
@@ -804,19 +804,17 @@ nsUrlClassifierDBServiceWorker::CloseDb(
 {
   if (mClassifier) {
     mClassifier->Close();
     mClassifier = nullptr;
   }
 
   // Clear last completion result when close db so we will still cache completion
   // result next time we re-open it.
-  if (mLastResults) {
-    mLastResults->Clear();
-  }
+  mLastResults.Clear();
 
   LOG(("urlclassifier db closed\n"));
 
   return NS_OK;
 }
 
 nsresult
 nsUrlClassifierDBServiceWorker::PreShutdown()
@@ -828,35 +826,32 @@ nsUrlClassifierDBServiceWorker::PreShutd
   }
 
   // WARNING: nothing we put here should affect an ongoing update thread. When in doubt,
   // put things in Shutdown() instead.
   return NS_OK;
 }
 
 nsresult
-nsUrlClassifierDBServiceWorker::CacheCompletions(CacheResultArray *results)
+nsUrlClassifierDBServiceWorker::CacheCompletions(const ConstCacheResultArray& aResults)
 {
   if (gShuttingDownThread) {
     return NS_ERROR_ABORT;
   }
 
   LOG(("nsUrlClassifierDBServiceWorker::CacheCompletions [%p]", this));
   if (!mClassifier) {
     return NS_OK;
   }
 
-  // Ownership is transferred in to us
-  nsAutoPtr<CacheResultArray> resultsPtr(results);
-
-  if (resultsPtr->Length() == 0) {
+  if (aResults.Length() == 0) {
     return NS_OK;
   }
 
-  if (IsSameAsLastResults(*resultsPtr)) {
+  if (IsSameAsLastResults(aResults)) {
     LOG(("Skipping completions that have just been cached already."));
     return NS_OK;
   }
 
   // Only cache results for tables that we have, don't take
   // in tables we might accidentally have hit during a completion.
   // This happens due to goog vs googpub lists existing.
   nsTArray<nsCString> tables;
@@ -870,19 +865,18 @@ nsUrlClassifierDBServiceWorker::CacheCom
       }
       s += tables[i];
     }
     LOG(("Active tables: %s", s.get()));
   }
 
   ConstTableUpdateArray updates;
 
-  for (uint32_t i = 0; i < resultsPtr->Length(); i++) {
+  for (const auto result : aResults) {
     bool activeTable = false;
-    CacheResult* result = resultsPtr->ElementAt(i).get();
 
     for (uint32_t table = 0; table < tables.Length(); table++) {
       if (tables[table].Equals(result->table)) {
         activeTable = true;
         break;
       }
     }
     if (activeTable) {
@@ -904,28 +898,28 @@ nsUrlClassifierDBServiceWorker::CacheCom
     } else {
       LOG(("Completion received, but table %s is not active, so not caching.",
            result->table.get()));
     }
   }
 
   rv = mClassifier->ApplyFullHashes(updates);
   if (NS_SUCCEEDED(rv)) {
-    mLastResults = std::move(resultsPtr);
+    mLastResults = aResults;
   }
   return rv;
 }
 
 nsresult
-nsUrlClassifierDBServiceWorker::CacheResultToTableUpdate(const CacheResult* aCacheResult,
+nsUrlClassifierDBServiceWorker::CacheResultToTableUpdate(RefPtr<const CacheResult> aCacheResult,
                                                          RefPtr<TableUpdate> aUpdate)
 {
   RefPtr<TableUpdateV2> tuV2 = TableUpdate::Cast<TableUpdateV2>(aUpdate);
   if (tuV2) {
-    const CacheResultV2* result = CacheResult::Cast<const CacheResultV2>(aCacheResult);
+    RefPtr<const CacheResultV2> result = CacheResult::Cast<const CacheResultV2>(aCacheResult);
     MOZ_ASSERT(result);
 
     if (result->miss) {
       return tuV2->NewMissPrefix(result->prefix);
     } else {
       LOG(("CacheCompletion hash %X, Addchunk %d", result->completion.ToUint32(),
            result->addChunk));
 
@@ -934,17 +928,17 @@ nsUrlClassifierDBServiceWorker::CacheRes
         return rv;
       }
       return tuV2->NewAddChunk(result->addChunk);
     }
   }
 
   RefPtr<TableUpdateV4> tuV4 = TableUpdate::Cast<TableUpdateV4>(aUpdate);
   if (tuV4) {
-    const CacheResultV4* result = CacheResult::Cast<const CacheResultV4>(aCacheResult);
+    RefPtr<const CacheResultV4> result = CacheResult::Cast<const CacheResultV4>(aCacheResult);
     MOZ_ASSERT(result);
 
     if (LOG_ENABLED()) {
       const FullHashExpiryCache& fullHashes = result->response.fullHashes;
       for (auto iter = fullHashes.ConstIter(); !iter.Done(); iter.Next()) {
         Completion completion;
         completion.Assign(iter.Key());
         LOG(("CacheCompletion(v4) hash %X, CacheExpireTime %" PRId64,
@@ -986,19 +980,17 @@ nsUrlClassifierDBServiceWorker::OpenDb()
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
 nsUrlClassifierDBServiceWorker::ClearLastResults()
 {
   MOZ_ASSERT(!NS_IsMainThread(), "Must be on the background thread");
-  if (mLastResults) {
-    mLastResults->Clear();
-  }
+  mLastResults.Clear();
   return NS_OK;
 }
 
 nsresult
 nsUrlClassifierDBServiceWorker::GetCacheInfo(const nsACString& aTable,
                                              nsIUrlClassifierCacheInfo** aCache)
 {
   MOZ_ASSERT(!NS_IsMainThread(), "Must be on the background thread");
@@ -1006,26 +998,26 @@ nsUrlClassifierDBServiceWorker::GetCache
     return NS_ERROR_NOT_AVAILABLE;
   }
 
   mClassifier->GetCacheInfo(aTable, aCache);
   return NS_OK;
 }
 
 bool
-nsUrlClassifierDBServiceWorker::IsSameAsLastResults(const CacheResultArray& aResult) const
+nsUrlClassifierDBServiceWorker::IsSameAsLastResults(const ConstCacheResultArray& aResult) const
 {
-  if (!mLastResults || mLastResults->Length() != aResult.Length()) {
+  if (mLastResults.Length() != aResult.Length()) {
     return false;
   }
 
   bool equal = true;
-  for (uint32_t i = 0; i < mLastResults->Length() && equal; i++) {
-    CacheResult* lhs = mLastResults->ElementAt(i).get();
-    CacheResult* rhs = aResult[i].get();
+  for (uint32_t i = 0; i < mLastResults.Length() && equal; i++) {
+    RefPtr<const CacheResult> lhs = mLastResults[i];
+    RefPtr<const CacheResult> rhs = aResult[i];
 
     if (lhs->Ver() != rhs->Ver()) {
       return false;
     }
 
     if (lhs->Ver() == CacheResult::V2) {
       equal = *(CacheResult::Cast<const CacheResultV2>(lhs)) ==
               *(CacheResult::Cast<const CacheResultV2>(rhs));
@@ -1060,24 +1052,24 @@ public:
     , mPendingCompletions(0)
     , mCallback(c)
     {}
 
 private:
   ~nsUrlClassifierLookupCallback();
 
   nsresult HandleResults();
-  nsresult ProcessComplete(CacheResult* aCacheResult);
+  nsresult ProcessComplete(RefPtr<CacheResult> aCacheResult);
   nsresult CacheMisses();
 
   RefPtr<nsUrlClassifierDBService> mDBService;
   nsAutoPtr<LookupResultArray> mResults;
 
   // Completed results to send back to the worker for caching.
-  nsAutoPtr<CacheResultArray> mCacheResults;
+  ConstCacheResultArray mCacheResults;
 
   uint32_t mPendingCompletions;
   nsCOMPtr<nsIUrlClassifierCallback> mCallback;
 };
 
 NS_IMPL_ISUPPORTS(nsUrlClassifierLookupCallback,
                   nsIUrlClassifierLookupCallback,
                   nsIUrlClassifierHashCompleterCallback)
@@ -1184,24 +1176,24 @@ nsUrlClassifierLookupCallback::Completio
                                             const nsACString& aTableName,
                                             uint32_t aChunkId)
 {
   LOG(("nsUrlClassifierLookupCallback::Completion [%p, %s, %d]",
        this, PromiseFlatCString(aTableName).get(), aChunkId));
 
   MOZ_ASSERT(!StringEndsWith(aTableName, NS_LITERAL_CSTRING("-proto")));
 
-  nsAutoPtr<CacheResultV2> result(new CacheResultV2);
+  RefPtr<CacheResultV2> result = new CacheResultV2();
 
   result->table = aTableName;
   result->prefix.Assign(aCompleteHash);
   result->completion.Assign(aCompleteHash);
   result->addChunk = aChunkId;
 
-  return ProcessComplete(result.forget());
+  return ProcessComplete(result);
 }
 
 NS_IMETHODIMP
 nsUrlClassifierLookupCallback::CompletionV4(const nsACString& aPartialHash,
                                             const nsACString& aTableName,
                                             uint32_t aNegativeCacheDuration,
                                             nsIArray* aFullHashes)
 {
@@ -1215,17 +1207,17 @@ nsUrlClassifierLookupCallback::Completio
   }
 
   if (aNegativeCacheDuration > MAXIMUM_NEGATIVE_CACHE_DURATION_SEC) {
     LOG(("Negative cache duration too large, clamping it down to"
          "a reasonable value."));
     aNegativeCacheDuration = MAXIMUM_NEGATIVE_CACHE_DURATION_SEC;
   }
 
-  nsAutoPtr<CacheResultV4> result(new CacheResultV4);
+  RefPtr<CacheResultV4> result = new CacheResultV4();
 
   int64_t nowSec = PR_Now() / PR_USEC_PER_SEC;
 
   result->table = aTableName;
   result->prefix.Assign(aPartialHash);
   result->response.negativeCacheExpirySec = nowSec + aNegativeCacheDuration;
 
   // Fill in positive cache entries.
@@ -1242,32 +1234,24 @@ nsUrlClassifierLookupCallback::Completio
     match->GetFullHash(fullHash);
 
     uint32_t duration;
     match->GetCacheDuration(&duration);
 
     result->response.fullHashes.Put(fullHash, nowSec + duration);
   }
 
-  return ProcessComplete(result.forget());
+  return ProcessComplete(result);
 }
 
 nsresult
-nsUrlClassifierLookupCallback::ProcessComplete(CacheResult* aCacheResult)
+nsUrlClassifierLookupCallback::ProcessComplete(RefPtr<CacheResult> aCacheResult)
 {
-  // Send this completion to the store for caching.
-  if (!mCacheResults) {
-    mCacheResults = new (fallible) CacheResultArray();
-    if (!mCacheResults) {
-      return NS_ERROR_OUT_OF_MEMORY;
-    }
-  }
-
   // OK if this fails, we just won't cache the item.
-  mCacheResults->AppendElement(aCacheResult, fallible);
+  mCacheResults.AppendElement(aCacheResult, fallible);
 
   // Check if this matched any of our results.
   for (uint32_t i = 0; i < mResults->Length(); i++) {
     LookupResult& result = mResults->ElementAt(i);
 
     // Now, see if it verifies a lookup
     if (!result.mNoise
         && result.mTableName.Equals(aCacheResult->table)
@@ -1329,21 +1313,20 @@ nsUrlClassifierLookupCallback::HandleRes
       classifyCallback->HandleResult(result.mTableName, fullHashString);
     }
   }
 
   // Some parts of this gethash request generated no hits at all.
   // Save the prefixes we checked to prevent repeated requests.
   CacheMisses();
 
-  if (mCacheResults) {
-    // This hands ownership of the cache results array back to the worker
-    // thread.
-    mDBService->CacheCompletions(mCacheResults.forget());
-  }
+  // This hands ownership of the cache results array back to the worker
+  // thread.
+  mDBService->CacheCompletions(mCacheResults);
+  mCacheResults.Clear();
 
   nsAutoCString tableStr;
   for (uint32_t i = 0; i < tables.Length(); i++) {
     if (i != 0)
       tableStr.Append(',');
     tableStr.Append(tables[i]);
   }
 
@@ -1356,29 +1339,22 @@ nsUrlClassifierLookupCallback::CacheMiss
   for (uint32_t i = 0; i < mResults->Length(); i++) {
     const LookupResult &result = mResults->ElementAt(i);
     // Skip V4 because cache information is already included in the
     // fullhash response so we don't need to manually add it here.
     if (!result.mProtocolV2 || result.Confirmed() || result.mNoise) {
       continue;
     }
 
-    if (!mCacheResults) {
-      mCacheResults = new (fallible) CacheResultArray();
-      if (!mCacheResults) {
-        return NS_ERROR_OUT_OF_MEMORY;
-      }
-    }
-
-    auto cacheResult = new CacheResultV2;
+    RefPtr<CacheResultV2> cacheResult = new CacheResultV2();
 
     cacheResult->table = result.mTableName;
     cacheResult->prefix = result.hash.fixedLengthPrefix;
     cacheResult->miss = true;
-    if (!mCacheResults->AppendElement(cacheResult, fallible)) {
+    if (!mCacheResults.AppendElement(cacheResult, fallible)) {
       return NS_ERROR_OUT_OF_MEMORY;
     }
   }
   return NS_OK;
 }
 
 struct Provider {
   nsCString name;
@@ -2426,17 +2402,17 @@ nsUrlClassifierDBService::GetCacheInfo(c
 {
   NS_ENSURE_TRUE(gDbBackgroundThread, NS_ERROR_NOT_INITIALIZED);
 
   return mWorkerProxy->GetCacheInfo(aTable, aCallback);
   return NS_OK;
 }
 
 nsresult
-nsUrlClassifierDBService::CacheCompletions(CacheResultArray *results)
+nsUrlClassifierDBService::CacheCompletions(const ConstCacheResultArray& results)
 {
   NS_ENSURE_TRUE(gDbBackgroundThread, NS_ERROR_NOT_INITIALIZED);
 
   return mWorkerProxy->CacheCompletions(results);
 }
 
 bool
 nsUrlClassifierDBService::CanComplete(const nsACString &aTableName)
--- a/toolkit/components/url-classifier/nsUrlClassifierDBService.h
+++ b/toolkit/components/url-classifier/nsUrlClassifierDBService.h
@@ -102,17 +102,17 @@ public:
   NS_DECL_NSIURLCLASSIFIERDBSERVICE
   NS_DECL_NSIURICLASSIFIER
   NS_DECL_NSIURLCLASSIFIERINFO
   NS_DECL_NSIOBSERVER
 
   bool CanComplete(const nsACString &tableName);
   bool GetCompleter(const nsACString& tableName,
                     nsIUrlClassifierHashCompleter** completer);
-  nsresult CacheCompletions(mozilla::safebrowsing::CacheResultArray *results);
+  nsresult CacheCompletions(const mozilla::safebrowsing::ConstCacheResultArray& results);
 
   static nsIThread* BackgroundThread();
 
   static bool ShutdownHasStarted();
 
 private:
 
   const nsTArray<nsCString> kObservedPrefs = {
@@ -216,17 +216,17 @@ public:
   // Open the DB connection
   nsresult GCC_MANGLING_WORKAROUND OpenDb();
 
   // Provide a way to forcibly close the db connection.
   nsresult GCC_MANGLING_WORKAROUND CloseDb();
 
   nsresult GCC_MANGLING_WORKAROUND PreShutdown();
 
-  nsresult CacheCompletions(CacheResultArray * aEntries);
+  nsresult CacheCompletions(const ConstCacheResultArray& aEntries);
 
   // Used to probe the state of the worker thread. When the update begins,
   // mUpdateObserver will be set. When the update finished, mUpdateObserver
   // will be nulled out in NotifyUpdateObserver.
   bool IsBusyUpdating() const { return !!mUpdateObserver; }
 
   // Check the DB ready state of the worker thread
   bool IsDBOpened() const { return !!mClassifier; }
@@ -261,36 +261,36 @@ private:
                     const nsACString& tables,
                     nsIUrlClassifierLookupCallback* c);
 
   nsresult AddNoise(const Prefix aPrefix,
                     const nsCString tableName,
                     uint32_t aCount,
                     LookupResultArray& results);
 
-  nsresult CacheResultToTableUpdate(const CacheResult* aCacheResult,
+  nsresult CacheResultToTableUpdate(RefPtr<const CacheResult> aCacheResult,
                                     RefPtr<TableUpdate> aUpdate);
 
-  bool IsSameAsLastResults(const CacheResultArray& aResult) const;
+  bool IsSameAsLastResults(const ConstCacheResultArray& aResult) const;
 
   nsAutoPtr<mozilla::safebrowsing::Classifier> mClassifier;
   // The class that actually parses the update chunks.
   nsAutoPtr<ProtocolParser> mProtocolParser;
 
   // Directory where to store the SB databases.
   nsCOMPtr<nsIFile> mCacheDir;
 
   RefPtr<nsUrlClassifierDBService> mDBService;
 
   TableUpdateArray mTableUpdates;
 
   uint32_t mUpdateWaitSec;
 
   // Stores the last results that triggered a table update.
-  nsAutoPtr<CacheResultArray> mLastResults;
+  ConstCacheResultArray mLastResults;
 
   nsresult mUpdateStatus;
   nsTArray<nsCString> mUpdateTables;
 
   nsCOMPtr<nsIUrlClassifierUpdateObserver> mUpdateObserver;
   bool mInStream;
 
   // The number of noise entries to add to the set of lookup results.
--- a/toolkit/components/url-classifier/nsUrlClassifierProxies.cpp
+++ b/toolkit/components/url-classifier/nsUrlClassifierProxies.cpp
@@ -220,17 +220,17 @@ UrlClassifierDBServiceWorkerProxy::PreSh
   nsCOMPtr<nsIRunnable> r =
     NewRunnableMethod("nsUrlClassifierDBServiceWorker::PreShutdown",
                       mTarget,
                       &nsUrlClassifierDBServiceWorker::PreShutdown);
   return DispatchToWorkerThread(r);
 }
 
 nsresult
-UrlClassifierDBServiceWorkerProxy::CacheCompletions(CacheResultArray * aEntries)
+UrlClassifierDBServiceWorkerProxy::CacheCompletions(const ConstCacheResultArray& aEntries)
 {
   nsCOMPtr<nsIRunnable> r = new CacheCompletionsRunnable(mTarget, aEntries);
   return DispatchToWorkerThread(r);
 }
 
 NS_IMETHODIMP
 UrlClassifierDBServiceWorkerProxy::CacheCompletionsRunnable::Run()
 {
--- a/toolkit/components/url-classifier/nsUrlClassifierProxies.h
+++ b/toolkit/components/url-classifier/nsUrlClassifierProxies.h
@@ -125,28 +125,28 @@ public:
     RefPtr<nsUrlClassifierDBServiceWorker> mTarget;
     nsCString mUpdateChunk;
   };
 
   class CacheCompletionsRunnable : public mozilla::Runnable
   {
   public:
     CacheCompletionsRunnable(nsUrlClassifierDBServiceWorker* aTarget,
-                             mozilla::safebrowsing::CacheResultArray* aEntries)
+                             const mozilla::safebrowsing::ConstCacheResultArray& aEntries)
       : mozilla::Runnable(
           "UrlClassifierDBServiceWorkerProxy::CacheCompletionsRunnable")
       , mTarget(aTarget)
       , mEntries(aEntries)
     { }
 
     NS_DECL_NSIRUNNABLE
 
   private:
     RefPtr<nsUrlClassifierDBServiceWorker> mTarget;
-     mozilla::safebrowsing::CacheResultArray *mEntries;
+    mozilla::safebrowsing::ConstCacheResultArray mEntries;
   };
 
   class DoLocalLookupRunnable : public mozilla::Runnable
   {
   public:
     DoLocalLookupRunnable(nsUrlClassifierDBServiceWorker* aTarget,
                           const nsACString& spec,
                           const nsACString& tables,
@@ -226,17 +226,17 @@ public:
   nsresult DoLocalLookup(const nsACString& spec,
                          const nsACString& tables,
                          mozilla::safebrowsing::LookupResultArray* results);
 
   nsresult OpenDb();
   nsresult CloseDb();
   nsresult PreShutdown();
 
-  nsresult CacheCompletions(mozilla::safebrowsing::CacheResultArray * aEntries);
+  nsresult CacheCompletions(const mozilla::safebrowsing::ConstCacheResultArray& aEntries);
 
   nsresult GetCacheInfo(const nsACString& aTable,
                         nsIUrlClassifierGetCacheCallback* aCallback);
 private:
   ~UrlClassifierDBServiceWorkerProxy() {}
 
   RefPtr<nsUrlClassifierDBServiceWorker> mTarget;
 };