[safe-browsing] Cleanup tests and remove unnecessary prefix tracking.

Shift PrefixSet::GetPrefixes() into private:, and make test users
friends appropriately.  This moved some tests into namespace
safe_browsing, removed duplicate safe_browsing:: in the PrefixSet
tests.

Modify manual chunk-loading code to use BeginChunk/FinishChunk around
logical chunks, and to not mix prefixes and full hashes in the same
chunk.  Remove cases where the tests were loading full hashes and the
corrosponding prefix, which was not representative of actual chunks
received from the server.

Remove the part of SBProcessSubs() which deleted full hashes by their
prefix component.  Since prefixes and full hashes cannot mix in a chunk,
this case should not be possible in the first place.

Fixed SBProcessSubsDeleteChunk to test what it claimed to test.

BUG=none

Review URL: https://codereview.chromium.org/263493002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@267178 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 287 insertions, 303 deletions

diff --git a/chrome/browser/safe_browsing/prefix_set.h b/chrome/browser/safe_browsing/prefix_set.h
index 7d02473..da4099e 100644
--- a/chrome/browser/safe_browsing/prefix_set.h
+++ b/chrome/browser/safe_browsing/prefix_set.h
@@ -51,6 +51,7 @@
 
 #include <vector>
 
+#include "base/gtest_prod_util.h"
 #include "base/memory/scoped_ptr.h"
 #include "chrome/browser/safe_browsing/safe_browsing_util.h"
 
@@ -71,13 +72,25 @@ class PrefixSet {
   static scoped_ptr<PrefixSet> LoadFile(const base::FilePath& filter_name);
   bool WriteFile(const base::FilePath& filter_name) const;
 
-  // Regenerate the vector of prefixes passed to the constructor into
-  // |prefixes|.  Prefixes will be added in sorted order.
-  void GetPrefixes(std::vector<SBPrefix>* prefixes) const;
-
  private:
   friend class PrefixSetBuilder;
 
+  friend class PrefixSetTest;
+  FRIEND_TEST_ALL_PREFIXES(PrefixSetTest, AllBig);
+  FRIEND_TEST_ALL_PREFIXES(PrefixSetTest, EdgeCases);
+  FRIEND_TEST_ALL_PREFIXES(PrefixSetTest, IntMinMax);
+  FRIEND_TEST_ALL_PREFIXES(PrefixSetTest, OneElement);
+  FRIEND_TEST_ALL_PREFIXES(PrefixSetTest, ReadWriteSigned);
+
+  FRIEND_TEST_ALL_PREFIXES(SafeBrowsingStoreFileTest, BasicStore);
+  FRIEND_TEST_ALL_PREFIXES(SafeBrowsingStoreFileTest, DeleteChunks);
+  FRIEND_TEST_ALL_PREFIXES(SafeBrowsingStoreFileTest, DetectsCorruption);
+  FRIEND_TEST_ALL_PREFIXES(SafeBrowsingStoreFileTest, Empty);
+  FRIEND_TEST_ALL_PREFIXES(SafeBrowsingStoreFileTest, PrefixMinMax);
+  FRIEND_TEST_ALL_PREFIXES(SafeBrowsingStoreFileTest, SubKnockout);
+  FRIEND_TEST_ALL_PREFIXES(SafeBrowsingStoreFileTest, Version7);
+  FRIEND_TEST_ALL_PREFIXES(SafeBrowsingStoreFileTest, Version8);
+
   // Maximum number of consecutive deltas to encode before generating
   // a new index entry.  This helps keep the worst-case performance
   // for |Exists()| under control.
@@ -93,6 +106,10 @@ class PrefixSet {
   void AddRun(SBPrefix index_prefix,
               const uint16* run_begin, const uint16* run_end);
 
+  // Regenerate the vector of prefixes passed to the constructor into
+  // |prefixes|.  Prefixes will be added in sorted order.  Useful for testing.
+  void GetPrefixes(std::vector<SBPrefix>* prefixes) const;
+
   // Used by |PrefixSetBuilder|.
   PrefixSet();
 
diff --git a/chrome/browser/safe_browsing/prefix_set_unittest.cc b/chrome/browser/safe_browsing/prefix_set_unittest.cc
index f935131..1e8b12d 100644
--- a/chrome/browser/safe_browsing/prefix_set_unittest.cc
+++ b/chrome/browser/safe_browsing/prefix_set_unittest.cc
@@ -26,6 +26,10 @@ namespace {
 const SBPrefix kHighBitClear = 1000u * 1000u * 1000u;
 const SBPrefix kHighBitSet = 3u * 1000u * 1000u * 1000u;
 
+}  // namespace
+
+namespace safe_browsing {
+
 class PrefixSetTest : public PlatformTest {
  protected:
   // Constants for the v1 format.
@@ -67,7 +71,7 @@ class PrefixSetTest : public PlatformTest {
   // Check that all elements of |prefixes| are in |prefix_set|, and
   // that nearby elements are not (for lack of a more sensible set of
   // items to check for absence).
-  static void CheckPrefixes(const safe_browsing::PrefixSet& prefix_set,
+  static void CheckPrefixes(const PrefixSet& prefix_set,
                             const std::vector<SBPrefix> &prefixes) {
     // The set can generate the prefixes it believes it has, so that's
     // a good starting point.
@@ -99,7 +103,7 @@ class PrefixSetTest : public PlatformTest {
 
     base::FilePath filename = temp_dir_.path().AppendASCII("PrefixSetTest");
 
-    safe_browsing::PrefixSetBuilder builder(shared_prefixes_);
+    PrefixSetBuilder builder(shared_prefixes_);
     if (!builder.GetPrefixSet()->WriteFile(filename))
       return false;
 
@@ -206,15 +210,15 @@ std::vector<SBPrefix> PrefixSetTest::shared_prefixes_;
 
 // Test that a small sparse random input works.
 TEST_F(PrefixSetTest, Baseline) {
-  safe_browsing::PrefixSetBuilder builder(shared_prefixes_);
+  PrefixSetBuilder builder(shared_prefixes_);
   CheckPrefixes(*builder.GetPrefixSet(), shared_prefixes_);
 }
 
 // Test that the empty set doesn't appear to have anything in it.
 TEST_F(PrefixSetTest, Empty) {
   const std::vector<SBPrefix> empty;
-  safe_browsing::PrefixSetBuilder builder(empty);
-  scoped_ptr<safe_browsing::PrefixSet> prefix_set = builder.GetPrefixSet();
+  PrefixSetBuilder builder(empty);
+  scoped_ptr<PrefixSet> prefix_set = builder.GetPrefixSet();
   for (size_t i = 0; i < shared_prefixes_.size(); ++i) {
     EXPECT_FALSE(prefix_set->Exists(shared_prefixes_[i]));
   }
@@ -223,8 +227,8 @@ TEST_F(PrefixSetTest, Empty) {
 // Single-element set should work fine.
 TEST_F(PrefixSetTest, OneElement) {
   const std::vector<SBPrefix> prefixes(100, 0u);
-  safe_browsing::PrefixSetBuilder builder(prefixes);
-  scoped_ptr<safe_browsing::PrefixSet> prefix_set = builder.GetPrefixSet();
+  PrefixSetBuilder builder(prefixes);
+  scoped_ptr<PrefixSet> prefix_set = builder.GetPrefixSet();
   EXPECT_FALSE(prefix_set->Exists(static_cast<SBPrefix>(-1)));
   EXPECT_TRUE(prefix_set->Exists(prefixes[0]));
   EXPECT_FALSE(prefix_set->Exists(1u));
@@ -253,8 +257,8 @@ TEST_F(PrefixSetTest, IntMinMax) {
   prefixes.push_back(0xFFFFFFFF);
 
   std::sort(prefixes.begin(), prefixes.end());
-  safe_browsing::PrefixSetBuilder builder(prefixes);
-  scoped_ptr<safe_browsing::PrefixSet> prefix_set = builder.GetPrefixSet();
+  PrefixSetBuilder builder(prefixes);
+  scoped_ptr<PrefixSet> prefix_set = builder.GetPrefixSet();
 
   // Check that |GetPrefixes()| returns the same set of prefixes as
   // was passed in.
@@ -276,8 +280,8 @@ TEST_F(PrefixSetTest, AllBig) {
   }
 
   std::sort(prefixes.begin(), prefixes.end());
-  safe_browsing::PrefixSetBuilder builder(prefixes);
-  scoped_ptr<safe_browsing::PrefixSet> prefix_set = builder.GetPrefixSet();
+  PrefixSetBuilder builder(prefixes);
+  scoped_ptr<PrefixSet> prefix_set = builder.GetPrefixSet();
 
   // Check that |GetPrefixes()| returns the same set of prefixes as
   // was passed in.
@@ -331,8 +335,8 @@ TEST_F(PrefixSetTest, EdgeCases) {
   }
 
   std::sort(prefixes.begin(), prefixes.end());
-  safe_browsing::PrefixSetBuilder builder(prefixes);
-  scoped_ptr<safe_browsing::PrefixSet> prefix_set = builder.GetPrefixSet();
+  PrefixSetBuilder builder(prefixes);
+  scoped_ptr<PrefixSet> prefix_set = builder.GetPrefixSet();
 
   // Check that |GetPrefixes()| returns the same set of prefixes as
   // was passed in.
@@ -366,8 +370,7 @@ TEST_F(PrefixSetTest, ReadWrite) {
   // the prefixes.  Leaves the path in |filename|.
   {
     ASSERT_TRUE(GetPrefixSetFile(&filename));
-    scoped_ptr<safe_browsing::PrefixSet> prefix_set =
-        safe_browsing::PrefixSet::LoadFile(filename);
+    scoped_ptr<PrefixSet> prefix_set = PrefixSet::LoadFile(filename);
     ASSERT_TRUE(prefix_set.get());
     CheckPrefixes(*prefix_set, shared_prefixes_);
   }
@@ -378,11 +381,10 @@ TEST_F(PrefixSetTest, ReadWrite) {
     prefixes.push_back(kHighBitClear);
     prefixes.push_back(kHighBitSet);
 
-    safe_browsing::PrefixSetBuilder builder(prefixes);
+    PrefixSetBuilder builder(prefixes);
     ASSERT_TRUE(builder.GetPrefixSet()->WriteFile(filename));
 
-    scoped_ptr<safe_browsing::PrefixSet> prefix_set =
-        safe_browsing::PrefixSet::LoadFile(filename);
+    scoped_ptr<PrefixSet> prefix_set = PrefixSet::LoadFile(filename);
     ASSERT_TRUE(prefix_set.get());
     CheckPrefixes(*prefix_set, prefixes);
   }
@@ -390,11 +392,10 @@ TEST_F(PrefixSetTest, ReadWrite) {
   // Test writing and reading an empty set.
   {
     std::vector<SBPrefix> prefixes;
-    safe_browsing::PrefixSetBuilder builder(prefixes);
+    PrefixSetBuilder builder(prefixes);
     ASSERT_TRUE(builder.GetPrefixSet()->WriteFile(filename));
 
-    scoped_ptr<safe_browsing::PrefixSet> prefix_set =
-        safe_browsing::PrefixSet::LoadFile(filename);
+    scoped_ptr<PrefixSet> prefix_set = PrefixSet::LoadFile(filename);
     ASSERT_TRUE(prefix_set.get());
     CheckPrefixes(*prefix_set, prefixes);
   }
@@ -409,8 +410,7 @@ TEST_F(PrefixSetTest, CorruptionHelpers) {
   base::ScopedFILE file(base::OpenFile(filename, "r+b"));
   IncrementIntAt(file.get(), kPayloadOffset, 1);
   file.reset();
-  scoped_ptr<safe_browsing::PrefixSet> prefix_set =
-      safe_browsing::PrefixSet::LoadFile(filename);
+  scoped_ptr<PrefixSet> prefix_set = PrefixSet::LoadFile(filename);
   ASSERT_FALSE(prefix_set.get());
 
   // Fix up the checksum and it will read successfully (though the
@@ -418,7 +418,7 @@ TEST_F(PrefixSetTest, CorruptionHelpers) {
   file.reset(base::OpenFile(filename, "r+b"));
   CleanChecksum(file.get());
   file.reset();
-  prefix_set = safe_browsing::PrefixSet::LoadFile(filename);
+  prefix_set = PrefixSet::LoadFile(filename);
   ASSERT_TRUE(prefix_set.get());
 }
 
@@ -429,8 +429,7 @@ TEST_F(PrefixSetTest, CorruptionMagic) {
 
   ASSERT_NO_FATAL_FAILURE(
       ModifyAndCleanChecksum(filename, kMagicOffset, 1));
-  scoped_ptr<safe_browsing::PrefixSet> prefix_set =
-      safe_browsing::PrefixSet::LoadFile(filename);
+  scoped_ptr<PrefixSet> prefix_set = PrefixSet::LoadFile(filename);
   ASSERT_FALSE(prefix_set.get());
 }
 
@@ -441,8 +440,7 @@ TEST_F(PrefixSetTest, CorruptionVersion) {
 
   ASSERT_NO_FATAL_FAILURE(
       ModifyAndCleanChecksum(filename, kVersionOffset, 10));
-  scoped_ptr<safe_browsing::PrefixSet> prefix_set =
-      safe_browsing::PrefixSet::LoadFile(filename);
+  scoped_ptr<PrefixSet> prefix_set = PrefixSet::LoadFile(filename);
   ASSERT_FALSE(prefix_set.get());
 }
 
@@ -453,8 +451,7 @@ TEST_F(PrefixSetTest, CorruptionIndexSize) {
 
   ASSERT_NO_FATAL_FAILURE(
       ModifyAndCleanChecksum(filename, kIndexSizeOffset, 1));
-  scoped_ptr<safe_browsing::PrefixSet> prefix_set =
-      safe_browsing::PrefixSet::LoadFile(filename);
+  scoped_ptr<PrefixSet> prefix_set = PrefixSet::LoadFile(filename);
   ASSERT_FALSE(prefix_set.get());
 }
 
@@ -465,8 +462,7 @@ TEST_F(PrefixSetTest, CorruptionDeltasSize) {
 
   ASSERT_NO_FATAL_FAILURE(
       ModifyAndCleanChecksum(filename, kDeltasSizeOffset, 1));
-  scoped_ptr<safe_browsing::PrefixSet> prefix_set =
-      safe_browsing::PrefixSet::LoadFile(filename);
+  scoped_ptr<PrefixSet> prefix_set = PrefixSet::LoadFile(filename);
   ASSERT_FALSE(prefix_set.get());
 }
 
@@ -479,8 +475,7 @@ TEST_F(PrefixSetTest, CorruptionPayload) {
   base::ScopedFILE file(base::OpenFile(filename, "r+b"));
   ASSERT_NO_FATAL_FAILURE(IncrementIntAt(file.get(), 666, 1));
   file.reset();
-  scoped_ptr<safe_browsing::PrefixSet> prefix_set =
-      safe_browsing::PrefixSet::LoadFile(filename);
+  scoped_ptr<PrefixSet> prefix_set = PrefixSet::LoadFile(filename);
   ASSERT_FALSE(prefix_set.get());
 }
 
@@ -495,8 +490,7 @@ TEST_F(PrefixSetTest, CorruptionDigest) {
   long digest_offset = static_cast<long>(size_64 - sizeof(base::MD5Digest));
   ASSERT_NO_FATAL_FAILURE(IncrementIntAt(file.get(), digest_offset, 1));
   file.reset();
-  scoped_ptr<safe_browsing::PrefixSet> prefix_set =
-      safe_browsing::PrefixSet::LoadFile(filename);
+  scoped_ptr<PrefixSet> prefix_set = PrefixSet::LoadFile(filename);
   ASSERT_FALSE(prefix_set.get());
 }
 
@@ -510,8 +504,7 @@ TEST_F(PrefixSetTest, CorruptionExcess) {
   const char buf[] = "im in ur base, killing ur d00dz.";
   ASSERT_EQ(strlen(buf), fwrite(buf, 1, strlen(buf), file.get()));
   file.reset();
-  scoped_ptr<safe_browsing::PrefixSet> prefix_set =
-      safe_browsing::PrefixSet::LoadFile(filename);
+  scoped_ptr<PrefixSet> prefix_set = PrefixSet::LoadFile(filename);
   ASSERT_FALSE(prefix_set.get());
 }
 
@@ -553,8 +546,7 @@ TEST_F(PrefixSetTest, SizeTRecovery) {
   CleanChecksum(file.get());
   file.reset();  // Flush updates.
 
-  scoped_ptr<safe_browsing::PrefixSet> prefix_set =
-      safe_browsing::PrefixSet::LoadFile(filename);
+  scoped_ptr<PrefixSet> prefix_set = PrefixSet::LoadFile(filename);
   ASSERT_FALSE(prefix_set.get());
 }
 
@@ -599,8 +591,7 @@ TEST_F(PrefixSetTest, ReadWriteSigned) {
   CleanChecksum(file.get());
   file.reset();  // Flush updates.
 
-  scoped_ptr<safe_browsing::PrefixSet> prefix_set =
-      safe_browsing::PrefixSet::LoadFile(filename);
+  scoped_ptr<PrefixSet> prefix_set = PrefixSet::LoadFile(filename);
   ASSERT_TRUE(prefix_set.get());
 
   // |Exists()| uses |std::upper_bound()| to find a starting point, which
@@ -635,11 +626,10 @@ TEST_F(PrefixSetTest, Version2) {
   golden_path = golden_path.AppendASCII("SafeBrowsing");
   golden_path = golden_path.AppendASCII(kBasename);
 
-  scoped_ptr<safe_browsing::PrefixSet> prefix_set =
-      safe_browsing::PrefixSet::LoadFile(golden_path);
+  scoped_ptr<PrefixSet> prefix_set = PrefixSet::LoadFile(golden_path);
   ASSERT_TRUE(prefix_set.get());
   CheckPrefixes(*prefix_set, ref_prefixes);
 }
 #endif
 
-}  // namespace
+}  // namespace safe_browsing
diff --git a/chrome/browser/safe_browsing/safe_browsing_store.cc b/chrome/browser/safe_browsing/safe_browsing_store.cc
index a942b9a..e4c15e7 100644
--- a/chrome/browser/safe_browsing/safe_browsing_store.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_store.cc
@@ -21,19 +21,16 @@ bool sorted(CTI beg, CTI end, LESS less) {
   return true;
 }
 
-// Find items matching between |subs| and |adds|, and remove them,
-// recording the item from |adds| in |adds_removed|.  To minimize
-// copies, the inputs are processing in parallel, so |subs| and |adds|
-// should be compatibly ordered (either by SBAddPrefixLess or
-// SBAddPrefixHashLess).
+// Find items matching between |subs| and |adds|, and remove them.  To minimize
+// copies, the inputs are processing in parallel, so |subs| and |adds| should be
+// compatibly ordered (either by SBAddPrefixLess or SBAddPrefixHashLess).
 //
-// |predAddSub| provides add < sub, |predSubAdd| provides sub < add,
-// for the tightest compare appropriate (see calls in SBProcessSubs).
+// |predAddSub| provides add < sub, |predSubAdd| provides sub < add, for the
+// tightest compare appropriate (see calls in SBProcessSubs).
 template <typename SubsT, typename AddsT,
           typename PredAddSubT, typename PredSubAddT>
 void KnockoutSubs(SubsT* subs, AddsT* adds,
-                  PredAddSubT predAddSub, PredSubAddT predSubAdd,
-                  AddsT* adds_removed) {
+                  PredAddSubT predAddSub, PredSubAddT predSubAdd) {
   // Keep a pair of output iterators for writing kept items.  Due to
   // deletions, these may lag the main iterators.  Using erase() on
   // individual items would result in O(N^2) copies.  Using std::list
@@ -62,9 +59,8 @@ void KnockoutSubs(SubsT* subs, AddsT* adds,
       ++add_out;
       ++add_iter;
 
-      // Record equal items and drop them.
+      // Drop equal items.
     } else {
-      adds_removed->push_back(*add_iter);
       ++add_iter;
       ++sub_iter;
     }
@@ -75,45 +71,6 @@ void KnockoutSubs(SubsT* subs, AddsT* adds,
   subs->erase(sub_out, sub_iter);
 }
 
-// Remove items in |removes| from |full_hashes|.  |full_hashes| and
-// |removes| should be ordered by SBAddPrefix component.
-template <typename HashesT, typename AddsT>
-void RemoveMatchingPrefixes(const AddsT& removes, HashesT* full_hashes) {
-  // This is basically an inline of std::set_difference().
-  // Unfortunately, that algorithm requires that the two iterator
-  // pairs use the same value types.
-
-  // Where to store kept items.
-  typename HashesT::iterator out = full_hashes->begin();
-
-  typename HashesT::iterator hash_iter = full_hashes->begin();
-  typename AddsT::const_iterator remove_iter = removes.begin();
-
-  while (hash_iter != full_hashes->end() && remove_iter != removes.end()) {
-    // Keep items less than |*remove_iter|.
-    if (SBAddPrefixLess(*hash_iter, *remove_iter)) {
-      *out = *hash_iter;
-      ++out;
-      ++hash_iter;
-
-      // No hit for |*remove_iter|, bump it forward.
-    } else if (SBAddPrefixLess(*remove_iter, *hash_iter)) {
-      ++remove_iter;
-
-      // Drop equal items, there may be multiple hits.
-    } else {
-      do {
-        ++hash_iter;
-      } while (hash_iter != full_hashes->end() &&
-               !SBAddPrefixLess(*remove_iter, *hash_iter));
-      ++remove_iter;
-    }
-  }
-
-  // Erase any leftover gap.
-  full_hashes->erase(out, hash_iter);
-}
-
 // Remove deleted items (|chunk_id| in |del_set|) from the container.
 template <typename ItemsT>
 void RemoveDeleted(ItemsT* items, const base::hash_set<int32>& del_set) {
@@ -155,25 +112,14 @@ void SBProcessSubs(SBAddPrefixes* add_prefixes,
                 SBAddPrefixHashLess<SBSubFullHash,SBSubFullHash>));
 
   // Factor out the prefix subs.
-  SBAddPrefixes removed_adds;
   KnockoutSubs(sub_prefixes, add_prefixes,
                SBAddPrefixLess<SBAddPrefix,SBSubPrefix>,
-               SBAddPrefixLess<SBSubPrefix,SBAddPrefix>,
-               &removed_adds);
-
-  // Remove the full-hashes corrosponding to the adds which
-  // KnockoutSubs() removed.  Processing these w/in KnockoutSubs()
-  // would make the code more complicated, and they are very small
-  // relative to the prefix lists so the gain would be modest.
-  RemoveMatchingPrefixes(removed_adds, add_full_hashes);
-  RemoveMatchingPrefixes(removed_adds, sub_full_hashes);
+               SBAddPrefixLess<SBSubPrefix,SBAddPrefix>);
 
   // Factor out the full-hash subs.
-  std::vector<SBAddFullHash> removed_full_adds;
   KnockoutSubs(sub_full_hashes, add_full_hashes,
                SBAddPrefixHashLess<SBAddFullHash,SBSubFullHash>,
-               SBAddPrefixHashLess<SBSubFullHash,SBAddFullHash>,
-               &removed_full_adds);
+               SBAddPrefixHashLess<SBSubFullHash,SBAddFullHash>);
 
   // Remove items from the deleted chunks.  This is done after other
   // processing to allow subs to knock out adds (and be removed) even
diff --git a/chrome/browser/safe_browsing/safe_browsing_store_file_unittest.cc b/chrome/browser/safe_browsing/safe_browsing_store_file_unittest.cc
index 864367f..0044cec 100644
--- a/chrome/browser/safe_browsing/safe_browsing_store_file_unittest.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_store_file_unittest.cc
@@ -29,10 +29,15 @@ const SBFullHash kHash2 = SBFullHashForString("two");
 const SBFullHash kHash3 = SBFullHashForString("three");
 const SBFullHash kHash4 = SBFullHashForString("four");
 const SBFullHash kHash5 = SBFullHashForString("five");
+const SBFullHash kHash6 = SBFullHashForString("six");
 
 const SBPrefix kMinSBPrefix = 0u;
 const SBPrefix kMaxSBPrefix = ~kMinSBPrefix;
 
+}  // namespace
+
+namespace safe_browsing {
+
 class SafeBrowsingStoreFileTest : public PlatformTest {
  public:
   virtual void SetUp() {
@@ -70,18 +75,26 @@ class SafeBrowsingStoreFileTest : public PlatformTest {
     EXPECT_TRUE(store_->CheckAddChunk(kAddChunk1));
     EXPECT_TRUE(store_->WriteAddPrefix(kAddChunk1, kHash1.prefix));
     EXPECT_TRUE(store_->WriteAddPrefix(kAddChunk1, kHash2.prefix));
-    EXPECT_TRUE(store_->WriteAddHash(kAddChunk1, now, kHash2));
+    EXPECT_TRUE(store_->FinishChunk());
 
+    EXPECT_TRUE(store_->BeginChunk());
     store_->SetSubChunk(kSubChunk1);
     EXPECT_TRUE(store_->CheckSubChunk(kSubChunk1));
     EXPECT_TRUE(store_->WriteSubPrefix(kSubChunk1, kAddChunk3, kHash3.prefix));
     EXPECT_TRUE(store_->WriteSubHash(kSubChunk1, kAddChunk3, kHash3));
     EXPECT_TRUE(store_->FinishChunk());
 
+    EXPECT_TRUE(store_->BeginChunk());
+    store_->SetAddChunk(kAddChunk2);
+    EXPECT_TRUE(store_->CheckAddChunk(kAddChunk2));
+    EXPECT_TRUE(store_->WriteAddHash(kAddChunk2, now, kHash4));
+    EXPECT_TRUE(store_->FinishChunk());
+
     // Chunk numbers shouldn't leak over.
     EXPECT_FALSE(store_->CheckAddChunk(kSubChunk1));
     EXPECT_FALSE(store_->CheckAddChunk(kAddChunk3));
     EXPECT_FALSE(store_->CheckSubChunk(kAddChunk1));
+    EXPECT_FALSE(store_->CheckSubChunk(kAddChunk2));
 
     safe_browsing::PrefixSetBuilder builder;
     std::vector<SBAddFullHash> add_full_hashes_result;
@@ -135,9 +148,9 @@ TEST_F(SafeBrowsingStoreFileTest, Empty) {
   EXPECT_TRUE(prefixes_result.empty());
 }
 
-// Write some prefix data to the store and verify that it looks like
-// it is still there after the transaction completes.
-TEST_F(SafeBrowsingStoreFileTest, StorePrefix) {
+// Write some prefix and hash data to the store, add more data in another
+// transaction, then verify that the union of all the data is present.
+TEST_F(SafeBrowsingStoreFileTest, BasicStore) {
   const base::Time now = base::Time::Now();
   PopulateStore(now);
 
@@ -145,37 +158,9 @@ TEST_F(SafeBrowsingStoreFileTest, StorePrefix) {
 
   std::vector<int> chunks;
   store_->GetAddChunks(&chunks);
-  ASSERT_EQ(1U, chunks.size());
-  EXPECT_EQ(kAddChunk1, chunks[0]);
-
-  store_->GetSubChunks(&chunks);
-  ASSERT_EQ(1U, chunks.size());
-  EXPECT_EQ(kSubChunk1, chunks[0]);
-
-  {
-    safe_browsing::PrefixSetBuilder builder;
-    std::vector<SBAddFullHash> add_full_hashes_result;
-    EXPECT_TRUE(store_->FinishUpdate(&builder, &add_full_hashes_result));
-
-    std::vector<SBPrefix> prefixes_result;
-    builder.GetPrefixSet()->GetPrefixes(&prefixes_result);
-    ASSERT_EQ(2U, prefixes_result.size());
-    EXPECT_EQ(kHash1.prefix, prefixes_result[0]);
-    EXPECT_EQ(kHash2.prefix, prefixes_result[1]);
-
-    ASSERT_EQ(1U, add_full_hashes_result.size());
-    EXPECT_EQ(kAddChunk1, add_full_hashes_result[0].chunk_id);
-    // EXPECT_TRUE(add_full_hashes_result[0].received == now)?
-    EXPECT_EQ(now.ToTimeT(), add_full_hashes_result[0].received);
-    EXPECT_TRUE(SBFullHashEqual(kHash2, add_full_hashes_result[0].full_hash));
-  }
-
-  ASSERT_TRUE(store_->BeginUpdate());
-
-  // Still has the chunks expected in the next update.
-  store_->GetAddChunks(&chunks);
-  ASSERT_EQ(1U, chunks.size());
+  ASSERT_EQ(2U, chunks.size());
   EXPECT_EQ(kAddChunk1, chunks[0]);
+  EXPECT_EQ(kAddChunk2, chunks[1]);
 
   store_->GetSubChunks(&chunks);
   ASSERT_EQ(1U, chunks.size());
@@ -184,22 +169,29 @@ TEST_F(SafeBrowsingStoreFileTest, StorePrefix) {
   EXPECT_TRUE(store_->CheckAddChunk(kAddChunk1));
   EXPECT_TRUE(store_->CheckSubChunk(kSubChunk1));
 
+  EXPECT_TRUE(store_->BeginChunk());
+  store_->SetAddChunk(kAddChunk3);
+  EXPECT_TRUE(store_->WriteAddPrefix(kAddChunk3, kHash5.prefix));
+  EXPECT_TRUE(store_->FinishChunk());
+
+  // Still has the chunks expected in the next update.
   {
     safe_browsing::PrefixSetBuilder builder;
     std::vector<SBAddFullHash> add_full_hashes_result;
     EXPECT_TRUE(store_->FinishUpdate(&builder, &add_full_hashes_result));
 
-    // Still has the expected contents.
     std::vector<SBPrefix> prefixes_result;
     builder.GetPrefixSet()->GetPrefixes(&prefixes_result);
-    ASSERT_EQ(2U, prefixes_result.size());
+    ASSERT_EQ(3U, prefixes_result.size());
     EXPECT_EQ(kHash1.prefix, prefixes_result[0]);
-    EXPECT_EQ(kHash2.prefix, prefixes_result[1]);
+    EXPECT_EQ(kHash5.prefix, prefixes_result[1]);
+    EXPECT_EQ(kHash2.prefix, prefixes_result[2]);
 
     ASSERT_EQ(1U, add_full_hashes_result.size());
-    EXPECT_EQ(kAddChunk1, add_full_hashes_result[0].chunk_id);
+    EXPECT_EQ(kAddChunk2, add_full_hashes_result[0].chunk_id);
+    // EXPECT_TRUE(add_full_hashes_result[0].received == now)?
     EXPECT_EQ(now.ToTimeT(), add_full_hashes_result[0].received);
-    EXPECT_TRUE(SBFullHashEqual(kHash2, add_full_hashes_result[0].full_hash));
+    EXPECT_TRUE(SBFullHashEqual(kHash4, add_full_hashes_result[0].full_hash));
   }
 }
 
@@ -211,9 +203,9 @@ TEST_F(SafeBrowsingStoreFileTest, PrefixMinMax) {
   ASSERT_TRUE(store_->BeginUpdate());
 
   EXPECT_TRUE(store_->BeginChunk());
-  store_->SetAddChunk(kAddChunk2);
-  EXPECT_TRUE(store_->WriteAddPrefix(kAddChunk2, kMinSBPrefix));
-  EXPECT_TRUE(store_->WriteAddPrefix(kAddChunk2, kMaxSBPrefix));
+  store_->SetAddChunk(kAddChunk3);
+  EXPECT_TRUE(store_->WriteAddPrefix(kAddChunk3, kMinSBPrefix));
+  EXPECT_TRUE(store_->WriteAddPrefix(kAddChunk3, kMaxSBPrefix));
   EXPECT_TRUE(store_->FinishChunk());
 
   {
@@ -234,8 +226,8 @@ TEST_F(SafeBrowsingStoreFileTest, PrefixMinMax) {
 
   EXPECT_TRUE(store_->BeginChunk());
   store_->SetAddChunk(kSubChunk2);
-  EXPECT_TRUE(store_->WriteSubPrefix(kSubChunk2, kAddChunk2, kMinSBPrefix));
-  EXPECT_TRUE(store_->WriteSubPrefix(kSubChunk2, kAddChunk2, kMaxSBPrefix));
+  EXPECT_TRUE(store_->WriteSubPrefix(kSubChunk2, kAddChunk3, kMinSBPrefix));
+  EXPECT_TRUE(store_->WriteSubPrefix(kSubChunk2, kAddChunk3, kMaxSBPrefix));
   EXPECT_TRUE(store_->FinishChunk());
 
   {
@@ -261,8 +253,14 @@ TEST_F(SafeBrowsingStoreFileTest, SubKnockout) {
   store_->SetAddChunk(kAddChunk1);
   EXPECT_TRUE(store_->WriteAddPrefix(kAddChunk1, kHash1.prefix));
   EXPECT_TRUE(store_->WriteAddPrefix(kAddChunk1, kHash2.prefix));
-  EXPECT_TRUE(store_->WriteAddHash(kAddChunk1, now, kHash2));
+  EXPECT_TRUE(store_->FinishChunk());
+
+  EXPECT_TRUE(store_->BeginChunk());
+  store_->SetAddChunk(kAddChunk2);
+  EXPECT_TRUE(store_->WriteAddHash(kAddChunk2, now, kHash4));
+  EXPECT_TRUE(store_->FinishChunk());
 
+  EXPECT_TRUE(store_->BeginChunk());
   store_->SetSubChunk(kSubChunk1);
   EXPECT_TRUE(store_->WriteSubPrefix(kSubChunk1, kAddChunk3, kHash3.prefix));
   EXPECT_TRUE(store_->WriteSubPrefix(kSubChunk1, kAddChunk1, kHash2.prefix));
@@ -278,7 +276,11 @@ TEST_F(SafeBrowsingStoreFileTest, SubKnockout) {
     builder.GetPrefixSet()->GetPrefixes(&prefixes_result);
     ASSERT_EQ(1U, prefixes_result.size());
     EXPECT_EQ(kHash1.prefix, prefixes_result[0]);
-    EXPECT_TRUE(add_full_hashes_result.empty());
+
+    ASSERT_EQ(1U, add_full_hashes_result.size());
+    EXPECT_EQ(kAddChunk2, add_full_hashes_result[0].chunk_id);
+    EXPECT_EQ(now.ToTimeT(), add_full_hashes_result[0].received);
+    EXPECT_TRUE(SBFullHashEqual(kHash4, add_full_hashes_result[0].full_hash));
   }
 
   ASSERT_TRUE(store_->BeginUpdate());
@@ -296,9 +298,13 @@ TEST_F(SafeBrowsingStoreFileTest, SubKnockout) {
 
     std::vector<SBPrefix> prefixes_result;
     builder.GetPrefixSet()->GetPrefixes(&prefixes_result);
-    EXPECT_EQ(1U, prefixes_result.size());
+    ASSERT_EQ(1U, prefixes_result.size());
     EXPECT_EQ(kHash1.prefix, prefixes_result[0]);
-    EXPECT_TRUE(add_full_hashes_result.empty());
+
+    ASSERT_EQ(1U, add_full_hashes_result.size());
+    EXPECT_EQ(kAddChunk2, add_full_hashes_result[0].chunk_id);
+    EXPECT_EQ(now.ToTimeT(), add_full_hashes_result[0].received);
+    EXPECT_TRUE(SBFullHashEqual(kHash4, add_full_hashes_result[0].full_hash));
   }
 
   ASSERT_TRUE(store_->BeginUpdate());
@@ -319,7 +325,11 @@ TEST_F(SafeBrowsingStoreFileTest, SubKnockout) {
     ASSERT_EQ(2U, prefixes_result.size());
     EXPECT_EQ(kHash1.prefix, prefixes_result[0]);
     EXPECT_EQ(kHash3.prefix, prefixes_result[1]);
-    EXPECT_TRUE(add_full_hashes_result.empty());
+
+    ASSERT_EQ(1U, add_full_hashes_result.size());
+    EXPECT_EQ(kAddChunk2, add_full_hashes_result[0].chunk_id);
+    EXPECT_EQ(now.ToTimeT(), add_full_hashes_result[0].received);
+    EXPECT_TRUE(SBFullHashEqual(kHash4, add_full_hashes_result[0].full_hash));
   }
 }
 
@@ -329,28 +339,32 @@ TEST_F(SafeBrowsingStoreFileTest, DeleteChunks) {
 
   const base::Time now = base::Time::Now();
 
-  // A chunk which will be deleted.
+  // A prefix chunk which will be deleted.
   EXPECT_FALSE(store_->CheckAddChunk(kAddChunk1));
   store_->SetAddChunk(kAddChunk1);
   EXPECT_TRUE(store_->BeginChunk());
   EXPECT_TRUE(store_->WriteAddPrefix(kAddChunk1, kHash1.prefix));
   EXPECT_TRUE(store_->WriteAddPrefix(kAddChunk1, kHash2.prefix));
-  EXPECT_TRUE(store_->WriteAddHash(kAddChunk1, now, kHash2));
   EXPECT_TRUE(store_->FinishChunk());
 
-  // Another which won't.
+  // A prefix chunk which won't be deleted.
   EXPECT_FALSE(store_->CheckAddChunk(kAddChunk2));
   store_->SetAddChunk(kAddChunk2);
   EXPECT_TRUE(store_->BeginChunk());
   EXPECT_TRUE(store_->WriteAddPrefix(kAddChunk2, kHash3.prefix));
-  EXPECT_TRUE(store_->WriteAddHash(kAddChunk2, now, kHash3));
+  EXPECT_TRUE(store_->FinishChunk());
+
+  // A full-hash chunk which won't be deleted.
+  EXPECT_FALSE(store_->CheckAddChunk(kAddChunk3));
+  store_->SetAddChunk(kAddChunk3);
+  EXPECT_TRUE(store_->BeginChunk());
+  EXPECT_TRUE(store_->WriteAddHash(kAddChunk3, now, kHash6));
   EXPECT_TRUE(store_->FinishChunk());
 
   // A sub chunk to delete.
   EXPECT_FALSE(store_->CheckSubChunk(kSubChunk1));
   store_->SetSubChunk(kSubChunk1);
   EXPECT_TRUE(store_->BeginChunk());
-  EXPECT_TRUE(store_->WriteSubPrefix(kSubChunk1, kAddChunk3, kHash4.prefix));
   EXPECT_TRUE(store_->WriteSubHash(kSubChunk1, kAddChunk3, kHash4));
   EXPECT_TRUE(store_->FinishChunk());
 
@@ -358,7 +372,6 @@ TEST_F(SafeBrowsingStoreFileTest, DeleteChunks) {
   EXPECT_FALSE(store_->CheckSubChunk(kSubChunk2));
   store_->SetSubChunk(kSubChunk2);
   EXPECT_TRUE(store_->BeginChunk());
-  EXPECT_TRUE(store_->WriteSubPrefix(kSubChunk2, kAddChunk4, kHash5.prefix));
   EXPECT_TRUE(store_->WriteSubHash(kSubChunk2, kAddChunk4, kHash5));
   EXPECT_TRUE(store_->FinishChunk());
 
@@ -368,6 +381,7 @@ TEST_F(SafeBrowsingStoreFileTest, DeleteChunks) {
   // Not actually deleted until finish.
   EXPECT_TRUE(store_->CheckAddChunk(kAddChunk1));
   EXPECT_TRUE(store_->CheckAddChunk(kAddChunk2));
+  EXPECT_TRUE(store_->CheckAddChunk(kAddChunk3));
   EXPECT_TRUE(store_->CheckSubChunk(kSubChunk1));
   EXPECT_TRUE(store_->CheckSubChunk(kSubChunk2));
 
@@ -378,36 +392,44 @@ TEST_F(SafeBrowsingStoreFileTest, DeleteChunks) {
 
     std::vector<SBPrefix> prefixes_result;
     builder.GetPrefixSet()->GetPrefixes(&prefixes_result);
-    EXPECT_EQ(1U, prefixes_result.size());
+    ASSERT_EQ(1U, prefixes_result.size());
     EXPECT_EQ(kHash3.prefix, prefixes_result[0]);
 
-    EXPECT_EQ(1U, add_full_hashes_result.size());
-    EXPECT_EQ(kAddChunk2, add_full_hashes_result[0].chunk_id);
+    ASSERT_EQ(1U, add_full_hashes_result.size());
+    EXPECT_EQ(kAddChunk3, add_full_hashes_result[0].chunk_id);
     EXPECT_EQ(now.ToTimeT(), add_full_hashes_result[0].received);
-    EXPECT_TRUE(SBFullHashEqual(kHash3, add_full_hashes_result[0].full_hash));
+    EXPECT_TRUE(SBFullHashEqual(kHash6, add_full_hashes_result[0].full_hash));
   }
 
   // Expected chunks are there in another update.
   ASSERT_TRUE(store_->BeginUpdate());
   EXPECT_FALSE(store_->CheckAddChunk(kAddChunk1));
   EXPECT_TRUE(store_->CheckAddChunk(kAddChunk2));
+  EXPECT_TRUE(store_->CheckAddChunk(kAddChunk3));
   EXPECT_FALSE(store_->CheckSubChunk(kSubChunk1));
   EXPECT_TRUE(store_->CheckSubChunk(kSubChunk2));
 
   // Delete them, too.
   store_->DeleteAddChunk(kAddChunk2);
+  store_->DeleteAddChunk(kAddChunk3);
   store_->DeleteSubChunk(kSubChunk2);
 
   {
     safe_browsing::PrefixSetBuilder builder;
     std::vector<SBAddFullHash> add_full_hashes_result;
     EXPECT_TRUE(store_->FinishUpdate(&builder, &add_full_hashes_result));
+
+    std::vector<SBPrefix> prefixes_result;
+    builder.GetPrefixSet()->GetPrefixes(&prefixes_result);
+    EXPECT_TRUE(prefixes_result.empty());
+    EXPECT_TRUE(add_full_hashes_result.empty());
   }
 
   // Expect no more chunks.
   ASSERT_TRUE(store_->BeginUpdate());
   EXPECT_FALSE(store_->CheckAddChunk(kAddChunk1));
   EXPECT_FALSE(store_->CheckAddChunk(kAddChunk2));
+  EXPECT_FALSE(store_->CheckAddChunk(kAddChunk3));
   EXPECT_FALSE(store_->CheckSubChunk(kSubChunk1));
   EXPECT_FALSE(store_->CheckSubChunk(kSubChunk2));
 
@@ -606,7 +628,13 @@ TEST_F(SafeBrowsingStoreFileTest, GetAddPrefixesAndHashes) {
   EXPECT_TRUE(store_->CheckAddChunk(kAddChunk1));
   EXPECT_TRUE(store_->WriteAddPrefix(kAddChunk1, kHash1.prefix));
   EXPECT_TRUE(store_->WriteAddPrefix(kAddChunk1, kHash2.prefix));
-  EXPECT_TRUE(store_->WriteAddHash(kAddChunk1, now, kHash2));
+  EXPECT_TRUE(store_->FinishChunk());
+
+  EXPECT_TRUE(store_->BeginChunk());
+  store_->SetAddChunk(kAddChunk2);
+  EXPECT_TRUE(store_->CheckAddChunk(kAddChunk2));
+  EXPECT_TRUE(store_->WriteAddHash(kAddChunk2, now, kHash4));
+  EXPECT_TRUE(store_->FinishChunk());
 
   store_->SetSubChunk(kSubChunk1);
   EXPECT_TRUE(store_->CheckSubChunk(kSubChunk1));
@@ -621,8 +649,9 @@ TEST_F(SafeBrowsingStoreFileTest, GetAddPrefixesAndHashes) {
 
   std::vector<int> chunks;
   store_->GetAddChunks(&chunks);
-  ASSERT_EQ(1U, chunks.size());
+  ASSERT_EQ(2U, chunks.size());
   EXPECT_EQ(kAddChunk1, chunks[0]);
+  EXPECT_EQ(kAddChunk2, chunks[1]);
 
   store_->GetSubChunks(&chunks);
   ASSERT_EQ(1U, chunks.size());
@@ -643,8 +672,8 @@ TEST_F(SafeBrowsingStoreFileTest, GetAddPrefixesAndHashes) {
   std::vector<SBAddFullHash> add_hashes;
   EXPECT_TRUE(store_->GetAddFullHashes(&add_hashes));
   ASSERT_EQ(1U, add_hashes.size());
-  EXPECT_EQ(kAddChunk1, add_hashes[0].chunk_id);
-  EXPECT_TRUE(SBFullHashEqual(kHash2, add_hashes[0].full_hash));
+  EXPECT_EQ(kAddChunk2, add_hashes[0].chunk_id);
+  EXPECT_TRUE(SBFullHashEqual(kHash4, add_hashes[0].full_hash));
 }
 
 // Test that the database handles resharding correctly, both when growing and
@@ -867,4 +896,4 @@ TEST_F(SafeBrowsingStoreFileTest, Version8) {
 }
 #endif
 
-}  // namespace
+}  // namespace safe_browsing
diff --git a/chrome/browser/safe_browsing/safe_browsing_store_unittest.cc b/chrome/browser/safe_browsing/safe_browsing_store_unittest.cc
index 420ed1c..c1214e7 100644
--- a/chrome/browser/safe_browsing/safe_browsing_store_unittest.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_store_unittest.cc
@@ -8,6 +8,51 @@
 
 namespace {
 
+const SBFullHash kHash1 = SBFullHashForString("one");
+const SBFullHash kHash2 = SBFullHashForString("two");
+const SBFullHash kHash3 = SBFullHashForString("three");
+const SBFullHash kHash4 = SBFullHashForString("four");
+const SBFullHash kHash5 = SBFullHashForString("five");
+const SBFullHash kHash6 = SBFullHashForString("six");
+const SBFullHash kHash7 = SBFullHashForString("seven");
+
+const int kAddChunk1 = 1;  // Use different chunk numbers just in case.
+const int kSubChunk1 = 2;
+const int kAddChunk2 = 3;
+const int kSubChunk2 = 4;
+const int kAddChunk3 = 5;
+const int kSubChunk3 = 6;
+const int kAddChunk4 = 7;
+const int kSubChunk4 = 8;
+const int kAddChunk5 = 9;
+const int kSubChunk5 = 10;
+const int kAddChunk6 = 11;
+const int kAddChunk7 = 12;
+
+SBFullHash ModifyHashAfterPrefix(SBFullHash hash, unsigned char mask) {
+  hash.full_hash[sizeof(hash.full_hash) - 1] ^= mask;
+  return hash;
+}
+
+void ProcessHelper(SBAddPrefixes* add_prefixes,
+                   SBSubPrefixes* sub_prefixes,
+                   std::vector<SBAddFullHash>* add_full_hashes,
+                   std::vector<SBSubFullHash>* sub_full_hashes,
+                   const base::hash_set<int32>& add_chunks_deleted,
+                   const base::hash_set<int32>& sub_chunks_deleted) {
+  std::sort(add_prefixes->begin(), add_prefixes->end(),
+            SBAddPrefixLess<SBAddPrefix,SBAddPrefix>);
+  std::sort(sub_prefixes->begin(), sub_prefixes->end(),
+            SBAddPrefixLess<SBSubPrefix,SBSubPrefix>);
+  std::sort(add_full_hashes->begin(), add_full_hashes->end(),
+            SBAddPrefixHashLess<SBAddFullHash,SBAddFullHash>);
+  std::sort(sub_full_hashes->begin(), sub_full_hashes->end(),
+            SBAddPrefixHashLess<SBSubFullHash,SBSubFullHash>);
+
+  SBProcessSubs(add_prefixes, sub_prefixes, add_full_hashes, sub_full_hashes,
+                add_chunks_deleted, sub_chunks_deleted);
+}
+
 TEST(SafeBrowsingStoreTest, SBAddPrefixLess) {
   // prefix dominates.
   EXPECT_TRUE(SBAddPrefixLess(SBAddPrefix(11, 1), SBAddPrefix(10, 2)));
@@ -130,175 +175,122 @@ TEST(SafeBrowsingStoreTest, SBProcessSubsEmpty) {
 // Test that subs knock out adds.
 TEST(SafeBrowsingStoreTest, SBProcessSubsKnockout) {
   const base::Time kNow = base::Time::Now();
-  const SBFullHash kHash1(SBFullHashForString("one"));
-  const SBFullHash kHash2(SBFullHashForString("two"));
-  const SBFullHash kHash3(SBFullHashForString("three"));
-  const SBFullHash kHash4(SBFullHashForString("four"));
-  const int kAddChunk1 = 1;  // Use different chunk numbers just in case.
-  const int kSubChunk1 = 2;
-
-  // Construct some full hashes which share prefix with another.
-  SBFullHash kHash1mod1 = kHash1;
-  kHash1mod1.full_hash[sizeof(kHash1mod1.full_hash) - 1] ++;
-  SBFullHash kHash1mod2 = kHash1mod1;
-  kHash1mod2.full_hash[sizeof(kHash1mod2.full_hash) - 1] ++;
-  SBFullHash kHash1mod3 = kHash1mod2;
-  kHash1mod3.full_hash[sizeof(kHash1mod3.full_hash) - 1] ++;
+
+  // A full hash which shares prefix with another.
+  const SBFullHash kHash1mod = ModifyHashAfterPrefix(kHash1, 1);
 
   // A second full-hash for the full-hash-sub test.
-  SBFullHash kHash4mod = kHash4;
-  kHash4mod.full_hash[sizeof(kHash4mod.full_hash) - 1] ++;
+  const SBFullHash kHash4mod = ModifyHashAfterPrefix(kHash4, 1);
 
   SBAddPrefixes add_prefixes;
   std::vector<SBAddFullHash> add_hashes;
   SBSubPrefixes sub_prefixes;
   std::vector<SBSubFullHash> sub_hashes;
 
-  // An add with prefix and a couple hashes, plus a sub for the prefix
-  // and a couple sub hashes.  The sub should knock all of them out.
-  add_prefixes.push_back(SBAddPrefix(kAddChunk1, kHash1.prefix));
-  add_hashes.push_back(SBAddFullHash(kAddChunk1, kNow, kHash1));
-  add_hashes.push_back(SBAddFullHash(kAddChunk1, kNow, kHash1mod1));
-  sub_prefixes.push_back(SBSubPrefix(kSubChunk1, kAddChunk1, kHash1.prefix));
-  sub_hashes.push_back(SBSubFullHash(kSubChunk1, kAddChunk1, kHash1mod2));
-  sub_hashes.push_back(SBSubFullHash(kSubChunk1, kAddChunk1, kHash1mod3));
+  // An add prefix plus a sub to knock it out.
+  add_prefixes.push_back(SBAddPrefix(kAddChunk1, kHash5.prefix));
+  sub_prefixes.push_back(SBSubPrefix(kSubChunk1, kAddChunk1, kHash5.prefix));
 
-  // An add with no corresponding sub.  Both items should be retained.
-  add_hashes.push_back(SBAddFullHash(kAddChunk1, kNow, kHash2));
-  add_prefixes.push_back(SBAddPrefix(kAddChunk1, kHash2.prefix));
+  // Add hashes with same prefix, plus subs to knock them out.
+  add_hashes.push_back(SBAddFullHash(kAddChunk2, kNow, kHash1));
+  add_hashes.push_back(SBAddFullHash(kAddChunk2, kNow, kHash1mod));
+  sub_hashes.push_back(SBSubFullHash(kSubChunk2, kAddChunk2, kHash1));
+  sub_hashes.push_back(SBSubFullHash(kSubChunk2, kAddChunk2, kHash1mod));
 
-  // A sub with no corresponding add.  Both items should be retained.
-  sub_hashes.push_back(SBSubFullHash(kSubChunk1, kAddChunk1, kHash3));
-  sub_prefixes.push_back(SBSubPrefix(kSubChunk1, kAddChunk1, kHash3.prefix));
+  // Adds with no corresponding sub.  Both items should be retained.
+  add_hashes.push_back(SBAddFullHash(kAddChunk6, kNow, kHash6));
+  add_prefixes.push_back(SBAddPrefix(kAddChunk7, kHash2.prefix));
 
-  // An add with prefix and a couple hashes, plus a sub for one of the
-  // hashes.
-  add_prefixes.push_back(SBAddPrefix(kAddChunk1, kHash4.prefix));
-  add_hashes.push_back(SBAddFullHash(kAddChunk1, kNow, kHash4));
-  add_hashes.push_back(SBAddFullHash(kAddChunk1, kNow, kHash4mod));
-  sub_hashes.push_back(SBSubFullHash(kSubChunk1, kAddChunk1, kHash4mod));
+  // Subs with no corresponding add.  Both items should be retained.
+  sub_hashes.push_back(SBSubFullHash(kSubChunk3, kAddChunk3, kHash7));
+  sub_prefixes.push_back(SBSubPrefix(kSubChunk4, kAddChunk4, kHash3.prefix));
 
-  std::sort(add_prefixes.begin(), add_prefixes.end(),
-            SBAddPrefixLess<SBAddPrefix,SBAddPrefix>);
-  std::sort(sub_prefixes.begin(), sub_prefixes.end(),
-            SBAddPrefixLess<SBSubPrefix,SBSubPrefix>);
-  std::sort(add_hashes.begin(), add_hashes.end(),
-            SBAddPrefixHashLess<SBAddFullHash,SBAddFullHash>);
-  std::sort(sub_hashes.begin(), sub_hashes.end(),
-            SBAddPrefixHashLess<SBSubFullHash,SBSubFullHash>);
+  // Add hashes with the same prefix, with a sub that will knock one of them
+  // out.
+  add_hashes.push_back(SBAddFullHash(kAddChunk5, kNow, kHash4));
+  add_hashes.push_back(SBAddFullHash(kAddChunk5, kNow, kHash4mod));
+  sub_hashes.push_back(SBSubFullHash(kSubChunk5, kAddChunk5, kHash4mod));
 
   const base::hash_set<int32> no_deletions;
-  SBProcessSubs(&add_prefixes, &sub_prefixes, &add_hashes, &sub_hashes,
+  ProcessHelper(&add_prefixes, &sub_prefixes, &add_hashes, &sub_hashes,
                 no_deletions, no_deletions);
 
-  ASSERT_LE(2U, add_prefixes.size());
-  EXPECT_EQ(2U, add_prefixes.size());
-  EXPECT_EQ(kAddChunk1, add_prefixes[0].chunk_id);
-  EXPECT_EQ(kHash4.prefix, add_prefixes[0].prefix);
-  EXPECT_EQ(kAddChunk1, add_prefixes[1].chunk_id);
-  EXPECT_EQ(kHash2.prefix, add_prefixes[1].prefix);
+  ASSERT_EQ(1U, add_prefixes.size());
+  EXPECT_EQ(kAddChunk7, add_prefixes[0].chunk_id);
+  EXPECT_EQ(kHash2.prefix, add_prefixes[0].prefix);
 
-  ASSERT_LE(2U, add_hashes.size());
-  EXPECT_EQ(2U, add_hashes.size());
-  EXPECT_EQ(kAddChunk1, add_hashes[0].chunk_id);
+  ASSERT_EQ(2U, add_hashes.size());
+  EXPECT_EQ(kAddChunk5, add_hashes[0].chunk_id);
   EXPECT_TRUE(SBFullHashEqual(kHash4, add_hashes[0].full_hash));
-  EXPECT_EQ(kAddChunk1, add_hashes[1].chunk_id);
-  EXPECT_TRUE(SBFullHashEqual(kHash2, add_hashes[1].full_hash));
+  EXPECT_EQ(kAddChunk6, add_hashes[1].chunk_id);
+  EXPECT_TRUE(SBFullHashEqual(kHash6, add_hashes[1].full_hash));
 
-  ASSERT_LE(1U, sub_prefixes.size());
-  EXPECT_EQ(1U, sub_prefixes.size());
-  EXPECT_EQ(kSubChunk1, sub_prefixes[0].chunk_id);
-  EXPECT_EQ(kAddChunk1, sub_prefixes[0].add_chunk_id);
+  ASSERT_EQ(1U, sub_prefixes.size());
+  EXPECT_EQ(kSubChunk4, sub_prefixes[0].chunk_id);
+  EXPECT_EQ(kAddChunk4, sub_prefixes[0].add_chunk_id);
   EXPECT_EQ(kHash3.prefix, sub_prefixes[0].add_prefix);
 
-  ASSERT_LE(1U, sub_hashes.size());
-  EXPECT_EQ(1U, sub_hashes.size());
-  EXPECT_EQ(kSubChunk1, sub_hashes[0].chunk_id);
-  EXPECT_EQ(kAddChunk1, sub_hashes[0].add_chunk_id);
-  EXPECT_TRUE(SBFullHashEqual(kHash3, sub_hashes[0].full_hash));
+  ASSERT_EQ(1U, sub_hashes.size());
+  EXPECT_EQ(kSubChunk3, sub_hashes[0].chunk_id);
+  EXPECT_EQ(kAddChunk3, sub_hashes[0].add_chunk_id);
+  EXPECT_TRUE(SBFullHashEqual(kHash7, sub_hashes[0].full_hash));
 }
 
 // Test chunk deletions, and ordering of deletions WRT subs knocking
 // out adds.
 TEST(SafeBrowsingStoreTest, SBProcessSubsDeleteChunk) {
   const base::Time kNow = base::Time::Now();
-  const SBFullHash kHash1(SBFullHashForString("one"));
-  const SBFullHash kHash2(SBFullHashForString("two"));
-  const SBFullHash kHash3(SBFullHashForString("three"));
-  const int kAddChunk1 = 1;  // Use different chunk numbers just in case.
-  const int kSubChunk1 = 2;
-
-  // Construct some full hashes which share prefix with another.
-  SBFullHash kHash1mod1 = kHash1;
-  kHash1mod1.full_hash[sizeof(kHash1mod1.full_hash) - 1] ++;
-  SBFullHash kHash1mod2 = kHash1mod1;
-  kHash1mod2.full_hash[sizeof(kHash1mod2.full_hash) - 1] ++;
-  SBFullHash kHash1mod3 = kHash1mod2;
-  kHash1mod3.full_hash[sizeof(kHash1mod3.full_hash) - 1] ++;
+
+  // A full hash which shares prefix with another.
+  const SBFullHash kHash1mod = ModifyHashAfterPrefix(kHash1, 1);
 
   SBAddPrefixes add_prefixes;
   std::vector<SBAddFullHash> add_hashes;
   SBSubPrefixes sub_prefixes;
   std::vector<SBSubFullHash> sub_hashes;
 
-  // An add with prefix and a couple hashes, plus a sub for the prefix
-  // and a couple sub hashes.  The sub should knock all of them out.
-  add_prefixes.push_back(SBAddPrefix(kAddChunk1, kHash1.prefix));
+
+  // An add prefix plus a sub to knock it out.
+  add_prefixes.push_back(SBAddPrefix(kAddChunk1, kHash5.prefix));
+  sub_prefixes.push_back(SBSubPrefix(kSubChunk1, kAddChunk1, kHash5.prefix));
+
+  // Add hashes with same prefix, plus subs to knock them out.
   add_hashes.push_back(SBAddFullHash(kAddChunk1, kNow, kHash1));
-  add_hashes.push_back(SBAddFullHash(kAddChunk1, kNow, kHash1mod1));
-  sub_prefixes.push_back(SBSubPrefix(kSubChunk1, kAddChunk1, kHash1.prefix));
-  sub_hashes.push_back(SBSubFullHash(kSubChunk1, kAddChunk1, kHash1mod2));
-  sub_hashes.push_back(SBSubFullHash(kSubChunk1, kAddChunk1, kHash1mod3));
+  add_hashes.push_back(SBAddFullHash(kAddChunk1, kNow, kHash1mod));
+  sub_hashes.push_back(SBSubFullHash(kSubChunk1, kAddChunk1, kHash1));
+  sub_hashes.push_back(SBSubFullHash(kSubChunk1, kAddChunk1, kHash1mod));
 
-  // An add with no corresponding sub.  Both items should be retained.
-  add_hashes.push_back(SBAddFullHash(kAddChunk1, kNow, kHash2));
+  // Adds with no corresponding sub.  Both items should be retained.
+  add_hashes.push_back(SBAddFullHash(kAddChunk1, kNow, kHash6));
   add_prefixes.push_back(SBAddPrefix(kAddChunk1, kHash2.prefix));
 
-  // A sub with no corresponding add.  Both items should be retained.
-  sub_hashes.push_back(SBSubFullHash(kSubChunk1, kAddChunk1, kHash3));
+  // Subs with no corresponding add.  Both items should be retained.
+  sub_hashes.push_back(SBSubFullHash(kSubChunk1, kAddChunk1, kHash7));
   sub_prefixes.push_back(SBSubPrefix(kSubChunk1, kAddChunk1, kHash3.prefix));
 
-  std::sort(add_prefixes.begin(), add_prefixes.end(),
-            SBAddPrefixLess<SBAddPrefix,SBAddPrefix>);
-  std::sort(sub_prefixes.begin(), sub_prefixes.end(),
-            SBAddPrefixLess<SBSubPrefix,SBSubPrefix>);
-  std::sort(add_hashes.begin(), add_hashes.end(),
-            SBAddPrefixHashLess<SBAddFullHash,SBAddFullHash>);
-  std::sort(sub_hashes.begin(), sub_hashes.end(),
-            SBAddPrefixHashLess<SBSubFullHash,SBSubFullHash>);
-
+  // Subs apply before being deleted.
   const base::hash_set<int32> no_deletions;
-  base::hash_set<int32> add_deletions;
-  add_deletions.insert(kAddChunk1);
-  SBProcessSubs(&add_prefixes, &sub_prefixes, &add_hashes, &sub_hashes,
-                add_deletions, no_deletions);
-
-  EXPECT_TRUE(add_prefixes.empty());
-  EXPECT_TRUE(add_hashes.empty());
+  base::hash_set<int32> sub_deletions;
+  sub_deletions.insert(kSubChunk1);
+  ProcessHelper(&add_prefixes, &sub_prefixes, &add_hashes, &sub_hashes,
+                no_deletions, sub_deletions);
 
-  EXPECT_EQ(1U, sub_prefixes.size());
-  EXPECT_EQ(kSubChunk1, sub_prefixes[0].chunk_id);
-  EXPECT_EQ(kAddChunk1, sub_prefixes[0].add_chunk_id);
-  EXPECT_EQ(kHash3.prefix, sub_prefixes[0].add_prefix);
+  ASSERT_EQ(1U, add_prefixes.size());
+  EXPECT_EQ(kAddChunk1, add_prefixes[0].chunk_id);
+  EXPECT_EQ(kHash2.prefix, add_prefixes[0].prefix);
 
-  EXPECT_EQ(1U, sub_hashes.size());
-  EXPECT_EQ(kSubChunk1, sub_hashes[0].chunk_id);
-  EXPECT_EQ(kAddChunk1, sub_hashes[0].add_chunk_id);
-  EXPECT_TRUE(SBFullHashEqual(kHash3, sub_hashes[0].full_hash));
+  ASSERT_EQ(1U, add_hashes.size());
+  EXPECT_EQ(kAddChunk1, add_hashes[0].chunk_id);
+  EXPECT_TRUE(SBFullHashEqual(kHash6, add_hashes[0].full_hash));
 
-  std::sort(add_prefixes.begin(), add_prefixes.end(),
-            SBAddPrefixLess<SBAddPrefix,SBAddPrefix>);
-  std::sort(sub_prefixes.begin(), sub_prefixes.end(),
-            SBAddPrefixLess<SBSubPrefix,SBSubPrefix>);
-  std::sort(add_hashes.begin(), add_hashes.end(),
-            SBAddPrefixHashLess<SBAddFullHash,SBAddFullHash>);
-  std::sort(sub_hashes.begin(), sub_hashes.end(),
-            SBAddPrefixHashLess<SBSubFullHash,SBSubFullHash>);
+  EXPECT_TRUE(sub_prefixes.empty());
+  EXPECT_TRUE(sub_hashes.empty());
 
-  base::hash_set<int32> sub_deletions;
-  sub_deletions.insert(kSubChunk1);
-  SBProcessSubs(&add_prefixes, &sub_prefixes, &add_hashes, &sub_hashes,
-                no_deletions, sub_deletions);
+  // Delete the adds, also.
+  base::hash_set<int32> add_deletions;
+  add_deletions.insert(kAddChunk1);
+  ProcessHelper(&add_prefixes, &sub_prefixes, &add_hashes, &sub_hashes,
+                add_deletions, no_deletions);
 
   EXPECT_TRUE(add_prefixes.empty());
   EXPECT_TRUE(add_hashes.empty());
diff --git a/chrome/browser/safe_browsing/safe_browsing_util_unittest.cc b/chrome/browser/safe_browsing/safe_browsing_util_unittest.cc
index 9ac061d..14f0bbf 100644
--- a/chrome/browser/safe_browsing/safe_browsing_util_unittest.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_util_unittest.cc
@@ -16,8 +16,6 @@ bool VectorContains(const std::vector<std::string>& data,
   return std::find(data.begin(), data.end(), str) != data.end();
 }
 
-}
-
 // Tests that we generate the required host/path combinations for testing
 // according to the Safe Browsing spec.
 // See section 6.2 in
@@ -334,3 +332,15 @@ TEST(SafeBrowsingUtilTest, StringToSBFullHashAndSBFullHashToString) {
   std::string hash_final = safe_browsing_util::SBFullHashToString(hash_out);
   EXPECT_EQ(hash_in, hash_final);
 }
+
+TEST(SafeBrowsingUtilTest, FullHashOperators) {
+  const SBFullHash kHash1 = SBFullHashForString("one");
+  const SBFullHash kHash2 = SBFullHashForString("two");
+
+  EXPECT_TRUE(SBFullHashEqual(kHash1, kHash1));
+  EXPECT_TRUE(SBFullHashEqual(kHash2, kHash2));
+  EXPECT_FALSE(SBFullHashEqual(kHash1, kHash2));
+  EXPECT_FALSE(SBFullHashEqual(kHash2, kHash1));
+}
+
+}  // namespace