Enforce that TemplateURLs' URLs cannot be empty.

This should already have been true in non-test code: the keyword editor, the importer, the extension system, and the search engine autodetection code all tried to ensure that the URLs are non-empty.  However, because we've seen problems occur, this change tries to sanitize the pref, database, and sync inputs to expunge any bogus entries.

We also fix tests, add some more DCHECKs to make this assertion clearer in some of the TemplateURL-adding paths, and then remove conditionals that checked for empty URLs.

This also cleans up a silly design where TemplateURLService listened for changes to the default search provider prefs -- meaning that anyone who tried to write them wound up calling back to the observer as every single pref was written.  This led to reading back bad state.  Eliminating this ought to allow for some other cleanups in TemplateURLService, but I haven't tried to make them here.

BUG=none
TEST=none
Review URL: https://chromiumcodereview.appspot.com/10014033

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

3 files changed, 66 insertions, 6 deletions

diff --git a/chrome/browser/webdata/keyword_table.cc b/chrome/browser/webdata/keyword_table.cc
index 0b59e71..4ecc860 100644
--- a/chrome/browser/webdata/keyword_table.cc
+++ b/chrome/browser/webdata/keyword_table.cc
@@ -4,6 +4,8 @@
 
 #include "chrome/browser/webdata/keyword_table.h"
 
+#include <set>
+
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/metrics/histogram.h"
@@ -149,11 +151,19 @@ bool KeywordTable::GetKeywords(Keywords* keywords) {
                     " FROM keywords ORDER BY id ASC");
   sql::Statement s(db_->GetUniqueStatement(query.c_str()));
 
+  std::set<TemplateURLID> bad_entries;
   while (s.Step()) {
     keywords->push_back(TemplateURLData());
-    GetKeywordDataFromStatement(s, &keywords->back());
+    if (!GetKeywordDataFromStatement(s, &keywords->back())) {
+      bad_entries.insert(s.ColumnInt64(0));
+      keywords->pop_back();
+    }
   }
-  return s.Succeeded();
+  bool succeeded = s.Succeeded();
+  for (std::set<TemplateURLID>::const_iterator i(bad_entries.begin());
+       i != bad_entries.end(); ++i)
+    succeeded &= RemoveKeyword(*i);
+  return succeeded;
 }
 
 bool KeywordTable::UpdateKeyword(const TemplateURL& url) {
@@ -203,7 +213,9 @@ bool KeywordTable::GetDefaultSearchProviderBackup(TemplateURLData* backup) {
     return NULL;
   }
 
-  GetKeywordDataFromStatement(s, backup);
+  if (!GetKeywordDataFromStatement(s, backup))
+    return false;
+
   // ID has no meaning for the backup and should be kInvalidTemplateURLID in
   // case the TemplateURL will be added to keywords if missing.
   backup->id = kInvalidTemplateURLID;
@@ -330,12 +342,18 @@ bool KeywordTable::MigrateToVersion44AddDefaultSearchProviderBackup() {
 }
 
 // static
-void KeywordTable::GetKeywordDataFromStatement(const sql::Statement& s,
+bool KeywordTable::GetKeywordDataFromStatement(const sql::Statement& s,
                                                TemplateURLData* data) {
   DCHECK(data);
   data->short_name = s.ColumnString16(1);
   data->SetKeyword(s.ColumnString16(2));
   data->SetAutogenerateKeyword(s.ColumnBool(13));
+  // Due to past bugs, we might have persisted entries with empty URLs.  Avoid
+  // reading these out.  (GetKeywords() will delete these entries on return.)
+  // NOTE: This code should only be needed as long as we might be reading such
+  // potentially-old data and can be removed afterward.
+  if (s.ColumnString(4).empty())
+    return false;
   data->SetURL(s.ColumnString(4));
   data->suggestions_url = s.ColumnString(11);
   data->instant_url = s.ColumnString(16);
@@ -351,6 +369,7 @@ void KeywordTable::GetKeywordDataFromStatement(const sql::Statement& s,
   data->usage_count = s.ColumnInt(8);
   data->prepopulate_id = s.ColumnInt(12);
   data->sync_guid = s.ColumnString(18);
+  return true;
 }
 
 bool KeywordTable::GetSignatureData(std::string* backup) {
diff --git a/chrome/browser/webdata/keyword_table.h b/chrome/browser/webdata/keyword_table.h
index da62e4c..a5285e7 100644
--- a/chrome/browser/webdata/keyword_table.h
+++ b/chrome/browser/webdata/keyword_table.h
@@ -150,13 +150,16 @@ class KeywordTable : public WebDatabaseTable {
   FRIEND_TEST_ALL_PREFIXES(KeywordTableTest, DefaultSearchProviderBackup);
   FRIEND_TEST_ALL_PREFIXES(KeywordTableTest, GetTableContents);
   FRIEND_TEST_ALL_PREFIXES(KeywordTableTest, GetTableContentsOrdering);
+  FRIEND_TEST_ALL_PREFIXES(KeywordTableTest, SanitizeURLs);
 
   // NOTE: Since the table columns have changed in different versions, many
   // functions below take a |table_version| argument which dictates which
   // version number's column set to use.
 
-  // Fills |data| with the data in |s|.
-  static void GetKeywordDataFromStatement(const sql::Statement& s,
+  // Fills |data| with the data in |s|.  Returns false if we couldn't fill
+  // |data| for some reason, e.g. |s| tried to set one of the fields to an
+  // illegal value.
+  static bool GetKeywordDataFromStatement(const sql::Statement& s,
                                           TemplateURLData* data);
 
   // Returns contents of |keywords_backup| table and default search provider
diff --git a/chrome/browser/webdata/keyword_table_unittest.cc b/chrome/browser/webdata/keyword_table_unittest.cc
index edba9d2..6d49701 100644
--- a/chrome/browser/webdata/keyword_table_unittest.cc
+++ b/chrome/browser/webdata/keyword_table_unittest.cc
@@ -405,3 +405,41 @@ TEST_F(KeywordTableTest, KeywordWithNoFavicon) {
             restored_keyword.safe_for_autoreplace);
   EXPECT_EQ(keyword.id, restored_keyword.id);
 }
+
+TEST_F(KeywordTableTest, SanitizeURLs) {
+  WebDatabase db;
+  ASSERT_EQ(sql::INIT_OK, db.Init(file_));
+  KeywordTable* keyword_table = db.GetKeywordTable();
+
+  TemplateURLData keyword;
+  keyword.short_name = ASCIIToUTF16("legit");
+  keyword.SetKeyword(ASCIIToUTF16("legit"));
+  keyword.SetURL("http://url/");
+  keyword.id = 1000;
+  TemplateURL url(keyword);
+  EXPECT_TRUE(keyword_table->AddKeyword(url));
+
+  keyword.short_name = ASCIIToUTF16("bogus");
+  keyword.SetKeyword(ASCIIToUTF16("bogus"));
+  keyword.id = 2000;
+  TemplateURL url2(keyword);
+  EXPECT_TRUE(keyword_table->AddKeyword(url2));
+
+  KeywordTable::Keywords keywords;
+  EXPECT_TRUE(keyword_table->GetKeywords(&keywords));
+  EXPECT_EQ(2U, keywords.size());
+  keywords.clear();
+
+  // Erase the URL field for the second keyword to simulate having bogus data
+  // previously saved into the database.
+  sql::Statement s(keyword_table->db_->GetUniqueStatement(
+      "UPDATE keywords SET url=? WHERE id=?"));
+  s.BindString16(0, string16());
+  s.BindInt64(1, 2000);
+  EXPECT_TRUE(s.Run());
+  EXPECT_TRUE(keyword_table->UpdateBackupSignature());
+
+  // GetKeywords() should erase the entry with the empty URL field.
+  EXPECT_TRUE(keyword_table->GetKeywords(&keywords));
+  EXPECT_EQ(1U, keywords.size());
+}