Fix ThumbnailDatabase::RetainDataForPageUrls() to drop unretained page mappings

components/history/core/browser/thumbnail_database.cc

Index: components/history/core/browser/thumbnail_database.cc
diff --git a/components/history/core/browser/thumbnail_database.cc b/components/history/core/browser/thumbnail_database.cc
index c115c3cc8ca06ae560759357babc4462f6c51c85..ae3d11f292ce7c16cba346cd0881c32cc7b72380 100644
--- a/components/history/core/browser/thumbnail_database.cc
+++ b/components/history/core/browser/thumbnail_database.cc
@@ -1041,9 +1041,25 @@ bool ThumbnailDatabase::RetainDataForPageUrls(
         "INSERT OR IGNORE INTO temp.icon_id_mapping (old_icon_id) "
         "SELECT icon_id FROM icon_mapping WHERE page_url = ?";
     sql::Statement statement(db_.GetUniqueStatement(kIconMappingSql));
-    for (std::vector<GURL>::const_iterator
-             i = urls_to_keep.begin(); i != urls_to_keep.end(); ++i) {
-      statement.BindString(0, URLDatabase::GURLToDatabaseURL(*i));
+    for (const GURL& url : urls_to_keep) {
+      statement.BindString(0, URLDatabase::GURLToDatabaseURL(url));
+      if (!statement.Run())
+        return false;
+      statement.Reset(true);
+    }
+  }
+
+  {
+    const char kCreateRetainedUrls[] =
+        "CREATE TEMP TABLE retained_urls (url LONGVARCHAR PRIMARY KEY)";
+    if (!db_.Execute(kCreateRetainedUrls))
+      return false;
+
+    const char kRetainedUrlSql[] =
+        "INSERT OR IGNORE INTO temp.retained_urls (url) VALUES (?)";
+    sql::Statement statement(db_.GetUniqueStatement(kRetainedUrlSql));
+    for (const GURL& url : urls_to_keep) {
+      statement.BindString(0, URLDatabase::GURLToDatabaseURL(url));
       if (!statement.Run())
         return false;
       statement.Reset(true);

[Scott Hess - ex-Googler, 2015/03/18 18:27:25]

Given this table, can the earlier loop be rewritten as an execute statement
like:
  INSERT OR IGNORE INTO temp.icon_id_mapping (old_icon_id) SELECT icon_id FROM
icon_mapping JOIN temp.retained_urls ON (temp.retained_urls.url =
icon_mapping.page_url)
That would make it harder to accidentally have these loops diverge.

@@ -1054,8 +1070,10 @@ bool ThumbnailDatabase::RetainDataForPageUrls(
       "ALTER TABLE icon_mapping RENAME TO old_icon_mapping";
   const char kCopyIconMapping[] =
       "INSERT INTO icon_mapping (page_url, icon_id) "
-      "SELECT old.page_url, mapping.new_icon_id "
-      "FROM old_icon_mapping AS old "
+      "SELECT retained_urls.url, mapping.new_icon_id "
+      "FROM temp.retained_urls AS retained_urls "

[Scott Hess - ex-Googler, 2015/03/18 18:27:25]

I don't think the AS adds much, since these lines aren't going to hit the
80-column limit AFAICT.  I think reflecting "This is our constructed temp table"
is worthwhile.

+      "JOIN old_icon_mapping AS old "
+      "ON (retained_urls.url = old.page_url) "
       "JOIN temp.icon_id_mapping AS mapping "
       "ON (old.icon_id = mapping.old_icon_id)";
   const char kDropOldIconMappingTable[] = "DROP TABLE old_icon_mapping";
@@ -1117,7 +1135,8 @@ bool ThumbnailDatabase::RetainDataForPageUrls(
     return false;
 
   const char kIconMappingDrop[] = "DROP TABLE temp.icon_id_mapping";
-  if (!db_.Execute(kIconMappingDrop))
+  const char kRetainedUrlsDrop[] = "DROP TABLE temp.retained_urls";
+  if (!db_.Execute(kIconMappingDrop) || !db_.Execute(kRetainedUrlsDrop))
     return false;
 
   return transaction.Commit();