Only consider PasswordFromManager which HasCompletedMatching when provisionally saving passwords

Only consider PasswordFromManager which HasCompletedMatching when provisionally saving passwords

In PasswordManager::ProvisionallySavePassword, PasswordFormManager instances are sought, which could be managing the form being saved. Some of those managers may not be ready, because they still did not complete matching against the PasswordStore. There might also be more form managers suitable for the form being saved, with some having completed the matching, and some not.

Currently, the code first finds the first suitable form manager, and then drops it if it has not completed matching. This ignores the fact that there might be a second choice form manager, which has completed matching. This happens currently on live.com (see the bug).

This CL ignores form managers which did not complete matching when looking for a suitable form manager for the form being saved.

This CL also improves a poorly phrased log message related to the situation when form managers have not completed matching.

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

[vabr (Chromium), 2014-07-11 09:00:10]

Balázs,
Could you please review the whole CL?

Ilya,
Could you please review save_password_progress_logger.*?

Thanks!
Vaclav

[engedy, 2014-07-11 17:05:36]

LGTM with some minor comments.

I must say that this is a very well tested CL for a very evasive bug, thanks a
lot for looking into this!

https://codereview.chromium.org/331593008/diff/60001/chrome/browser/password_...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/331593008/diff/60001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:1091: // former
did not finished matching, and the latter did, the latter should be
nit: s/did/has/

https://codereview.chromium.org/331593008/diff/60001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:1094:
TryToUsePasswordFormManagerWhichFinishedMatching) {
nit: s/TryToUse/Prefer/

https://codereview.chromium.org/331593008/diff/60001/chrome/test/data/passwor...
File chrome/test/data/password/create_form_copy_on_submit.html (right):

https://codereview.chromium.org/331593008/diff/60001/chrome/test/data/passwor...
chrome/test/data/password/create_form_copy_on_submit.html:3: On password form
submit, the page's JavaScript creates another
nit: Spell it out here more that there is a static form, and a *second* form is
created.

https://codereview.chromium.org/331593008/diff/60001/chrome/test/data/passwor...
chrome/test/data/password/create_form_copy_on_submit.html:8:
http://crbug.com/367768, PasswordManager is not able to process
nit: Add one more line here to describe the problem is with not being able to
complete matching early enough.

https://codereview.chromium.org/331593008/diff/60001/chrome/test/data/passwor...
chrome/test/data/password/create_form_copy_on_submit.html:20: // Spin the
message loop to let the deletion settle in.
Could you please describe more why this is needed?

https://codereview.chromium.org/331593008/diff/60001/chrome/test/data/passwor...
chrome/test/data/password/create_form_copy_on_submit.html:26:
addForm(createForm());
This might be a terribly bad idea, but perhaps this method could take the action
as an argument, and we could use it to create the first form as well.

I am not sure when document.body gets created, though.

https://codereview.chromium.org/331593008/diff/60001/chrome/test/data/passwor...
File chrome/test/data/password/form_utils.js (right):

https://codereview.chromium.org/331593008/diff/60001/chrome/test/data/passwor...
chrome/test/data/password/form_utils.js:1: function createForm() {
Now that we are exporting this into a util, I think it might make sense to add
comments above this and rename it to something more specific (e.g.,
createSimplePasswordForm()).

https://codereview.chromium.org/331593008/diff/60001/chrome/test/data/passwor...
chrome/test/data/password/form_utils.js:6: username_label.setAttribute('for',
'username');
We could use the short form here (and in 1 more place):

username_label.htmlFor = 'username';

https://codereview.chromium.org/331593008/diff/60001/chrome/test/data/passwor...
chrome/test/data/password/form_utils.js:35: function addForm(form) {
I think this might be suitable for inlining after below change.

https://codereview.chromium.org/331593008/diff/60001/chrome/test/data/passwor...
chrome/test/data/password/form_utils.js:36:
document.getElementsByTagName('body')[0].appendChild(form);
We could just use the short form here:

document.body.appendChild(form);

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

https://codereview.chromium.org/331593008/diff/60001/components/password_mana...
components/password_manager/core/browser/password_manager.cc:154: bool
successful_candidates_found = false;
nit: I would rename this to |has_found_matching_managers|, so as to prevent
introducing a new concept.

https://codereview.chromium.org/331593008/diff/60001/components/password_mana...
components/password_manager/core/browser/password_manager.cc:160: if (result !=
PasswordFormManager::RESULT_NO_MATCH) {
I wonder if it would make the control flow more readable if we swapped the two
nested conditionals (and update contents accordingly). Just an idea, I would
leave it up to you.

[vabr (Chromium), 2014-07-11 18:15:48]

Hi Balázs,

Please take a look again, most of your comments were addressed. However, I was
not sure how to address your comment about creating both forms dynamically.

Cheers,
Vaclav

https://codereview.chromium.org/331593008/diff/60001/chrome/browser/password_...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/331593008/diff/60001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:1091: // former
did not finished matching, and the latter did, the latter should be
On 2014/07/11 17:05:35, engedy wrote:
> nit: s/did/has/

Done.

https://codereview.chromium.org/331593008/diff/60001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:1094:
TryToUsePasswordFormManagerWhichFinishedMatching) {
On 2014/07/11 17:05:35, engedy wrote:
> nit: s/TryToUse/Prefer/

Done.

https://codereview.chromium.org/331593008/diff/60001/chrome/test/data/passwor...
File chrome/test/data/password/create_form_copy_on_submit.html (right):

https://codereview.chromium.org/331593008/diff/60001/chrome/test/data/passwor...
chrome/test/data/password/create_form_copy_on_submit.html:3: On password form
submit, the page's JavaScript creates another
On 2014/07/11 17:05:35, engedy wrote:
> nit: Spell it out here more that there is a static form, and a *second* form
is
> created.

Done.

https://codereview.chromium.org/331593008/diff/60001/chrome/test/data/passwor...
chrome/test/data/password/create_form_copy_on_submit.html:8:
http://crbug.com/367768, PasswordManager is not able to process
On 2014/07/11 17:05:35, engedy wrote:
> nit: Add one more line here to describe the problem is with not being able to
> complete matching early enough.

Done.

https://codereview.chromium.org/331593008/diff/60001/chrome/test/data/passwor...
chrome/test/data/password/create_form_copy_on_submit.html:20: // Spin the
message loop to let the deletion settle in.
On 2014/07/11 17:05:35, engedy wrote:
> Could you please describe more why this is needed?

Done. The problem is I'm not sure it is needed. It would probably work without
it, but I don't know Blink and V8 well enough to be certain there is not a race
between the DOM modification and JS execution -- note that the form elements
need to have the same IDs, so it would be bad if we got hold of a wrong version
of the elements in the code below.
Also, I'm not sure setTimeout actually guarantees that the above does not
happen, it's just my attempt to give the system a chance and hope for the best.

https://codereview.chromium.org/331593008/diff/60001/chrome/test/data/passwor...
chrome/test/data/password/create_form_copy_on_submit.html:26:
addForm(createForm());
On 2014/07/11 17:05:36, engedy wrote:
> This might be a terribly bad idea, but perhaps this method could take the
action
> as an argument, and we could use it to create the first form as well.
> 
> I am not sure when document.body gets created, though.

I'm not sure if it is a bad idea, I just don't see why that would be preferable.
What were your reasons?
I like the static form below, because it is easier to understand it's structure.
Having said that, I acknowledge that the code hides the fact that the new form
is almost the same. Maybe the createForm function could take an existing form as
an argument, and copy that, but maybe that's just overengineering a simple test.
WDYT?

https://codereview.chromium.org/331593008/diff/60001/chrome/test/data/passwor...
File chrome/test/data/password/form_utils.js (right):

https://codereview.chromium.org/331593008/diff/60001/chrome/test/data/passwor...
chrome/test/data/password/form_utils.js:1: function createForm() {
On 2014/07/11 17:05:36, engedy wrote:
> Now that we are exporting this into a util, I think it might make sense to add
> comments above this and rename it to something more specific (e.g.,
> createSimplePasswordForm()).

Done.

https://codereview.chromium.org/331593008/diff/60001/chrome/test/data/passwor...
chrome/test/data/password/form_utils.js:6: username_label.setAttribute('for',
'username');
On 2014/07/11 17:05:36, engedy wrote:
> We could use the short form here (and in 1 more place):
> 
> username_label.htmlFor = 'username';

Done.

https://codereview.chromium.org/331593008/diff/60001/chrome/test/data/passwor...
chrome/test/data/password/form_utils.js:35: function addForm(form) {
On 2014/07/11 17:05:36, engedy wrote:
> I think this might be suitable for inlining after below change.

Done.

https://codereview.chromium.org/331593008/diff/60001/chrome/test/data/passwor...
chrome/test/data/password/form_utils.js:36:
document.getElementsByTagName('body')[0].appendChild(form);
On 2014/07/11 17:05:36, engedy wrote:
> We could just use the short form here:
> 
> document.body.appendChild(form);

Done.

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

https://codereview.chromium.org/331593008/diff/60001/components/password_mana...
components/password_manager/core/browser/password_manager.cc:154: bool
successful_candidates_found = false;
On 2014/07/11 17:05:36, engedy wrote:
> nit: I would rename this to |has_found_matching_managers|, so as to prevent
> introducing a new concept.

Done.

https://codereview.chromium.org/331593008/diff/60001/components/password_mana...
components/password_manager/core/browser/password_manager.cc:160: if (result !=
PasswordFormManager::RESULT_NO_MATCH) {
On 2014/07/11 17:05:36, engedy wrote:
> I wonder if it would make the control flow more readable if we swapped the two
> nested conditionals (and update contents accordingly). Just an idea, I would
> leave it up to you.

Done.

[engedy, 2014-07-11 18:26:11]

https://codereview.chromium.org/331593008/diff/60001/chrome/test/data/passwor...
File chrome/test/data/password/create_form_copy_on_submit.html (right):

https://codereview.chromium.org/331593008/diff/60001/chrome/test/data/passwor...
chrome/test/data/password/create_form_copy_on_submit.html:20: // Spin the
message loop to let the deletion settle in.
On 2014/07/11 18:15:47, vabr (Chromium) wrote:
> On 2014/07/11 17:05:35, engedy wrote:
> > Could you please describe more why this is needed?
> 
> Done. The problem is I'm not sure it is needed. It would probably work without
> it, but I don't know Blink and V8 well enough to be certain there is not a
race
> between the DOM modification and JS execution -- note that the form elements
> need to have the same IDs, so it would be bad if we got hold of a wrong
version
> of the elements in the code below.
> Also, I'm not sure setTimeout actually guarantees that the above does not
> happen, it's just my attempt to give the system a chance and hope for the
best.

Acknowledged.

https://codereview.chromium.org/331593008/diff/60001/chrome/test/data/passwor...
chrome/test/data/password/create_form_copy_on_submit.html:26:
addForm(createForm());
On 2014/07/11 18:15:47, vabr (Chromium) wrote:
> On 2014/07/11 17:05:36, engedy wrote:
> > This might be a terribly bad idea, but perhaps this method could take the
> action
> > as an argument, and we could use it to create the first form as well.
> > 
> > I am not sure when document.body gets created, though.
> 
> I'm not sure if it is a bad idea, I just don't see why that would be
preferable.
> What were your reasons?
> I like the static form below, because it is easier to understand it's
structure.
> Having said that, I acknowledge that the code hides the fact that the new form
> is almost the same. Maybe the createForm function could take an existing form
as
> an argument, and copy that, but maybe that's just overengineering a simple
test.
> WDYT?

Yes, I wanted to reduce the duplication, but I think you are right, let us just
keep the static form, it is cleaner.

[vabr (Chromium), 2014-07-11 18:51:37]

Thanks, Balázs,
(Also for your initial review and whiteboard discussion we had about this.)
Since your new two comments did not require any follow-up, I take it as your
recent approval still holds.

Once Ilya does the OWNERS review of save_password_progress_logger.*, I'll send
this to CQ.

Cheers,
Vaclav

[engedy, 2014-07-11 19:01:46]

On 2014/07/11 18:51:37, vabr (Chromium) wrote:
> Thanks, Balázs,
> (Also for your initial review and whiteboard discussion we had about this.)
> Since your new two comments did not require any follow-up, I take it as your
> recent approval still holds.
> 
> Once Ilya does the OWNERS review of save_password_progress_logger.*, I'll send
> this to CQ.
> 
> Cheers,
> Vaclav

Yes, still LGTM, thanks!

[Ilya Sherman, 2014-07-11 23:47:02]

//components/autofill changes LGTM % a nit.  Thanks, Václav!

https://codereview.chromium.org/331593008/diff/80001/components/autofill/core...
File components/autofill/core/common/save_password_progress_logger.cc (right):

https://codereview.chromium.org/331593008/diff/80001/components/autofill/core...
components/autofill/core/common/save_password_progress_logger.cc:141: return "No
form manager completed matching.";
nit: Please insert the word "has" prior to "completed".

[vabr (Chromium), 2014-07-14 07:06:55]

Thanks for review, Ilya, I addressed your comment in patch set 5.

I also rebased the CL on ToT. Since https://codereview.chromium.org/378643006
landed in the meantime, I could (also, had to) simplify the password prompt
check in the browser test -- that's patch set 6.

If the trybots are happy, I'll put this this in the queue for commit.

Cheers,
Vaclav

[vabr (Chromium), 2014-07-14 07:59:07]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2014-07-14 07:59:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vabr@chromium.org/331593008/120001

[commit-bot: I haz the power, 2014-07-14 09:31:52]

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

[commit-bot: I haz the power, 2014-07-14 09:34:54]

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

[commit-bot: I haz the power, 2014-07-14 09:34:55]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[vabr (Chromium), 2014-07-14 09:42:24]

Looks like I forgot to put a copyright message in the new JavaScript file,
fixing that.
Because that is a straightforward change and in comments only, I'm immediately
send this to the CQ again.

Cheers,
Vaclav

[vabr (Chromium), 2014-07-14 09:42:39]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2014-07-14 09:43:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vabr@chromium.org/331593008/140001

[commit-bot: I haz the power, 2014-07-14 12:33:34]

Change committed as 282935