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

components/history/core/browser/thumbnail_database.cc

 // Copyright (c) 2012 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 "components/history/core/browser/thumbnail_database.h"
 
 #include <algorithm>
 #include <string>
 
 #include "base/bind.h"
@@ skipping 1023 matching lines @@
         "old_icon_id INTEGER NOT NULL UNIQUE"
         ")";
     if (!db_.Execute(kIconMappingCreate))
       return false;
 
     // 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));
+    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.

     }
   }
 
   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 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";
 
   const char kRenameFaviconsTable[] =
       "ALTER TABLE favicons RENAME TO old_favicons";
   const char kCopyFavicons[] =
       "INSERT INTO favicons (id, url, icon_type) "
       "SELECT mapping.new_icon_id, old.url, old.icon_type "
       "FROM old_favicons AS old "
@@ skipping 41 matching lines @@
     return false;
   }
 
   // Recreate the indices.
   // TODO(shess): UNIQUE indices could fail due to duplication.  This
   // could happen in case of corruption.
   if (!InitIndices(&db_))
     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();
 }
 
 sql::InitStatus ThumbnailDatabase::OpenDatabase(sql::Connection* db,
                                                 const base::FilePath& db_name) {
   size_t startup_kb = 0;
   int64 size_64;
   if (base::GetFileSize(db_name, &size_64))
@@ skipping 182 matching lines @@
   meta_table_.SetVersionNumber(7);
   meta_table_.SetCompatibleVersionNumber(std::min(7, kCompatibleVersionNumber));
   return true;
 }
 
 bool ThumbnailDatabase::IsFaviconDBStructureIncorrect() {
   return !db_.IsSQLValid("SELECT id, url, icon_type FROM favicons");
 }
 
 }  // namespace history