Fix password mgr heuristics for sites that keep the login form around after signin

Fix password mgr heuristics for sites that keep the login form around after signin
The test for whether the login form reappeared on the next page load now ignores invisible forms.
BUG=28911
TEST=none (manual testing with reduced web pages)

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=45841

[Jens Alfke, 2010-04-15 20:03:03]

See implementation notes in the bug report.
FYI, this code will not yet compile on trunk because it needs two new API calls
in WebKit which I'm having reviewed in parallel with this.

[tim (not reviewing), 2010-04-19 23:11:01]

http://codereview.chromium.org/1656005/diff/2001/3003
File chrome/browser/password_manager/password_manager.h (right):

http://codereview.chromium.org/1656005/diff/2001/3003#newcode49
chrome/browser/password_manager/password_manager.h:49: const std::vector<uint8>&
visible);
Why not put this into PasswordForm? It's strange to have all the other data
points in there but have one here. What's the criteria for being a param versus
a field?

http://codereview.chromium.org/1656005/diff/2001/3010
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/1656005/diff/2001/3010#newcode2674
chrome/renderer/render_view.cc:2674: SendPasswordForms(frame);
As you mention in the bug, this doesn't affect the order of operations *on form
submit*, but it does affect the actual password *autofill* on a form that has
just appeared and we have saved passwords for.  This is the case we're really
trying to optimize by doing this work on didFinishDocumentLoad; we quickly send
a request to the browser to query sqlite and send back saved data ASAP,
potentially before all subresources have loaded.  I think this change will block
that work until more has loaded, which can hurt the experience. Am I missing
something? It has been a while since I looked at this.

[Jens Alfke, 2010-04-19 23:29:59]

http://codereview.chromium.org/1656005/diff/2001/3003
File chrome/browser/password_manager/password_manager.h (right):

http://codereview.chromium.org/1656005/diff/2001/3003#newcode49
chrome/browser/password_manager/password_manager.h:49: const std::vector<uint8>&
visible);
On 2010/04/19 23:11:01, timsteele wrote:
> Why not put this into PasswordForm?

I considered doing that, but it seemed like a bad match because it's not an
attribute of the form itself, but how it's presented on one page-view.
PasswordForm is used for things like matching two appearances of a form, so it
felt out of place there. But if you really think it's a better design that way,
I could change it.

http://codereview.chromium.org/1656005/diff/2001/3010
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/1656005/diff/2001/3010#newcode2674
chrome/renderer/render_view.cc:2674: SendPasswordForms(frame);
On 2010/04/19 23:11:01, timsteele wrote:
> I think this change will block
> that work until more has loaded, which can hurt the experience. 

Hm. I guess it depends on how much asynchrony there is. Nothing's going to be
visible in the page anyway until the first layout occurs, which is the point
where this happens. Is the code here running in a background thread
independently from the browser's painting the window? I didn't see it being
handed off to a separate thread. If it is, then you're right that there could be
a win to doing this earlier.

[tim (not reviewing), 2010-04-20 00:04:39]

http://codereview.chromium.org/1656005/diff/2001/3003
File chrome/browser/password_manager/password_manager.h (right):

http://codereview.chromium.org/1656005/diff/2001/3003#newcode49
chrome/browser/password_manager/password_manager.h:49: const std::vector<uint8>&
visible);
On 2010/04/19 23:30:00, Jens Alfke wrote:
> On 2010/04/19 23:11:01, timsteele wrote:
> > Why not put this into PasswordForm?
> 
> I considered doing that, but it seemed like a bad match because it's not an
> attribute of the form itself, but how it's presented on one page-view.
> PasswordForm is used for things like matching two appearances of a form, so it
> felt out of place there. But if you really think it's a better design that
way,
> I could change it.

I see. I guess one difference is that this isn't something we'd ever want to
persist. Besides that it is somewhat similar to how ssl_valid works, and it
really is an attribute of the html form that we need to examine in the browser. 
I guess I'm fine with either approach, though personally prefer the consistent
way. If more of these descriptive attributes start showing up we could add a
transient state struct to either PasswordForm or PasswordFormsSeen.

http://codereview.chromium.org/1656005/diff/2001/3010
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/1656005/diff/2001/3010#newcode2674
chrome/renderer/render_view.cc:2674: SendPasswordForms(frame);
On 2010/04/19 23:30:00, Jens Alfke wrote:
> On 2010/04/19 23:11:01, timsteele wrote:
> > I think this change will block
> > that work until more has loaded, which can hurt the experience. 
> 
> Hm. I guess it depends on how much asynchrony there is. Nothing's going to be
> visible in the page anyway until the first layout occurs, which is the point
> where this happens. Is the code here running in a background thread
> independently from the browser's painting the window? I didn't see it being
> handed off to a separate thread. If it is, then you're right that there could
be
> a win to doing this earlier.

I'm not totally sure about the painting, but the way I see it is doc loaded
notification comes in, we send an IPC to the browser which does async sqlite,
and all of that can happen in parallel with whatever happens between
didFinishDocumentLoad and didFirstVisuallyNonEmptyLayout (which includes waiting
on the network afaik).

[Jens Alfke, 2010-04-20 22:59:07]

OK, I have restructured the code:

* There is no longer an extra parameter to PasswordFormsSeen. Instead, I've
reverted it to its existing behavior (with one detail -- see below) and renamed
it PasswordFormsFound.
* I've added a new message PasswordFormsVisible, sent when loading is complete
and only contains the user-visible forms (if any).
* The PasswordManager only checks the forms against provisional_save_manager_
during its PasswordFormsVisible callback. This still happens before the
DidStopLoading call, so DidStopLoading can still check provisional_save_manager_
to see whether the login form is visible.

The only thing I'm not certain about is a slight change to the behavior of
PasswordManager::PasswordFormsFound. In trunk, if it finds a form matching
provisional_save_manager_, it takes p_s_m_ and adds it to the list of
pending_login_managers_, then clears p_s_m_. I can't do that anymore because I
don't want to clear p_s_m_ yet, since I don't know until the later
PasswordFormsVisible call whether this form is visible. And I can't have the
same PasswordFormManager in both p_s_m_ and p_l_m_ at the same time because the
class isn't ref-counted. So I changed PasswordFormsFound to _always_ create new
PasswordFormManager objects.

I've tried this out and haven't found any difference in behavior. I'm not
certain what the re-use of the p_s_m_ object was for, so please let me know if
I've broken something without realizing it.

[Jens Alfke, 2010-04-26 18:34:03]

OK, I have added a unit test now. Thanks for adding the test code!

[tim (not reviewing), 2010-04-26 23:10:00]

http://codereview.chromium.org/1656005/diff/10001/11001
File chrome/browser/chromeos/login/account_creation_view.cc (right):

http://codereview.chromium.org/1656005/diff/10001/11001#newcode57
chrome/browser/chromeos/login/account_creation_view.cc:57: // becuase password
value is always empty for account create page.
nitpick - this isn't your change but I just dont agree with this comment, since
it is possible that detecting the form here on submit will let us autofill it
the next time we see it (instead of having to wait for one more login). Hm...
well, not a big deal.
Also - 80 characters, and s/becuase/because.

http://codereview.chromium.org/1656005/diff/10001/11004
File chrome/browser/password_manager/password_manager.h (right):

http://codereview.chromium.org/1656005/diff/10001/11004#newcode70
chrome/browser/password_manager/password_manager.h:70: // 1. form "seen"
can you update this ascii art?

http://codereview.chromium.org/1656005/diff/10001/11011
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/1656005/diff/10001/11011#newcode1661
chrome/renderer/render_view.cc:1661: 
extra whitespace

http://codereview.chromium.org/1656005/diff/20001/21003
File chrome/browser/password_manager/password_manager.cc (right):

http://codereview.chromium.org/1656005/diff/20001/21003#newcode135
chrome/browser/password_manager/password_manager.cc:135:
pending_login_managers_.push_back(manager);
can't this result in storing two PasswordFormManagers for a single form?

http://codereview.chromium.org/1656005/diff/20001/21005
File chrome/browser/password_manager/password_manager_unittest.cc (right):

http://codereview.chromium.org/1656005/diff/20001/21005#newcode213
chrome/browser/password_manager/password_manager_unittest.cc:213: 
a test that calls PasswordFormsFound but not Visible (during initial
observation) might also be interesting.  E.g, still want to EXPECT_CALL
FillPasswordForm.

http://codereview.chromium.org/1656005/diff/20001/21005#newcode236
chrome/browser/password_manager/password_manager_unittest.cc:236:
.WillOnce(WithArg<0>(SaveToScopedPtr(&form_to_save)));
nit- indent is off.

http://codereview.chromium.org/1656005/diff/20001/21012
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/1656005/diff/20001/21012#newcode1659
chrome/renderer/render_view.cc:1659: 
nit - extra whitespace

[Jens Alfke, 2010-04-26 23:58:57]

OK, I have uploaded a new draft incorporating these points.

http://codereview.chromium.org/1656005/diff/10001/11001
File chrome/browser/chromeos/login/account_creation_view.cc (right):

http://codereview.chromium.org/1656005/diff/10001/11001#newcode57
chrome/browser/chromeos/login/account_creation_view.cc:57: // becuase password
value is always empty for account create page.
On 2010/04/26 23:10:01, timsteele wrote:
> nitpick - this isn't your change but I just dont agree with this comment,
since
> it is possible that detecting the form here on submit will let us autofill it
> the next time we see it (instead of having to wait for one more login). Hm...
> well, not a big deal.
> Also - 80 characters, and s/becuase/because.

Done.

http://codereview.chromium.org/1656005/diff/10001/11004
File chrome/browser/password_manager/password_manager.h (right):

http://codereview.chromium.org/1656005/diff/10001/11004#newcode70
chrome/browser/password_manager/password_manager.h:70: // 1. form "seen"
On 2010/04/26 23:10:01, timsteele wrote:
> can you update this ascii art?

I don't know what's wrong with it. The "layout" looks OK to me...

http://codereview.chromium.org/1656005/diff/20001/21003
File chrome/browser/password_manager/password_manager.cc (right):

http://codereview.chromium.org/1656005/diff/20001/21003#newcode135
chrome/browser/password_manager/password_manager.cc:135:
pending_login_managers_.push_back(manager);
On 2010/04/26 23:10:01, timsteele wrote:
> can't this result in storing two PasswordFormManagers for a single form?

You mean for the form that already has the provisional_save_manager_? Yes, there
would be a second one. I couldn't see how to get around this because the same
manager object can't appear in both places, since the class isn't ref-counted.
Is it bad to have to objects? I didn't notice anything going wrong in my
testing.

http://codereview.chromium.org/1656005/diff/20001/21005
File chrome/browser/password_manager/password_manager_unittest.cc (right):

http://codereview.chromium.org/1656005/diff/20001/21005#newcode213
chrome/browser/password_manager/password_manager_unittest.cc:213: 
On 2010/04/26 23:10:01, timsteele wrote:
> a test that calls PasswordFormsFound but not Visible (during initial
> observation) might also be interesting.  E.g, still want to EXPECT_CALL
> FillPasswordForm.

Good idea. I'll add one.

http://codereview.chromium.org/1656005/diff/20001/21005#newcode236
chrome/browser/password_manager/password_manager_unittest.cc:236:
.WillOnce(WithArg<0>(SaveToScopedPtr(&form_to_save)));
On 2010/04/26 23:10:01, timsteele wrote:
> nit- indent is off.

Done.

http://codereview.chromium.org/1656005/diff/20001/21012
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/1656005/diff/20001/21012#newcode1659
chrome/renderer/render_view.cc:1659: 
On 2010/04/26 23:10:01, timsteele wrote:
> nit - extra whitespace

Done.

[tim (not reviewing), 2010-04-27 00:55:58]

LGTM! Thanks for fixing this!

http://codereview.chromium.org/1656005/diff/10001/11004
File chrome/browser/password_manager/password_manager.h (right):

http://codereview.chromium.org/1656005/diff/10001/11004#newcode70
chrome/browser/password_manager/password_manager.h:70: // 1. form "seen"
On 2010/04/26 23:58:57, Jens Alfke wrote:
> On 2010/04/26 23:10:01, timsteele wrote:
> > can you update this ascii art?
> 
> I don't know what's wrong with it. The "layout" looks OK to me...

hm, I guess you're right.  I wrote that comment before having totally understood
the scope of changes, basically was looking for "forms visible" to be
incorporated, but I don't think that's necessary.

http://codereview.chromium.org/1656005/diff/20001/21003
File chrome/browser/password_manager/password_manager.cc (right):

http://codereview.chromium.org/1656005/diff/20001/21003#newcode135
chrome/browser/password_manager/password_manager.cc:135:
pending_login_managers_.push_back(manager);
On 2010/04/26 23:58:57, Jens Alfke wrote:
> On 2010/04/26 23:10:01, timsteele wrote:
> > can't this result in storing two PasswordFormManagers for a single form?
> 
> You mean for the form that already has the provisional_save_manager_? Yes,
there
> would be a second one. I couldn't see how to get around this because the same
> manager object can't appear in both places, since the class isn't ref-counted.
> Is it bad to have to objects? I didn't notice anything going wrong in my
> testing.

Okay, I see.. this part didn't change... just the part to postpone failure
detection.  Looks fine.