Bug 1434206 - Make TableUpdate objects const as much as possible. r?gcp draft
authorFrancois Marier <francois@mozilla.com>
Fri, 11 May 2018 16:02:37 -0700
changeset 805422 4fd6aa3f48122572764b3a89725bdede902e2fd6
parent 805421 efdd28ddff6a77fbac97f70720424a09f1646f68
child 805423 97f8995e094b26367034b10a14289698ed461346
push id112654
push userfmarier@mozilla.com
push dateThu, 07 Jun 2018 20:10:46 +0000
reviewersgcp
bugs1434206
milestone62.0a1
Bug 1434206 - Make TableUpdate objects const as much as possible. r?gcp I tried to make TableUpdateArray point to const TableUpdate objects everywhere but there were two problems: - HashStore::ApplyUpdate() triggers a few Merge() calls which include sorting the underlying TableUpdate object first. - LookupCacheV4::ApplyUpdate() calls TableUpdateV4::NewChecksum() when the checksum is missing and that sets mChecksum. MozReview-Commit-ID: LIhJcoxo7e7
toolkit/components/url-classifier/Classifier.cpp
toolkit/components/url-classifier/Classifier.h
toolkit/components/url-classifier/HashStore.cpp
toolkit/components/url-classifier/HashStore.h
toolkit/components/url-classifier/LookupCache.cpp
toolkit/components/url-classifier/LookupCache.h
toolkit/components/url-classifier/LookupCacheV4.cpp
toolkit/components/url-classifier/LookupCacheV4.h
toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
toolkit/components/url-classifier/tests/gtest/TestProtocolParser.cpp
--- a/toolkit/components/url-classifier/Classifier.cpp
+++ b/toolkit/components/url-classifier/Classifier.cpp
@@ -787,17 +787,17 @@ Classifier::ApplyUpdatesBackground(Table
   if (NS_FAILED(rv)) {
     LOG(("Failed to copy in-use directory for update."));
     return rv;
   }
 
   LOG(("Applying %zu table updates.", aUpdates.Length()));
 
   for (uint32_t i = 0; i < aUpdates.Length(); i++) {
-    RefPtr<TableUpdate> update = aUpdates[i];
+    RefPtr<const TableUpdate> update = aUpdates[i];
     if (!update) {
       // Previous UpdateHashStore() may have consumed this update..
       continue;
     }
 
     // Run all updates for one table
     nsAutoCString updateTable(update->TableName());
 
@@ -848,17 +848,17 @@ Classifier::ApplyUpdatesForeground(nsres
   }
   if (NS_ERROR_OUT_OF_MEMORY != aBackgroundRv) {
     ResetTables(Clear_All, nsTArray<nsCString> { nsCString(aFailedTableName) });
   }
   return aBackgroundRv;
 }
 
 nsresult
-Classifier::ApplyFullHashes(TableUpdateArray& aUpdates)
+Classifier::ApplyFullHashes(ConstTableUpdateArray& aUpdates)
 {
   MOZ_ASSERT(NS_GetCurrentThread() != mUpdateThread,
              "ApplyFullHashes() MUST NOT be called on update thread");
   MOZ_ASSERT(!NS_IsMainThread(),
              "ApplyFullHashes() must be called on the classifier worker thread.");
 
   LOG(("Applying %zu table gethashes.", aUpdates.Length()));
 
@@ -1139,17 +1139,17 @@ bool
 Classifier::CheckValidUpdate(TableUpdateArray& aUpdates,
                              const nsACString& aTable)
 {
   // take the quick exit if there is no valid update for us
   // (common case)
   uint32_t validupdates = 0;
 
   for (uint32_t i = 0; i < aUpdates.Length(); i++) {
-    RefPtr<TableUpdate> update = aUpdates[i];
+    RefPtr<const TableUpdate> update = aUpdates[i];
     if (!update || !update->TableName().Equals(aTable)) {
       continue;
     }
     if (update->Empty()) {
       aUpdates[i] = nullptr;
       continue;
     }
     validupdates++;
@@ -1299,17 +1299,17 @@ Classifier::UpdateTableV4(TableUpdateArr
   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;
 
-  TableUpdateV4* lastAppliedUpdate = nullptr;
+  RefPtr<const TableUpdateV4> lastAppliedUpdate = nullptr;
   for (uint32_t i = 0; i < aUpdates.Length(); i++) {
     RefPtr<TableUpdate> update = aUpdates[i];
     if (!update || !update->TableName().Equals(aTable)) {
       continue;
     }
 
     RefPtr<TableUpdateV4> updateV4 = TableUpdate::Cast<TableUpdateV4>(update);
     NS_ENSURE_TRUE(updateV4, NS_ERROR_UC_UPDATE_UNEXPECTED_VERSION);
@@ -1364,42 +1364,42 @@ Classifier::UpdateTableV4(TableUpdateArr
   }
 
   LOG(("Successfully updated %s\n", PromiseFlatCString(aTable).get()));
 
   return NS_OK;
 }
 
 nsresult
-Classifier::UpdateCache(RefPtr<TableUpdate> aUpdate)
+Classifier::UpdateCache(RefPtr<const TableUpdate> aUpdate)
 {
   if (!aUpdate) {
     return NS_OK;
   }
 
   nsAutoCString table(aUpdate->TableName());
   LOG(("Classifier::UpdateCache(%s)", table.get()));
 
   LookupCache *lookupCache = GetLookupCache(table);
   if (!lookupCache) {
     return NS_ERROR_FAILURE;
   }
 
   auto lookupV2 = LookupCache::Cast<LookupCacheV2>(lookupCache);
   if (lookupV2) {
-    RefPtr<TableUpdateV2> updateV2 = TableUpdate::Cast<TableUpdateV2>(aUpdate);
+    RefPtr<const TableUpdateV2> updateV2 = TableUpdate::Cast<TableUpdateV2>(aUpdate);
     lookupV2->AddGethashResultToCache(updateV2->AddCompletes(),
                                       updateV2->MissPrefixes());
   } else {
     auto lookupV4 = LookupCache::Cast<LookupCacheV4>(lookupCache);
     if (!lookupV4) {
       return NS_ERROR_FAILURE;
     }
 
-    RefPtr<TableUpdateV4> updateV4 = TableUpdate::Cast<TableUpdateV4>(aUpdate);
+    RefPtr<const TableUpdateV4> updateV4 = TableUpdate::Cast<TableUpdateV4>(aUpdate);
     lookupV4->AddFullHashResponseToCache(updateV4->FullHashResponse());
   }
 
 #if defined(DEBUG)
   lookupCache->DumpCache();
 #endif
 
   return NS_OK;
--- a/toolkit/components/url-classifier/Classifier.h
+++ b/toolkit/components/url-classifier/Classifier.h
@@ -75,17 +75,17 @@ public:
    * is fired. Once this function returns, AsyncApplyUpdates is
    * no longer available.
    */
   void FlushAndDisableAsyncUpdate();
 
   /**
    * Apply full hashes retrived from gethash to cache.
    */
-  nsresult ApplyFullHashes(TableUpdateArray& aUpdates);
+  nsresult ApplyFullHashes(ConstTableUpdateArray& aUpdates);
 
   /*
    * Get a bunch of extra prefixes to query for completion
    * and mask the real entry being requested
    */
   nsresult ReadNoiseEntries(const Prefix& aPrefix,
                             const nsACString& aTableName,
                             uint32_t aCount,
@@ -149,17 +149,17 @@ private:
   nsresult ScanStoreDir(nsIFile* aDirectory, nsTArray<nsCString>& aTables);
 
   nsresult UpdateHashStore(TableUpdateArray& aUpdates,
                            const nsACString& aTable);
 
   nsresult UpdateTableV4(TableUpdateArray& aUpdates,
                          const nsACString& aTable);
 
-  nsresult UpdateCache(RefPtr<TableUpdate> aUpdates);
+  nsresult UpdateCache(RefPtr<const TableUpdate> aUpdates);
 
   LookupCache *GetLookupCacheForUpdate(const nsACString& aTable) {
     return GetLookupCache(aTable, true);
   }
 
   LookupCache *GetLookupCacheFrom(const nsACString& aTable,
                                   nsTArray<LookupCache*>& aLookupCaches,
                                   nsIFile* aRootStoreDirectory);
--- a/toolkit/components/url-classifier/HashStore.cpp
+++ b/toolkit/components/url-classifier/HashStore.cpp
@@ -566,17 +566,17 @@ HashStore::BeginUpdate()
 
   return NS_OK;
 }
 
 template<class T>
 static nsresult
 Merge(ChunkSet* aStoreChunks,
       FallibleTArray<T>* aStorePrefixes,
-      ChunkSet& aUpdateChunks,
+      const ChunkSet& aUpdateChunks,
       FallibleTArray<T>& aUpdatePrefixes,
       bool aAllowMerging = false)
 {
   EntrySort(aUpdatePrefixes);
 
   auto storeIter = aStorePrefixes->begin();
   auto storeEnd = aStorePrefixes->end();
 
--- a/toolkit/components/url-classifier/HashStore.h
+++ b/toolkit/components/url-classifier/HashStore.h
@@ -37,27 +37,32 @@ public:
 
   // Common interfaces.
   const nsCString& TableName() const { return mTable; }
 
   template<typename T>
   static T* Cast(TableUpdate* aThat) {
     return (T::TAG == aThat->Tag() ? reinterpret_cast<T*>(aThat) : nullptr);
   }
+  template<typename T>
+  static const T* Cast(const TableUpdate* aThat) {
+    return (T::TAG == aThat->Tag() ? reinterpret_cast<const T*>(aThat) : nullptr);
+  }
 
 protected:
   virtual ~TableUpdate() {}
 
 private:
   virtual int Tag() const = 0;
 
   const nsCString mTable;
 };
 
 typedef nsTArray<RefPtr<TableUpdate>> TableUpdateArray;
+typedef nsTArray<RefPtr<const TableUpdate>> ConstTableUpdateArray;
 
 // A table update is built from a single update chunk from the server. As the
 // protocol parser processes each chunk, it constructs a table update with the
 // new hashes.
 class TableUpdateV2 : public TableUpdate {
 public:
   explicit TableUpdateV2(const nsACString& aTable)
     : TableUpdate(aTable) {}
@@ -94,31 +99,32 @@ public:
                                      uint32_t aSubChunk);
   MOZ_MUST_USE nsresult NewAddComplete(uint32_t aChunk,
                                        const Completion& aCompletion);
   MOZ_MUST_USE nsresult NewSubComplete(uint32_t aAddChunk,
                                        const Completion& aCompletion,
                                        uint32_t aSubChunk);
   MOZ_MUST_USE nsresult NewMissPrefix(const Prefix& aPrefix);
 
-  ChunkSet& AddChunks() { return mAddChunks; }
-  ChunkSet& SubChunks() { return mSubChunks; }
+  const ChunkSet& AddChunks() const { return mAddChunks; }
+  const ChunkSet& SubChunks() const { return mSubChunks; }
 
   // Expirations for chunks.
-  ChunkSet& AddExpirations() { return mAddExpirations; }
-  ChunkSet& SubExpirations() { return mSubExpirations; }
+  const ChunkSet& AddExpirations() const { return mAddExpirations; }
+  const ChunkSet& SubExpirations() const { return mSubExpirations; }
 
   // Hashes associated with this chunk.
   AddPrefixArray& AddPrefixes() { return mAddPrefixes; }
   SubPrefixArray& SubPrefixes() { return mSubPrefixes; }
+  const AddCompleteArray& AddCompletes() const { return mAddCompletes; }
   AddCompleteArray& AddCompletes() { return mAddCompletes; }
   SubCompleteArray& SubCompletes() { return mSubCompletes; }
 
   // Entries that cannot be completed.
-  MissPrefixArray& MissPrefixes() { return mMissPrefixes; }
+  const MissPrefixArray& MissPrefixes() const { return mMissPrefixes; }
 
   // For downcasting.
   static const int TAG = 2;
 
 private:
 
   // The list of chunk numbers that we have for each of the type of chunks.
   ChunkSet mAddChunks;
@@ -157,18 +163,18 @@ public:
   bool Empty() const override
   {
     return mPrefixesMap.IsEmpty() &&
            mRemovalIndiceArray.IsEmpty() &&
            mFullHashResponseMap.IsEmpty();
   }
 
   bool IsFullUpdate() const { return mFullUpdate; }
-  const PrefixStringMap& Prefixes() { return mPrefixesMap; }
-  RemovalIndiceArray& RemovalIndices() { return mRemovalIndiceArray; }
+  const PrefixStringMap& Prefixes() const { return mPrefixesMap; }
+  const RemovalIndiceArray& RemovalIndices() const { return mRemovalIndiceArray; }
   const nsACString& ClientState() const { return mClientState; }
   const nsACString& Checksum() const { return mChecksum; }
   const FullHashResponseMap& FullHashResponse() const { return mFullHashResponseMap; }
 
   // For downcasting.
   static const int TAG = 4;
 
   void SetFullUpdate(bool aIsFullUpdate) { mFullUpdate = aIsFullUpdate; }
--- a/toolkit/components/url-classifier/LookupCache.cpp
+++ b/toolkit/components/url-classifier/LookupCache.cpp
@@ -641,18 +641,18 @@ LookupCacheV2::GetPrefixes(FallibleTArra
     // This can happen if its a new table, so no error.
     LOG(("GetPrefixes from empty LookupCache"));
     return NS_OK;
   }
   return mPrefixSet->GetPrefixesNative(aAddPrefixes);
 }
 
 void
-LookupCacheV2::AddGethashResultToCache(AddCompleteArray& aAddCompletes,
-                                       MissPrefixArray& aMissPrefixes,
+LookupCacheV2::AddGethashResultToCache(const AddCompleteArray& aAddCompletes,
+                                       const MissPrefixArray& aMissPrefixes,
                                        int64_t aExpirySec)
 {
   int64_t defaultExpirySec = PR_Now() / PR_USEC_PER_SEC + V2_CACHE_DURATION_SEC;
   if (aExpirySec != 0) {
     defaultExpirySec = aExpirySec;
   }
 
   for (const AddComplete& add : aAddCompletes) {
--- a/toolkit/components/url-classifier/LookupCache.h
+++ b/toolkit/components/url-classifier/LookupCache.h
@@ -283,18 +283,18 @@ public:
 
   nsresult Build(AddPrefixArray& aAddPrefixes,
                  AddCompleteArray& aAddCompletes);
 
   nsresult GetPrefixes(FallibleTArray<uint32_t>& aAddPrefixes);
 
   // This will Clear() the passed arrays when done.
   // 'aExpirySec' is used by testcase to config an expired time.
-  void AddGethashResultToCache(AddCompleteArray& aAddCompletes,
-                               MissPrefixArray& aMissPrefixes,
+  void AddGethashResultToCache(const AddCompleteArray& aAddCompletes,
+                               const MissPrefixArray& aMissPrefixes,
                                int64_t aExpirySec = 0);
 
 #if DEBUG
   void DumpCompletions();
 #endif
 
   static const int VER;
 
--- a/toolkit/components/url-classifier/LookupCacheV4.cpp
+++ b/toolkit/components/url-classifier/LookupCacheV4.cpp
@@ -277,17 +277,17 @@ LookupCacheV4::ApplyUpdate(RefPtr<TableU
   // addPSet contains prefixes stored in tableUpdate which should be merged with oldPSet.
   VLPrefixSet oldPSet(aInputMap);
   VLPrefixSet addPSet(aTableUpdate->Prefixes());
 
   // RemovalIndiceArray is a sorted integer array indicating the index of prefix we should
   // remove from the old prefix set(according to lexigraphic order).
   // |removalIndex| is the current index of RemovalIndiceArray.
   // |numOldPrefixPicked| is used to record how many prefixes we picked from the old map.
-  TableUpdateV4::RemovalIndiceArray& removalArray = aTableUpdate->RemovalIndices();
+  const TableUpdateV4::RemovalIndiceArray& removalArray = aTableUpdate->RemovalIndices();
   uint32_t removalIndex = 0;
   int32_t numOldPrefixPicked = -1;
 
   nsAutoCString smallestOldPrefix;
   nsAutoCString smallestAddPrefix;
 
   bool isOldMapEmpty = false, isAddMapEmpty = false;
 
@@ -520,17 +520,17 @@ ReadValue(nsIInputStream* aInputStream, 
 
   return rv;
 }
 
 } // end of unnamed namespace.
 ////////////////////////////////////////////////////////////////////////
 
 nsresult
-LookupCacheV4::WriteMetadata(TableUpdateV4* aTableUpdate)
+LookupCacheV4::WriteMetadata(RefPtr<const TableUpdateV4> aTableUpdate)
 {
   NS_ENSURE_ARG_POINTER(aTableUpdate);
   if (nsUrlClassifierDBService::ShutdownHasStarted()) {
     return NS_ERROR_ABORT;
   }
 
   nsCOMPtr<nsIFile> metaFile;
   nsresult rv = mStoreDirectory->Clone(getter_AddRefs(metaFile));
--- a/toolkit/components/url-classifier/LookupCacheV4.h
+++ b/toolkit/components/url-classifier/LookupCacheV4.h
@@ -38,17 +38,17 @@ public:
 
   // ApplyUpdate will merge data stored in aTableUpdate with prefixes in aInputMap.
   nsresult ApplyUpdate(RefPtr<TableUpdateV4> aTableUpdate,
                        PrefixStringMap& aInputMap,
                        PrefixStringMap& aOutputMap);
 
   nsresult AddFullHashResponseToCache(const FullHashResponseMap& aResponseMap);
 
-  nsresult WriteMetadata(TableUpdateV4* aTableUpdate);
+  nsresult WriteMetadata(RefPtr<const TableUpdateV4> aTableUpdate);
   nsresult LoadMetadata(nsACString& aState, nsACString& aChecksum);
 
   static const int VER;
   static const uint32_t MAX_METADATA_VALUE_LENGTH;
 
 protected:
   virtual nsresult ClearPrefixes() override;
   virtual nsresult StoreToFile(nsIFile* aFile) override;
--- a/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
+++ b/toolkit/components/url-classifier/nsUrlClassifierDBService.cpp
@@ -868,17 +868,17 @@ nsUrlClassifierDBServiceWorker::CacheCom
       if (!s.IsEmpty()) {
         s += ",";
       }
       s += tables[i];
     }
     LOG(("Active tables: %s", s.get()));
   }
 
-  TableUpdateArray updates;
+  ConstTableUpdateArray updates;
 
   for (uint32_t i = 0; i < resultsPtr->Length(); i++) {
     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;
--- a/toolkit/components/url-classifier/tests/gtest/TestProtocolParser.cpp
+++ b/toolkit/components/url-classifier/tests/gtest/TestProtocolParser.cpp
@@ -84,18 +84,18 @@ TEST(UrlClassifierProtocolParser, Single
   response.SerializeToString(&s);
 
   // Feed data to the protocol parser.
   ProtocolParser* p = new ProtocolParserProtobuf();
   p->SetRequestedTables({ nsCString("googpub-phish-proto") });
   p->AppendStream(nsCString(s.c_str(), s.length()));
   p->End();
 
-  auto& tus = p->GetTableUpdates();
-  auto tuv4 = TableUpdate::Cast<TableUpdateV4>(tus[0]);
+  const TableUpdateArray& tus = p->GetTableUpdates();
+  RefPtr<const TableUpdateV4> tuv4 = TableUpdate::Cast<TableUpdateV4>(tus[0]);
   auto& prefixMap = tuv4->Prefixes();
   for (auto iter = prefixMap.ConstIter(); !iter.Done(); iter.Next()) {
     // This prefix map should contain only a single 4-byte prefixe.
     ASSERT_EQ(iter.Key(), 4u);
 
     // The fixed-length prefix string from ProtocolParser should
     // exactly match the expected prefix string.
     nsCString* prefix = iter.Data();