Do not save passwords if the landing page has HTTP status 5xx or 4xx

Do not save passwords if the landing page has HTTP status 5xx or 4xx

If the server throws internal error, access denied, page not found etc.
after a login attempt, there is no reason to believe that the user put
in credentials they would like to save. So we should not suggest
saving in that case.

BUG=432592
Committed: https://crrev.com/a3d924feb1bd3e3e2dab6ba85786db63334e8312
Cr-Commit-Position: refs/heads/master@{#304783}

[Sunil Ratnu, 2014-11-17 03:27:15]

Patchset #1 (id:1) has been deleted

[Sunil Ratnu, 2014-11-17 03:28:29]

sunil.ratnu@samsung.com changed reviewers:
+ vabr@chromium.org

[Sunil Ratnu, 2014-11-17 03:28:30]

Hi Vaclav,

Please have a look.

Thanks!
Sunil

[vabr (Chromium), 2014-11-17 15:32:15]

Thanks, Sunil Ratnu.

So far this looks good (do read the comments, though).

But please add a test. You don't have to test for all possible codes, and
checking for HTTP 404 should be easy to set up in the browser test.

Cheers,
Vaclav

https://codereview.chromium.org/733953002/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/733953002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/chrome_password_manager_client.cc:294:
DCHECK(web_contents());
Please create here an instance of BrowserSavePasswordProgressLogger and log:
(1) the name of this method (via LogMessage)
(2) the HTTP status code number (if obtained, via LogNumber)

Only create the browser if IsLoggingActive() is true, and add new string IDs for
both new messages. Feel free to get inspiration from logging in, e.g.,
PasswordManager::ProvisionallySavePassword.

https://codereview.chromium.org/733953002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/chrome_password_manager_client.cc:300: if
(http_status_code >= 400 && http_status_code <= 600)
Please change "<= 600" to "< 600".
There does not seem to be a HTTP 600 status code, but if somebody later
introduces the 6xx family of codes, the code should not make false assumptions
about it.

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

https://codereview.chromium.org/733953002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_manager.cc:493: if
(client_->WasLastNavigationHTTPError())
Would you mind moving this whole check up, to just after the DCHECK on line 452?
It is a simple information to check, so let's make sure we don't have to do the
for-loop if this check fails.

https://codereview.chromium.org/733953002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_manager.cc:494: return;
You need to call SubmitFailed() on the saved form and release it, as is done
above in the for-cycle iterating over all_visible_forms_.
Also, please log STRING_DECISION_DROP here.

[Sunil Ratnu, 2014-11-18 06:17:17]

Patchset #2 (id:40001) has been deleted

[Sunil Ratnu, 2014-11-18 06:40:06]

Hi Vaclav,

Thanks for the review. I've made the suggested changes and also written the test
but unexpectedly the test is timing out i.e. the form (present in browser window
created by test suit) does not move to the next does_not_exist page (Ideally it
should have). This should have atleast either failed or passed. Please have a
look if I am missing something?

Thanks!
Sunil

https://codereview.chromium.org/733953002/diff/20001/chrome/browser/password_...
File chrome/browser/password_manager/chrome_password_manager_client.cc (right):

https://codereview.chromium.org/733953002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/chrome_password_manager_client.cc:294:
DCHECK(web_contents());
On 2014/11/17 15:32:15, vabr (Chromium) wrote:
> Please create here an instance of BrowserSavePasswordProgressLogger and log:
> (1) the name of this method (via LogMessage)
> (2) the HTTP status code number (if obtained, via LogNumber)
> 
> Only create the browser if IsLoggingActive() is true, and add new string IDs
for
> both new messages. Feel free to get inspiration from logging in, e.g.,
> PasswordManager::ProvisionallySavePassword.

Done.

https://codereview.chromium.org/733953002/diff/20001/chrome/browser/password_...
chrome/browser/password_manager/chrome_password_manager_client.cc:300: if
(http_status_code >= 400 && http_status_code <= 600)
On 2014/11/17 15:32:15, vabr (Chromium) wrote:
> Please change "<= 600" to "< 600".
> There does not seem to be a HTTP 600 status code, but if somebody later
> introduces the 6xx family of codes, the code should not make false assumptions
> about it.

Done.

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

https://codereview.chromium.org/733953002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_manager.cc:493: if
(client_->WasLastNavigationHTTPError())
On 2014/11/17 15:32:15, vabr (Chromium) wrote:
> Would you mind moving this whole check up, to just after the DCHECK on line
452?
> It is a simple information to check, so let's make sure we don't have to do
the
> for-loop if this check fails.

Done.

https://codereview.chromium.org/733953002/diff/20001/components/password_mana...
components/password_manager/core/browser/password_manager.cc:494: return;
On 2014/11/17 15:32:15, vabr (Chromium) wrote:
> You need to call SubmitFailed() on the saved form and release it, as is done
> above in the for-cycle iterating over all_visible_forms_.
> Also, please log STRING_DECISION_DROP here.

Done.

[vabr (Chromium), 2014-11-18 10:10:11]

Hi Sunil,

Thanks for the new patch set, I added some comments.

As for the browser test: you will need to register a request handler for a fixed
URL, and set the return HTTP status code in the returned HttpResponse there
manually. See the initial comments in
net/test/embedded_test_server/embedded_test_server.h
(https://code.google.com/p/chromium/codesearch#chromium/src/net/test/embedded_...).
Although not as simple as requesting a non-existent file (which I hoped would
have worked, but it apparently hadn't), it has the nice feature of choosing the
response HTTP status code directly.

Cheers,
Vaclav

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

https://codereview.chromium.org/733953002/diff/60001/chrome/browser/password_...
chrome/browser/password_manager/chrome_password_manager_client.cc:303:
ChromePasswordManagerClient* client =
Don't retrieve the |client| and use |this| instead. ;-)

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

https://codereview.chromium.org/733953002/diff/60001/components/autofill/core...
components/autofill/core/common/save_password_progress_logger.cc:171: return
"HTTP Status Code for Landing Page";
nit: No need to capitalise non-names. Please write:
"HTTP status code for landing page"

https://codereview.chromium.org/733953002/diff/60001/components/autofill/core...
File components/autofill/core/common/save_password_progress_logger.h (right):

https://codereview.chromium.org/733953002/diff/60001/components/autofill/core...
components/autofill/core/common/save_password_progress_logger.h:103:
STRING_WAS_LAST_NAVIGATION_HTTP_ERROR,
nit: Please be consistent with the rest of names and append _METHOD at the enum
name, to signal that this string describes a method name:
STRING_WAS_LAST_NAVIGATION_HTTP_ERROR_METHOD

[Sunil Ratnu, 2014-11-18 12:58:39]

Hi Vaclav,

Thank you for the review and valuable suggestions. The test works fine now. The
culprit being the use of document.getElementsById() instead of the correct
document.getElementById().

Please have a look once again.

Thanks!
Sunil

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

https://codereview.chromium.org/733953002/diff/60001/chrome/browser/password_...
chrome/browser/password_manager/chrome_password_manager_client.cc:303:
ChromePasswordManagerClient* client =
On 2014/11/18 10:10:11, vabr (Chromium) wrote:
> Don't retrieve the |client| and use |this| instead. ;-)

The function had const in its signature. Hence, I did that. :)

Now we have to remove const otherwise it will throw a compile time error as we
are trying to pass const ChromePasswordManagerClient and there would be no
conversion from const ChromePasswordManagerClient* into PasswordManagerClient*.

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

https://codereview.chromium.org/733953002/diff/60001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:884:
"document.getElementsById('username_field_http_error').value = 'temp';"
The culprit for timing out of the test. :/ Corrected it now and everything works
fine now. Sigh :)

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

https://codereview.chromium.org/733953002/diff/60001/components/autofill/core...
components/autofill/core/common/save_password_progress_logger.cc:171: return
"HTTP Status Code for Landing Page";
On 2014/11/18 10:10:11, vabr (Chromium) wrote:
> nit: No need to capitalise non-names. Please write:
> "HTTP status code for landing page"

Done.

https://codereview.chromium.org/733953002/diff/60001/components/autofill/core...
File components/autofill/core/common/save_password_progress_logger.h (right):

https://codereview.chromium.org/733953002/diff/60001/components/autofill/core...
components/autofill/core/common/save_password_progress_logger.h:103:
STRING_WAS_LAST_NAVIGATION_HTTP_ERROR,
On 2014/11/18 10:10:11, vabr (Chromium) wrote:
> nit: Please be consistent with the rest of names and append _METHOD at the
enum
> name, to signal that this string describes a method name:
> STRING_WAS_LAST_NAVIGATION_HTTP_ERROR_METHOD

Done.

[vabr (Chromium), 2014-11-18 14:22:49]

LGTM, thanks!

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

https://codereview.chromium.org/733953002/diff/60001/chrome/browser/password_...
chrome/browser/password_manager/chrome_password_manager_client.cc:303:
ChromePasswordManagerClient* client =
On 2014/11/18 12:58:39, Sunil Ratnu wrote:
> On 2014/11/18 10:10:11, vabr (Chromium) wrote:
> > Don't retrieve the |client| and use |this| instead. ;-)
> 
> The function had const in its signature. Hence, I did that. :)
> 
> Now we have to remove const otherwise it will throw a compile time error as we
> are trying to pass const ChromePasswordManagerClient and there would be no
> conversion from const ChromePasswordManagerClient* into
PasswordManagerClient*.

This should be fixed by https://codereview.chromium.org/735733002/.
Your CL probably lands first, so I will rebase and mark your new method as const
again.
If your CL gets delayed, you can mark it as const yourself.

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

https://codereview.chromium.org/733953002/diff/60001/chrome/browser/password_...
chrome/browser/password_manager/password_manager_browsertest.cc:884:
"document.getElementsById('username_field_http_error').value = 'temp';"
On 2014/11/18 12:58:39, Sunil Ratnu wrote:
> The culprit for timing out of the test. :/ Corrected it now and everything
works
> fine now. Sigh :)

Good catch! Glad to see keeping this test simple.

[Sunil Ratnu, 2014-11-18 14:45:12]

The CQ bit was checked by sunil.ratnu@samsung.com

[commit-bot: I haz the power, 2014-11-18 14:46:57]

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

[commit-bot: I haz the power, 2014-11-18 15:26:30]

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

[commit-bot: I haz the power, 2014-11-18 15:26:31]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[vabr (Chromium), 2014-11-18 15:38:06]

The CQ bit was checked by vabr@chromium.org

[commit-bot: I haz the power, 2014-11-18 15:38:41]

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

[commit-bot: I haz the power, 2014-11-18 16:37:04]

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

[commit-bot: I haz the power, 2014-11-18 16:37:05]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[vabr (Chromium), 2014-11-18 16:44:31]

Sunil Ratnu,

It looks like PromptAfterSubmitWithSubFrameNavigation is broken now -- I guess
it navigates to some non-existing files, which should be easy to fix by adding
them (or changing them to existing files).

While you are fixing this, https://codereview.chromium.org/735733002/ might
land. In that case please make sure to rebase and make your newly added method
const.

Thanks!
Vaclav

[Sunil Ratnu, 2014-11-19 06:31:55]

Patchset #4 (id:100001) has been deleted

[Sunil Ratnu, 2014-11-19 07:22:26]

On 2014/11/18 16:44:31, vabr (Chromium) wrote:
> Sunil Ratnu,
> 
> It looks like PromptAfterSubmitWithSubFrameNavigation is broken now -- I guess
> it navigates to some non-existing files, which should be easy to fix by adding
> them (or changing them to existing files).
> 
> While you are fixing this, https://codereview.chromium.org/735733002/ might
> land. In that case please make sure to rebase and make your newly added method
> const.
> 
> Thanks!
> Vaclav

Thanks Vaclav for the review. I've added the missing file and rebased the patch.
The test doesn't fail anymore.

Thanks!
Sunil

[Sunil Ratnu, 2014-11-19 07:24:13]

The CQ bit was checked by sunil.ratnu@samsung.com

[commit-bot: I haz the power, 2014-11-19 07:24:55]

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

[commit-bot: I haz the power, 2014-11-19 08:23:44]

Committed patchset #4 (id:120001)

[commit-bot: I haz the power, 2014-11-19 08:24:24]

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

[vabr (Chromium), 2014-11-19 10:04:02]

Thank you, Sunil Ratnu.

The landed patch set LGTM.

I have one post-landing commit, though. If you find time and fix that in a short
follow-up CL, that would be great.

Thanks!
Vaclav

https://codereview.chromium.org/733953002/diff/120001/chrome/test/data/passwo...
File chrome/test/data/password/other.html (right):

https://codereview.chromium.org/733953002/diff/120001/chrome/test/data/passwo...
chrome/test/data/password/other.html:3: Navigation complete.
Could you please add a not in this file, saying something like:
"Do not merge this file with done.html, the test
PromptAfterSubmitWithSubFrameNavigation needs two such files, with different
filenames."

[Sunil Ratnu, 2014-11-19 11:20:50]

https://codereview.chromium.org/733953002/diff/120001/chrome/test/data/passwo...
File chrome/test/data/password/other.html (right):

https://codereview.chromium.org/733953002/diff/120001/chrome/test/data/passwo...
chrome/test/data/password/other.html:3: Navigation complete.
On 2014/11/19 10:04:02, vabr (Chromium) wrote:
> Could you please add a not in this file, saying something like:
> "Do not merge this file with done.html, the test
> PromptAfterSubmitWithSubFrameNavigation needs two such files, with different
> filenames."

Thanks for that point Vaclav. Made the changes in
https://codereview.chromium.org/726703006/ .