Fix bug in history salt computation

Fix bug in history salt computation

Since the salt is stored on disk, also increase the db version number. This will cause a rebuild of the history database.

I added the following warning to clang to make sure we don't make this mistake anywhere else in the codebase:

/Volumes/MacintoshHD2/src/chrome-git/src/chrome/browser/visitedlink/visitedlink_master.cc:968:30: error: argument to 'sizeof' in 'memcpy' call is the same expression as the source; did you mean to provide an explicit length? [-Werror,-Wsizeof-pointer-memaccess]
  memcpy(salt_, salt, sizeof(salt));
                ~~~~         ^~~~
/Volumes/MacintoshHD2/src/chrome-git/src/chrome/browser/visitedlink/visitedlink_master.cc:964:17: note: declared here
    const uint8 salt[LINK_SALT_LENGTH])
                ^

/Volumes/MacintoshHD2/src/chrome-git/src/chrome/common/visitedlink_common.cc:84:31: error: sizeof on array function parameter will return size of 'const uint8 *' (aka 'const unsigned char *') instead of 'const uint8 [8]' [-Werror,-Wsizeof-array-argument]
  MD5Update(&ctx, salt, sizeof(salt));
                              ^
/Volumes/MacintoshHD2/src/chrome-git/src/chrome/common/visitedlink_common.cc:79:17: note: declared here
    const uint8 salt[LINK_SALT_LENGTH]) {
                ^

BUG=25629
TEST=none


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

[brettw, 2011-05-25 22:39:01]

http://codereview.chromium.org/7068017/diff/1/chrome/common/visitedlink_commo...
File chrome/common/visitedlink_common.cc (right):

http://codereview.chromium.org/7068017/diff/1/chrome/common/visitedlink_commo...
chrome/common/visitedlink_common.cc:89: MD5Update(&ctx, salt, sizeof(uint8*));
Hm, it sucks that this is different on 64 and 32 bit systems. This means the
file will be corrupted if you change bitedness, which isn't true for any other
file :(

Maybe we should just regenerate everybody's database file? I'm kind of depressed
about both of these options.

[Nico, 2011-05-25 22:41:56]

http://codereview.chromium.org/7068017/diff/1/chrome/common/visitedlink_commo...
File chrome/common/visitedlink_common.cc (right):

http://codereview.chromium.org/7068017/diff/1/chrome/common/visitedlink_commo...
chrome/common/visitedlink_common.cc:89: MD5Update(&ctx, salt, sizeof(uint8*));
On 2011/05/25 22:39:01, brettw wrote:
> Hm, it sucks that this is different on 64 and 32 bit systems. This means the
> file will be corrupted if you change bitedness, which isn't true for any other
> file :(
> 
> Maybe we should just regenerate everybody's database file? I'm kind of
depressed
> about both of these options.

Tell me what you want (and if it's 2, a three-sentence summary on how to do
that), and I'll make it happen.

2 seems like the Right Thing, and that's usually better in the long run.

[brettw, 2011-05-25 22:52:08]

On Wed, May 25, 2011 at 3:41 PM,  <thakis@chromium.org> wrote:
>
>
http://codereview.chromium.org/7068017/diff/1/chrome/common/visitedlink_commo...
> File chrome/common/visitedlink_common.cc (right):
>
>
http://codereview.chromium.org/7068017/diff/1/chrome/common/visitedlink_commo...
> chrome/common/visitedlink_common.cc:89: MD5Update(&ctx, salt,
> sizeof(uint8*));
> On 2011/05/25 22:39:01, brettw wrote:
>>
>> Hm, it sucks that this is different on 64 and 32 bit systems. This
>
> means the
>>
>> file will be corrupted if you change bitedness, which isn't true for
>
> any other
>>
>> file :(
>
>> Maybe we should just regenerate everybody's database file? I'm kind of
>
> depressed
>>
>> about both of these options.
>
> Tell me what you want (and if it's 2, a three-sentence summary on how to
> do that), and I'll make it happen.
>
> 2 seems like the Right Thing, and that's usually better in the long run.

Theoretically it would work like this: we'd fix the bug and use the
entire hash, and then update the version number at the top of
visitedlink_master.cc. Then everything should magically get
regenerated from history using a new salt and properly using the
entire salt value.

In practice, this needs to be tested :) and also make sure this update
doesn't thrash the disk too much on the first startup after the
upgrade.

Brett

[brettw, 2011-05-25 22:54:10]

On Wed, May 25, 2011 at 3:52 PM, Brett Wilson <brettw@chromium.org> wrote:
> On Wed, May 25, 2011 at 3:41 PM,  <thakis@chromium.org> wrote:
>>
>>
http://codereview.chromium.org/7068017/diff/1/chrome/common/visitedlink_commo...
>> File chrome/common/visitedlink_common.cc (right):
>>
>>
http://codereview.chromium.org/7068017/diff/1/chrome/common/visitedlink_commo...
>> chrome/common/visitedlink_common.cc:89: MD5Update(&ctx, salt,
>> sizeof(uint8*));
>> On 2011/05/25 22:39:01, brettw wrote:
>>>
>>> Hm, it sucks that this is different on 64 and 32 bit systems. This
>>
>> means the
>>>
>>> file will be corrupted if you change bitedness, which isn't true for
>>
>> any other
>>>
>>> file :(
>>
>>> Maybe we should just regenerate everybody's database file? I'm kind of
>>
>> depressed
>>>
>>> about both of these options.
>>
>> Tell me what you want (and if it's 2, a three-sentence summary on how to
>> do that), and I'll make it happen.
>>
>> 2 seems like the Right Thing, and that's usually better in the long run.
>
> Theoretically it would work like this: we'd fix the bug and use the
> entire hash, and then update the version number at the top of
> visitedlink_master.cc. Then everything should magically get
> regenerated from history using a new salt and properly using the
> entire salt value.
>
> In practice, this needs to be tested :) and also make sure this update
> doesn't thrash the disk too much on the first startup after the
> upgrade.

On the plus side, this regeneration does happen in real life sometimes
when we encounter an error, so it *should* be quite stable.

Brett

[Nico, 2011-06-16 21:39:14]

I changed the CL to fix the bug instead, and bumped the DB version. I also fixed
a similar bug in another place. Please take another look.

For testing, I ran chromium with this patch with
--user-data-dir=$MY_NORMAL_CHROME_PROFILE, which has a bit of history data.
Chromium didn't start perceivably slower than usual, and about:history looked
sane. Is there more testing you want me to do?

Since clang now warns about this, further clang rolls are blocked on getting
some kind of resolution to this problem.

[brettw, 2011-06-16 23:43:51]

LGTM, good luck!

[commit-bot: I haz the power, 2011-06-17 02:40:56]

Change committed as 89443