Fix for scoring password autofill candidates:

Fix for scoring password autofill candidates:

components/password_manager/core/browser/password_form_manager.cc:
PasswordFormManager::ScoreResult() - added signon realm matching. This fixes
a following situation: we have passwords saved in login db for 2 websites:
"https://service1.somedomain.com/path1" and "https://service2.somedomain.com/path2",
using the same username "someusername". We display login form on
"https://service1.somedomain.com/path3" and we receive password autofill
suggestion for "https://service2.somedomain.com/path2".

BUG=363090
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267659

[Garrett Casto, 2014-04-17 18:43:54]

Nice catch. Could you add a test for this in PasswordFormManagerTest? The case
that you mention in the description seems like it would be perfect.

[Garrett Casto, 2014-04-17 21:12:39]

Sorry, few more things.

I also saw a bug report for this today. Can you set BUG=363090 for this? The
problem they describe is slightly different since it's basic auth vs. HTML auth
but the fix is the same.

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

https://codereview.chromium.org/241033002/diff/1/components/password_manager/...
components/password_manager/core/browser/password_form_manager.cc:650:
candidate.original_signon_realm == observed_form_.signon_realm) {
Actually, I think this should just be !candidate.IsPublicSuffixMatch(). This is
equivalent to the first statement. The second statement shouldn't matter, as
original_signon_realm should only be set if it doesn't match
observer_form_.signon_realm.

https://codereview.chromium.org/241033002/diff/1/components/password_manager/...
components/password_manager/core/browser/password_form_manager.cc:653: score +=
(1 << 7);
This isn't completely consistent here, but I think that you should drop the
parenthesis.

https://codereview.chromium.org/241033002/diff/1/components/password_manager/...
components/password_manager/core/browser/password_form_manager.cc:677: if
(!candidate.IsPublicSuffixMatch())
This should be removed.

[Lukasz Jagielski, 2014-04-18 07:56:17]

Fixups

[Garrett Casto, 2014-04-18 17:49:47]

Not sure if you saw the request for a test. Code itself looks fine.

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

https://codereview.chromium.org/241033002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_form_manager.cc:650: //
original_signon_realm will be non-empty string
Nit:This comment seems unnecessary now.

[Lukasz Jagielski, 2014-04-22 07:41:33]

Yeah, I saw it but I didn't have enough time to write it. I'll try to do it
today.

[Lukasz Jagielski, 2014-04-24 09:21:04]

Test added.

[Garrett Casto, 2014-04-24 22:35:28]

https://codereview.chromium.org/241033002/diff/40001/components/password_mana...
File components/password_manager/core/browser/password_form_manager.cc (right):

https://codereview.chromium.org/241033002/diff/40001/components/password_mana...
components/password_manager/core/browser/password_form_manager.cc:650: } else if
(candidate.origin == observed_form_.origin) {
Sorry about this, but I realized this isn't quite right. There shouldn't be an
else here.

if (!candidate.IsPublicSuffixMatch() {
  ...
}

if (candidate.origin == observed_form_.origin)
...

The issue is that you may have multiple matches that are from the same realm but
different origin (e.g. example.com/first vs. example.com/second) and these
should score differently. The fact that this didn't fail a test isn't great
either, so we should probably add something to verify this.

https://codereview.chromium.org/241033002/diff/40001/components/password_mana...
File components/password_manager/core/browser/password_form_manager_unittest.cc
(right):

https://codereview.chromium.org/241033002/diff/40001/components/password_mana...
components/password_manager/core/browser/password_form_manager_unittest.cc:719:
// match should force higher score, so the second one should be preserved.
I think the ordering of the results doesn't match the ordering that you talk
about here (second has different signon_realm, first has different origin and
action). This last sentence also doesn't ready very smoothly. Could you say
something like "Signon realm is the most important, so the first is preserved".

[Garrett Casto, 2014-04-25 23:36:05]

LGTM with nits

https://codereview.chromium.org/241033002/diff/60001/components/password_mana...
File components/password_manager/core/browser/password_form_manager_unittest.cc
(right):

https://codereview.chromium.org/241033002/diff/60001/components/password_mana...
components/password_manager/core/browser/password_form_manager_unittest.cc:725:
results[0]->origin = GURL("http://accounts.google.com/a/ServiceLoginAuth2");
The order of these still doesn't match the description above. First has same
signon_realm but different origin and action, second has different signon_realm.

[Lukasz Jagielski, 2014-04-29 12:15:44]

Sorry for the delay, I was looking at the test's code and couldn't find the
problem. I hope this is the last fixup.

[Garrett Casto, 2014-04-30 18:09:06]

The CQ bit was checked by gcasto@chromium.org

[Garrett Casto, 2014-04-30 18:09:07]

The CQ bit was unchecked by gcasto@chromium.org

[Garrett Casto, 2014-04-30 18:09:31]

The CQ bit was checked by gcasto@chromium.org

[commit-bot: I haz the power, 2014-04-30 18:11:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ljagielski@opera.com/241033002/80001

[commit-bot: I haz the power, 2014-04-30 18:15:24]

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

[commit-bot: I haz the power, 2014-04-30 18:15:24]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium

[Garrett Casto, 2014-05-01 20:48:45]

The CQ bit was checked by gcasto@chromium.org

[commit-bot: I haz the power, 2014-05-01 20:49:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ljagielski@opera.com/241033002/80001

[commit-bot: I haz the power, 2014-05-01 23:28:26]

Change committed as 267659

[tuxianster, 2014-05-05 13:49:34]

On 2014/05/01 23:28:26, I haz the power (commit-bot) wrote:
> Change committed as 267659

We also have this problem since upgrading to version 34.0.1847.131 m on Windows
7 64bit.

Example: Password is stored for https://subdomain1.internex.at/ and is filled in
then for https://subdomain2.internex.at/

The subdomain1 can be a .htaccess "AuthType Basic" Authentication or a login
form but the webpage subdomain2 where the wrong passwords are filled in is
always a .htaccess "AuthType Basic" Authentication.