Force WebDatabase version to max(version, compatible version) before migration.

Force WebDatabase version to max(version, compatible version) before migration.


BUG=135342
TEST=WebDatabaseMigrationTest.*


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=145665

[Ivan Korotkov, 2012-07-06 16:05:36]

Hi Peter and Scott,

I guess the easiest way around the AVG hack is to force db version to
last_compatible_version before migration, if version < last_compatible_version
(which should never happen, right?). WDYT?

[Peter Kasting, 2012-07-06 17:05:12]

LGTM as far as this goes, but I think we should also make changes to the message
we display in this case, to suggest that other programs might have corrupted the
profile, and hopefully link to some Google help center article that we can keep
up-to-date with a list of offenders and workarounds.  Can we get the conops
folks to help with this?

http://codereview.chromium.org/10696127/diff/1/chrome/browser/webdata/web_dat...
File chrome/browser/webdata/web_database.cc (right):

http://codereview.chromium.org/10696127/diff/1/chrome/browser/webdata/web_dat...
chrome/browser/webdata/web_database.cc:161: // Ensure that version is not
smaller than the compatible version. This is a
Would this be any clearer?:

  // Some malware tries to force protector to re-sign things by lowering the
  // version number, causing migration to fail.  Ensure the version number is at
  // least as high as the compatible version number.
  int current_version = std::max(meta_table_.GetVersionNumber(),
                                 meta_table_.GetCompatibleVersionNumber());
  if (current_version < meta_table_.GetCompatibleVersionNumber())
    ChangeVersion(&meta_table_, current_version, false);

[Ivan Korotkov, 2012-07-09 10:37:38]

http://codereview.chromium.org/10696127/diff/1/chrome/browser/webdata/web_dat...
File chrome/browser/webdata/web_database.cc (right):

http://codereview.chromium.org/10696127/diff/1/chrome/browser/webdata/web_dat...
chrome/browser/webdata/web_database.cc:161: // Ensure that version is not
smaller than the compatible version. This is a
On 2012/07/06 17:05:12, Peter Kasting wrote:
> Would this be any clearer?:
> 
>   // Some malware tries to force protector to re-sign things by lowering the
>   // version number, causing migration to fail.  Ensure the version number is
at
>   // least as high as the compatible version number.
>   int current_version = std::max(meta_table_.GetVersionNumber(),
>                                  meta_table_.GetCompatibleVersionNumber());
>   if (current_version < meta_table_.GetCompatibleVersionNumber())
>     ChangeVersion(&meta_table_, current_version, false);

Done.

[commit-bot: I haz the power, 2012-07-09 10:39:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ivankr@chromium.org/10696127/1004

[commit-bot: I haz the power, 2012-07-09 14:36:35]

Change committed as 145665

[dhollowa, 2012-07-09 20:26:06]

https://chromiumcodereview.appspot.com/10696127/diff/1004/chrome/browser/webd...
File chrome/browser/webdata/web_database.cc (right):

https://chromiumcodereview.appspot.com/10696127/diff/1004/chrome/browser/webd...
chrome/browser/webdata/web_database.cc:166: if (current_version <
meta_table_.GetCompatibleVersionNumber())
Shouldn't this be:
  if (meta_table_.GetVersionNumber() < meta_table_.GetCompatibleVersionNumber())
...

[Ivan Korotkov, 2012-07-09 20:51:04]

https://chromiumcodereview.appspot.com/10696127/diff/1004/chrome/browser/webd...
File chrome/browser/webdata/web_database.cc (right):

https://chromiumcodereview.appspot.com/10696127/diff/1004/chrome/browser/webd...
chrome/browser/webdata/web_database.cc:166: if (current_version <
meta_table_.GetCompatibleVersionNumber())
On 2012/07/09 20:26:06, dhollowa wrote:
> Shouldn't this be:
>   if (meta_table_.GetVersionNumber() <
meta_table_.GetCompatibleVersionNumber())
> ...

Ouch, you must be right. The test passed because there was a migration to verson
46 which still incremented the version field.

[Peter Kasting, 2012-07-09 21:22:01]

https://chromiumcodereview.appspot.com/10696127/diff/1004/chrome/browser/webd...
File chrome/browser/webdata/web_database.cc (right):

https://chromiumcodereview.appspot.com/10696127/diff/1004/chrome/browser/webd...
chrome/browser/webdata/web_database.cc:166: if (current_version <
meta_table_.GetCompatibleVersionNumber())
On 2012/07/09 20:51:04, Ivan Korotkov wrote:
> On 2012/07/09 20:26:06, dhollowa wrote:
> > Shouldn't this be:
> >   if (meta_table_.GetVersionNumber() <
> meta_table_.GetCompatibleVersionNumber())
> > ...
> 
> Ouch, you must be right. The test passed because there was a migration to
verson
> 46 which still incremented the version field.

I'm sorry for suggesting this incorrect code to Ivan in the first place!

Ivan, you have LGTM in advance from me to fix/land/merge this.  (Feel free to
TBR.)