Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
293093002:
Don't show "Save password" prompt for a failed login (Closed)
|
Created:
6 years, 7 months ago by ziran.sun Modified:
6 years, 6 months ago CC:
chromium-reviews, benquan, jam, browser-components-watch_chromium.org, darin-cc_chromium.org, Dane Wallinga, dyu1, mkwst+watchlist_chromium.org, estade+watch_chromium.org, Ilya Sherman, rouslan+autofillwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionDon't show Save password prompt for a failed login.
A login page that has two |WebFrame|s, one is for login and the other is not related to login. When the user enters wrong login credentials and submits the form, he is navigated to the failed login page. At this point, Chrome (mistakenly) compares the state of initial site with the irrelevant |WebFrame|'s, and since the irrelevant |WebFrame| doesn't have any login forms, Chrome assumes that this is a successful login.
We record all visible forms seen during a page load, in all frames of the page. When the page stops loading, the password manager checks if one of the recorded forms matches the login form from the previous page. "Save password" prompt is only shown when a matched form is found.
R=vabr@chromium.org, gcasto@chromium.org, isherman@chromium.org, tsepez@chromium.org
BUG=177260, 344299
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=279123
Patch Set 1 #Patch Set 2 : Clean code and add tests. #Patch Set 3 : Rebase #
Total comments: 7
Patch Set 4 : Update code as per vabr's review comments. #Patch Set 5 : Documentation update. #
Total comments: 6
Patch Set 6 : Update code as per vabr's further review comments. #Patch Set 7 : Add missed filed from last upload. #
Total comments: 2
Patch Set 8 : Use exising done.html file for the test case that a iframe doesn't contain any form. #
Total comments: 1
Patch Set 9 : Update code as per Ilya's review comments. #Patch Set 10 : Rebase #Messages
Total messages: 22 (0 generated)
Please review. Thanks!
Thanks for working on this! A couple of comments in addition to those code-bound below: 1) Your browser test is broken (it works without your patch as well), and does not seem to be repeating what happens for kickstarter.com. What I see on kickstarter is: * the login form is in the main frame (so that submitting it navigates the whole page) * the re-appeared login form is in the main frame as well * there are some small iframes, which load quicker than the main one (that's probably what made Chrome believe the login was OK) Is the above true? If yes, I'm wondering if we should simply only check for the visible forms coming from the main frame, instead of accumulating the forms from all frames as you do now. 2) Please update the CL description to explain the problem and the fix. As a bonus point, please make the links to bugs clickable (http://crbug.com/123 instead [Bug123]). Also, why do you mention http://crbug.com/338650, when that one had already been fixed? Cheers, Vaclav https://codereview.chromium.org/293093002/diff/40001/chrome/browser/password_... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/293093002/diff/40001/chrome/browser/password_... chrome/browser/password_manager/password_manager_browsertest.cc:383: // Verify that we do not show password prompt when login failed from one Sorry, I have troubles understanding the test from the comment: 1) "failed from one iframe" -- did you mean "failed in one iframe", or something else? 2) Could you also explain both the navigation which happens in the iframe, and in the main frame, and how is that supposed to be interpreted by the password manager? https://codereview.chromium.org/293093002/diff/40001/chrome/browser/password_... chrome/browser/password_manager/password_manager_browsertest.cc:394: "window.location.href = 'failed.html';")); Why do you navigate the main frame to failed.html -- should that work with any other navigation as well? I just ran the test with the rest of your patch disabled (always sending true with AutofillHostMsg_PasswordFormsRendered from the PasswordAutofillAgent, and the test passed as well. https://codereview.chromium.org/293093002/diff/40001/components/autofill/cont... File components/autofill/content/common/autofill_messages.h (right): https://codereview.chromium.org/293093002/diff/40001/components/autofill/cont... components/autofill/content/common/autofill_messages.h:193: // forms are visible on the page (e.g. not set to display:none.), and if nit: if -> whether https://codereview.chromium.org/293093002/diff/40001/components/password_mana... File components/password_manager/core/browser/password_manager.cc (right): https://codereview.chromium.org/293093002/diff/40001/components/password_mana... components/password_manager/core/browser/password_manager.cc:366: // Records all visible forms in the page. page or frame? https://codereview.chromium.org/293093002/diff/40001/components/password_mana... components/password_manager/core/browser/password_manager.cc:368: all_visible_forms_.push_back(visible_forms[i]); nit: consider using std::vector::insert https://codereview.chromium.org/293093002/diff/40001/components/password_mana... components/password_manager/core/browser/password_manager.cc:388: while (all_visible_forms_.size()) { Did you just mean all_visible_forms_.clear()? This while loop looks very inefficient to achieve clear(), but maybe I'm missing something? Also below. https://codereview.chromium.org/293093002/diff/40001/components/password_mana... File components/password_manager/core/browser/password_manager.h (right): https://codereview.chromium.org/293093002/diff/40001/components/password_mana... components/password_manager/core/browser/password_manager.h:186: // Records all visible forms in a page. Please comment on the time interval for which this stores the visible forms (i.e., when the first form is inserted, and when the vector gets cleared again).
On 2014/05/23 12:18:36, vabr (Chromium) wrote: > Thanks for working on this! > > A couple of comments in addition to those code-bound below: > > 1) Your browser test is broken (it works without your patch as well), and does > not seem to be repeating what happens for http://kickstarter.com. You are right. Sorry about this. I have updated it locally but would like to confirm the question below with you before upload it. > What I see on kickstarter is: > * the login form is in the main frame (so that submitting it navigates the > whole page) > * the re-appeared login form is in the main frame as well > * there are some small iframes, which load quicker than the main one (that's > probably what made Chrome believe the login was OK) > > Is the above true? If yes, I'm wondering if we should simply only check for the > visible forms coming from the main frame, instead of accumulating the forms from > all frames as you do now. > It is true for http://kickstarter.com. Just a question - are there cases that login form is from a sub-frame rather than main frame? We might need to update some existing tests if login forms are always from main frame, for example, PasswordManagerBrowserTest.PromptAfterSubmitWithSubFrameNavigation. > 2) Please update the CL description to explain the problem and the fix. > As a bonus point, please make the links to bugs clickable > (http://crbug.com/123 instead [Bug123]). > Also, why do you mention http://crbug.com/338650, when that one had already > been fixed? > > Cheers, > Vaclav > > https://codereview.chromium.org/293093002/diff/40001/chrome/browser/password_... > File chrome/browser/password_manager/password_manager_browsertest.cc (right): > > https://codereview.chromium.org/293093002/diff/40001/chrome/browser/password_... > chrome/browser/password_manager/password_manager_browsertest.cc:383: // Verify > that we do not show password prompt when login failed from one > Sorry, I have troubles understanding the test from the comment: > 1) "failed from one iframe" -- did you mean "failed in one iframe", or something > else? > 2) Could you also explain both the navigation which happens in the iframe, and > in the main frame, and how is that supposed to be interpreted by the password > manager? > > https://codereview.chromium.org/293093002/diff/40001/chrome/browser/password_... > chrome/browser/password_manager/password_manager_browsertest.cc:394: > "window.location.href = 'failed.html';")); > Why do you navigate the main frame to failed.html -- should that work with any > other navigation as well? > > I just ran the test with the rest of your patch disabled (always sending true > with AutofillHostMsg_PasswordFormsRendered from the PasswordAutofillAgent, and > the test passed as well. > > https://codereview.chromium.org/293093002/diff/40001/components/autofill/cont... > File components/autofill/content/common/autofill_messages.h (right): > > https://codereview.chromium.org/293093002/diff/40001/components/autofill/cont... > components/autofill/content/common/autofill_messages.h:193: // forms are visible > on the page (e.g. not set to display:none.), and if > nit: if -> whether > > https://codereview.chromium.org/293093002/diff/40001/components/password_mana... > File components/password_manager/core/browser/password_manager.cc (right): > > https://codereview.chromium.org/293093002/diff/40001/components/password_mana... > components/password_manager/core/browser/password_manager.cc:366: // Records all > visible forms in the page. > page or frame? > > https://codereview.chromium.org/293093002/diff/40001/components/password_mana... > components/password_manager/core/browser/password_manager.cc:368: > all_visible_forms_.push_back(visible_forms[i]); > nit: consider using std::vector::insert > > https://codereview.chromium.org/293093002/diff/40001/components/password_mana... > components/password_manager/core/browser/password_manager.cc:388: while > (all_visible_forms_.size()) { > Did you just mean all_visible_forms_.clear()? > > This while loop looks very inefficient to achieve clear(), but maybe I'm missing > something? > > Also below. > > https://codereview.chromium.org/293093002/diff/40001/components/password_mana... > File components/password_manager/core/browser/password_manager.h (right): > > https://codereview.chromium.org/293093002/diff/40001/components/password_mana... > components/password_manager/core/browser/password_manager.h:186: // Records all > visible forms in a page. > Please comment on the time interval for which this stores the visible forms > (i.e., when the first form is inserted, and when the vector gets cleared again).
Hi, Sorry for the delay, Monday was a public holiday in where I work. On 2014/06/06 16:10:47, ziran.sun wrote: > On 2014/05/23 12:18:36, vabr (Chromium) wrote: > > Thanks for working on this! > > > > A couple of comments in addition to those code-bound below: > > > > 1) Your browser test is broken (it works without your patch as well), and does > > not seem to be repeating what happens for http://kickstarter.com. > > You are right. Sorry about this. I have updated it locally but would like to > confirm the question below with you before upload it. > > > What I see on kickstarter is: > > * the login form is in the main frame (so that submitting it navigates the > > whole page) > > * the re-appeared login form is in the main frame as well > > * there are some small iframes, which load quicker than the main one (that's > > probably what made Chrome believe the login was OK) > > > > Is the above true? If yes, I'm wondering if we should simply only check for > the > > visible forms coming from the main frame, instead of accumulating the forms > from > > all frames as you do now. > > > It is true for http://kickstarter.com. Just a question - are there cases that > login form is from a sub-frame rather than main frame? We might need to update > some existing tests if login forms are always from main frame, for example, > PasswordManagerBrowserTest.PromptAfterSubmitWithSubFrameNavigation. No, not all login forms are in the main frame. For example, the login form for espn.com opens in an iframe. Cheers, Vaclav > > > 2) Please update the CL description to explain the problem and the fix. > > As a bonus point, please make the links to bugs clickable > > (http://crbug.com/123 instead [Bug123]). > > Also, why do you mention http://crbug.com/338650, when that one had already > > been fixed? > > > > Cheers, > > Vaclav > > > > > https://codereview.chromium.org/293093002/diff/40001/chrome/browser/password_... > > File chrome/browser/password_manager/password_manager_browsertest.cc (right): > > > > > https://codereview.chromium.org/293093002/diff/40001/chrome/browser/password_... > > chrome/browser/password_manager/password_manager_browsertest.cc:383: // Verify > > that we do not show password prompt when login failed from one > > Sorry, I have troubles understanding the test from the comment: > > 1) "failed from one iframe" -- did you mean "failed in one iframe", or > something > > else? > > 2) Could you also explain both the navigation which happens in the iframe, and > > in the main frame, and how is that supposed to be interpreted by the password > > manager? > > > > > https://codereview.chromium.org/293093002/diff/40001/chrome/browser/password_... > > chrome/browser/password_manager/password_manager_browsertest.cc:394: > > "window.location.href = 'failed.html';")); > > Why do you navigate the main frame to failed.html -- should that work with any > > other navigation as well? > > > > I just ran the test with the rest of your patch disabled (always sending true > > with AutofillHostMsg_PasswordFormsRendered from the PasswordAutofillAgent, and > > the test passed as well. > > > > > https://codereview.chromium.org/293093002/diff/40001/components/autofill/cont... > > File components/autofill/content/common/autofill_messages.h (right): > > > > > https://codereview.chromium.org/293093002/diff/40001/components/autofill/cont... > > components/autofill/content/common/autofill_messages.h:193: // forms are > visible > > on the page (e.g. not set to display:none.), and if > > nit: if -> whether > > > > > https://codereview.chromium.org/293093002/diff/40001/components/password_mana... > > File components/password_manager/core/browser/password_manager.cc (right): > > > > > https://codereview.chromium.org/293093002/diff/40001/components/password_mana... > > components/password_manager/core/browser/password_manager.cc:366: // Records > all > > visible forms in the page. > > page or frame? > > > > > https://codereview.chromium.org/293093002/diff/40001/components/password_mana... > > components/password_manager/core/browser/password_manager.cc:368: > > all_visible_forms_.push_back(visible_forms[i]); > > nit: consider using std::vector::insert > > > > > https://codereview.chromium.org/293093002/diff/40001/components/password_mana... > > components/password_manager/core/browser/password_manager.cc:388: while > > (all_visible_forms_.size()) { > > Did you just mean all_visible_forms_.clear()? > > > > This while loop looks very inefficient to achieve clear(), but maybe I'm > missing > > something? > > > > Also below. > > > > > https://codereview.chromium.org/293093002/diff/40001/components/password_mana... > > File components/password_manager/core/browser/password_manager.h (right): > > > > > https://codereview.chromium.org/293093002/diff/40001/components/password_mana... > > components/password_manager/core/browser/password_manager.h:186: // Records > all > > visible forms in a page. > > Please comment on the time interval for which this stores the visible forms > > (i.e., when the first form is inserted, and when the vector gets cleared > again).
On 2014/06/10 12:05:36, vabr (Chromium) wrote: > Hi, > > Sorry for the delay, Monday was a public holiday in where I work. > > On 2014/06/06 16:10:47, ziran.sun wrote: > > On 2014/05/23 12:18:36, vabr (Chromium) wrote: > > > Thanks for working on this! > > > > > > A couple of comments in addition to those code-bound below: > > > > > > 1) Your browser test is broken (it works without your patch as well), and > does > > > not seem to be repeating what happens for http://kickstarter.com. > > > > You are right. Sorry about this. I have updated it locally but would like to > > confirm the question below with you before upload it. > > > > > What I see on kickstarter is: > > > * the login form is in the main frame (so that submitting it navigates the > > > whole page) > > > * the re-appeared login form is in the main frame as well > > > * there are some small iframes, which load quicker than the main one > (that's > > > probably what made Chrome believe the login was OK) > > > > > > Is the above true? If yes, I'm wondering if we should simply only check for > > the > > > visible forms coming from the main frame, instead of accumulating the forms > > from > > > all frames as you do now. > > > > > It is true for http://kickstarter.com. Just a question - are there cases that > > login form is from a sub-frame rather than main frame? We might need to update > > some existing tests if login forms are always from main frame, for example, > > PasswordManagerBrowserTest.PromptAfterSubmitWithSubFrameNavigation. > > No, not all login forms are in the main frame. > For example, the login form for http://espn.com opens in an iframe. > > Cheers, > Vaclav > Thanks Vaclav. So I take that we have to go for the solution to accumulate the forms from all frames? > > > > > 2) Please update the CL description to explain the problem and the fix. > > > As a bonus point, please make the links to bugs clickable > > > (http://crbug.com/123 instead [Bug123]). > > > Also, why do you mention http://crbug.com/338650, when that one had > already > > > been fixed? > > > > > > Cheers, > > > Vaclav > > > > > > > > > https://codereview.chromium.org/293093002/diff/40001/chrome/browser/password_... > > > File chrome/browser/password_manager/password_manager_browsertest.cc > (right): > > > > > > > > > https://codereview.chromium.org/293093002/diff/40001/chrome/browser/password_... > > > chrome/browser/password_manager/password_manager_browsertest.cc:383: // > Verify > > > that we do not show password prompt when login failed from one > > > Sorry, I have troubles understanding the test from the comment: > > > 1) "failed from one iframe" -- did you mean "failed in one iframe", or > > something > > > else? > > > 2) Could you also explain both the navigation which happens in the iframe, > and > > > in the main frame, and how is that supposed to be interpreted by the > password > > > manager? > > > > > > > > > https://codereview.chromium.org/293093002/diff/40001/chrome/browser/password_... > > > chrome/browser/password_manager/password_manager_browsertest.cc:394: > > > "window.location.href = 'failed.html';")); > > > Why do you navigate the main frame to failed.html -- should that work with > any > > > other navigation as well? > > > > > > I just ran the test with the rest of your patch disabled (always sending > true > > > with AutofillHostMsg_PasswordFormsRendered from the PasswordAutofillAgent, > and > > > the test passed as well. > > > > > > > > > https://codereview.chromium.org/293093002/diff/40001/components/autofill/cont... > > > File components/autofill/content/common/autofill_messages.h (right): > > > > > > > > > https://codereview.chromium.org/293093002/diff/40001/components/autofill/cont... > > > components/autofill/content/common/autofill_messages.h:193: // forms are > > visible > > > on the page (e.g. not set to display:none.), and if > > > nit: if -> whether > > > > > > > > > https://codereview.chromium.org/293093002/diff/40001/components/password_mana... > > > File components/password_manager/core/browser/password_manager.cc (right): > > > > > > > > > https://codereview.chromium.org/293093002/diff/40001/components/password_mana... > > > components/password_manager/core/browser/password_manager.cc:366: // Records > > all > > > visible forms in the page. > > > page or frame? > > > > > > > > > https://codereview.chromium.org/293093002/diff/40001/components/password_mana... > > > components/password_manager/core/browser/password_manager.cc:368: > > > all_visible_forms_.push_back(visible_forms[i]); > > > nit: consider using std::vector::insert > > > > > > > > > https://codereview.chromium.org/293093002/diff/40001/components/password_mana... > > > components/password_manager/core/browser/password_manager.cc:388: while > > > (all_visible_forms_.size()) { > > > Did you just mean all_visible_forms_.clear()? > > > > > > This while loop looks very inefficient to achieve clear(), but maybe I'm > > missing > > > something? > > > > > > Also below. > > > > > > > > > https://codereview.chromium.org/293093002/diff/40001/components/password_mana... > > > File components/password_manager/core/browser/password_manager.h (right): > > > > > > > > > https://codereview.chromium.org/293093002/diff/40001/components/password_mana... > > > components/password_manager/core/browser/password_manager.h:186: // Records > > all > > > visible forms in a page. > > > Please comment on the time interval for which this stores the visible forms > > > (i.e., when the first form is inserted, and when the vector gets cleared > > again).
On 2014/06/10 12:48:52, ziran.sun wrote: > On 2014/06/10 12:05:36, vabr (Chromium) wrote: > > Hi, > > > > Sorry for the delay, Monday was a public holiday in where I work. > > > > On 2014/06/06 16:10:47, ziran.sun wrote: > > > On 2014/05/23 12:18:36, vabr (Chromium) wrote: > > > > Thanks for working on this! > > > > > > > > A couple of comments in addition to those code-bound below: > > > > > > > > 1) Your browser test is broken (it works without your patch as well), and > > does > > > > not seem to be repeating what happens for http://kickstarter.com. > > > > > > You are right. Sorry about this. I have updated it locally but would like to > > > confirm the question below with you before upload it. > > > > > > > What I see on kickstarter is: > > > > * the login form is in the main frame (so that submitting it navigates > the > > > > whole page) > > > > * the re-appeared login form is in the main frame as well > > > > * there are some small iframes, which load quicker than the main one > > (that's > > > > probably what made Chrome believe the login was OK) > > > > > > > > Is the above true? If yes, I'm wondering if we should simply only check > for > > > the > > > > visible forms coming from the main frame, instead of accumulating the > forms > > > from > > > > all frames as you do now. > > > > > > > It is true for http://kickstarter.com. Just a question - are there cases > that > > > login form is from a sub-frame rather than main frame? We might need to > update > > > some existing tests if login forms are always from main frame, for example, > > > PasswordManagerBrowserTest.PromptAfterSubmitWithSubFrameNavigation. > > > > No, not all login forms are in the main frame. > > For example, the login form for http://espn.com opens in an iframe. > > > > Cheers, > > Vaclav > > > > Thanks Vaclav. So I take that we have to go for the solution to accumulate the > forms from all frames? Oh, you are right, the failed login might be indicated by a form in a sub-frame. So accumulating forms from all frames sounds like a reasonable solution. Thanks! Vaclav > > > > > > > > 2) Please update the CL description to explain the problem and the fix. > > > > As a bonus point, please make the links to bugs clickable > > > > (http://crbug.com/123 instead [Bug123]). > > > > Also, why do you mention http://crbug.com/338650, when that one had > > already > > > > been fixed? > > > > > > > > Cheers, > > > > Vaclav > > > > > > > > > > > > > > https://codereview.chromium.org/293093002/diff/40001/chrome/browser/password_... > > > > File chrome/browser/password_manager/password_manager_browsertest.cc > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/293093002/diff/40001/chrome/browser/password_... > > > > chrome/browser/password_manager/password_manager_browsertest.cc:383: // > > Verify > > > > that we do not show password prompt when login failed from one > > > > Sorry, I have troubles understanding the test from the comment: > > > > 1) "failed from one iframe" -- did you mean "failed in one iframe", or > > > something > > > > else? > > > > 2) Could you also explain both the navigation which happens in the iframe, > > and > > > > in the main frame, and how is that supposed to be interpreted by the > > password > > > > manager? > > > > > > > > > > > > > > https://codereview.chromium.org/293093002/diff/40001/chrome/browser/password_... > > > > chrome/browser/password_manager/password_manager_browsertest.cc:394: > > > > "window.location.href = 'failed.html';")); > > > > Why do you navigate the main frame to failed.html -- should that work with > > any > > > > other navigation as well? > > > > > > > > I just ran the test with the rest of your patch disabled (always sending > > true > > > > with AutofillHostMsg_PasswordFormsRendered from the PasswordAutofillAgent, > > and > > > > the test passed as well. > > > > > > > > > > > > > > https://codereview.chromium.org/293093002/diff/40001/components/autofill/cont... > > > > File components/autofill/content/common/autofill_messages.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/293093002/diff/40001/components/autofill/cont... > > > > components/autofill/content/common/autofill_messages.h:193: // forms are > > > visible > > > > on the page (e.g. not set to display:none.), and if > > > > nit: if -> whether > > > > > > > > > > > > > > https://codereview.chromium.org/293093002/diff/40001/components/password_mana... > > > > File components/password_manager/core/browser/password_manager.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/293093002/diff/40001/components/password_mana... > > > > components/password_manager/core/browser/password_manager.cc:366: // > Records > > > all > > > > visible forms in the page. > > > > page or frame? > > > > > > > > > > > > > > https://codereview.chromium.org/293093002/diff/40001/components/password_mana... > > > > components/password_manager/core/browser/password_manager.cc:368: > > > > all_visible_forms_.push_back(visible_forms[i]); > > > > nit: consider using std::vector::insert > > > > > > > > > > > > > > https://codereview.chromium.org/293093002/diff/40001/components/password_mana... > > > > components/password_manager/core/browser/password_manager.cc:388: while > > > > (all_visible_forms_.size()) { > > > > Did you just mean all_visible_forms_.clear()? > > > > > > > > This while loop looks very inefficient to achieve clear(), but maybe I'm > > > missing > > > > something? > > > > > > > > Also below. > > > > > > > > > > > > > > https://codereview.chromium.org/293093002/diff/40001/components/password_mana... > > > > File components/password_manager/core/browser/password_manager.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/293093002/diff/40001/components/password_mana... > > > > components/password_manager/core/browser/password_manager.h:186: // > Records > > > all > > > > visible forms in a page. > > > > Please comment on the time interval for which this stores the visible > forms > > > > (i.e., when the first form is inserted, and when the vector gets cleared > > > again).
On 2014/06/10 13:03:43, vabr (Chromium) wrote: > On 2014/06/10 12:48:52, ziran.sun wrote: > > On 2014/06/10 12:05:36, vabr (Chromium) wrote: > > > Hi, > > > > > > Sorry for the delay, Monday was a public holiday in where I work. > > > > > > On 2014/06/06 16:10:47, ziran.sun wrote: > > > > On 2014/05/23 12:18:36, vabr (Chromium) wrote: > > > > > Thanks for working on this! > > > > > > > > > > A couple of comments in addition to those code-bound below: > > > > > > > > > > 1) Your browser test is broken (it works without your patch as well), > and > > > does > > > > > not seem to be repeating what happens for http://kickstarter.com. > > > > > > > > You are right. Sorry about this. I have updated it locally but would like > to > > > > confirm the question below with you before upload it. > > > > > > > > > What I see on kickstarter is: > > > > > * the login form is in the main frame (so that submitting it navigates > > the > > > > > whole page) > > > > > * the re-appeared login form is in the main frame as well > > > > > * there are some small iframes, which load quicker than the main one > > > (that's > > > > > probably what made Chrome believe the login was OK) > > > > > > > > > > Is the above true? If yes, I'm wondering if we should simply only check > > for > > > > the > > > > > visible forms coming from the main frame, instead of accumulating the > > forms > > > > from > > > > > all frames as you do now. > > > > > > > > > It is true for http://kickstarter.com. Just a question - are there cases > > that > > > > login form is from a sub-frame rather than main frame? We might need to > > update > > > > some existing tests if login forms are always from main frame, for > example, > > > > PasswordManagerBrowserTest.PromptAfterSubmitWithSubFrameNavigation. > > > > > > No, not all login forms are in the main frame. > > > For example, the login form for http://espn.com opens in an iframe. > > > > > > Cheers, > > > Vaclav > > > > > > > Thanks Vaclav. So I take that we have to go for the solution to accumulate the > > forms from all frames? > Oh, you are right, the failed login might be indicated by a form in a sub-frame. > So accumulating forms from all frames sounds like a reasonable solution. > > Thanks! > Vaclav > > > > > > > > > > > > 2) Please update the CL description to explain the problem and the fix. > > > > > As a bonus point, please make the links to bugs clickable > > > > > (http://crbug.com/123 instead [Bug123]). > > > > > Also, why do you mention http://crbug.com/338650, when that one had > > > already > > > > > been fixed? > > > > > > > > > > Cheers, > > > > > Vaclav > > > > > > > > > > > > > > > > > > > In http://www.moneycontrol.com given by http://crbug.com/338650, if you login with wrong credentials, then refresh the page, you will see "Save password" prompt. > https://codereview.chromium.org/293093002/diff/40001/chrome/browser/password_... > > > > > File chrome/browser/password_manager/password_manager_browsertest.cc > > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/293093002/diff/40001/chrome/browser/password_... > > > > > chrome/browser/password_manager/password_manager_browsertest.cc:383: // > > > Verify > > > > > that we do not show password prompt when login failed from one > > > > > Sorry, I have troubles understanding the test from the comment: > > > > > 1) "failed from one iframe" -- did you mean "failed in one iframe", or > > > > something > > > > > else? > > > > > 2) Could you also explain both the navigation which happens in the > iframe, > > > and > > > > > in the main frame, and how is that supposed to be interpreted by the > > > password > > > > > manager? > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/293093002/diff/40001/chrome/browser/password_... > > > > > chrome/browser/password_manager/password_manager_browsertest.cc:394: > > > > > "window.location.href = 'failed.html';")); > > > > > Why do you navigate the main frame to failed.html -- should that work > with > > > any > > > > > other navigation as well? > > > > > > > > > > I just ran the test with the rest of your patch disabled (always sending > > > true > > > > > with AutofillHostMsg_PasswordFormsRendered from the > PasswordAutofillAgent, > > > and > > > > > the test passed as well. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/293093002/diff/40001/components/autofill/cont... > > > > > File components/autofill/content/common/autofill_messages.h (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/293093002/diff/40001/components/autofill/cont... > > > > > components/autofill/content/common/autofill_messages.h:193: // forms are > > > > visible > > > > > on the page (e.g. not set to display:none.), and if > > > > > nit: if -> whether > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/293093002/diff/40001/components/password_mana... > > > > > File components/password_manager/core/browser/password_manager.cc > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/293093002/diff/40001/components/password_mana... > > > > > components/password_manager/core/browser/password_manager.cc:366: // > > Records > > > > all > > > > > visible forms in the page. > > > > > page or frame? > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/293093002/diff/40001/components/password_mana... > > > > > components/password_manager/core/browser/password_manager.cc:368: > > > > > all_visible_forms_.push_back(visible_forms[i]); > > > > > nit: consider using std::vector::insert > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/293093002/diff/40001/components/password_mana... > > > > > components/password_manager/core/browser/password_manager.cc:388: while > > > > > (all_visible_forms_.size()) { > > > > > Did you just mean all_visible_forms_.clear()? > > > > > > > > > > This while loop looks very inefficient to achieve clear(), but maybe I'm > > > > missing > > > > > something? > > > > > > > > > > Also below. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/293093002/diff/40001/components/password_mana... > > > > > File components/password_manager/core/browser/password_manager.h > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/293093002/diff/40001/components/password_mana... > > > > > components/password_manager/core/browser/password_manager.h:186: // > > Records > > > > all > > > > > visible forms in a page. > > > > > Please comment on the time interval for which this stores the visible > > forms > > > > > (i.e., when the first form is inserted, and when the vector gets cleared > > > > again).
Hi, > In http://www.moneycontrol.com given by http://crbug.com/338650, if you login with wrong credentials, then refresh the page, you will see "Save password" prompt. I don't understand, what you mean. The issue described in http://crbug.com/338650 (seeing prompt on just reload of the page) does not reproduce for me with a tip-of-the-tree chromium build. If it does reproduce for you, then please re-open the bug and post which version and reproduction steps you took. If you only mean the page http://www.moneycontrol.com, but not the bug http://crbug.com/338650, then do not mention the bug here. In particular, if the issue with the page is that you enter wrong credentials, click "Sign In", and see the prompt, then it is not http://crbug.com/338650. You can still file a new bug for http://www.moneycontrol.com, including the steps to reproduce the issue (I could not reproduce that issue either), or add that as a case for an existing bug report if that describes the same bug. In your code, I still have some issues with the browser test (commented below). For the rest I only have minor comments. Please have a look at them. Also, please make sure to loop in an OWNER for components/autofill/content/common/autofill_messages.h. That file needs to be seen by somebody from Chrome security. Thanks, Vaclav https://codereview.chromium.org/293093002/diff/80001/chrome/browser/password_... File chrome/browser/password_manager/password_manager_browsertest.cc (right): https://codereview.chromium.org/293093002/diff/80001/chrome/browser/password_... chrome/browser/password_manager/password_manager_browsertest.cc:384: // with a sub-frame navigation happens first. nit: with -> when https://codereview.chromium.org/293093002/diff/80001/chrome/browser/password_... chrome/browser/password_manager/password_manager_browsertest.cc:398: ASSERT_TRUE(content::ExecuteScript(RenderViewHost(), fill_and_submit)); Sorry, but I still don't see how this test relates to the original problem. Why does there need to be the navigation to the second frame before the form is even submitted? Should not the test be something like: 1. Submit a form in a sub-frame. 2. Navigate the main frame to multi_frames.html again. Then it would test that it only takes one sub-frame to contain the login form again, to stop the infobar from showing. https://codereview.chromium.org/293093002/diff/80001/components/password_mana... File components/password_manager/core/browser/password_manager.cc (right): https://codereview.chromium.org/293093002/diff/80001/components/password_mana... components/password_manager/core/browser/password_manager.cc:366: // Stores all visible forms from the frame. optional nit: Stores -> Store We usually use imperative to describe what the coming line does, see, e.g., the "Clear..." comments below. https://codereview.chromium.org/293093002/diff/80001/components/password_mana... components/password_manager/core/browser/password_manager.cc:366: // Stores all visible forms from the frame. optional nit: It's up to you if you say "record" or "store", but please be consistent. Currently you say "record" in the header, and "store" in the .cc file. That's confusing, as it sounds like you distinguish between them on purpose, because they are two different things. https://codereview.chromium.org/293093002/diff/80001/components/password_mana... components/password_manager/core/browser/password_manager.cc:368: all_visible_forms_.insert(all_visible_forms_.end(), visible_forms[i]); No, this is not what I meant. :) I meant: "Use insert to spare yourself the for-cycle", in other words this: all_visible_forms_.insert(all_visible_forms_.end(), visible_forms.begin(), visible_forms.end()); https://codereview.chromium.org/293093002/diff/80001/components/password_mana... File components/password_manager/core/browser/password_manager.h (right): https://codereview.chromium.org/293093002/diff/80001/components/password_mana... components/password_manager/core/browser/password_manager.h:186: // Records all visible forms in a page. It starts storing when the first Please always make sure to give context, especially when using pronouns. (What is "It"?) Suggested reformulation: Records all visible forms seen during a page load, in all frames of the page. When the page stops loading, the password manager checks if one of the recorded forms matches the login form from the previous page (to see if the login was a failure), and clears the vector.
On 2014/06/11 08:22:12, vabr (Chromium) wrote: > Hi, > > > In http://www.moneycontrol.com given by http://crbug.com/338650, if you login > with wrong credentials, then refresh the page, you will see "Save password" > prompt. > > I don't understand, what you mean. > > The issue described in http://crbug.com/338650 (seeing prompt on just reload of > the page) does not reproduce for me with a tip-of-the-tree chromium build. If it > does reproduce for you, then please re-open the bug and post which version and > reproduction steps you took. > > If you only mean the page http://www.moneycontrol.com, but not the bug > http://crbug.com/338650, then do not mention the bug here. In particular, if the > issue with the page is that you enter wrong credentials, click "Sign In", and > see the prompt, then it is not http://crbug.com/338650. You can still file a new > bug for http://www.moneycontrol.com, including the steps to reproduce the issue > (I could not reproduce that issue either), or add that as a case for an existing > bug report if that describes the same bug. > > > In your code, I still have some issues with the browser test (commented below). > For the rest I only have minor comments. Please have a look at them. > > Also, please make sure to loop in an OWNER for > components/autofill/content/common/autofill_messages.h. That file needs to be > seen by somebody from Chrome security. > > Thanks, > Vaclav > > https://codereview.chromium.org/293093002/diff/80001/chrome/browser/password_... > File chrome/browser/password_manager/password_manager_browsertest.cc (right): > > https://codereview.chromium.org/293093002/diff/80001/chrome/browser/password_... > chrome/browser/password_manager/password_manager_browsertest.cc:384: // with a > sub-frame navigation happens first. > nit: with -> when > > https://codereview.chromium.org/293093002/diff/80001/chrome/browser/password_... > chrome/browser/password_manager/password_manager_browsertest.cc:398: > ASSERT_TRUE(content::ExecuteScript(RenderViewHost(), fill_and_submit)); > Sorry, but I still don't see how this test relates to the original problem. Why > does there need to be the navigation to the second frame before the form is even > submitted? > > Should not the test be something like: > 1. Submit a form in a sub-frame. > 2. Navigate the main frame to multi_frames.html again. > > Then it would test that it only takes one sub-frame to contain the login form > again, to stop the infobar from showing. > > https://codereview.chromium.org/293093002/diff/80001/components/password_mana... > File components/password_manager/core/browser/password_manager.cc (right): > > https://codereview.chromium.org/293093002/diff/80001/components/password_mana... > components/password_manager/core/browser/password_manager.cc:366: // Stores all > visible forms from the frame. > optional nit: Stores -> Store > We usually use imperative to describe what the coming line does, see, e.g., the > "Clear..." comments below. > > https://codereview.chromium.org/293093002/diff/80001/components/password_mana... > components/password_manager/core/browser/password_manager.cc:366: // Stores all > visible forms from the frame. > optional nit: It's up to you if you say "record" or "store", but please be > consistent. Currently you say "record" in the header, and "store" in the .cc > file. That's confusing, as it sounds like you distinguish between them on > purpose, because they are two different things. > > https://codereview.chromium.org/293093002/diff/80001/components/password_mana... > components/password_manager/core/browser/password_manager.cc:368: > all_visible_forms_.insert(all_visible_forms_.end(), visible_forms[i]); > No, this is not what I meant. :) > I meant: "Use insert to spare yourself the for-cycle", in other words this: > > all_visible_forms_.insert(all_visible_forms_.end(), > visible_forms.begin(), > visible_forms.end()); > > https://codereview.chromium.org/293093002/diff/80001/components/password_mana... > File components/password_manager/core/browser/password_manager.h (right): > > https://codereview.chromium.org/293093002/diff/80001/components/password_mana... > components/password_manager/core/browser/password_manager.h:186: // Records all > visible forms in a page. It starts storing when the first > Please always make sure to give context, especially when using pronouns. (What > is "It"?) Suggested reformulation: > > Records all visible forms seen during a page load, in all frames of the page. > When the page stops loading, the password manager checks if one of the recorded > forms matches the login form from the previous page (to see if the login was a > failure), and clears the vector. Thanks very much vabr. You points on tests make sense. All comments have been addressed.
Updated code as per vabr's review comments. Please review. Thanks!
Added OWNERs in reviewer list. Thanks.
What are the implications if the frames are cross-origin, e.g. site A is framing a login from site B. Do we leak information to back to A that would otherwise be unavailable?
Thanks for improving the tests, ziran.sun! LGTM. Tom, > What are the implications if the frames are > cross-origin, e.g. site A is framing a login > from site B. Do we leak information to back > to A that would otherwise be unavailable? I don't think this leak could happen. The renderer does not put the forms from different frames together. It's first in browser, when the forms are put together in one vector. That vector or any derived information is never sent back to the renderer, and the only side-effect is whether the save-password prompt is shown or not. I'll let ziran.sun comment on your question as well, in case he wants to add something. Cheers, Vaclav https://codereview.chromium.org/293093002/diff/120001/chrome/test/data/passwo... File chrome/test/data/password/multi_frames.html (right): https://codereview.chromium.org/293093002/diff/120001/chrome/test/data/passwo... chrome/test/data/password/multi_frames.html:4: <iframe src="no_form.html" id="no_form_frame" name="no_form_frame"> optional nit: You could also re-use 'done.html' and add a sentence in the body of done.html saying something like: "This file should not contain any forms." But if you prefer a separate no_form.html, that's fine with me.
On 2014/06/13 08:12:44, vabr (Chromium) wrote: > Thanks for improving the tests, ziran.sun! LGTM. > > Tom, > > What are the implications if the frames are > > cross-origin, e.g. site A is framing a login > > from site B. Do we leak information to back > > to A that would otherwise be unavailable? > I don't think this leak could happen. > The renderer does not put the forms from different > frames together. It's first in browser, when the > forms are put together in one vector. That > vector or any derived information is never sent > back to the renderer, and the only side-effect > is whether the save-password prompt is shown > or not. > > I'll let ziran.sun comment on your question as > well, in case he wants to add something. > > Cheers, > Vaclav Yes, the forms are put together in browser and never sent back to renderer. I personally don't see information leak here either. > > https://codereview.chromium.org/293093002/diff/120001/chrome/test/data/passwo... > File chrome/test/data/password/multi_frames.html (right): > > https://codereview.chromium.org/293093002/diff/120001/chrome/test/data/passwo... > chrome/test/data/password/multi_frames.html:4: <iframe src="no_form.html" > id="no_form_frame" name="no_form_frame"> > optional nit: You could also re-use 'done.html' and add a sentence in the body > of done.html saying something like: "This file should not contain any forms." > But if you prefer a separate no_form.html, that's fine with me.
Update test code as per Vaclav's suggestion. https://codereview.chromium.org/293093002/diff/120001/chrome/test/data/passwo... File chrome/test/data/password/multi_frames.html (right): https://codereview.chromium.org/293093002/diff/120001/chrome/test/data/passwo... chrome/test/data/password/multi_frames.html:4: <iframe src="no_form.html" id="no_form_frame" name="no_form_frame"> On 2014/06/13 08:12:44, vabr (Chromium) wrote: > optional nit: You could also re-use 'done.html' and add a sentence in the body > of done.html saying something like: "This file should not contain any forms." > But if you prefer a separate no_form.html, that's fine with me. Done.
Messages LGTM.
LGTM. Sorry, I didn't realize that you were waiting for my review!
Actually, LGTM with a nit: (I only looked at the *autofill/* changes) https://codereview.chromium.org/293093002/diff/140001/components/autofill/con... File components/autofill/content/common/autofill_messages.h (right): https://codereview.chromium.org/293093002/diff/140001/components/autofill/con... components/autofill/content/common/autofill_messages.h:202: bool /* true if all frames have been rendered */) nit: The comment should be a variable name, rather than a sentence.
The CQ bit was checked by ziran.sun@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ziran.sun@samsung.com/293093002/180001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
Message was sent while issue was closed.
Change committed as 279123 |
