Safe-browsing PrefixSet cleanups.

chrome/browser/safe_browsing/safe_browsing_database.cc

Index: chrome/browser/safe_browsing/safe_browsing_database.cc
diff --git a/chrome/browser/safe_browsing/safe_browsing_database.cc b/chrome/browser/safe_browsing/safe_browsing_database.cc
index e432edec8c88201d036cdf98602d4879c370438d..133f331346d65ee75456b6a830e59567702375ad 100644
--- a/chrome/browser/safe_browsing/safe_browsing_database.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_database.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -214,6 +214,8 @@ enum PrefixSetEvent {
   PREFIX_SET_EVENT_BLOOM_MISS_PREFIX_HIT,
   PREFIX_SET_EVENT_BLOOM_MISS_PREFIX_HIT_INVALID,
   PREFIX_SET_GETPREFIXES_BROKEN,
+  PREFIX_SET_GETPREFIXES_FIRST_BROKEN,
+  PREFIX_SET_SBPREFIX_WAS_BROKEN,
 
   // Memory space for histograms is determined by the max.  ALWAYS ADD
   // NEW VALUES BEFORE THIS ONE.
@@ -225,6 +227,41 @@ void RecordPrefixSetInfo(PrefixSetEvent event_type) {
                             PREFIX_SET_EVENT_MAX);
 }
 
+// Verify that |GetPrefixes()| returns the same set of prefixes as
+// |prefixes|.  This is necessary so that the
+// PREFIX_SET_EVENT_BLOOM_MISS_PREFIX_HIT_INVALID histogram in
+// ContainsBrowseUrl() can be trustworthy.
+void CheckPrefixSet(const safe_browsing::PrefixSet& prefix_set,
+                    const std::vector<SBPrefix>& prefixes) {
+  std::vector<SBPrefix> restored;
+  prefix_set.GetPrefixes(&restored);
+
+  const std::set<SBPrefix> unique(prefixes.begin(), prefixes.end());
+
+  // Expect them to be equal.
+  if (restored.size() == unique.size() &&
+      std::equal(unique.begin(), unique.end(), restored.begin()))
+    return;
+
+  // Log BROKEN for continuity with previous release, and SIZE to
+  // distinguish which test failed.
+  NOTREACHED();
+  RecordPrefixSetInfo(PREFIX_SET_GETPREFIXES_BROKEN);

[lzheng, 2011/03/18 00:37:34]

When you get here, do you want to see if it is restored.size() != unique.size()
or std::equal(unique.begin(), unique.end(), restored.begin())?

[Scott Hess - ex-Googler, 2011/03/18 02:04:59]

Actually, I had another histogram for this, I can pull it over.

+
+  // Try to distinguish between updates from one broken user and a
+  // distributed problem.
+  static bool logged_broken = false;
+  if (!logged_broken) {
+    RecordPrefixSetInfo(PREFIX_SET_GETPREFIXES_FIRST_BROKEN);
+    logged_broken = true;
+  }
+
+  // This seems so very very unlikely.  But if it ever were true, then
+  // it could explain why GetPrefixes() seemed broken.
+  if (sizeof(int) != sizeof(int32))
+    RecordPrefixSetInfo(PREFIX_SET_SBPREFIX_WAS_BROKEN);
+}
+
 }  // namespace
 
 // The default SafeBrowsingDatabaseFactory.
@@ -973,16 +1010,7 @@ void SafeBrowsingDatabaseNew::UpdateBrowseStore() {
   scoped_ptr<safe_browsing::PrefixSet>
       prefix_set(new safe_browsing::PrefixSet(prefixes));
 
-  // Verify that |GetPrefixes()| returns the same set of prefixes as
-  // was passed to the constructor.
-  std::vector<SBPrefix> restored;
-  prefix_set->GetPrefixes(&restored);
-  prefixes.erase(std::unique(prefixes.begin(), prefixes.end()), prefixes.end());
-  if (restored.size() != prefixes.size() ||
-      !std::equal(prefixes.begin(), prefixes.end(), restored.begin())) {
-    NOTREACHED();
-    RecordPrefixSetInfo(PREFIX_SET_GETPREFIXES_BROKEN);
-  }
+  CheckPrefixSet(*(prefix_set.get()), prefixes);
 
   // This needs to be in sorted order by prefix for efficient access.
   std::sort(add_full_hashes.begin(), add_full_hashes.end(),
@@ -1096,18 +1124,7 @@ void SafeBrowsingDatabaseNew::LoadBloomFilter() {
   }
   std::sort(prefixes.begin(), prefixes.end());
   prefix_set_.reset(new safe_browsing::PrefixSet(prefixes));
-
-  // Double-check the prefixes so that the
-  // PREFIX_SET_EVENT_BLOOM_MISS_PREFIX_HIT_INVALID histogram in
-  // ContainsBrowseUrl() can be trustworthy.
-  std::vector<SBPrefix> restored;
-  prefix_set_->GetPrefixes(&restored);
-  std::set<SBPrefix> unique(prefixes.begin(), prefixes.end());
-  if (restored.size() != unique.size() ||
-      !std::equal(unique.begin(), unique.end(), restored.begin())) {
-    NOTREACHED();
-    RecordPrefixSetInfo(PREFIX_SET_GETPREFIXES_BROKEN);
-  }
+  CheckPrefixSet(*(prefix_set_.get()), prefixes);
 }
 
 bool SafeBrowsingDatabaseNew::Delete() {