Use higher precision to store the login database's date_created field.

Use higher precision to store the login database's date_created field.

When a password is created or updated, its creation_date is saved in the login
database with precision seconds. The same data is stored in the password sync
server with precision microseconds. As a result, the two passwords always
appear to differ.

I've changed the code so that the creation_date is stored in the login database
with precision microseconds, and I've created a migration to update the
existing login database entries. Prior to the migration, creation_date was a
time_t, which has precision seconds and counts from the UTC epoch. After the
migration, creation_date has precision microseconds, and counts from Chrome's
platform independent epoch.

Note that the very first time passwords are all synced, they will still appear
to differ since the login database lost 6 decimal places of precision. This CL
makes it so that on the second sync (and subsequent syncs), the passwords will
no longer differ.

BUG=408800
Committed: https://crrev.com/5408e45b57a5bbd2c60a863f11b35ad3fe6b97fa
Cr-Commit-Position: refs/heads/master@{#303783}

[erikchen, 2014-11-05 20:41:22]

erikchen@chromium.org changed reviewers:
+ gcasto@chromium.org

[erikchen, 2014-11-05 20:41:22]

gcasto: Please review.

[Garrett Casto, 2014-11-05 22:15:00]

It would be great if you could add a test for this. Perhaps saving a golden
version of the current database, and then verifying that you get the same output
when reading version 8 with base::Time::FromTimeT() and version 9 with
base::Time::FromInternalValue().

https://codereview.chromium.org/702893005/diff/1/components/password_manager/...
File components/password_manager/core/browser/login_database.cc (right):

https://codereview.chromium.org/702893005/diff/1/components/password_manager/...
components/password_manager/core/browser/login_database.cc:266: s.BindInt64(0,
base::Time::kMicrosecondsPerSecond);
It doesn't look like this is safe. Epoch for time_t isn't the same as for
base::Time(). I think that we need to round trip through base::Time() to be safe
(base::Time::FromTimeT().toInternalValue()).

[erikchen, 2014-11-06 01:35:01]

gcasto: PTAL.

I did as you suggested and saved a golden version of the current DB, and wrote a
test to perform the migration from version 8 to the latest version (at least
version 9). I then checked that the dates match up appropriately.

https://codereview.chromium.org/702893005/diff/1/components/password_manager/...
File components/password_manager/core/browser/login_database.cc (right):

https://codereview.chromium.org/702893005/diff/1/components/password_manager/...
components/password_manager/core/browser/login_database.cc:266: s.BindInt64(0,
base::Time::kMicrosecondsPerSecond);
On 2014/11/05 22:14:59, Garrett Casto wrote:
> It doesn't look like this is safe. Epoch for time_t isn't the same as for
> base::Time(). I think that we need to round trip through base::Time() to be
safe
> (base::Time::FromTimeT().toInternalValue()).

You're right. I updated the SQL statement to correctly perform the epoch change.

[Garrett Casto, 2014-11-06 23:17:16]

https://codereview.chromium.org/702893005/diff/20001/components/password_mana...
File components/password_manager/core/browser/login_database.cc (right):

https://codereview.chromium.org/702893005/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database.cc:265: "(date_created *
?) + ?"));
It would be nicer if we didn't have to do the conversion by hand and could use
base::Time::FromTimeT().ToInternalValue() instead. Maybe something like

"SELECT origin_url, username_element, username_value, password_element,
submit_element, signon_ream, date_created FROM logins";

Loop over results:

"UPDATE logins SET date_created = ? WHERE username_element = ? AND ..."

Not as concise, and likely slower, but less likely to improperly convert. We do
have unit tests now so the current approach isn't a deal breaker, but it does
worry me. On the plus side it also means you don't need to get approval from a
base/ OWNER to make base::Time::kTimeTToMicrosecondsOffset public, which you may
get pushback for. WDYT?

https://codereview.chromium.org/702893005/diff/20001/components/password_mana...
File components/password_manager/core/browser/login_database_unittest.cc
(right):

https://codereview.chromium.org/702893005/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database_unittest.cc:974:
.AppendASCII("login_db_v8.sqlite3");
Instead of storing the actual database, can you store the output of .dump and
use CreateDatabaseFromSQL() to recreate? Makes it easier to see what is being
tested.

https://codereview.chromium.org/702893005/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database_unittest.cc:989: int64_t
GetDateCreated(const base::FilePath& db_path) {
Nit: Maybe have 2 elements in the SQL database to make sure that we properly
convert multiple entries?

https://codereview.chromium.org/702893005/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database_unittest.cc:1018:
ASSERT_EQ(1402955745, date_created);
Nit: I don't think that you need to check the actual values here. We only care
that the base::Time values are equivalent.

https://codereview.chromium.org/702893005/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database_unittest.cc:1032:
base::Time::FromTimeT(date_created).ToInternalValue());
Nit: Can you make this compare in base::Time() since that is the format we care
about. So EXPECT_EQ(base::Time::FromInternalValue(new_date_created),
base::Time::FromTimeT(date_created))

[erikchen, 2014-11-07 03:24:04]

PTAL

https://codereview.chromium.org/702893005/diff/20001/components/password_mana...
File components/password_manager/core/browser/login_database.cc (right):

https://codereview.chromium.org/702893005/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database.cc:265: "(date_created *
?) + ?"));
On 2014/11/06 23:17:15, Garrett Casto wrote:
> It would be nicer if we didn't have to do the conversion by hand and could use
> base::Time::FromTimeT().ToInternalValue() instead. Maybe something like
> 
> "SELECT origin_url, username_element, username_value, password_element,
> submit_element, signon_ream, date_created FROM logins";
> 
> Loop over results:
> 
> "UPDATE logins SET date_created = ? WHERE username_element = ? AND ..."
> 
> Not as concise, and likely slower, but less likely to improperly convert. We
do
> have unit tests now so the current approach isn't a deal breaker, but it does
> worry me. On the plus side it also means you don't need to get approval from a
> base/ OWNER to make base::Time::kTimeTToMicrosecondsOffset public, which you
may
> get pushback for. WDYT?

If the value of kTimeTToMicrosecondsOffset changed, or the implicit behavior of
base::Time changed such that the conversion no longer worked, then migrations
performed by this code pre-breakage will behave differently from migrations
performed by this code post-breakage, which violates standard migration sanity
checks. If you prefer, we can copy the value into this file rather than touch
time.h.

As an aside, I looked into possible methods of implementing your suggestion that
would not take a significant performance hit. (I consider running N updates,
where N is the number of rows in the table, to be unacceptably slow). I could
not find a way of performing the migration in an efficient fashion given the
existing table schema.

https://codereview.chromium.org/702893005/diff/20001/components/password_mana...
File components/password_manager/core/browser/login_database_unittest.cc
(right):

https://codereview.chromium.org/702893005/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database_unittest.cc:974:
.AppendASCII("login_db_v8.sqlite3");
On 2014/11/06 23:17:16, Garrett Casto wrote:
> Instead of storing the actual database, can you store the output of .dump and
> use CreateDatabaseFromSQL() to recreate? Makes it easier to see what is being
> tested.

Good call. Done.

https://codereview.chromium.org/702893005/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database_unittest.cc:989: int64_t
GetDateCreated(const base::FilePath& db_path) {
On 2014/11/06 23:17:15, Garrett Casto wrote:
> Nit: Maybe have 2 elements in the SQL database to make sure that we properly
> convert multiple entries? 

Done.

https://codereview.chromium.org/702893005/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database_unittest.cc:1018:
ASSERT_EQ(1402955745, date_created);
On 2014/11/06 23:17:16, Garrett Casto wrote:
> Nit: I don't think that you need to check the actual values here. We only care
> that the base::Time values are equivalent.

I disagree. If we don't check actual values, and someone changes the internals
of time.h (e.g. the value of kTimeTToMicrosecondsOffset), then this test will
still succeed. However, previous migrations will have different behavior from
future migrations, which is an error that we want to catch.

https://codereview.chromium.org/702893005/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database_unittest.cc:1032:
base::Time::FromTimeT(date_created).ToInternalValue());
On 2014/11/06 23:17:15, Garrett Casto wrote:
> Nit: Can you make this compare in base::Time() since that is the format we
care
> about. So EXPECT_EQ(base::Time::FromInternalValue(new_date_created),
> base::Time::FromTimeT(date_created))

Done.

[Garrett Casto, 2014-11-09 09:15:23]

LGTM

Thanks!

https://codereview.chromium.org/702893005/diff/20001/components/password_mana...
File components/password_manager/core/browser/login_database.cc (right):

https://codereview.chromium.org/702893005/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database.cc:265: "(date_created *
?) + ?"));
On 2014/11/07 03:24:04, erikchen wrote:
> On 2014/11/06 23:17:15, Garrett Casto wrote:
> > It would be nicer if we didn't have to do the conversion by hand and could
use
> > base::Time::FromTimeT().ToInternalValue() instead. Maybe something like
> > 
> > "SELECT origin_url, username_element, username_value, password_element,
> > submit_element, signon_ream, date_created FROM logins";
> > 
> > Loop over results:
> > 
> > "UPDATE logins SET date_created = ? WHERE username_element = ? AND ..."
> > 
> > Not as concise, and likely slower, but less likely to improperly convert. We
> do
> > have unit tests now so the current approach isn't a deal breaker, but it
does
> > worry me. On the plus side it also means you don't need to get approval from
a
> > base/ OWNER to make base::Time::kTimeTToMicrosecondsOffset public, which you
> may
> > get pushback for. WDYT?
> 
> If the value of kTimeTToMicrosecondsOffset changed, or the implicit behavior
of
> base::Time changed such that the conversion no longer worked, then migrations
> performed by this code pre-breakage will behave differently from migrations
> performed by this code post-breakage, which violates standard migration sanity
> checks. If you prefer, we can copy the value into this file rather than touch
> time.h.
> 

It's true that if kTimeTToMicroseconds changes the update no longer works, but
that's just generally a problem with serializing the data. We'd be screwed
loading any data at that point. I'm assuming that the representation is stable
already since we, and other databases, depend on it.

I would rather avoid copying the constant unless requested by the base reviewer.

> As an aside, I looked into possible methods of implementing your suggestion
that
> would not take a significant performance hit. (I consider running N updates,
> where N is the number of rows in the table, to be unacceptably slow). I could
> not find a way of performing the migration in an efficient fashion given the
> existing table schema. 

My SQL isn't great but I couldn't think of anything better either. This only
needs to happen once, but unfortunately it's going to happen on the UI thread.
Most people have very small tables and I doubt it would matter either way, but I
don't have a  good sense of how slow this would be for outliers with ~1000 rows.

My philosophical objections can be overruled by practical constraints. I'm okay
with this as is if there are no better alternatives.

https://codereview.chromium.org/702893005/diff/20001/components/password_mana...
File components/password_manager/core/browser/login_database_unittest.cc
(right):

https://codereview.chromium.org/702893005/diff/20001/components/password_mana...
components/password_manager/core/browser/login_database_unittest.cc:1018:
ASSERT_EQ(1402955745, date_created);
On 2014/11/07 03:24:04, erikchen wrote:
> On 2014/11/06 23:17:16, Garrett Casto wrote:
> > Nit: I don't think that you need to check the actual values here. We only
care
> > that the base::Time values are equivalent.
> 
> I disagree. If we don't check actual values, and someone changes the internals
> of time.h (e.g. the value of kTimeTToMicrosecondsOffset), then this test will
> still succeed. However, previous migrations will have different behavior from
> future migrations, which is an error that we want to catch.

Since we generally depend on kTimeTToMicrosecondsOffset being fixed (or more
precisely that FromTimeT() and ToTimeT() are stable), I would think that if we
wanted to verify this it would be it's own test. It's not a huge deal either way
though.

[erikchen, 2014-11-10 18:32:09]

erikchen@chromium.org changed reviewers:
+ blundell@chromium.org, thestig@chromium.org

[erikchen, 2014-11-10 18:32:10]

thestig: Looking for an OWNER review of base/*
blundell: Looking for an OWNER review of components/*

[Lei Zhang, 2014-11-10 20:59:52]

base/ lgtm

[erikchen, 2014-11-11 19:43:55]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-11-11 19:45:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/702893005/40001

[commit-bot: I haz the power, 2014-11-11 20:21:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-11 20:21:40]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[erikchen, 2014-11-11 21:22:17]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-11-11 21:23:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/702893005/60001

[commit-bot: I haz the power, 2014-11-11 22:10:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-11 22:10:31]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[erikchen, 2014-11-12 01:28:56]

The CQ bit was checked by erikchen@chromium.org

[commit-bot: I haz the power, 2014-11-12 01:30:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/702893005/80001

[commit-bot: I haz the power, 2014-11-12 02:58:09]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-11-12 02:58:47]

Patchset 5 (id:??) landed as
https://crrev.com/5408e45b57a5bbd2c60a863f11b35ad3fe6b97fa
Cr-Commit-Position: refs/heads/master@{#303783}

[blundell, 2014-11-12 10:04:00]

lgtm

components_tests.gyp actually has per-file OWNERS of *