PasswordSyncableService shouldn't tolerate the situation when 2 or more passwords in the password s…

PasswordSyncableService shouldn't tolerate the situation when 2 or more passwords in the password store have the same sync tag.

BUG=396900
Committed: https://crrev.com/113b2b0be2c459b77718ee2ed99495d4138251a6
Cr-Commit-Position: refs/heads/master@{#291661}

[vasilii, 2014-08-22 16:42:17]

Hi Ilya,

we don't know why the crash happens. One theoretical possibility is the mess
around the unique key for password. LoginDatabase includes submit_element into
it while Sync does not. That means that two distinct password forms can be seen
as one entity by Sync. It'd be useful to see if it's a practical concern.

[Ilya Sherman, 2014-08-22 23:34:15]

LGTM % nits.  Do you have a plan for recovering users whose Sync will break with
this change?

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

https://codereview.chromium.org/493233003/diff/1/components/password_manager/...
components/password_manager/core/browser/password_syncable_service.cc:146:
DCHECK(sync_error_factory_);
nit: This seems redundant with the use of the variable immediately below.

https://codereview.chromium.org/493233003/diff/1/components/password_manager/...
components/password_manager/core/browser/password_syncable_service.cc:149:
"There are passwords with identical sync tag in the database."));
nit: "tag" -> "tags"

[vasilii, 2014-08-25 08:29:32]

The mergable fix would be to change LoginDatabase::RemoveLogin so it uses 5
attributes instead of 6. Thus the sync could threat all the passwords with the
same sync tag as one entity ("UPDATE" already uses 5 attributes).
The proper fix is to switch to the 5 attributes unique key in  LoginDatabase.
I'm concerned that the crash isn't happening anymore. So we may see nothing
despite people on beta may face the problem.

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

https://codereview.chromium.org/493233003/diff/1/components/password_manager/...
components/password_manager/core/browser/password_syncable_service.cc:146:
DCHECK(sync_error_factory_);
On 2014/08/22 23:34:15, Ilya Sherman wrote:
> nit: This seems redundant with the use of the variable immediately below.

Done.

https://codereview.chromium.org/493233003/diff/1/components/password_manager/...
components/password_manager/core/browser/password_syncable_service.cc:149:
"There are passwords with identical sync tag in the database."));
On 2014/08/22 23:34:15, Ilya Sherman wrote:
> nit: "tag" -> "tags"

Done.

[vasilii, 2014-08-25 08:29:51]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2014-08-25 08:30:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vasilii@chromium.org/493233003/20001

[commit-bot: I haz the power, 2014-08-25 09:23:16]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-08-25 11:17:27]

Committed patchset #2 (20001) as 46c114cb678a949f058441a67cce08befd3cc41b

[commit-bot: I haz the power, 2014-09-10 02:33:46]

Patchset 2 (id:??) landed as
https://crrev.com/113b2b0be2c459b77718ee2ed99495d4138251a6
Cr-Commit-Position: refs/heads/master@{#291661}