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

This CL makes always ThumbnailDatabase::RetainDataForPageUrls()  drop
Page URL -> Icon URL mappings for unretained page URLs.

Previously, a Page URL -> Icon URL (e.g. PageURL1 -> IconURLA) mapping was not
dropped for an unretained Page URL (PageURL1) if:
- the favicon data for a different page URL (e.g. PageURL2) was retained
AND
- PageURL2 is mapped to the same Icon URL as the unretained page URL
  (PageURL2 -> IconURLA)

BUG=468353
TEST=ThumbnailDatabaseTest.RetainDataForPageUrls

Review URL: https://codereview.chromium.org/1005783003

Cr-Commit-Position: refs/heads/master@{#321221}

chrome/browser/history/thumbnail_database_unittest.cc

@@ -306,10 +306,15 @@ TEST_F(ThumbnailDatabaseTest, RetainDataForPageUrls) {
 
   db.BeginTransaction();
 
-  // Build a database mapping kPageUrl1 -> kIconUrl1, kPageUrl2 ->
-  // kIconUrl2, kPageUrl3 -> kIconUrl1, and kPageUrl5 -> kIconUrl5.
-  // Then retain kPageUrl1, kPageUrl3, and kPageUrl5.  kPageUrl2
-  // should go away, but the others should be retained correctly.
+  // Build a database mapping
+  // kPageUrl1 -> kIconUrl1
+  // kPageUrl2 -> kIconUrl2
+  // kPageUrl3 -> kIconUrl1
+  // kPageUrl4 -> kIconUrl1
+  // kPageUrl5 -> kIconUrl5
+  // Then retain kPageUrl1, kPageUrl3, and kPageUrl5. kPageUrl2
+  // and kPageUrl4 should go away, but the others should be retained
+  // correctly.
 
   // TODO(shess): This would probably make sense as a golden file.
 
@@ -323,6 +328,7 @@ TEST_F(ThumbnailDatabaseTest, RetainDataForPageUrls) {
   db.AddFaviconBitmap(kept_id1, favicon1, base::Time::Now(), kLargeSize);
   db.AddIconMapping(kPageUrl1, kept_id1);
   db.AddIconMapping(kPageUrl3, kept_id1);
+  db.AddIconMapping(kPageUrl4, kept_id1);
 
   favicon_base::FaviconID unkept_id =
       db.AddFavicon(kIconUrl2, favicon_base::FAVICON);
@@ -367,8 +373,9 @@ TEST_F(ThumbnailDatabaseTest, RetainDataForPageUrls) {
                                sizeof(kBlob2),
                                kBlob2));
 
-  // The one not retained should be missing.
+  // The ones not retained should be missing.
   EXPECT_FALSE(db.GetFaviconIDForFaviconURL(kPageUrl2, false, NULL));
+  EXPECT_FALSE(db.GetFaviconIDForFaviconURL(kPageUrl4, false, NULL));
 
   // Schema should be the same.
   EXPECT_EQ(original_schema, db.db_.GetSchema());

components/history/core/browser/thumbnail_database.cc

@@ -1024,6 +1024,24 @@ bool ThumbnailDatabase::RetainDataForPageUrls(
   if (!transaction.Begin())
     return false;
 
+  // Populate temp.retained_urls with |urls_to_keep|.
+  {
+    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);
+    }
+  }
+
   // temp.icon_id_mapping generates new icon ids as consecutive
   // integers starting from 1, and maps them to the old icon ids.
   {
@@ -1039,23 +1057,21 @@ bool ThumbnailDatabase::RetainDataForPageUrls(
     // Insert the icon ids for retained urls, skipping duplicates.
     const char kIconMappingSql[] =
         "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));
-      if (!statement.Run())
-        return false;
-      statement.Reset(true);
-    }
+        "SELECT icon_id FROM icon_mapping "
+        "JOIN temp.retained_urls "
+        "ON (temp.retained_urls.url = icon_mapping.page_url)";
+    if (!db_.Execute(kIconMappingSql))
+      return false;
   }
 
   const char kRenameIconMappingTable[] =
       "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 temp.retained_urls.url, mapping.new_icon_id "
+      "FROM temp.retained_urls "
+      "JOIN old_icon_mapping AS old "
+      "ON (temp.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 +1133,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();