Bug 1362761 - Safer Clean() and IsEmpty() handling in PrefixSet. r=dimi

This simplifies the logic around clearing the prefix set and also adds
the clearing of the mIndexDeltasChecksum which should have been done
as part of 3a00711bb0e6.

Additionally, the checks for whether or not the prefix set is empty
include some sanity-checking asserts.

Finally, mTotalPrefixes could be out of sync with mIndexPrefixes
and mIndexDeltas if LoadPrefixes() or MakePrefixSet() fail so we
now only update it once all elements have been added successfully.

There is now a release assert to catch grossly out-of-sync (or
corrupt) values of mTotalPrefixes.

MozReview-Commit-ID: BSbyD2dGsUY

Differential Revision: https://phabricator.services.mozilla.com/D2062

--HG--
extra : moz-landing-system : lando

toolkit/components/url-classifier/VariableLengthPrefixSet.cpp

@@ -98,6 +98,7 @@ VariableLengthPrefixSet::SetPrefixes(const PrefixStringMap& aPrefixMap)
       array.AppendElement(BigEndian::readUint32(begin), fallible);
       begin += sizeof(uint32_t);
     }
+    MOZ_ASSERT(array.Length() == numPrefixes);
 
     const uint32_t* arrayPtr = array.Elements();
 #endif

toolkit/components/url-classifier/nsUrlClassifierPrefixSet.cpp

@@ -87,6 +87,7 @@ nsUrlClassifierPrefixSet::Clear()
 {
   LOG(("[%s] Clearing PrefixSet", mName.get()));
   mIndexDeltas.Clear();
+  mIndexDeltasChecksum = ~0;
   mIndexPrefixes.Clear();
   mTotalPrefixes = 0;
 }
@@ -97,14 +98,13 @@ nsUrlClassifierPrefixSet::SetPrefixes(const uint32_t* aArray, uint32_t aLength)
   MutexAutoLock lock(mLock);
 
   nsresult rv = NS_OK;
+  Clear();
 
-  if (aLength <= 0) {
-    if (mIndexPrefixes.Length() > 0) {
-      Clear();
-    }
-  } else {
-    MOZ_ASSERT(aArray);
+  if (aLength > 0) {
     rv = MakePrefixSet(aArray, aLength);
+    if (NS_WARN_IF(NS_FAILED(rv))) {
+      Clear(); // clear out any leftovers
+    }
   }
 
   return rv;
@@ -115,9 +115,8 @@ nsUrlClassifierPrefixSet::MakePrefixSet(const uint32_t* aPrefixes, uint32_t aLen
 {
   mLock.AssertCurrentThreadOwns();
 
-  if (aLength == 0) {
-    return NS_OK;
-  }
+  MOZ_ASSERT(aPrefixes);
+  MOZ_ASSERT(aLength > 0);
 
 #ifdef DEBUG
   for (uint32_t i = 1; i < aLength; i++) {
@@ -125,9 +124,6 @@ nsUrlClassifierPrefixSet::MakePrefixSet(const uint32_t* aPrefixes, uint32_t aLen
   }
 #endif
 
-  Clear();
-  mTotalPrefixes = aLength;
-
   mIndexPrefixes.AppendElement(aPrefixes[0]);
   mIndexDeltas.AppendElement();
 
@@ -161,6 +157,8 @@ nsUrlClassifierPrefixSet::MakePrefixSet(const uint32_t* aPrefixes, uint32_t aLen
     previousItem = aPrefixes[i];
   }
 
+  mTotalPrefixes = aLength;
+
   mIndexDeltas.LastElement().Compact();
 
   // The hdr pointer of the last element of nsTArray may change after calling
@@ -267,7 +265,7 @@ nsUrlClassifierPrefixSet::Contains(uint32_t aPrefix, bool* aFound)
 
   *aFound = false;
 
-  if (mIndexPrefixes.Length() == 0) {
+  if (IsEmptyInternal()) {
     return NS_OK;
   }
 
@@ -344,12 +342,25 @@ nsUrlClassifierPrefixSet::SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeO
   return n;
 }
 
+bool
+nsUrlClassifierPrefixSet::IsEmptyInternal() const
+{
+  if (mIndexPrefixes.IsEmpty()) {
+    MOZ_ASSERT(mIndexDeltas.IsEmpty() && mTotalPrefixes == 0,
+               "If we're empty, there should be no leftovers.");
+    return true;
+  }
+
+  MOZ_ASSERT(mTotalPrefixes >= mIndexPrefixes.Length());
+  return false;
+}
+
 NS_IMETHODIMP
 nsUrlClassifierPrefixSet::IsEmpty(bool * aEmpty)
 {
   MutexAutoLock lock(mLock);
 
-  *aEmpty = (mIndexPrefixes.Length() == 0);
+  *aEmpty = IsEmptyInternal();
   return NS_OK;
 }
 
@@ -433,6 +444,8 @@ nsUrlClassifierPrefixSet::LoadPrefixes(nsCOMPtr<nsIInputStream>& in)
 {
   mCanary.Check();
 
+  Clear();
+
   uint32_t magic;
   uint32_t read;
 
@@ -514,6 +527,7 @@ uint32_t
 nsUrlClassifierPrefixSet::CalculatePreallocateSize() const
 {
   uint32_t fileSize = 4 * sizeof(uint32_t);
+  MOZ_RELEASE_ASSERT(mTotalPrefixes >= mIndexPrefixes.Length());
   uint32_t deltas = mTotalPrefixes - mIndexPrefixes.Length();
   fileSize += 2 * mIndexPrefixes.Length() * sizeof(uint32_t);
   fileSize += deltas * sizeof(uint16_t);

toolkit/components/url-classifier/nsUrlClassifierPrefixSet.h

@@ -64,7 +64,7 @@ private:
   void Clear();
   nsresult MakePrefixSet(const uint32_t* aArray, uint32_t aLength);
   uint32_t BinSearch(uint32_t start, uint32_t end, uint32_t target) const;
-
+  bool IsEmptyInternal() const;
   uint32_t CalculatePreallocateSize() const;
   nsresult WritePrefixes(nsCOMPtr<nsIOutputStream>& out) const;
   nsresult LoadPrefixes(nsCOMPtr<nsIInputStream>& in);