[Top Sites] Encoding redirects field in TopSitesDatabase, and adding validations

chrome/browser/history/top_sites_database.cc

Index: chrome/browser/history/top_sites_database.cc
diff --git a/chrome/browser/history/top_sites_database.cc b/chrome/browser/history/top_sites_database.cc
index ff20d4db8e3756beaeec1e264ac13030bced5bce..c25646c6e17fda6a28d8206a02f7c3eca18d08cb 100644
--- a/chrome/browser/history/top_sites_database.cc
+++ b/chrome/browser/history/top_sites_database.cc
@@ -26,8 +26,9 @@
 //                    with the highest rank will be the next one evicted. Forced
 //                    thumbnails have a rank of -1.
 //   title            The title to display under that thumbnail.
-//   redirects        A space separated list of URLs that are known to redirect
-//                    to this url.
+//   redirects        A comma-separated list of URLs that are known to redirect
+//                    to this url. Each URL is surrounded by quotes, and any
+//                    existing quote is escaped to two quotes.
 //   boring_score     How "boring" that thumbnail is. See ThumbnailScore.
 //   good_clipping    True if the thumbnail was clipped from the bottom, keeping
 //                    the entire width of the window. See ThumbnailScore.
@@ -49,6 +50,7 @@ namespace {
 // fatal (in fact, very old data may be expired immediately at startup
 // anyhow).
 
+// Version 4: by huangs@chromium.org
 // Version 3: b6d6a783/r231648 by beaudoin@chromium.org on 2013-10-29
 // Version 2: eb0b24e6/r87284 by satorux@chromium.org on 2011-05-31
 // Version 1: 809cc4d8/r64072 by sky@chromium.org on 2010-10-27 (deprecated)
@@ -58,7 +60,7 @@ namespace {
 // NOTE(shess): RecoverDatabaseOrRaze() depends on the specific
 // version number.  The code is subtle and in development, contact me
 // if the necessary changes are not obvious.
-static const int kVersionNumber = 3;
+static const int kVersionNumber = 4;
 static const int kDeprecatedVersionNumber = 1;  // and earlier.
 
 bool InitTables(sql::Connection* db) {
@@ -78,22 +80,6 @@ bool InitTables(sql::Connection* db) {
   return db->Execute(kThumbnailsSql);
 }
 
-// Encodes redirects into a string.
-std::string GetRedirects(const history::MostVisitedURL& url) {
-  std::vector<std::string> redirects;
-  for (size_t i = 0; i < url.redirects.size(); i++)
-    redirects.push_back(url.redirects[i].spec());
-  return JoinString(redirects, ' ');
-}
-
-// Decodes redirects from a string and sets them for the url.
-void SetRedirects(const std::string& redirects, history::MostVisitedURL* url) {
-  std::vector<std::string> redirects_vector;
-  base::SplitStringAlongWhitespace(redirects, &redirects_vector);
-  for (size_t i = 0; i < redirects_vector.size(); ++i)
-    url->redirects.push_back(GURL(redirects_vector[i]));
-}
-
 // Track various failure (and success) cases in recovery code.
 //
 // TODO(shess): The recovery code is complete, but by nature runs in challenging
@@ -196,7 +182,7 @@ void FixThumbnailsTable(sql::Connection* db) {
 // possible.
 void RecoverDatabaseOrRaze(sql::Connection* db, const base::FilePath& db_path) {
   // NOTE(shess): If the version changes, review this code.
-  DCHECK_EQ(3, kVersionNumber);
+  DCHECK_EQ(4, kVersionNumber);
 
   // It is almost certain that some operation against |db| will fail, prevent
   // reentry.
@@ -237,13 +223,13 @@ void RecoverDatabaseOrRaze(sql::Connection* db, const base::FilePath& db_path) {
   // TODO(shess): Earlier versions have been deprecated, later versions should
   // be impossible.  Unrecoverable() seems like a feasible response if this is
   // infrequent enough.
-  if (version != 2 && version != 3) {
+  if (version != 2 && version != 3 && version != 4) {
     RecordRecoveryEvent(RECOVERY_EVENT_FAILED_META_WRONG_VERSION);
     sql::Recovery::Rollback(recovery.Pass());
     return;
   }
 
-  // Both v2 and v3 recover to current schema version.
+  // v2, v3, and v4 recover to current schema version.
   sql::MetaTable recover_meta_table;
   if (!recover_meta_table.Init(recovery->db(), kVersionNumber,
                                kVersionNumber)) {
@@ -413,6 +399,13 @@ bool TopSitesDatabase::InitImpl(const base::FilePath& db_name) {
     }
   }
 
+  if (meta_table_.GetVersionNumber() == 3) {
+    if (!UpgradeToVersion4()) {
+      LOG(WARNING) << "Unable to upgrade top sites database to version 4.";
+      return false;
+    }
+  }
+
   // Version check.
   if (meta_table_.GetVersionNumber() != kVersionNumber)
     return false;
@@ -435,6 +428,49 @@ bool TopSitesDatabase::UpgradeToVersion3() {
   return true;
 }
 
+bool TopSitesDatabase::UpgradeToVersion4() {
+  LOG(ERROR) << "Migrating to V4";
+
+  // Migrate the "redirects" field from space-separated list in v3 to
+  // comma-separated list of quoted strings in v4.
+  const char kSelectSql[] = "SELECT redirects, rowid FROM thumbnails";
+  sql::Statement select_statement(db_->GetUniqueStatement(kSelectSql));
+
+  const char kUpdateSql[] =
+      "UPDATE thumbnails SET redirects = ? WHERE rowid = ?";
+  sql::Statement update_statement(db_->GetUniqueStatement(kUpdateSql));
+
+  while (select_statement.Step()) {
+    const std::string encoded_redirects_v3(select_statement.ColumnString(0));
+
+    LOG(ERROR) << "Old: " << encoded_redirects_v3;
+    // Skip if empty or if previously migrated
+    if (encoded_redirects_v3.empty() || encoded_redirects_v3[0] == '"')
+      continue;
+
+    // Parse v3 redirects, which is a space-separated list of URLs.
+    std::vector<std::string> redirects_vector;
+    RedirectList redirects;
+    base::SplitStringAlongWhitespace(encoded_redirects_v3, &redirects_vector);
+    for (size_t i = 0; i < redirects_vector.size(); ++i) {
+      GURL redirect_url(redirects_vector[i]);
+      if (redirect_url.is_valid())
+        redirects.push_back(redirect_url);
+    }
+
+    std::string encoded_redirects(EncodeRedirects(redirects));
+    LOG(ERROR) << "New: " << encoded_redirects;
+
+    update_statement.Reset(true);
+    update_statement.BindString(0, encoded_redirects);
+    update_statement.BindInt64(1, select_statement.ColumnInt64(1));
+    update_statement.Run();
+  }
+
+  meta_table_.SetVersionNumber(4);
+  return true;
+}
+
 void TopSitesDatabase::GetPageThumbnails(MostVisitedURLList* urls,
                                          URLToImagesMap* thumbnails) {
   sql::Statement statement(db_->GetCachedStatement(
@@ -460,8 +496,9 @@ void TopSitesDatabase::GetPageThumbnails(MostVisitedURLList* urls,
     url.title = statement.ColumnString16(2);
     url.last_forced_time =
         base::Time::FromInternalValue(statement.ColumnInt64(10));
-    std::string redirects = statement.ColumnString(4);
-    SetRedirects(redirects, &url);
+    LOG(ERROR) << "Read db: redirects = " << statement.ColumnString(4);
+    std::string encoded_redirects = statement.ColumnString(4);
+    DecodeRedirects(encoded_redirects, &url.redirects);
     urls->push_back(url);
 
     std::vector<unsigned char> data;
@@ -510,7 +547,7 @@ bool TopSitesDatabase::UpdatePageThumbnail(
     statement.BindBlob(1, thumbnail.thumbnail->front(),
                        static_cast<int>(thumbnail.thumbnail->size()));
   }
-  statement.BindString(2, GetRedirects(url));
+  statement.BindString(2, EncodeRedirects(url.redirects));
   const ThumbnailScore& score = thumbnail.thumbnail_score;
   statement.BindDouble(3, score.boring_score);
   statement.BindBool(4, score.good_clipping);
@@ -539,7 +576,7 @@ void TopSitesDatabase::AddPageThumbnail(const MostVisitedURL& url,
     statement.BindBlob(3, thumbnail.thumbnail->front(),
                        static_cast<int>(thumbnail.thumbnail->size()));
   }
-  statement.BindString(4, GetRedirects(url));
+  statement.BindString(4, EncodeRedirects(url.redirects));
   const ThumbnailScore& score = thumbnail.thumbnail_score;
   statement.BindDouble(5, score.boring_score);
   statement.BindBool(6, score.good_clipping);
@@ -559,6 +596,103 @@ void TopSitesDatabase::AddPageThumbnail(const MostVisitedURL& url,
     UpdatePageRankNoTransaction(url, new_rank);
 }
 
+// static
+std::string TopSitesDatabase::EncodeCSVString(
+    const std::vector<std::string> str_list) {
+  std::string csv;
+  for (std::vector<std::string>::const_iterator it = str_list.begin();
+       it != str_list.end(); ++it) {
+    const std::string& str = *it;
+    if (it != str_list.begin())
+      csv += ',';
+    csv += '"';
+    for (std::string::const_iterator jt = str.begin(); jt != str.end(); ++jt) {
+      if (*jt == '"')
+        csv += '"';
+      csv += *jt;
+    }
+    csv += '"';
+  }
+  return csv;
+}
+
+// static
+bool TopSitesDatabase::DecodeCSVString(const std::string csv,
+                                       std::vector<std::string>* str_list) {
+  if (csv.empty()) {
+    str_list->clear();
+    return true;
+  }
+
+  enum {
+    SEEK_QUOTE,
+    READ_STRING,
+    SAW_ONE_QUOTE
+  } state = SEEK_QUOTE;
+  std::vector<std::string> out_list;
+  std::string str;
+  for (std::string::const_iterator it = csv.begin(); it != csv.end(); ++it) {
+    const char ch = *it;
+    if (state == SEEK_QUOTE) {
+      if (ch != '"')
+        return false;
+      state = READ_STRING;
+    } else if (state == READ_STRING) {
+      if (ch == '"')
+        state = SAW_ONE_QUOTE;
+      else
+        str += ch;
+    } else if (state == SAW_ONE_QUOTE) {
+      if (ch == '"') {
+        str += ch;
+        state = READ_STRING;
+      } else if (ch == ',') {
+        out_list.push_back(str);
+        str.clear();
+        // For simplicity, disallow spaces around commas.
+        state = SEEK_QUOTE;
+      } else {
+        return false;
+      }
+    } else {
+      NOTREACHED();
+      return false;
+    }
+  }
+  if (state != SAW_ONE_QUOTE)
+    return false;
+  out_list.push_back(str);
+  str_list->swap(out_list);
+  return true;
+}
+
+// static
+std::string TopSitesDatabase::EncodeRedirects(const RedirectList& redirects) {
+  std::vector<std::string> valid_urls;
+  for (size_t i = 0; i < redirects.size(); i++) {
+    // Example of invalid URL that may end up here:
+    // "data:text/plain,this string contains space".
+    if (redirects[i].is_valid())
+      valid_urls.push_back(redirects[i].spec());
+  }
+  return EncodeCSVString(valid_urls);
+}
+
+// static
+void TopSitesDatabase::DecodeRedirects(const std::string& encoded_redirects,
+                                       RedirectList* redirects) {
+  std::vector<std::string> redirects_vector;
+  if (!DecodeCSVString(encoded_redirects, &redirects_vector)) {
+    // Fall back to space-delimited list for backward compatibility.
+    base::SplitStringAlongWhitespace(encoded_redirects, &redirects_vector);
+  }
+  for (size_t i = 0; i < redirects_vector.size(); ++i) {
+    GURL redirect_url(redirects_vector[i]);
+    if (redirect_url.is_valid())
+      redirects->push_back(redirect_url);
+  }
+}
+
 void TopSitesDatabase::UpdatePageRank(const MostVisitedURL& url,
                                       int new_rank) {
   DCHECK((url.last_forced_time.ToInternalValue() == 0) ==