Fix the no password save issue for ajax login

Fix the no password save issue for ajax login 

The idea is that if a navigation is triggered by non-user gesture after a password form is submitted, then we assume that this navigation is related to the previous form submit and chrome should prompt for saving password if all criteria for provisionally saving password are met. 

The link between the previous form submit and the coming navigation is weak, so its possible for chrome to inherit password forms from previous state when it should not. However most false cases would be aborted at PasswordManager::ProvisionallySavePassword, which does a list of sanity checks for saving password, such as if the form is already seen on the current page. 

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

[Ilya Sherman, 2013-03-22 03:26:48]

https://codereview.chromium.org/12713007/diff/1/chrome/browser/password_manag...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/12713007/diff/1/chrome/browser/password_manag...
chrome/browser/password_manager/password_manager_browsertest.cc:1: // Copyright
(c) 2013 The Chromium Authors. All rights reserved.
ultra nit: Omit the "(c)"

https://codereview.chromium.org/12713007/diff/1/chrome/browser/password_manag...
chrome/browser/password_manager/password_manager_browsertest.cc:27: explicit
NavigationObserver(Browser* browser)
nit: Can you pass in just the WebContents, rather than the full Browser?

https://codereview.chromium.org/12713007/diff/1/chrome/browser/password_manag...
chrome/browser/password_manager/password_manager_browsertest.cc:36:
content::NotificationService::AllSources());
nit: Can you listen to notifications from a specific source?  Better yet, can
you avoid listening for notifications at all, i.e. hook into this event in a
more direct way?

https://codereview.chromium.org/12713007/diff/1/chrome/browser/password_manag...
chrome/browser/password_manager/password_manager_browsertest.cc:46:
infobar_service_->RemoveInfoBar(infobar);
Hmm, why do you need to do this manually?  Doesn't calling Accept() on the
infobar cause it to be closed?

https://codereview.chromium.org/12713007/diff/1/chrome/browser/password_manag...
chrome/browser/password_manager/password_manager_browsertest.cc:89: bool
has_run_message_loop_;
Use content::MessageLoopRunner instead.

https://codereview.chromium.org/12713007/diff/1/chrome/browser/password_manag...
chrome/browser/password_manager/password_manager_browsertest.cc:95: };
nit: Tuck this into an anonymous namespace.

https://codereview.chromium.org/12713007/diff/1/chrome/browser/password_manag...
chrome/browser/password_manager/password_manager_browsertest.cc:99:
content::RenderViewHost* render_view_host() {
nit: This should use MixedCaps rather than hacker_case.

https://codereview.chromium.org/12713007/diff/1/chrome/browser/password_manag...
chrome/browser/password_manager/password_manager_browsertest.cc:105:
IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTest, PromptForXHRSubmit) {
As long as you're adding test coverage (thanks!), could you add a test for the
regular (non-XHR) case as well?

https://codereview.chromium.org/12713007/diff/1/chrome/browser/password_manag...
chrome/browser/password_manager/password_manager_browsertest.cc:112: // in it's
onsubmit handler but instead logs in/navigates via XHR.
nit: "it's" -> "its"

https://codereview.chromium.org/12713007/diff/1/chrome/browser/password_manag...
chrome/browser/password_manager/password_manager_browsertest.cc:120:
ASSERT_TRUE(content::ExecuteScript(render_view_host(), fill_and_submit));
Can you have the loaded page send a result when it's done loading (using
something like ExecuteScriptAndExtractBool), and use a
WindowedNotificationObserver that listens for that instead?  That seems like it
would be less code, and possibly less error-prone as well.

https://codereview.chromium.org/12713007/diff/1/chrome/browser/password_manag...
chrome/browser/password_manager/password_manager_browsertest.cc:122:
ASSERT_TRUE(observer.InfoBarWasShown());
nit: I think this can be EXPECT_TRUE.

https://codereview.chromium.org/12713007/diff/1/chrome/browser/password_manag...
chrome/browser/password_manager/password_manager_browsertest.cc:143:
ASSERT_FALSE(observer.InfoBarWasShown());
nit: I think this can be EXPECT_FALSE.

https://codereview.chromium.org/12713007/diff/1/chrome/test/data/password/pas...
File chrome/test/data/password/password_xhr_submit.html (right):

https://codereview.chromium.org/12713007/diff/1/chrome/test/data/password/pas...
chrome/test/data/password/password_xhr_submit.html:8: }
nit: Omit the curly braces?

https://codereview.chromium.org/12713007/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/12713007/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:3434: // If the navigation is not triggered
by user gesture, e.g. by some ajax
nit: "by user gesture" -> "by a user gesture"

https://codereview.chromium.org/12713007/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:3437: // crbug/43219. Note that there are
still some sites that this fails for
nit: Add "http://", and ideally find some way to separate the dot from the URL,
e.g. "[ http://crbug.com/43219 ].", so that linkified code is likely to work
correctly.

https://codereview.chromium.org/12713007/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:3447: scoped_ptr<content::PasswordForm>(
nit: Consider using make_scoped_ptr() here.

[Garrett Casto, 2013-03-22 21:05:05]

https://codereview.chromium.org/12713007/diff/1/chrome/browser/password_manag...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/12713007/diff/1/chrome/browser/password_manag...
chrome/browser/password_manager/password_manager_browsertest.cc:1: // Copyright
(c) 2013 The Chromium Authors. All rights reserved.
On 2013/03/22 03:26:48, Ilya Sherman wrote:
> ultra nit: Omit the "(c)"

Done.

https://codereview.chromium.org/12713007/diff/1/chrome/browser/password_manag...
chrome/browser/password_manager/password_manager_browsertest.cc:27: explicit
NavigationObserver(Browser* browser)
On 2013/03/22 03:26:48, Ilya Sherman wrote:
> nit: Can you pass in just the WebContents, rather than the full Browser?

Done.

https://codereview.chromium.org/12713007/diff/1/chrome/browser/password_manag...
chrome/browser/password_manager/password_manager_browsertest.cc:36:
content::NotificationService::AllSources());
On 2013/03/22 03:26:48, Ilya Sherman wrote:
> nit: Can you listen to notifications from a specific source?  Better yet, can
> you avoid listening for notifications at all, i.e. hook into this event in a
> more direct way?

Changed to use a specific source. I don't think that I can hook this in a more
direct way at the moment. I could add a hook into the PasswordManager where the
password is being saved, but I'm not sure if it's worth it.

https://codereview.chromium.org/12713007/diff/1/chrome/browser/password_manag...
chrome/browser/password_manager/password_manager_browsertest.cc:46:
infobar_service_->RemoveInfoBar(infobar);
On 2013/03/22 03:26:48, Ilya Sherman wrote:
> Hmm, why do you need to do this manually?  Doesn't calling Accept() on the
> infobar cause it to be closed?

I actually just pulled this from autofill_browsertest, but it does look like it
isn't necessary. Removed.

https://codereview.chromium.org/12713007/diff/1/chrome/browser/password_manag...
chrome/browser/password_manager/password_manager_browsertest.cc:89: bool
has_run_message_loop_;
On 2013/03/22 03:26:48, Ilya Sherman wrote:
> Use content::MessageLoopRunner instead.

Done.

https://codereview.chromium.org/12713007/diff/1/chrome/browser/password_manag...
chrome/browser/password_manager/password_manager_browsertest.cc:95: };
On 2013/03/22 03:26:48, Ilya Sherman wrote:
> nit: Tuck this into an anonymous namespace.

Done.

https://codereview.chromium.org/12713007/diff/1/chrome/browser/password_manag...
chrome/browser/password_manager/password_manager_browsertest.cc:99:
content::RenderViewHost* render_view_host() {
On 2013/03/22 03:26:48, Ilya Sherman wrote:
> nit: This should use MixedCaps rather than hacker_case.

Done.

https://codereview.chromium.org/12713007/diff/1/chrome/browser/password_manag...
chrome/browser/password_manager/password_manager_browsertest.cc:105:
IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTest, PromptForXHRSubmit) {
On 2013/03/22 03:26:48, Ilya Sherman wrote:
> As long as you're adding test coverage (thanks!), could you add a test for the
> regular (non-XHR) case as well?

There is actually a full pyauto test suite for the password manager that I
didn't know about until recently. I was going to file a bug to migrate it to
BrowserTest. That would include testing for the normal case.

https://codereview.chromium.org/12713007/diff/1/chrome/browser/password_manag...
chrome/browser/password_manager/password_manager_browsertest.cc:112: // in it's
onsubmit handler but instead logs in/navigates via XHR.
On 2013/03/22 03:26:48, Ilya Sherman wrote:
> nit: "it's" -> "its"

Done.

https://codereview.chromium.org/12713007/diff/1/chrome/browser/password_manag...
chrome/browser/password_manager/password_manager_browsertest.cc:120:
ASSERT_TRUE(content::ExecuteScript(render_view_host(), fill_and_submit));
On 2013/03/22 03:26:48, Ilya Sherman wrote:
> Can you have the loaded page send a result when it's done loading (using
> something like ExecuteScriptAndExtractBool), and use a
> WindowedNotificationObserver that listens for that instead?  That seems like
it
> would be less code, and possibly less error-prone as well.

I'm not sure what exactly you have in mind here. I can imagine setting some
variable during onload of the next page, and then extracting it with
ExecuteScriptAndExtractBool, but this extraction would have to be a loop because
it may run before the page is finished loading, right? That seems like a step in
the wrong direction. Or were you thinking of some other way of doing this?

https://codereview.chromium.org/12713007/diff/1/chrome/browser/password_manag...
chrome/browser/password_manager/password_manager_browsertest.cc:122:
ASSERT_TRUE(observer.InfoBarWasShown());
On 2013/03/22 03:26:48, Ilya Sherman wrote:
> nit: I think this can be EXPECT_TRUE.

Done.

https://codereview.chromium.org/12713007/diff/1/chrome/browser/password_manag...
chrome/browser/password_manager/password_manager_browsertest.cc:143:
ASSERT_FALSE(observer.InfoBarWasShown());
On 2013/03/22 03:26:48, Ilya Sherman wrote:
> nit: I think this can be EXPECT_FALSE.

Done.

https://codereview.chromium.org/12713007/diff/1/content/renderer/render_view_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/12713007/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:3434: // If the navigation is not triggered
by user gesture, e.g. by some ajax
On 2013/03/22 03:26:48, Ilya Sherman wrote:
> nit: "by user gesture" -> "by a user gesture"

Done.

https://codereview.chromium.org/12713007/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:3437: // crbug/43219. Note that there are
still some sites that this fails for
On 2013/03/22 03:26:48, Ilya Sherman wrote:
> nit: Add "http://", and ideally find some way to separate the dot from the
URL,
> e.g. "[ http://crbug.com/43219 ].", so that linkified code is likely to work
> correctly.

Done.

https://codereview.chromium.org/12713007/diff/1/content/renderer/render_view_...
content/renderer/render_view_impl.cc:3447: scoped_ptr<content::PasswordForm>(
On 2013/03/22 03:26:48, Ilya Sherman wrote:
> nit: Consider using make_scoped_ptr() here.

Done.

[Charlie Reis, 2013-03-23 01:00:26]

Thanks for adding the test and comments!

https://codereview.chromium.org/12713007/diff/21001/content/public/renderer/d...
File content/public/renderer/document_state.h (right):

https://codereview.chromium.org/12713007/diff/21001/content/public/renderer/d...
content/public/renderer/document_state.h:173: // navigation (there is some
ambiguity in the case of javascript initiated
I really appreciate this comment.

However, it's also concerning.  Ambiguity suggests we could end up sending the
password to the wrong place.  If that's not possible, please update the comment
to say what safeguards are in place to prevent this.  (For example, are we
conservative here by never including password data when it's ambiguous?)

[Ilya Sherman, 2013-03-23 01:10:58]

Thanks, LGTM (but please wait for other reviewers as well).

https://codereview.chromium.org/12713007/diff/1/chrome/browser/password_manag...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/12713007/diff/1/chrome/browser/password_manag...
chrome/browser/password_manager/password_manager_browsertest.cc:105:
IN_PROC_BROWSER_TEST_F(PasswordManagerBrowserTest, PromptForXHRSubmit) {
On 2013/03/22 21:05:05, Garrett Casto wrote:
> On 2013/03/22 03:26:48, Ilya Sherman wrote:
> > As long as you're adding test coverage (thanks!), could you add a test for
the
> > regular (non-XHR) case as well?
> 
> There is actually a full pyauto test suite for the password manager that I
> didn't know about until recently. I was going to file a bug to migrate it to
> BrowserTest. That would include testing for the normal case. 

Ok, fair enough :)

https://codereview.chromium.org/12713007/diff/1/chrome/browser/password_manag...
chrome/browser/password_manager/password_manager_browsertest.cc:120:
ASSERT_TRUE(content::ExecuteScript(render_view_host(), fill_and_submit));
On 2013/03/22 21:05:05, Garrett Casto wrote:
> On 2013/03/22 03:26:48, Ilya Sherman wrote:
> > Can you have the loaded page send a result when it's done loading (using
> > something like ExecuteScriptAndExtractBool), and use a
> > WindowedNotificationObserver that listens for that instead?  That seems like
> it
> > would be less code, and possibly less error-prone as well.
> 
> I'm not sure what exactly you have in mind here. I can imagine setting some
> variable during onload of the next page, and then extracting it with
> ExecuteScriptAndExtractBool, but this extraction would have to be a loop
because
> it may run before the page is finished loading, right? That seems like a step
in
> the wrong direction. Or were you thinking of some other way of doing this?

Hmm, I kind of ignored the fact that the JavaScript context probably doesn't
last across page loads.  So, never mind about this comment :)

https://codereview.chromium.org/12713007/diff/21001/chrome/browser/password_m...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/12713007/diff/21001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager_browsertest.cc:85: }
nit: Please leave a blank line after this one.

[Garrett Casto, 2013-03-26 00:00:26]

https://codereview.chromium.org/12713007/diff/21001/chrome/browser/password_m...
File chrome/browser/password_manager/password_manager_browsertest.cc (right):

https://codereview.chromium.org/12713007/diff/21001/chrome/browser/password_m...
chrome/browser/password_manager/password_manager_browsertest.cc:85: }
On 2013/03/23 01:10:58, Ilya Sherman wrote:
> nit: Please leave a blank line after this one.

Done.

https://codereview.chromium.org/12713007/diff/21001/content/public/renderer/d...
File content/public/renderer/document_state.h (right):

https://codereview.chromium.org/12713007/diff/21001/content/public/renderer/d...
content/public/renderer/document_state.h:173: // navigation (there is some
ambiguity in the case of javascript initiated
On 2013/03/23 01:00:26, creis wrote:
> I really appreciate this comment.
> 
> However, it's also concerning.  Ambiguity suggests we could end up sending the
> password to the wrong place.  If that's not possible, please update the
comment
> to say what safeguards are in place to prevent this.  (For example, are we
> conservative here by never including password data when it's ambiguous?)

I'm not sure what you mean by sending here. If you mean sending over the wire,
this code has no control over that. If you mean populating this field, then
there are no particular safeguards other than the fact that the ambiguous case
should be rare and the side effects are limited.

To be clear, the issue occurs if the user clicks on a submit button for a form
but the form doesn't submit (either thought the normal submit process or using
XHR in the onsubmit handler, perhaps because the form failed validation). If the
user later leaves the current page due to a NavigationGestureAuto, then we will
prompt the user to save the data that they had filled out in the form when they
clicked submit.

I've added some additional comments here. Hopefully this helps.

[Charlie Reis, 2013-03-27 05:15:04]

Hmm.  I'd feel better if I knew where we were associating the actual password
with an actual origin, but that explanation does help for this CL.  LGTM with
nits.

https://codereview.chromium.org/12713007/diff/45001/content/public/renderer/d...
File content/public/renderer/document_state.h (right):

https://codereview.chromium.org/12713007/diff/45001/content/public/renderer/d...
content/public/renderer/document_state.h:178: // origin we associate it with,
only if we prompt the user to save it.
nit: if -> whether

https://codereview.chromium.org/12713007/diff/45001/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/12713007/diff/45001/content/renderer/render_v...
content/renderer/render_view_impl.cc:3367: DocumentState* document_state_old =
DocumentState::FromDataSource(
nit: old_document_state (more consistent with other names)

https://codereview.chromium.org/12713007/diff/45001/content/renderer/render_v...
content/renderer/render_view_impl.cc:3369: const content::PasswordForm*
password_form_old =
nit: old_password_form

[Garrett Casto, 2013-03-27 18:57:25]

On 2013/03/27 05:15:04, creis wrote:
> Hmm.  I'd feel better if I knew where we were associating the actual password
> with an actual origin, but that explanation does help for this CL.  LGTM with
> nits.
> 

The association happens at
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
when a content::PasswordForm is created.

>
https://codereview.chromium.org/12713007/diff/45001/content/public/renderer/d...
> File content/public/renderer/document_state.h (right):
> 
>
https://codereview.chromium.org/12713007/diff/45001/content/public/renderer/d...
> content/public/renderer/document_state.h:178: // origin we associate it with,
> only if we prompt the user to save it.
> nit: if -> whether
> 
>
https://codereview.chromium.org/12713007/diff/45001/content/renderer/render_v...
> File content/renderer/render_view_impl.cc (right):
> 
>
https://codereview.chromium.org/12713007/diff/45001/content/renderer/render_v...
> content/renderer/render_view_impl.cc:3367: DocumentState* document_state_old =
> DocumentState::FromDataSource(
> nit: old_document_state (more consistent with other names)
> 
>
https://codereview.chromium.org/12713007/diff/45001/content/renderer/render_v...
> content/renderer/render_view_impl.cc:3369: const content::PasswordForm*
> password_form_old =
> nit: old_password_form

[Garrett Casto, 2013-03-27 18:59:44]

https://codereview.chromium.org/12713007/diff/45001/content/public/renderer/d...
File content/public/renderer/document_state.h (right):

https://codereview.chromium.org/12713007/diff/45001/content/public/renderer/d...
content/public/renderer/document_state.h:178: // origin we associate it with,
only if we prompt the user to save it.
On 2013/03/27 05:15:04, creis wrote:
> nit: if -> whether

Done.

https://codereview.chromium.org/12713007/diff/45001/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/12713007/diff/45001/content/renderer/render_v...
content/renderer/render_view_impl.cc:3367: DocumentState* document_state_old =
DocumentState::FromDataSource(
On 2013/03/27 05:15:04, creis wrote:
> nit: old_document_state (more consistent with other names)

Done.

https://codereview.chromium.org/12713007/diff/45001/content/renderer/render_v...
content/renderer/render_view_impl.cc:3369: const content::PasswordForm*
password_form_old =
On 2013/03/27 05:15:04, creis wrote:
> nit: old_password_form

Done.

[commit-bot: I haz the power, 2013-03-27 19:02:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gcasto@chromium.org/12713007/65001

[commit-bot: I haz the power, 2013-03-27 21:17:58]

Change committed as 191024