The importance of the sort key is that it defines which stored passwords are deemed to be duplicates -- those are exactly the passwords with the same sort key.

Ideally, this bug would be solved by removing the password value from the sort key while not changing which password entries are marked as duplicates.

The general idea is that a signon realm defines a single set of accounts, so there should only be one username. If that were true, password would not be needed in the sort key to begin with.

The LoginDatabase defines 5 unique keys. In terms of PasswordForm data members those are:
(1) origin
(2) username_element
(3) username_value
(4) password_element
(5) signon_realm

If two entries for the same signon_realm and username are stored, they have to differ in the fields (1), (2), or (4).

I failed to achieve that. This is what I tried (the URLs are not important, only the fact that their origin only differs in paths):
(A) I saved a password "123" for a username "a" on http://localhost/www/password_forms.html.
(B) I submitted username "a" and password "X" on http://localhost/www/password_forms.html again, but in a different form, which had different field names.
(C) I submitted username "a" and password "X" on http://localhost/www/a-off.html.
In both the steps (B) and (C), Chrome asked me to update the existing credential. So while theoretically Chrome could save them separately, it does not allow the user to do that.

So it seems to me that we could actually just drop the password from the sort key.

Vasilii, Jan, Maxim, Vadym -- you all have experience with this code. What do you think?

Some updates after speaking to Ioana, Jan and Vadym:

Vadym tells me the ability to save two different passwords for the same username in the same signon_realm was a glitch in the past, which got fixed. The users still might have such credentials saved, and as long as we include the password in the sort key, they are able to view all of them.

We all agreed that a reasonable next step is to add UMA counting how frequently users have two passwords for a single username and signon realm. Once we have that in the stable, we can discuss whether it is low enough for removing the passwords from the sort key is safe. I plan to collect this UMA during the sort key computation, to inspect the effect on the set of users who actually view the passwords (as opposed to counting that during PasswordStore init).

Because that will take a couple of releases to produce useful data, Jan is looking into alternatives to unblock his bug fix, which originally prompted the question of dropping passwords from the sort key.

The CL with the UMA is at https://crrev.com/c/776684

Can you clarify more the security reason?

This is how imagine the structure.
- PasswordManagerPresenter knows all the passwords. 
- It decides to group some of the entries on some criteria (I personally think that items with different passwords should never be in the same group).
- It creates a second array of those groups.
- We share only the strings we need to show with the JS code. Every row in the Polymer knows its index in the second array in C++.

Note that as long as the sort key contains the password, the index in the sorted list is a hash of that password. So what you describe here is a particular instance of Jan's proposal with a hash. I don't think the security implications are serious (I'm also not a security engineer) and in any case this information is already available to the renderer.

In the meantime Jan resolved to deal with the original issue by fixing the way unmasked passwords are attached to the page structure (and you provided some helpful comments on the corresponding e-mail thread).

You raise a good point that items with different passwords should never be in the same group. What I was aiming at here was ensuring that whenever two entries have the same signon realm and username, they cannot have the same password. If we are sufficiently certain of that we might drop the passwords from the key (and perhaps add a DCHECK). On the other hand, given that we are not going to introduce any more information sharing between browser and renderer, perhaps keeping the key in is the best option.

Are we still interested in the UMA from https://crrev.com/c/776684 ?

Sorry for a typo:
"perhaps keeping the key in is the best option"
should have been
"perhaps keeping the passwords in the key in is the best option"

By index I meant an integer, nothing more. Let's say the list of the password is filtered and sorted by the date (a feature from the future). Still Polymer knows that the first row displayed has the index 7 in the C++ model.

Regarding different passwords for the same origin and username. I think it's good to tackle this problem. I don't think it's related to the password list. It should not assume too much about the data stored.

IIUC, we now have an agreement that password_manager::CreateSortKey() should keep passwords in the sort key to make it clear that two entries with different passwords will never be merged.

As for the ability to store two different passwords for the same signon realm and username, this has not been possible for some time now. The only thing we don't know is how many old credentials with different passwords and same signon realm and username are still stored. I'm not sure if knowing that would influence any future decision. If id would, then I could add UMA in LoginDatabase::ReportMetrics to measure that. Otherwise I suggest doing nothing more.

Re-reading #8, I'm not sure if there is anything to do for this any more. Sounds like the code is working as intended, and there seem to also be no TODOs linked to this bug.

I'm closing for now, but happy to reopen if somebody knows a concrete action item I missed.