Autofill database migration to clean up bogus profiles.

Autofill database migration to clean up bogus profiles.

Adds a |WebDatabase| migration to clean up old profiles.  The profiles are merged in much the same way as when aggregated using form submission.  Profiles that are identical to another, subsets of another, or contain invalid or incomplete email, state, zip, or address get filtered out.

Additionally, the filtered profiles are remembered in a "trash can" mechanism to later reflect these changes in Sync, so that they don't reappear unintentionally.

BUG=65625
TEST=PersonalDataManagerTest.*:AutoFillMergeTest.DataDrivenMergeProfiles:WebDatabaseMigrationTest.*:WebDatabaseTest.*

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

[Ilya Sherman, 2011-03-16 04:58:57]

http://codereview.chromium.org/6676031/diff/1/chrome/browser/autofill/autofil...
File chrome/browser/autofill/autofill_country.cc (right):

http://codereview.chromium.org/6676031/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_country.cc:640: 
nit: This line is extraneous.

http://codereview.chromium.org/6676031/diff/1/chrome/browser/autofill/autofil...
File chrome/browser/autofill/autofill_country.h (right):

http://codereview.chromium.org/6676031/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_country.h:41: static bool IsValidCountry(const
string16& value);
I would prefer not to add this method, as the implementation is a little
expensive.  Perhaps we should also annotate |GetCountryCode()| with a comment
that mentions that calls to the function are a little expensive.

(The expensive part is getting the ICU collator sort key for the string.  It's
not wildly expensive, but it's expensive enough to where we shouldn't do it when
it's easy to avoid.)

http://codereview.chromium.org/6676031/diff/1/chrome/browser/autofill/persona...
File chrome/browser/autofill/personal_data_manager.cc (right):

http://codereview.chromium.org/6676031/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager.cc:88: bool IsValidZip(const
string16& value) {
nit: Perhaps "IsValidUSZip()"?

http://codereview.chromium.org/6676031/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager.cc:272: }
Checking whether a string is a country name is a little expensive, so we should
move this below the call to SetInfo(), as

// Reject profiles with invalid country information.
if (field_type.field_type() == ADDRESS_HOME_COUNTRY &&
    !value.empty()  && imported_profile->CountryCode().empty())
  ...
}

http://codereview.chromium.org/6676031/diff/1/chrome/browser/autofill/persona...
File chrome/browser/autofill/personal_data_manager.h (right):

http://codereview.chromium.org/6676031/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager.h:154: static bool
IsSuitableMergeProfile(const AutofillProfile& profile);
nit: I don't like that this function name includes "Merge" -- it's not really
about merging so much as about implicitly learning.  Perhaps
"IsValidImplicitProfile()" or "IsValidLearnableProfile()" or something like
that?

http://codereview.chromium.org/6676031/diff/1/chrome/browser/autofill/select_...
File chrome/browser/autofill/select_control_handler.h (right):

http://codereview.chromium.org/6676031/diff/1/chrome/browser/autofill/select_...
chrome/browser/autofill/select_control_handler.h:29: bool IsValidState(string16
value);
nit: Perhaps "IsValidUSState()"?

http://codereview.chromium.org/6676031/diff/1/chrome/browser/webdata/web_data...
File chrome/browser/webdata/web_data_service.cc (right):

http://codereview.chromium.org/6676031/diff/1/chrome/browser/webdata/web_data...
chrome/browser/webdata/web_data_service.cc:1124: if
(!db_->EmptyAutofillProfilesTrash()) {
I would have expected that we only empty the trash if
 (a) sync has never been configured; or
 (b) we got a notification from sync indicating that it has processed the remove
request.

Do we not need the extra care for (b)?

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

http://codereview.chromium.org/6676031/diff/1/chrome/browser/webdata/web_data...
chrome/browser/webdata/web_database.cc:168: //                      the phone or
fax number belongs.
nit: phone/fax is unrelated ;)

http://codereview.chromium.org/6676031/diff/1/chrome/browser/webdata/web_data...
chrome/browser/webdata/web_database.cc:2060: return true;
Shouldn't we remove the profile from the trash here?

http://codereview.chromium.org/6676031/diff/1/chrome/browser/webdata/web_data...
File chrome/browser/webdata/web_database_unittest.cc (right):

http://codereview.chromium.org/6676031/diff/1/chrome/browser/webdata/web_data...
chrome/browser/webdata/web_database_unittest.cc:1600:
EXPECT_TRUE(db.AddAutofillGUIDToTrash(profile.guid()));
I would consider this a database error -- we shouldn't ever have a profile both
in the profiles table and in the trash table... should we?

http://codereview.chromium.org/6676031/diff/1/chrome/browser/webdata/web_data...
chrome/browser/webdata/web_database_unittest.cc:3376: // Check that there 6
trashed profile after the merge.
nit: 5 trashed profiles

[Ilya Sherman, 2011-03-16 05:09:52]

I noticed while testing that we don't merge the following two profiles:

John Doe, 1 Nowhere Ln, Stanford, CA, 94305, john.doe@example.com; and
John Doe, 1 Nowhere Ln, Stanford, California, 94305

This is because the first profile's "primary value" includes the email address,
so the two profiles' primary values don't match.  We should probably fix this
bug *before* the migration.

[Ilya Sherman, 2011-03-16 12:10:01]

See also http://code.google.com/p/chromium/issues/detail?id=76359

[dhollowa, 2011-03-16 16:13:35]

http://codereview.chromium.org/6676031/diff/1/chrome/browser/autofill/autofil...
File chrome/browser/autofill/autofill_country.cc (right):

http://codereview.chromium.org/6676031/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_country.cc:640: 
On 2011/03/16 04:58:57, Ilya Sherman wrote:
> nit: This line is extraneous.

Done.

http://codereview.chromium.org/6676031/diff/1/chrome/browser/autofill/autofil...
File chrome/browser/autofill/autofill_country.h (right):

http://codereview.chromium.org/6676031/diff/1/chrome/browser/autofill/autofil...
chrome/browser/autofill/autofill_country.h:41: static bool IsValidCountry(const
string16& value);
On 2011/03/16 04:58:57, Ilya Sherman wrote:
> I would prefer not to add this method, as the implementation is a little
> expensive.  Perhaps we should also annotate |GetCountryCode()| with a comment
> that mentions that calls to the function are a little expensive.
> 
> (The expensive part is getting the ICU collator sort key for the string.  It's
> not wildly expensive, but it's expensive enough to where we shouldn't do it
when
> it's easy to avoid.)

Done.

http://codereview.chromium.org/6676031/diff/1/chrome/browser/autofill/persona...
File chrome/browser/autofill/personal_data_manager.cc (right):

http://codereview.chromium.org/6676031/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager.cc:88: bool IsValidZip(const
string16& value) {
On 2011/03/16 04:58:57, Ilya Sherman wrote:
> nit: Perhaps "IsValidUSZip()"?

I added comment, but I prefer as is.

http://codereview.chromium.org/6676031/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager.cc:272: }
On 2011/03/16 04:58:57, Ilya Sherman wrote:
> Checking whether a string is a country name is a little expensive, so we
should
> move this below the call to SetInfo(), as
> 
> // Reject profiles with invalid country information.
> if (field_type.field_type() == ADDRESS_HOME_COUNTRY &&
>     !value.empty()  && imported_profile->CountryCode().empty())
>   ...
> }

Done.

http://codereview.chromium.org/6676031/diff/1/chrome/browser/autofill/persona...
File chrome/browser/autofill/personal_data_manager.h (right):

http://codereview.chromium.org/6676031/diff/1/chrome/browser/autofill/persona...
chrome/browser/autofill/personal_data_manager.h:154: static bool
IsSuitableMergeProfile(const AutofillProfile& profile);
On 2011/03/16 04:58:57, Ilya Sherman wrote:
> nit: I don't like that this function name includes "Merge" -- it's not really
> about merging so much as about implicitly learning.  Perhaps
> "IsValidImplicitProfile()" or "IsValidLearnableProfile()" or something like
> that?

Done.

http://codereview.chromium.org/6676031/diff/1/chrome/browser/autofill/select_...
File chrome/browser/autofill/select_control_handler.h (right):

http://codereview.chromium.org/6676031/diff/1/chrome/browser/autofill/select_...
chrome/browser/autofill/select_control_handler.h:29: bool IsValidState(string16
value);
On 2011/03/16 04:58:57, Ilya Sherman wrote:
> nit: Perhaps "IsValidUSState()"?

I like as is, but have added a comment.

http://codereview.chromium.org/6676031/diff/1/chrome/browser/webdata/web_data...
File chrome/browser/webdata/web_data_service.cc (right):

http://codereview.chromium.org/6676031/diff/1/chrome/browser/webdata/web_data...
chrome/browser/webdata/web_data_service.cc:1124: if
(!db_->EmptyAutofillProfilesTrash()) {
On 2011/03/16 04:58:57, Ilya Sherman wrote:
> I would have expected that we only empty the trash if
>  (a) sync has never been configured; or
>  (b) we got a notification from sync indicating that it has processed the
remove
> request.
> 
> Do we not need the extra care for (b)?

No, we know at this point that sync is in a "receptive" state i.e.
|ShouldPushChanges| returns true.  The notifications get sent when this is the
case.  And it is "fire and forget".  We assume that sync cleans up its side
correctly.

There is no return notification from sync.

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

http://codereview.chromium.org/6676031/diff/1/chrome/browser/webdata/web_data...
chrome/browser/webdata/web_database.cc:168: //                      the phone or
fax number belongs.
On 2011/03/16 04:58:57, Ilya Sherman wrote:
> nit: phone/fax is unrelated ;)

Done.

http://codereview.chromium.org/6676031/diff/1/chrome/browser/webdata/web_data...
chrome/browser/webdata/web_database.cc:2060: return true;
On 2011/03/16 04:58:57, Ilya Sherman wrote:
> Shouldn't we remove the profile from the trash here?

The trash is emptied by the WDS.  This should be sufficient and the semantics
are somewhat clearer this way, i.e.: a trashed item implies no-op for
add/update/remove.

However, it is conceivable that Sync sees an item deleted on another client that
is also "trashed" here.  In this case it is safer to clear that item from the
trash at this point.

http://codereview.chromium.org/6676031/diff/1/chrome/browser/webdata/web_data...
File chrome/browser/webdata/web_database_unittest.cc (right):

http://codereview.chromium.org/6676031/diff/1/chrome/browser/webdata/web_data...
chrome/browser/webdata/web_database_unittest.cc:1600:
EXPECT_TRUE(db.AddAutofillGUIDToTrash(profile.guid()));
That is correct.  This is just a way to test the interaction.  I've added
comment.

On 2011/03/16 04:58:57, Ilya Sherman wrote:
> I would consider this a database error -- we shouldn't ever have a profile
both
> in the profiles table and in the trash table... should we?

http://codereview.chromium.org/6676031/diff/1/chrome/browser/webdata/web_data...
chrome/browser/webdata/web_database_unittest.cc:3376: // Check that there 6
trashed profile after the merge.
On 2011/03/16 04:58:57, Ilya Sherman wrote:
> nit: 5 trashed profiles

Done.

[lipalani1, 2011-03-16 19:53:38]

The observerlist is not thread safe. We can fix that in our end.

http://codereview.chromium.org/6676031/diff/5018/chrome/browser/autofill/pers...
File chrome/browser/autofill/personal_data_manager.cc (right):

http://codereview.chromium.org/6676031/diff/5018/chrome/browser/autofill/pers...
chrome/browser/autofill/personal_data_manager.cc:881:
web_data_service->EmptyMigrationTrash(true);
This is probably not sufficient. Sorry for not mentioning this in the meeting.
When you call this function sync might not have been initialized yet. What you
need to is this: in the else part
sync_service->AddObserver(this);
Then check for ShouldPushChanges(). If it returns true then call removeobserver
right away and call EmptyMigrationTrash.
If it returns false wait for OnStateChanged.
When you get a OnStateChanged() callback you check to see if(shouldpushchange)
again and if it is true then call emptymigration trash and removeobserver.

[lipalani, 2011-03-16 20:00:30]

Hi,
after talking to Tim it looks like this class is UI thread only class. We
cannot make it threadsafe.
In fact making it work across multiple threads would be a huge deal. Can you
confirm if the call to ShouldPushChanges is currently happening on the UI
thread? If not can you proxy it over.
Thanks!
regards,
Lingesh

On Wed, Mar 16, 2011 at 12:53 PM, <lipalani@chromium.org> wrote:

> The observerlist is not thread safe. We can fix that in our end.
>
>
>
>
http://codereview.chromium.org/6676031/diff/5018/chrome/browser/autofill/pers...
>
> File chrome/browser/autofill/personal_data_manager.cc (right):
>
>
>
http://codereview.chromium.org/6676031/diff/5018/chrome/browser/autofill/pers...
> chrome/browser/autofill/personal_data_manager.cc:881:
> web_data_service->EmptyMigrationTrash(true);
> This is probably not sufficient. Sorry for not mentioning this in the
> meeting. When you call this function sync might not have been
> initialized yet. What you need to is this: in the else part
> sync_service->AddObserver(this);
> Then check for ShouldPushChanges(). If it returns true then call
> removeobserver right away and call EmptyMigrationTrash.
> If it returns false wait for OnStateChanged.
> When you get a OnStateChanged() callback you check to see
> if(shouldpushchange) again and if it is true then call emptymigration
> trash and removeobserver.
>
>
> http://codereview.chromium.org/6676031/
>

[dhollowa, 2011-03-16 20:09:15]

Yes, I only ever call |ShouldPushChanges| on the UI thread.

On 2011/03/16 20:00:30, lipalani wrote:
> Hi,
> after talking to Tim it looks like this class is UI thread only class. We
> cannot make it threadsafe.
> In fact making it work across multiple threads would be a huge deal. Can you
> confirm if the call to ShouldPushChanges is currently happening on the UI
> thread? If not can you proxy it over.
> Thanks!
> regards,
> Lingesh
> 
> On Wed, Mar 16, 2011 at 12:53 PM, <mailto:lipalani@chromium.org> wrote:
> 
> > The observerlist is not thread safe. We can fix that in our end.
> >
> >
> >
> >
>
http://codereview.chromium.org/6676031/diff/5018/chrome/browser/autofill/pers...
> >
> > File chrome/browser/autofill/personal_data_manager.cc (right):
> >
> >
> >
>
http://codereview.chromium.org/6676031/diff/5018/chrome/browser/autofill/pers...
> > chrome/browser/autofill/personal_data_manager.cc:881:
> > web_data_service->EmptyMigrationTrash(true);
> > This is probably not sufficient. Sorry for not mentioning this in the
> > meeting. When you call this function sync might not have been
> > initialized yet. What you need to is this: in the else part
> > sync_service->AddObserver(this);
> > Then check for ShouldPushChanges(). If it returns true then call
> > removeobserver right away and call EmptyMigrationTrash.
> > If it returns false wait for OnStateChanged.
> > When you get a OnStateChanged() callback you check to see
> > if(shouldpushchange) again and if it is true then call emptymigration
> > trash and removeobserver.
> >
> >
> > http://codereview.chromium.org/6676031/
> >

[dhollowa, 2011-03-16 22:42:49]

http://codereview.chromium.org/6676031/diff/5018/chrome/browser/autofill/pers...
File chrome/browser/autofill/personal_data_manager.cc (right):

http://codereview.chromium.org/6676031/diff/5018/chrome/browser/autofill/pers...
chrome/browser/autofill/personal_data_manager.cc:881:
web_data_service->EmptyMigrationTrash(true);
As discussed, the sync service was being notified by user subsequent user
manipulation of the profiles (triggered from PersonalDataManager::UpdateProfile
and PersonalDataManager::ReceiveLoadedProfiles) so this would have been fine.

But a superior mechanism, suggested by Lingesh here, is to empty the trash on
response to sync notifying its observers that it has changed state.  This
empties the trash earlier in the startup.

So I've changed the implementation to that.  I'm still kicking the first attempt
off when the profiles are loaded initially.

On 2011/03/16 19:53:38, lipalani1 wrote:
> This is probably not sufficient. Sorry for not mentioning this in the meeting.
> When you call this function sync might not have been initialized yet. What you
> need to is this: in the else part
> sync_service->AddObserver(this);
> Then check for ShouldPushChanges(). If it returns true then call
removeobserver
> right away and call EmptyMigrationTrash.
> If it returns false wait for OnStateChanged.
> When you get a OnStateChanged() callback you check to see if(shouldpushchange)
> again and if it is true then call emptymigration trash and removeobserver.

[lipalani1, 2011-03-16 22:46:45]

LGTM from sync side!!!

[Ilya Sherman, 2011-03-16 23:07:15]

http://codereview.chromium.org/6676031/diff/5021/chrome/browser/autofill/auto...
File chrome/browser/autofill/autofill_profile.cc (left):

http://codereview.chromium.org/6676031/diff/5021/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_profile.cc:315:
GetFieldText(AutofillType(EMAIL_ADDRESS));
This isn't the right fix for not merging with empty emails.  For one, the same
problem exists with empty names.  Moreover, we added the email address to the
"primary value" because we got a lot of pushback when we started automatically
merging email addresses --
http://code.google.com/p/chromium/issues/detail?id=64669

I think it would be better to address this in a separate cl, for clarity's sake.
 However, if we want to cover this case in the migration that *this* CL
implements, then we should make sure to commit the other CL first.  I think it's
actually ok to just not worry about it for now -- we're going to do another
migration later, so it should be fine to handle edge cases like this one at that
time.

http://codereview.chromium.org/6676031/diff/5021/chrome/browser/autofill/pers...
File chrome/browser/autofill/personal_data_manager.cc (right):

http://codereview.chromium.org/6676031/diff/5021/chrome/browser/autofill/pers...
chrome/browser/autofill/personal_data_manager.cc:14: #include
"chrome/browser/autofill/autofill_country.h"
nit: No longer needed.

http://codereview.chromium.org/6676031/diff/5021/chrome/browser/autofill/pers...
chrome/browser/autofill/personal_data_manager.cc:184: // |EmptyMigrationTrash|
once the sync service becomes available.
We already call into EmptyMigrationTrash() at various intervals -- what does
adding this method buy us?

[dhollowa, 2011-03-17 00:02:41]

http://codereview.chromium.org/6676031/diff/5021/chrome/browser/autofill/auto...
File chrome/browser/autofill/autofill_profile.cc (left):

http://codereview.chromium.org/6676031/diff/5021/chrome/browser/autofill/auto...
chrome/browser/autofill/autofill_profile.cc:315:
GetFieldText(AutofillType(EMAIL_ADDRESS));
Yes, true.  Will address later.

On 2011/03/16 23:07:16, Ilya Sherman wrote:
> This isn't the right fix for not merging with empty emails.  For one, the same
> problem exists with empty names.  Moreover, we added the email address to the
> "primary value" because we got a lot of pushback when we started automatically
> merging email addresses --
> http://code.google.com/p/chromium/issues/detail?id=64669
> 
> I think it would be better to address this in a separate cl, for clarity's
sake.
>  However, if we want to cover this case in the migration that *this* CL
> implements, then we should make sure to commit the other CL first.  I think
it's
> actually ok to just not worry about it for now -- we're going to do another
> migration later, so it should be fine to handle edge cases like this one at
that
> time.
>

http://codereview.chromium.org/6676031/diff/5021/chrome/browser/autofill/pers...
File chrome/browser/autofill/personal_data_manager.cc (right):

http://codereview.chromium.org/6676031/diff/5021/chrome/browser/autofill/pers...
chrome/browser/autofill/personal_data_manager.cc:14: #include
"chrome/browser/autofill/autofill_country.h"
On 2011/03/16 23:07:16, Ilya Sherman wrote:
> nit: No longer needed.

Done.

http://codereview.chromium.org/6676031/diff/5021/chrome/browser/autofill/pers...
chrome/browser/autofill/personal_data_manager.cc:184: // |EmptyMigrationTrash|
once the sync service becomes available.
It does the "empty" sooner in the cycle.  I.e. as soon as Sync becomes
available.  This is preferable to waiting for a user action to trigger it.

On 2011/03/16 23:07:16, Ilya Sherman wrote:
> We already call into EmptyMigrationTrash() at various intervals -- what does
> adding this method buy us?

[Ilya Sherman, 2011-03-17 00:38:46]

LGTM, thanks

[dhollowa, 2011-03-17 03:12:00]

Hi Folks, I noticed and fixed an additional issue I was seeing.  In our merge
process we alter some of the profiles.  But Sync was overwriting these
modifications.  So I've changed the logic to disable the updates until the trash
has been emptied.

When the trash gets emptied I'm sending update notifications to Sync for the
remaining items (the ones that weren't trashed).

I've tested all this manually with two different clients, with deletes and
modifications.  And everything is propagating back and forth through Sync
correctly now.

Please see the changes in:
  WebDatabase::UpdateAutofillProfile, and 
  WebDataService::EmptyMigrationTrashImpl

Thanks.

[Ilya Sherman, 2011-03-17 03:22:12]

SLGTM

[lipalani1, 2011-03-17 18:51:52]

LGTM from sync side.
All updates until sync has been notified are being discarded. This is actually
the lesser of 2 evils. If we let any updates take place before sync had a chance
to start up it would overwrite those updates any way. 

Even currently with out any of this change we have the issue that if sync is in
an unrecoverable state all updates will be overwritten when sync comes back up
online. Which is a different issue that needs to be solved separately.