Bug 1434206 - Keep CacheResult objects in smart pointers. r?gcp draft
authorFrancois Marier <francois@mozilla.com>
Wed, 23 May 2018 17:07:42 -0700
changeset 467873 220bf5cd9e2a
parent 467872 b1b041990577
child 467874 c927a5205abe
push id180
push userfmarier@mozilla.com
push dateTue, 29 May 2018 01:16:16 +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
@@ -803,19 +803,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()
@@ -827,35 +825,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;
@@ -869,19 +864,19 @@ nsUrlClassifierDBServiceWorker::CacheCom
       }
       s += tables[i];
     }
     LOG(("Active tables: %s", s.get()));
   }
 
   ConstTableUpdateArray updates;
 
-  for (uint32_t i = 0; i < resultsPtr->Length(); i++) {
+  for (auto iter = aResults.cbegin(); iter != aResults.cend(); ++iter) {
     bool activeTable = false;
-    CacheResult* result = resultsPtr->ElementAt(i).get();
+    RefPtr<const CacheResult> result = *iter;
 
     for (uint32_t table = 0; table < tables.Length(); table++) {
       if (tables[table].Equals(result->table)) {
         activeTable = true;
         break;
       }
     }
     if (activeTable) {
@@ -903,27 +898,27 @@ nsUrlClassifierDBServiceWorker::CacheCom
     } else {
       LOG(("Completion received, but table %s is not active, so not caching.",
            result->table.get()));
     }
   }
 
   rv = mClassifier->ApplyFullHashes(updates);
   NS_ENSURE_SUCCESS(rv, rv);
-  mLastResults = Move(resultsPtr);
+  mLastResults = aResults;
   return NS_OK;
 }
 
 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));
 
@@ -932,17 +927,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,
@@ -984,19 +979,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");
@@ -1004,26 +997,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));
@@ -1058,24 +1051,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)
@@ -1182,24 +1175,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)
 {
@@ -1213,17 +1206,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.
@@ -1240,32 +1233,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)
@@ -1327,21 +1312,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]);
   }
 
@@ -1354,29 +1338,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;
@@ -2421,17 +2398,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;
 };