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

chrome/browser/history/top_sites_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 "chrome/browser/history/top_sites_database.h"
 
 #include "base/files/file_util.h"
 #include "base/memory/ref_counted.h"
 #include "base/metrics/histogram.h"
 #include "base/strings/string_split.h"
@@ skipping 40 matching lines @@
 
 // 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)
 
 // NOTE(shess): When changing the version, add a new golden file for
 // the new version and a test to verify that Init() works with it.
 // 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;

[beaudoin, 2014/09/09 21:32:27]

Aren't you changing the DB in a way that requires bumping the version number and
adding a migration step? The code here will no longer be able to read a DB
version 3.

[huangs, 2014/09/11 01:58:13]

The code is backwards compatible for reading.

 static const int kDeprecatedVersionNumber = 1;  // and earlier.
 
 bool InitTables(sql::Connection* db) {
   const char kThumbnailsSql[] =
       "CREATE TABLE IF NOT EXISTS thumbnails ("
       "url LONGVARCHAR PRIMARY KEY,"
       "url_rank INTEGER,"
       "title LONGVARCHAR,"
       "thumbnail BLOB,"
       "redirects LONGVARCHAR,"
       "boring_score DOUBLE DEFAULT 1.0,"
       "good_clipping INTEGER DEFAULT 0,"
       "at_top INTEGER DEFAULT 0,"
       "last_updated INTEGER DEFAULT 0,"
       "load_completed INTEGER DEFAULT 0,"
       "last_forced INTEGER DEFAULT 0)";
   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
 // circumstances, so initially the default error response is to leave the
 // existing database in place.  This histogram is intended to expose the
 // failures seen in the fleet.  Frequent failure cases can be explored more
 // deeply to see if the complexity to fix them is warranted.  Infrequent failure
 // cases can be resolved by marking the database unrecoverable (which will
 // delete the data).
 //
@@ skipping 18 matching lines @@
 
   // Track invariants resolved by FixThumbnailsTable().
   RECOVERY_EVENT_INVARIANT_RANK,
   RECOVERY_EVENT_INVARIANT_REDIRECT,
   RECOVERY_EVENT_INVARIANT_CONTIGUOUS,
 
   // Always keep this at the end.
   RECOVERY_EVENT_MAX,
 };
 
+const char kDataUrlPrefix[] = "data:";
+
 void RecordRecoveryEvent(RecoveryEventType recovery_event) {
   UMA_HISTOGRAM_ENUMERATION("History.TopSitesRecovery",
                             recovery_event, RECOVERY_EVENT_MAX);
 }
 
 // Most corruption comes down to atomic updates between pages being broken
 // somehow.  This can result in either missing data, or overlapping data,
 // depending on the operation broken.  This table has large rows, which will use
 // overflow pages, so it is possible (though unlikely) that a chain could fit
 // together and yield a row with errors.
@@ skipping 308 matching lines @@
 
   while (statement.Step()) {
     // Results are sorted by url_rank. For forced thumbnails with url_rank = -1,
     // thumbnails are sorted by last_forced.
     MostVisitedURL url;
     GURL gurl(statement.ColumnString(0));
     url.url = gurl;
     url.title = statement.ColumnString16(2);
     url.last_forced_time =
         base::Time::FromInternalValue(statement.ColumnInt64(10));
-    std::string redirects = statement.ColumnString(4);
-    SetRedirects(redirects, &url);
+    std::string encoded_redirects = statement.ColumnString(4);
+    DecodeRedirects(encoded_redirects, &url.redirects);
     urls->push_back(url);
 
     std::vector<unsigned char> data;
     statement.ColumnBlobAsVector(3, &data);
     Images thumbnail;
     if (!data.empty())
       thumbnail.thumbnail = base::RefCountedBytes::TakeVector(&data);
     thumbnail.thumbnail_score.boring_score = statement.ColumnDouble(5);
     thumbnail.thumbnail_score.good_clipping = statement.ColumnBool(6);
     thumbnail.thumbnail_score.at_top = statement.ColumnBool(7);
@@ skipping 28 matching lines @@
       "UPDATE thumbnails SET "
       "title = ?, thumbnail = ?, redirects = ?, "
       "boring_score = ?, good_clipping = ?, at_top = ?, last_updated = ?, "
       "load_completed = ?, last_forced = ?"
       "WHERE url = ? "));
   statement.BindString16(0, url.title);
   if (thumbnail.thumbnail.get() && thumbnail.thumbnail->front()) {
     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);
   statement.BindBool(5, score.at_top);
   statement.BindInt64(6, score.time_at_snapshot.ToInternalValue());
   statement.BindBool(7, score.load_completed);
   statement.BindInt64(8, url.last_forced_time.ToInternalValue());
   statement.BindString(9, url.url.spec());
 
   return statement.Run();
 }
 
 void TopSitesDatabase::AddPageThumbnail(const MostVisitedURL& url,
                                         int new_rank,
                                         const Images& thumbnail) {
   sql::Statement statement(db_->GetCachedStatement(
       SQL_FROM_HERE,
       "INSERT OR REPLACE INTO thumbnails "
       "(url, url_rank, title, thumbnail, redirects, "
       "boring_score, good_clipping, at_top, last_updated, load_completed, "
       "last_forced) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)"));
   statement.BindString(0, url.url.spec());
   statement.BindInt(1, kRankOfForcedURL);  // Fist make it a forced thumbnail.
   statement.BindString16(2, url.title);
   if (thumbnail.thumbnail.get() && thumbnail.thumbnail->front()) {
     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);
   statement.BindBool(7, score.at_top);
   statement.BindInt64(8, score.time_at_snapshot.ToInternalValue());
   statement.BindBool(9, score.load_completed);
   int64 last_forced = url.last_forced_time.ToInternalValue();
   DCHECK((last_forced == 0) == (new_rank != kRankOfForcedURL))
       << "Thumbnail without a forced time stamp has a forced rank, or the "
       << "opposite.";
   statement.BindInt64(10, last_forced);
   if (!statement.Run())
     return;
 
   // Update rank if this is not a forced thumbnail.
   if (new_rank != kRankOfForcedURL)
     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 += '"';

[beaudoin, 2014/09/09 21:32:27]

Looks like you escape quotes as two quotes. Your comment should be clarified.

[huangs, 2014/09/11 01:58:13]

Comment in .h file updated.

+      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();
+        state = SEEK_QUOTE;
+      } else {
+        return false;

[beaudoin, 2014/09/09 21:32:27]

You're strict about the fact that the comma must immediately follow the closing
quote without spaces. You should make that clear.

[huangs, 2014/09/11 01:58:13]

Done.

+      }
+    } 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);

[beaudoin, 2014/09/09 21:32:27]

I see, so you support DB in the old format too. It's a bit fishy. I'd rather you
bumped the version number and rejected badly formatted entries.

[huangs, 2014/09/11 01:58:13]

Changing version, but have to maintain backward compatibility for unit tests...

+  }
+  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) ==
          (new_rank != kRankOfForcedURL))
       << "Thumbnail without a forced time stamp has a forced rank, or the "
       << "opposite.";
   sql::Transaction transaction(db_.get());
   transaction.Begin();
   UpdatePageRankNoTransaction(url, new_rank);
   transaction.Commit();
@@ skipping 152 matching lines @@
                                     db.get(), db_name));
   db->set_page_size(4096);
   db->set_cache_size(32);
 
   if (!db->Open(db_name))
     return NULL;
   return db.release();
 }
 
 }  // namespace history