Fix the issue where a sub chunk coming in before an add chunk, both containg the same prefixes, will lead to the whole hostname looking like it's blocked.

BUG=1358531
Review URL: http://codereview.chromium.org/439

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

3 files changed, 42 insertions, 3 deletions

diff --git a/chrome/browser/safe_browsing/safe_browsing_database.cc b/chrome/browser/safe_browsing/safe_browsing_database.cc
index 518cc8b..0dea6e3 100644
--- a/chrome/browser/safe_browsing/safe_browsing_database.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_database.cc
@@ -17,7 +17,7 @@
 
 // Database version.  If this is different than what's stored on disk, the
 // database is reset.
-static const int kDatabaseVersion = 3;
+static const int kDatabaseVersion = 4;
 
 // Filename suffix for the bloom filter.
 static const wchar_t kBloomFilterFile[] = L" Filter";
diff --git a/chrome/browser/safe_browsing/safe_browsing_util.cc b/chrome/browser/safe_browsing/safe_browsing_util.cc
index 9ddfbb3..8037154 100644
--- a/chrome/browser/safe_browsing/safe_browsing_util.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_util.cc
@@ -414,6 +414,7 @@ void SBHostInfo::Add(const SBEntry* entry) {
 
 void SBHostInfo::AddPrefixes(SBEntry* entry) {
   DCHECK(entry->IsAdd());
+  bool insert_entry = true;
   const SBEntry* sub_entry = NULL;
   // Remove any prefixes for which a sub already came.
   while (GetNextEntry(&sub_entry)) {
@@ -434,8 +435,15 @@ void SBHostInfo::AddPrefixes(SBEntry* entry) {
     // Remove any matching prefixes.
     for (int i = 0; i < sub_entry->prefix_count(); ++i) {
       for (int j = 0; j < entry->prefix_count(); ++j) {
-        if (entry->PrefixesMatch(j, sub_entry, i))
+        if (entry->PrefixesMatch(j, sub_entry, i)) {
           entry->RemovePrefix(j--);
+          if (!entry->prefix_count()) {
+            // The add entry used to have prefixes, but they were all removed
+            // because of matching sub entries.  We don't want to add this
+            // empty add entry, because it would block that entire host.
+            insert_entry = false;
+          }
+        }
       }
     }
 
@@ -443,7 +451,8 @@ void SBHostInfo::AddPrefixes(SBEntry* entry) {
     break;
   }
 
-  Add(entry);
+  if (insert_entry)
+    Add(entry);
   DCHECK(IsValid());
 }
 
diff --git a/chrome/browser/safe_browsing/safe_browsing_util_unittest.cc b/chrome/browser/safe_browsing/safe_browsing_util_unittest.cc
index 0a6f66f..9cb1167 100644
--- a/chrome/browser/safe_browsing/safe_browsing_util_unittest.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_util_unittest.cc
@@ -263,3 +263,33 @@ TEST(SafeBrowsing, HostInfo2) {
   // Any prefix except the one removed should still be blocked.
   EXPECT_TRUE(info.Contains(full_hashes, &list_id, &prefix_hits));
 }
+
+// Checks that if we get a sub chunk with one prefix, then get the add chunk
+// for that same prefix afterwards, the entry becomes empty.
+TEST(SafeBrowsing, HostInfo3) { 
+  SBHostInfo info;
+
+  // Add a sub prefix.
+  SBEntry* entry = SBEntry::Create(SBEntry::SUB_PREFIX, 1);
+  entry->SetPrefixAt(0, 0x01000000);
+  entry->SetChunkIdAtPrefix(0, 1);
+  entry->set_list_id(1);
+  info.RemovePrefixes(entry, true);
+  entry->Destroy();
+
+  int list_id;
+  std::vector<SBFullHash> full_hashes;
+  full_hashes.push_back(CreateFullHash(0x01000000));
+  std::vector<SBPrefix> prefix_hits;
+  EXPECT_FALSE(info.Contains(full_hashes, &list_id, &prefix_hits));
+
+  // Now add the prefix.
+  entry = SBEntry::Create(SBEntry::ADD_PREFIX, 1);
+  entry->SetPrefixAt(0, 0x01000000);
+  entry->set_list_id(1);
+  entry->set_chunk_id(1);
+  info.AddPrefixes(entry);
+  entry->Destroy();
+
+  EXPECT_FALSE(info.Contains(full_hashes, &list_id, &prefix_hits));
+}