Reland 115318 - Return backup TemplateURL on default search change.

Memleak in WebDataService that caused the revert fixed.

BUG=None
TEST=None

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

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

10 files changed, 96 insertions, 36 deletions

diff --git a/chrome/browser/protector/default_search_provider_change.cc b/chrome/browser/protector/default_search_provider_change.cc
index cf702be..75d9957 100644
--- a/chrome/browser/protector/default_search_provider_change.cc
+++ b/chrome/browser/protector/default_search_provider_change.cc
@@ -165,6 +165,8 @@ DefaultSearchProviderChange::DefaultSearchProviderChange(
     // search provider will be used.
     old_id_ = 0;
   }
+  // TODO(avayvod): Keep the URL and delete it later.
+  delete old_url;
 }
 
 DefaultSearchProviderChange::~DefaultSearchProviderChange() {
diff --git a/chrome/browser/search_engines/template_url_service.cc b/chrome/browser/search_engines/template_url_service.cc
index 1b85e63..9886e64 100644
--- a/chrome/browser/search_engines/template_url_service.cc
+++ b/chrome/browser/search_engines/template_url_service.cc
@@ -533,12 +533,13 @@ void TemplateURLService::OnWebDataServiceRequestDone(
   // search may be changed below by Sync which effectively undoes the hijacking.
   bool is_default_search_hijacked = false;
   const TemplateURL* hijacked_default_search_provider = NULL;
-  const TemplateURL* backup_default_search_provider = NULL;
+  scoped_ptr<TemplateURL> backup_default_search_provider;
   // No check is required if the default search is managed.
-  if (!is_default_search_managed_ &&
-      DidDefaultSearchProviderChange(*result,
-                                     template_urls,
-                                     &backup_default_search_provider)) {
+  // |DidDefaultSearchProviderChange| must always be called because it will
+  // take care of the unowned backup default search provider instance.
+  if (DidDefaultSearchProviderChange(*result,
+                                     &backup_default_search_provider) &&
+      !is_default_search_managed_) {
     hijacked_default_search_provider = default_search_provider;
     is_default_search_hijacked = true;
   }
@@ -633,7 +634,7 @@ void TemplateURLService::OnWebDataServiceRequestDone(
       protector::Protector* protector = new protector::Protector(profile());
       protector->ShowChange(protector::CreateDefaultSearchProviderChange(
           hijacked_default_search_provider,
-          backup_default_search_provider));
+          backup_default_search_provider.release()));
     }
   }
 
diff --git a/chrome/browser/search_engines/util.cc b/chrome/browser/search_engines/util.cc
index bee5bac..5bafd43 100644
--- a/chrome/browser/search_engines/util.cc
+++ b/chrome/browser/search_engines/util.cc
@@ -211,10 +211,9 @@ void GetSearchProvidersUsingKeywordResult(
 
 bool DidDefaultSearchProviderChange(
     const WDTypedResult& result,
-    const std::vector<TemplateURL*>& template_urls,
-    const TemplateURL** backup_default_search_provider) {
+    scoped_ptr<TemplateURL>* backup_default_search_provider) {
   DCHECK(backup_default_search_provider);
-  DCHECK(*backup_default_search_provider == NULL);
+  DCHECK(!backup_default_search_provider->get());
   DCHECK_EQ(result.GetType(), KEYWORDS_RESULT);
 
   WDKeywordsResult keyword_result = reinterpret_cast<
@@ -223,9 +222,8 @@ bool DidDefaultSearchProviderChange(
   if (!keyword_result.did_default_search_provider_change)
     return false;
 
-  *backup_default_search_provider = GetTemplateURLByID(
-      template_urls,
-      keyword_result.default_search_provider_id_backup);
+  backup_default_search_provider->reset(
+      keyword_result.default_search_provider_backup);
   return true;
 }
 
diff --git a/chrome/browser/search_engines/util.h b/chrome/browser/search_engines/util.h
index 09f001f..c2ba446 100644
--- a/chrome/browser/search_engines/util.h
+++ b/chrome/browser/search_engines/util.h
@@ -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.
 
@@ -9,6 +9,7 @@
 // This file contains utility functions for search engine functionality.
 #include <vector>
 
+#include "base/memory/scoped_ptr.h"
 #include "base/string16.h"
 
 class PrefService;
@@ -46,7 +47,6 @@ void GetSearchProvidersUsingKeywordResult(
 // lost.
 bool DidDefaultSearchProviderChange(
     const WDTypedResult& result,
-    const std::vector<TemplateURL*>& template_urls,
-    const TemplateURL** backup_default_search_provider);
+    scoped_ptr<TemplateURL>* backup_default_search_provider);
 
 #endif  // CHROME_BROWSER_SEARCH_ENGINES_UTIL_H_
diff --git a/chrome/browser/webdata/keyword_table.cc b/chrome/browser/webdata/keyword_table.cc
index c7e123e..9243902 100644
--- a/chrome/browser/webdata/keyword_table.cc
+++ b/chrome/browser/webdata/keyword_table.cc
@@ -5,6 +5,7 @@
 #include "chrome/browser/webdata/keyword_table.h"
 
 #include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/metrics/histogram.h"
 #include "base/metrics/stats_counters.h"
 #include "base/string_number_conversions.h"
@@ -222,12 +223,40 @@ int64 KeywordTable::GetDefaultSearchProviderID() {
   return value;
 }
 
-int64 KeywordTable::GetDefaultSearchProviderIDBackup() {
+TemplateURL* KeywordTable::GetDefaultSearchProviderBackup() {
   if (!IsBackupSignatureValid())
-    return 0;
-  int64 backup_value = 0;
-  meta_table_->GetValue(kDefaultSearchIDBackupKey, &backup_value);
-  return backup_value;
+    return NULL;
+
+  int64 backup_id = 0;
+  if (!meta_table_->GetValue(kDefaultSearchIDBackupKey, &backup_id)) {
+    LOG(ERROR) << "No default search id backup found.";
+    return NULL;
+  }
+  sql::Statement s(db_->GetUniqueStatement(
+      "SELECT id, short_name, keyword, favicon_url, url, "
+      "safe_for_autoreplace, originating_url, date_created, "
+      "usage_count, input_encodings, show_in_default_list, "
+      "suggest_url, prepopulate_id, autogenerate_keyword, logo_id, "
+      "created_by_policy, instant_url, last_modified, sync_guid "
+      "FROM keywords_backup WHERE id=?"));
+  if (!s) {
+    NOTREACHED() << "Statement prepare failed";
+    return NULL;
+  }
+  s.BindInt64(0, backup_id);
+  if (!s.Step()) {
+    LOG(ERROR) << "No default search provider with backup id.";
+    return NULL;
+  }
+
+  scoped_ptr<TemplateURL> template_url(new TemplateURL());
+  GetURLFromStatement(s, template_url.get());
+
+  if (!s.Succeeded()) {
+    LOG(ERROR) << "Statement has not succeeded.";
+    return NULL;
+  }
+  return template_url.release();
 }
 
 bool KeywordTable::DidDefaultSearchProviderChange() {
@@ -236,7 +265,7 @@ bool KeywordTable::DidDefaultSearchProviderChange() {
         protector::kProtectorHistogramDefaultSearchProvider,
         protector::kProtectorErrorBackupInvalid,
         protector::kProtectorErrorCount);
-    LOG(ERROR) << "Backup signature is invalid";
+    LOG(ERROR) << "Backup signature is invalid.";
     return true;
   }
 
@@ -269,7 +298,7 @@ bool KeywordTable::DidDefaultSearchProviderChange() {
       protector::kProtectorHistogramDefaultSearchProvider,
       protector::kProtectorErrorValueChanged,
       protector::kProtectorErrorCount);
-  LOG(ERROR) << "Default Search Provider is changed.";
+  LOG(WARNING) << "Default Search Provider is changed.";
   return true;
 }
 
diff --git a/chrome/browser/webdata/keyword_table.h b/chrome/browser/webdata/keyword_table.h
index 4914384..504dcd7 100644
--- a/chrome/browser/webdata/keyword_table.h
+++ b/chrome/browser/webdata/keyword_table.h
@@ -107,8 +107,9 @@ class KeywordTable : public WebDatabaseTable {
   bool SetDefaultSearchProviderID(int64 id);
   int64 GetDefaultSearchProviderID();
 
-  // Backup of the default search provider. 0 if the setting can't be verified.
-  int64 GetDefaultSearchProviderIDBackup();
+  // Backup of the default search provider. NULL if the backup is invalid. The
+  // returned TemplateURL is owned by the caller.
+  TemplateURL* GetDefaultSearchProviderBackup();
 
   // Returns true if the default search provider has been changed out under
   // us. This can happen if another process modifies our database or the
diff --git a/chrome/browser/webdata/keyword_table_unittest.cc b/chrome/browser/webdata/keyword_table_unittest.cc
index dcf9bfa..b90dd7f 100644
--- a/chrome/browser/webdata/keyword_table_unittest.cc
+++ b/chrome/browser/webdata/keyword_table_unittest.cc
@@ -195,7 +195,6 @@ TEST_F(KeywordTableTest, DefaultSearchProviderBackup) {
   template_url.set_short_name(ASCIIToUTF16("short_name"));
   template_url.set_keyword(ASCIIToUTF16("keyword"));
   GURL favicon_url("http://favicon.url/");
-  GURL originating_url("http://originating.url/");
   template_url.SetFaviconURL(favicon_url);
   template_url.SetURL("http://url/", 0, 0);
   template_url.set_safe_for_autoreplace(true);
@@ -208,7 +207,17 @@ TEST_F(KeywordTableTest, DefaultSearchProviderBackup) {
   ASSERT_TRUE(db.GetKeywordTable()->SetDefaultSearchProviderID(1));
   EXPECT_TRUE(db.GetKeywordTable()->IsBackupSignatureValid());
   EXPECT_EQ(1, db.GetKeywordTable()->GetDefaultSearchProviderID());
-  EXPECT_EQ(1, db.GetKeywordTable()->GetDefaultSearchProviderIDBackup());
+
+  scoped_ptr<TemplateURL> backup_url(
+      db.GetKeywordTable()->GetDefaultSearchProviderBackup());
+  EXPECT_EQ(1, backup_url->id());
+  EXPECT_EQ(ASCIIToUTF16("short_name"), backup_url->short_name());
+  EXPECT_EQ(ASCIIToUTF16("keyword"), backup_url->keyword());
+  EXPECT_TRUE(favicon_url == backup_url->GetFaviconURL());
+  EXPECT_EQ("http://url/", backup_url->url()->url());
+  EXPECT_TRUE(backup_url->safe_for_autoreplace());
+  EXPECT_TRUE(backup_url->show_in_default_list());
+  EXPECT_EQ("url2", backup_url->suggestions_url()->url());
   EXPECT_FALSE(db.GetKeywordTable()->DidDefaultSearchProviderChange());
 
   // Change the actual setting.
@@ -216,7 +225,16 @@ TEST_F(KeywordTableTest, DefaultSearchProviderBackup) {
       "Default Search Provider ID", 2));
   EXPECT_TRUE(db.GetKeywordTable()->IsBackupSignatureValid());
   EXPECT_EQ(2, db.GetKeywordTable()->GetDefaultSearchProviderID());
-  EXPECT_EQ(1, db.GetKeywordTable()->GetDefaultSearchProviderIDBackup());
+
+  backup_url.reset(db.GetKeywordTable()->GetDefaultSearchProviderBackup());
+  EXPECT_EQ(1, backup_url->id());
+  EXPECT_EQ(ASCIIToUTF16("short_name"), backup_url->short_name());
+  EXPECT_EQ(ASCIIToUTF16("keyword"), backup_url->keyword());
+  EXPECT_TRUE(favicon_url == backup_url->GetFaviconURL());
+  EXPECT_EQ("http://url/", backup_url->url()->url());
+  EXPECT_TRUE(backup_url->safe_for_autoreplace());
+  EXPECT_TRUE(backup_url->show_in_default_list());
+  EXPECT_EQ("url2", backup_url->suggestions_url()->url());
   EXPECT_TRUE(db.GetKeywordTable()->DidDefaultSearchProviderChange());
 
   // Change the backup.
@@ -226,7 +244,7 @@ TEST_F(KeywordTableTest, DefaultSearchProviderBackup) {
       "Default Search Provider ID Backup", 2));
   EXPECT_FALSE(db.GetKeywordTable()->IsBackupSignatureValid());
   EXPECT_EQ(1, db.GetKeywordTable()->GetDefaultSearchProviderID());
-  EXPECT_EQ(0, db.GetKeywordTable()->GetDefaultSearchProviderIDBackup());
+  EXPECT_EQ(NULL, db.GetKeywordTable()->GetDefaultSearchProviderBackup());
   EXPECT_TRUE(db.GetKeywordTable()->DidDefaultSearchProviderChange());
 
   // Change the signature.
@@ -236,7 +254,7 @@ TEST_F(KeywordTableTest, DefaultSearchProviderBackup) {
       "Default Search Provider ID Backup Signature", ""));
   EXPECT_FALSE(db.GetKeywordTable()->IsBackupSignatureValid());
   EXPECT_EQ(1, db.GetKeywordTable()->GetDefaultSearchProviderID());
-  EXPECT_EQ(0, db.GetKeywordTable()->GetDefaultSearchProviderIDBackup());
+  EXPECT_EQ(NULL, db.GetKeywordTable()->GetDefaultSearchProviderBackup());
   EXPECT_TRUE(db.GetKeywordTable()->DidDefaultSearchProviderChange());
 
   // Change keywords.
@@ -246,7 +264,16 @@ TEST_F(KeywordTableTest, DefaultSearchProviderBackup) {
   ASSERT_TRUE(remove_keyword.Run());
   EXPECT_TRUE(db.GetKeywordTable()->IsBackupSignatureValid());
   EXPECT_EQ(1, db.GetKeywordTable()->GetDefaultSearchProviderID());
-  EXPECT_EQ(1, db.GetKeywordTable()->GetDefaultSearchProviderIDBackup());
+
+  backup_url.reset(db.GetKeywordTable()->GetDefaultSearchProviderBackup());
+  EXPECT_EQ(1, backup_url->id());
+  EXPECT_EQ(ASCIIToUTF16("short_name"), backup_url->short_name());
+  EXPECT_EQ(ASCIIToUTF16("keyword"), backup_url->keyword());
+  EXPECT_TRUE(favicon_url == backup_url->GetFaviconURL());
+  EXPECT_EQ("http://url/", backup_url->url()->url());
+  EXPECT_TRUE(backup_url->safe_for_autoreplace());
+  EXPECT_TRUE(backup_url->show_in_default_list());
+  EXPECT_EQ("url2", backup_url->suggestions_url()->url());
   EXPECT_TRUE(db.GetKeywordTable()->DidDefaultSearchProviderChange());
 
   // Change keywords backup.
@@ -256,7 +283,7 @@ TEST_F(KeywordTableTest, DefaultSearchProviderBackup) {
   ASSERT_TRUE(remove_keyword_backup.Run());
   EXPECT_FALSE(db.GetKeywordTable()->IsBackupSignatureValid());
   EXPECT_EQ(1, db.GetKeywordTable()->GetDefaultSearchProviderID());
-  EXPECT_EQ(0, db.GetKeywordTable()->GetDefaultSearchProviderIDBackup());
+  EXPECT_EQ(NULL, db.GetKeywordTable()->GetDefaultSearchProviderBackup());
   EXPECT_TRUE(db.GetKeywordTable()->DidDefaultSearchProviderChange());
 }
 
diff --git a/chrome/browser/webdata/web_data_service.cc b/chrome/browser/webdata/web_data_service.cc
index f260fe9..b23cf90 100644
--- a/chrome/browser/webdata/web_data_service.cc
+++ b/chrome/browser/webdata/web_data_service.cc
@@ -806,10 +806,12 @@ void WebDataService::GetKeywordsImpl(WebDataRequest* request) {
         db_->GetKeywordTable()->GetDefaultSearchProviderID();
     result.builtin_keyword_version =
         db_->GetKeywordTable()->GetBuiltinKeywordVersion();
-    result.default_search_provider_id_backup =
-        db_->GetKeywordTable()->GetDefaultSearchProviderIDBackup();
     result.did_default_search_provider_change =
         db_->GetKeywordTable()->DidDefaultSearchProviderChange();
+    result.default_search_provider_backup =
+        result.did_default_search_provider_change ?
+        db_->GetKeywordTable()->GetDefaultSearchProviderBackup() :
+        NULL;
     request->SetResult(
         new WDResult<WDKeywordsResult>(KEYWORDS_RESULT, result));
   }
diff --git a/chrome/browser/webdata/web_data_service.h b/chrome/browser/webdata/web_data_service.h
index eedc118..61e6fc2 100644
--- a/chrome/browser/webdata/web_data_service.h
+++ b/chrome/browser/webdata/web_data_service.h
@@ -111,8 +111,8 @@ struct WDKeywordsResult {
   int64 default_search_provider_id;
   // Version of the built-in keywords. A value of 0 indicates a first run.
   int builtin_keyword_version;
-  // Backup of the default search provider ID.
-  int64 default_search_provider_id_backup;
+  // Backup of the default search provider. NULL if the backup is invalid.
+  TemplateURL* default_search_provider_backup;
   // Indicates if default search provider has been changed by something
   // other than user's action in the browser.
   bool did_default_search_provider_change;
diff --git a/chrome/browser/webdata/web_data_service_unittest.cc b/chrome/browser/webdata/web_data_service_unittest.cc
index a6e4891..b816772 100644
--- a/chrome/browser/webdata/web_data_service_unittest.cc
+++ b/chrome/browser/webdata/web_data_service_unittest.cc
@@ -731,5 +731,5 @@ TEST_F(WebDataServiceTest, DidDefaultSearchProviderChangeOnNewProfile) {
   KeywordsConsumer::WaitUntilCalled();
   ASSERT_TRUE(consumer.load_succeeded);
   EXPECT_FALSE(consumer.keywords_result.did_default_search_provider_change);
-  EXPECT_EQ(0, consumer.keywords_result.default_search_provider_id_backup);
+  EXPECT_EQ(NULL, consumer.keywords_result.default_search_provider_backup);
 }