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

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

Previously the redirects field in TopSitesDatabase was encoded as a
space-seprated list of URLs. Problem arises when data URLs such as
"data:text/plain,this text  has space" gets added. When decoded, non-URLs
like "text" and "has" trigger DCHECK in URL() downstream, thereby crashing
debug Chrome.

In this CL we encode the redirects field as comma separated values (strings).
For decoding, we keeping old space-delimited list as fallback.

We are also migrating Top Sites database from v3 to v4.

BUG=358034

[huangs, 2014-09-09 20:03:32]

huangs@chromium.org changed reviewers:
+ beaudoin@chromium.org

[huangs, 2014-09-09 20:03:33]

PTAL.

[huangs, 2014-09-09 20:52:03]

huangs@chromium.org changed reviewers:
+ shess@chromium.org

[huangs, 2014-09-09 20:52:03]

+shess@, PTAL.

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

https://codereview.chromium.org/560543002/diff/1/chrome/browser/history/top_s...
File chrome/browser/history/top_sites_database.cc (right):

https://codereview.chromium.org/560543002/diff/1/chrome/browser/history/top_s...
chrome/browser/history/top_sites_database.cc:61: static const int kVersionNumber
= 3;
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.

https://codereview.chromium.org/560543002/diff/1/chrome/browser/history/top_s...
chrome/browser/history/top_sites_database.cc:560: csv += '"';
Looks like you escape quotes as two quotes. Your comment should be clarified.

https://codereview.chromium.org/560543002/diff/1/chrome/browser/history/top_s...
chrome/browser/history/top_sites_database.cc:603: return false;
You're strict about the fact that the comma must immediately follow the closing
quote without spaces. You should make that clear.

https://codereview.chromium.org/560543002/diff/1/chrome/browser/history/top_s...
chrome/browser/history/top_sites_database.cc:635:
base::SplitStringAlongWhitespace(encoded_redirects, &redirects_vector);
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.

https://codereview.chromium.org/560543002/diff/1/chrome/browser/history/top_s...
File chrome/browser/history/top_sites_database.h (right):

https://codereview.chromium.org/560543002/diff/1/chrome/browser/history/top_s...
chrome/browser/history/top_sites_database.h:91: // Encodes a vector of strings
as a single string, as a comma separated list
as a single string --> inside a single string

https://codereview.chromium.org/560543002/diff/1/chrome/browser/history/top_s...
chrome/browser/history/top_sites_database.h:92: // of quoted strings. Existing
quotation marks are escaped as "\"\"".
You mean existing quotation marks are escaped by prepending them with a
backslash? Are existing backslashes escaped?

https://codereview.chromium.org/560543002/diff/1/chrome/browser/history/top_s...
chrome/browser/history/top_sites_database.h:100: // Encodes redirects in |url|
as string.
|url|?

https://codereview.chromium.org/560543002/diff/1/chrome/browser/history/top_s...
chrome/browser/history/top_sites_database.h:103: // Decodes a redirects string,
appends the result ot |redirects|.
ot -> to

[Scott Hess - ex-Googler, 2014-09-10 19:33:42]

I agree with the comment about bumping the version number and doing a migration.
 With a mixed fallback system like you're suggesting, it is hard to reason about
the state of databases in the wild.

Also, I'm finding myself dubious on the entire enterprise.  You earlier
suggested that this made sense because it is simple to write - but to my mind
it's a bunch of new code to solve a problem which only comes up for the
intersection of people doing development and people navigating the data: URLs. 
I'm not even sure that data: URLs make sense in this context - if not, we
shouldn't contort ourselves to make them work, we should just drop them.

Let's loop in sky@ offline and see if it makes sense to worry about this.

[huangs, 2014-09-11 01:58:14]

This is a progress update.  Notes:
- The Recovery4 test does not work.  The test is unreasonably sensitive to
changes in TopSites.v4.sql .
- Need to reimplement encoding scheme, depending on consensus on what should be
done.
- Need to get rid of debug output.

PTAL, especially regarding refactoring in tests and overall structure of
migration code.

https://codereview.chromium.org/560543002/diff/1/chrome/browser/history/top_s...
File chrome/browser/history/top_sites_database.cc (right):

https://codereview.chromium.org/560543002/diff/1/chrome/browser/history/top_s...
chrome/browser/history/top_sites_database.cc:61: static const int kVersionNumber
= 3;
The code is backwards compatible for reading.

https://codereview.chromium.org/560543002/diff/1/chrome/browser/history/top_s...
chrome/browser/history/top_sites_database.cc:560: csv += '"';
Comment in .h file updated.

https://codereview.chromium.org/560543002/diff/1/chrome/browser/history/top_s...
chrome/browser/history/top_sites_database.cc:603: return false;
On 2014/09/09 21:32:27, beaudoin wrote:
> You're strict about the fact that the comma must immediately follow the
closing
> quote without spaces. You should make that clear.

Done.

https://codereview.chromium.org/560543002/diff/1/chrome/browser/history/top_s...
chrome/browser/history/top_sites_database.cc:635:
base::SplitStringAlongWhitespace(encoded_redirects, &redirects_vector);
Changing version, but have to maintain backward compatibility for unit tests...

https://codereview.chromium.org/560543002/diff/1/chrome/browser/history/top_s...
File chrome/browser/history/top_sites_database.h (right):

https://codereview.chromium.org/560543002/diff/1/chrome/browser/history/top_s...
chrome/browser/history/top_sites_database.h:91: // Encodes a vector of strings
as a single string, as a comma separated list
On 2014/09/09 21:32:27, beaudoin wrote:
> as a single string --> inside a single string

Done.

https://codereview.chromium.org/560543002/diff/1/chrome/browser/history/top_s...
chrome/browser/history/top_sites_database.h:92: // of quoted strings. Existing
quotation marks are escaped as "\"\"".
Backslash is not involved; we substituted '"' to '""'.

https://codereview.chromium.org/560543002/diff/1/chrome/browser/history/top_s...
chrome/browser/history/top_sites_database.h:100: // Encodes redirects in |url|
as string.
On 2014/09/09 21:32:27, beaudoin wrote:
> |url|?

Done.

https://codereview.chromium.org/560543002/diff/1/chrome/browser/history/top_s...
chrome/browser/history/top_sites_database.h:103: // Decodes a redirects string,
appends the result ot |redirects|.
On 2014/09/09 21:32:27, beaudoin wrote:
> ot -> to

Done.

[huangs, 2015-03-06 18:48:05]

Abandoning this CL, since the main bug was fixed (imperfect solution but at
least no crashing), and proposed CSV method is not gaining support.