Fixes crasher in TemplateURLModel that occurred when the db had

multiple keywords with the same prepopulate id. We had this for
Hungary.

BUG=3192
TEST=make sure you don't see any problems with keywords.

Review URL: http://codereview.chromium.org/6284

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

5 files changed, 112 insertions, 8 deletions

diff --git a/chrome/browser/template_url_model.cc b/chrome/browser/template_url_model.cc
index c93da96..ca0cedf 100644
--- a/chrome/browser/template_url_model.cc
+++ b/chrome/browser/template_url_model.cc
@@ -602,9 +602,18 @@ void TemplateURLModel::OnWebDataServiceRequestDone(
 
   // Compiler won't convert std::vector<TemplateURL*> to
   // std::vector<const TemplateURL*>.
-  SetTemplateURLs(
+  std::vector<const TemplateURL*> template_urls =
       *reinterpret_cast<std::vector<const TemplateURL*>* >(
-          &keyword_result.keywords));
+          &keyword_result.keywords);
+  const int resource_keyword_version =
+      TemplateURLPrepopulateData::GetDataVersion();
+  if (keyword_result.builtin_keyword_version != resource_keyword_version) {
+    // There should never be duplicate TemplateURLs. We had a bug such that
+    // duplicate TemplateURLs existed for one locale. As such we invoke
+    // RemoveDuplicatePrepopulateIDs to nuke the duplicates.
+    RemoveDuplicatePrepopulateIDs(&template_urls);
+  }
+  SetTemplateURLs(template_urls);
 
   if (keyword_result.default_search_provider_id) {
     // See if we can find the default search provider.
@@ -617,8 +626,6 @@ void TemplateURLModel::OnWebDataServiceRequestDone(
     }
   }
 
-  const int resource_keyword_version =
-      TemplateURLPrepopulateData::GetDataVersion();
   if (keyword_result.builtin_keyword_version != resource_keyword_version) {
     MergeEnginesFromPrepopulateData();
     service_->SetBuiltinKeywordVersion(resource_keyword_version);
@@ -649,6 +656,28 @@ void TemplateURLModel::OnWebDataServiceRequestDone(
   NotifyLoaded();
 }
 
+void TemplateURLModel::RemoveDuplicatePrepopulateIDs(
+    std::vector<const TemplateURL*>* urls) {
+  std::set<int> ids;
+  for (std::vector<const TemplateURL*>::iterator i = urls->begin();
+       i != urls->end(); ) {
+    int prepopulate_id = (*i)->prepopulate_id();
+    if (prepopulate_id) {
+      if (ids.find(prepopulate_id) != ids.end()) {
+        if (service_.get())
+          service_->RemoveKeyword(**i);
+        delete *i;
+        i = urls->erase(i);
+      } else {
+        ids.insert(prepopulate_id);
+        ++i;
+      }
+    } else {
+      ++i;
+    }
+  }
+}
+
 void TemplateURLModel::Observe(NotificationType type,
                                const NotificationSource& source,
                                const NotificationDetails& details) {
@@ -721,6 +750,7 @@ void TemplateURLModel::MergeEnginesFromPrepopulateData() {
         loaded_urls[i]->set_short_name(existing_url->short_name());
       }
       Replace(existing_url, loaded_urls[i]);
+      id_to_turl[t_url->prepopulate_id()] = loaded_urls[i];
     } else {
       Add(loaded_urls[i]);
     }
diff --git a/chrome/browser/template_url_model.h b/chrome/browser/template_url_model.h
index d540f89..8fd65a7 100644
--- a/chrome/browser/template_url_model.h
+++ b/chrome/browser/template_url_model.h
@@ -180,6 +180,12 @@ class TemplateURLModel : public WebDataServiceConsumer,
   virtual void OnWebDataServiceRequestDone(WebDataService::Handle h,
                                            const WDTypedResult* result);
 
+  // Removes (and deletes) TemplateURLs from |urls| that have duplicate
+  // prepopulate ids. Duplicate prepopulate ids are not allowed, but due to a
+  // bug it was possible get dups. This step is only called when the version
+  // number changes.
+  void RemoveDuplicatePrepopulateIDs(std::vector<const TemplateURL*>* urls);
+
   // NotificationObserver method. TemplateURLModel listens for three
   // notification types:
   // . NOTIFY_HISTORY_URL_VISITED: adds keyword search terms if the visit
diff --git a/chrome/browser/template_url_prepopulate_data.cc b/chrome/browser/template_url_prepopulate_data.cc
index 0f8e299..db63148 100644
--- a/chrome/browser/template_url_prepopulate_data.cc
+++ b/chrome/browser/template_url_prepopulate_data.cc
@@ -45,7 +45,7 @@ struct PrepopulatedEngine {
   // to appear for one country (e.g. Live Search U.S. English and Spanish), we
   // must use two different unique IDs (and different keywords).
   //
-  // The following unique IDs are available: 6, 92, 93, 103+
+  // The following unique IDs are available: 92, 93, 103+
   // NOTE: CHANGE THE ABOVE NUMBERS IF YOU ADD A NEW ENGINE; ID conflicts = bad!
   const int id;
 };
@@ -1394,7 +1394,7 @@ const PrepopulatedEngine ok = {
   L"http://ok.hu/katalogus?q={searchTerms}",
   "ISO-8859-2",
   NULL,
-  58,
+  6,
 };
 
 const PrepopulatedEngine onet = {
@@ -2665,6 +2665,8 @@ void GetPrepopulationSetFromGeoID(PrefService* prefs,
                                   size_t* num_engines) {
   // NOTE: This function should ALWAYS set its outparams.
 
+  // If you add a new geo id make sure and update the unit test for coverage.
+
   // GeoIDs are from http://msdn.microsoft.com/en-us/library/ms776390.aspx .
   // Country codes and names are from http://www.geonames.org/countries/ .
   switch (GetGeoIDFromPrefs(prefs)) {
@@ -3004,8 +3006,8 @@ void RegisterUserPrefs(PrefService* prefs) {
 }
 
 int GetDataVersion() {
-  return 15;  // Increment this if you change the above data in ways that mean
-             // users with existing data should get a new version.
+  return 16;  // Increment this if you change the above data in ways that mean
+              // users with existing data should get a new version.
 }
 
 void GetPrepopulatedEngines(PrefService* prefs,
diff --git a/chrome/browser/template_url_prepopulate_data_unittest.cc b/chrome/browser/template_url_prepopulate_data_unittest.cc
new file mode 100644
index 0000000..f7ebfd4
--- /dev/null
+++ b/chrome/browser/template_url_prepopulate_data_unittest.cc
@@ -0,0 +1,58 @@
+// Copyright (c) 2008 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.
+
+#include "chrome/browser/template_url.h"
+#include "chrome/browser/template_url_prepopulate_data.h"
+#include "chrome/common/pref_names.h"
+#include "chrome/test/testing_profile.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+typedef testing::Test TemplateURLPrepopulateDataTest;
+
+// Verifies the set of prepopulate data doesn't contain entries with duplicate
+// ids.
+TEST_F(TemplateURLPrepopulateDataTest, UniqueIDs) {
+  // GEO ids.
+  int ids[] = { 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, 0x8, 0x9, 0xA, 0xB, 0xC,
+                0xE, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, 0x18, 0x19,
+                0x1A, 0x1B, 0x1C, 0x1D, 0x1E, 0x20, 0x22, 0x23, 0x25, 0x26,
+                0x27, 0x28, 0x29, 0x2A, 0x2B, 0x2C, 0x2D, 0x2E, 0x31, 0x32,
+                0x33, 0x36, 0x37, 0x38, 0x39, 0x3B, 0x3D, 0x3E, 0x3F, 0x41,
+                0x42, 0x43, 0x44, 0x45, 0x46, 0x47, 0x48, 0x49, 0x4B, 0x4D,
+                0x4E, 0x50, 0x51, 0x54, 0x56, 0x57, 0x58, 0x59, 0x5A, 0x5B,
+                0x5D, 0x5E, 0x62, 0x63, 0x64, 0x65, 0x67, 0x68, 0x6A, 0x6C,
+                0x6D, 0x6E, 0x6F, 0x71, 0x72, 0x74, 0x75, 0x76, 0x77, 0x79,
+                0x7A, 0x7C, 0x7D, 0x7E, 0x7F, 0x81, 0x82, 0x83, 0x85, 0x86,
+                0x88, 0x89, 0x8A, 0x8B, 0x8C, 0x8D, 0x8E, 0x8F, 0x91, 0x92,
+                0x93, 0x94, 0x95, 0x97, 0x98, 0x9A, 0x9C, 0x9D, 0x9E, 0x9F,
+                0xA0, 0xA2, 0xA3, 0xA4, 0xA5, 0xA6, 0xA7, 0xA8, 0xAD, 0xAE,
+                0xAF, 0xB0, 0xB1, 0xB2, 0xB4, 0xB5, 0xB6, 0xB7, 0xB8, 0xB9,
+                0xBB, 0xBE, 0xBF, 0xC0, 0xC1, 0xC2, 0xC3, 0xC4, 0xC5, 0xC6,
+                0xC7, 0xC8, 0xC9, 0xCA, 0xCB, 0xCC, 0xCD, 0xCE, 0xCF, 0xD0,
+                0xD1, 0xD2, 0xD4, 0xD5, 0xD6, 0xD7, 0xD8, 0xD9, 0xDA, 0xDB,
+                0xDC, 0xDD, 0xDE, 0xDF, 0xE0, 0xE1, 0xE3, 0xE4, 0xE7, 0xE8,
+                0xE9, 0xEA, 0xEB, 0xEC, 0xED, 0xEE, 0xEF, 0xF0, 0xF1, 0xF2,
+                0xF4, 0xF5, 0xF6, 0xF7, 0xF8, 0xF9, 0xFB, 0xFC, 0xFD, 0xFE,
+                0x102, 0x103, 0x104, 0x105, 0x107, 0x108, 0x10D, 0x12C, 0x12D,
+                0x12E, 0x12F, 0x130, 0x131, 0x132, 0x133, 0x134, 0x135, 0x136,
+                0x137, 0x138, 0x139, 0x13A, 0x13B, 0x13D, 0x13E, 0x13F, 0x141,
+                0x142, 0x143, 0x144, 0x145, 0x146, 0x147, 0x148, 0x149, 0x14A,
+                0x14B, 0x14C, 0x14D, 0x14E, 0x14F, 0x150, 0x151, 0x152, 0x153,
+                0x154, 0x155, 0x156, 0x157, 0x15A, 0x15B, 0x15C, 0x15D, 0x15F,
+                0x160, 0x3B16, 0x4CA2, 0x52FA, 0x6F60E7, -1 };
+  TestingProfile profile;
+  for (size_t i = 0; i < arraysize(ids); ++i) {
+    profile.GetPrefs()->SetInteger(prefs::kGeoIDAtInstall, ids[i]);
+    std::vector<TemplateURL*> urls;
+    size_t url_count;
+    TemplateURLPrepopulateData::GetPrepopulatedEngines(
+        profile.GetPrefs(), &urls, &url_count);
+    std::set<int> unique_ids;
+    for (size_t turl_i = 0; turl_i < urls.size(); ++turl_i) {
+      ASSERT_TRUE(unique_ids.find(urls[turl_i]->prepopulate_id()) ==
+                  unique_ids.end());
+      unique_ids.insert(urls[turl_i]->prepopulate_id());
+    }
+  }
+}
diff --git a/chrome/test/unit/unittests.vcproj b/chrome/test/unit/unittests.vcproj
index 0a3260b..df63415 100644
--- a/chrome/test/unit/unittests.vcproj
+++ b/chrome/test/unit/unittests.vcproj
@@ -535,6 +535,14 @@
 			</File>
 		</Filter>
 		<Filter
+			Name="TestTemplateURLPrepopulateData"
+			>
+			<File
+				RelativePath="..\..\browser\template_url_prepopulate_data_unittest.cc"
+				>
+			</File>
+		</Filter>
+		<Filter
 			Name="TestTemplateURL"
 			>
 			<File