Credential Manager: Wire `notifySignedOut` to the PasswordStore.

Credential Manager: Wire `notifySignedOut` to the PasswordStore.

When `notifySignedOut` is called, we need to ensure that we stop
automagically providing a user's credentials to the website that
triggered the notification. This CL does that in the brute-force
way that we all know and love: grab all forms, walk through each
and set a flag for matching origins.

BUG=450581
Committed: https://crrev.com/05f2ff8ae2b920a989372b5c4ae0e5a908a2c880
Cr-Commit-Position: refs/heads/master@{#315016}

[Mike West, 2015-02-05 09:55:01]

mkwst@chromium.org changed reviewers:
+ vabr@chromium.org, vasilii@chromium.org

[Mike West, 2015-02-05 09:55:01]

vabr@, vasilii@, mind taking a look?

[vabr (Chromium), 2015-02-05 10:27:10]

LGTM with comments.
I also did not fully understand your two TODOs. Which might be OK as long as you
understand them, and don't get yourself killed by a bus. :)

Cheers,
Vaclav

https://codereview.chromium.org/902843002/diff/1/components/password_manager/...
File
components/password_manager/content/browser/credential_manager_dispatcher.cc
(right):

https://codereview.chromium.org/902843002/diff/1/components/password_manager/...
components/password_manager/content/browser/credential_manager_dispatcher.cc:115:
: dispatcher_(dispatcher), id_(request_id), origin_(origin) {}
As discussed before on engedy's CL
https://codereview.chromium.org/868253005/#msg11, I actually see good reasons
against inlining the methods here.

https://codereview.chromium.org/902843002/diff/1/components/password_manager/...
components/password_manager/content/browser/credential_manager_dispatcher.cc:118:
void OnGetPasswordStoreResults(
A heads-up: This will conflict with a change in signature I am doing in
https://codereview.chromium.org/866983003/. Your CL probably lands first, but if
not, and you get in troubles merging, just let me know.

[Mike West, 2015-02-05 13:50:59]

New patchsets have been uploaded after l-g-t-m from vabr@chromium.org

[Mike West, 2015-02-05 13:51:01]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2015-02-05 13:51:38]

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

[Mike West, 2015-02-05 13:52:13]

The CQ bit was unchecked by mkwst@chromium.org

[Mike West, 2015-02-05 14:05:00]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2015-02-05 14:05:46]

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

[vabr (Chromium), 2015-02-05 14:06:48]

https://codereview.chromium.org/902843002/diff/20001/components/password_mana...
File
components/password_manager/content/browser/credential_manager_dispatcher.cc
(right):

https://codereview.chromium.org/902843002/diff/20001/components/password_mana...
components/password_manager/content/browser/credential_manager_dispatcher.cc:130:
CredentialManagerDispatcher::PendingSignedOutTask(
CredentialManagerDispatcher::PendingSignedOutTask::PendingSignedOutTask

https://codereview.chromium.org/902843002/diff/20001/components/password_mana...
components/password_manager/content/browser/credential_manager_dispatcher.cc:139:
const std::vector<autofill::PasswordForm*>& results) {
Also, "The implementation owns all PasswordForms in the vector."
So you should probably delete them.

[vabr (Chromium), 2015-02-05 14:08:41]

I see I came too late. :)

One more nit, though.

https://codereview.chromium.org/902843002/diff/40001/components/password_mana...
File
components/password_manager/content/browser/credential_manager_dispatcher.cc
(right):

https://codereview.chromium.org/902843002/diff/40001/components/password_mana...
components/password_manager/content/browser/credential_manager_dispatcher.cc:150:
delete form;
Why not just delete them all directly? UpdateLogin takes a copy of the form
anyway.

[vasilii, 2015-02-05 14:49:31]

https://codereview.chromium.org/902843002/diff/40001/components/password_mana...
File
components/password_manager/content/browser/credential_manager_dispatcher.cc
(right):

https://codereview.chromium.org/902843002/diff/40001/components/password_mana...
components/password_manager/content/browser/credential_manager_dispatcher.cc:112:
PendingSignedOutTask(CredentialManagerDispatcher* const dispatcher,
'const' doesn't make a lot of sense here.

https://codereview.chromium.org/902843002/diff/40001/components/password_mana...
components/password_manager/content/browser/credential_manager_dispatcher.cc:150:
delete form;
On 2015/02/05 14:08:40, vabr (Chromium) wrote:
> Why not just delete them all directly? UpdateLogin takes a copy of the form
> anyway.

Acknowledged.

https://codereview.chromium.org/902843002/diff/40001/components/password_mana...
components/password_manager/content/browser/credential_manager_dispatcher.cc:226:
if (!pending_sign_out_ && store) {
I don't see the place where pending_sign_out_ actually becomes NULL.

[Mike West, 2015-02-05 14:51:03]

The CQ bit was unchecked by mkwst@chromium.org

[vabr (Chromium), 2015-02-06 08:32:32]

This LGTM.

I was trying to think of any further potential collisions (in addition to the
one in the comment below). One thing that could happen is the user signing out
and trying to sign in with a different credential. But even with the worst
timing, the only thing that can happen is that the user signs in and the
zero-click flag would be reset. I don't see an issue with that.

Cheers,
Vaclav

https://codereview.chromium.org/902843002/diff/60001/components/password_mana...
File
components/password_manager/content/browser/credential_manager_dispatcher.cc
(right):

https://codereview.chromium.org/902843002/diff/60001/components/password_mana...
components/password_manager/content/browser/credential_manager_dispatcher.cc:156:
dispatcher_->DoneSigningOut();
optional: Maybe add a note like "Destroys |this|" to make it clear this has to
be the last line of the method.

https://codereview.chromium.org/902843002/diff/60001/components/password_mana...
components/password_manager/content/browser/credential_manager_dispatcher.cc:239:
pending_sign_out_->AddOrigin(
I'm just explicitly acknowledging that although there is a short window, when
the password store might get a new credential for the current origin, it is
rather unlikely, that that would happen (hardly through user interaction, maybe
via sync, but that would mean this happens at the right time during start-up).
And if it does happen, the user will probably notice that they are being signed
in again, and repeating the sign-out should stick.

[Mike West, 2015-02-06 12:46:25]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2015-02-06 12:47:25]

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

[commit-bot: I haz the power, 2015-02-06 12:51:34]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-06 12:52:35]

Patchset 4 (id:??) landed as
https://crrev.com/05f2ff8ae2b920a989372b5c4ae0e5a908a2c880
Cr-Commit-Position: refs/heads/master@{#315016}