New issue
Advanced search Search tips

Issue 821763 link

Starred by 6 users

Issue metadata

Status: Fixed
Owner: ----
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 3
Type: Bug



Sign in to add a comment

Password manager revives a credential on reload

Project Member Reported by vasi...@chromium.org, Mar 14 2018

Issue description

Chrome Version: 67
OS: desktop

What steps will reproduce the problem?
(1) Run 'python -m SimpleHTTPServer 8080' and load 'http://localhost:8080/chrome/test/data/password/password_submit_from_iframe.html' in Chrome. It's a testing file.
(2) Type something into the first form and save the password via manual fallback (basically presave a credential somehow).
(3) Open the bubble and delete the password. Make sure it's gone in chrome://settings/passwords.
(4) Reload the page

What is the expected result?
The password isn't filled.

What happens instead?
The password is filled and revived in the password store.

Apparently Chrome detects successful login and updates the credential by removing and adding it:
#0  0x000055555b5d6d34 in password_manager::PasswordStore::UpdateLoginWithPrimaryKey(autofill::PasswordForm const&, autofill::PasswordForm const&) (this=0x22fa4e63cda0, new_form=..., old_primary_key=...)
    at ../../components/password_manager/core/browser/password_store.cc:153
#1  0x000055555b56bf03 in password_manager::FormSaverImpl::SaveImpl(autofill::PasswordForm const&, bool, std::__1::map<std::__1::basic_string<unsigned short, base::string16_internals::string16_char_traits, std::__1::allocator<unsigned short> >, autofill::PasswordForm const*, std::__1::less<std::__1::basic_string<unsigned short, base::string16_internals::string16_char_traits, std::__1::allocator<unsigned short> > >, std::__1::allocator<std::__1::pair<std::__1::basic_string<unsigned short, base::string16_internals::string16_char_traits, std::__1::allocator<unsigned short> > const, autofill::PasswordForm const*> > > const&, std::__1::vector<autofill::PasswordForm, std::__1::allocator<autofill::PasswordForm> > const*, autofill::PasswordForm const*) (this=0x22fa51ae7ed0, pending=..., is_new_login=false, best_matches=..., credentials_to_update=0x7fffffff37e0, old_primary_key=0x7fffffff38d8)
    at ../../components/password_manager/core/browser/form_saver_impl.cc:125
#2  0x000055555b56c169 in password_manager::FormSaverImpl::Update(autofill::PasswordForm const&, std::__1::map<std::__1::basic_string<unsigned short, base::string16_internals::string16_char_traits, std::__1::allocator<unsigned short> >, autofill::PasswordForm const*, std::__1::less<std::__1::basic_string<unsigned short, base::string16_internals::string16_char_traits, std::__1::allocator<unsigned short> > >, std::__1::allocator<std::__1::pair<std::__1::basic_string<unsigned short, base::string16_internals::string16_char_traits, std::__1::allocator<unsigned short> > const, autofill::PasswordForm const*> > > const&, std::__1::vector<autofill::PasswordForm, std::__1::allocator<autofill::PasswordForm> > const*, autofill::PasswordForm const*) (this=0x22fa51ae7ed0, pending=..., best_matches=..., credentials_to_update=0x7fffffff37e0, old_primary_key=0x7fffffff38d8) at ../../components/password_manager/core/browser/form_saver_impl.cc:51
#3  0x000055555b5757e0 in password_manager::PasswordFormManager::Save() (this=0x22fa513a9420)
    at ../../components/password_manager/core/browser/password_form_manager.cc:418
#4  0x000055555b55efa2 in password_manager::PasswordManager::OnLoginSuccessful() (this=0x22fa4f344060)
    at ../../components/password_manager/core/browser/password_manager.cc:915
#5  0x000055555b55d6dc in password_manager::PasswordManager::OnPasswordFormSubmittedNoChecks(password_manager::PasswordManagerDriver*, autofill::PasswordForm const&) (this=0x22fa4f344060, driver=0x22fa51b001a0, password_form=...) at ../../components/password_manager/core/browser/password_manager.cc:509
#6  0x000055555b563815 in password_manager::PasswordManager::OnSameDocumentNavigation(password_manager::PasswordManagerDriver*, autofill::PasswordForm const&) (this=0x22fa4f344060, driver=0x22fa51b001a0, password_form=...) at ../../components/password_manager/core/browser/password_manager.cc:835
#7  0x000055555cb5b1b8 in password_manager::ContentPasswordManagerDriver::SameDocumentNavigation(autofill::PasswordForm const&) (this=0x22fa51b001a0, password_form=...)

 
Cc: rsesek@chromium.org vasi...@chromium.org
 Issue 824474  has been merged into this issue.
I'm having this problem in 66.0.3359.139 (Official Build) (64-bit) with Windows 10.

At https://quizlet.com/

If I click the site's Login link, then click the bubble and delete the password, nothing has changed after a reload, and this repeats indefinitely.

If I close the bubble and reopen it before reloading the page, it says "No passwords saved for this site" before the reload, and goes back to "Saved passwords for this site" after the reload.
Cc: vabr@chromium.org
 Issue 833115  has been merged into this issue.
Project Member

Comment 4 by bugdroid1@chromium.org, May 17 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/47d67ced179788afbbf6e1cd106194b5ba67c8ae

commit 47d67ced179788afbbf6e1cd106194b5ba67c8ae
Author: Vasilii Sukhanov <vasilii@chromium.org>
Date: Thu May 17 15:14:22 2018

Update PasswordManager when a relevant credential is removed by the user.

Before this CL this behavior is possible:
- a credential is autofilled.
- user removes the credential via settings or "Manage passwords" bubble.
- user clicks "Login" or reloads the page. In other words makes an action that we consider a successful form submission.
- password manager updates the credential in the store by reviving it.

After this CL the password manager is informed as soon as the password is removed. Thus, the copy of the credential in the memory should go away.

Bug:  821763 , 841853 
Change-Id: If6c371312a9ed55217f5998989dd0457296a538a
Reviewed-on: https://chromium-review.googlesource.com/1064118
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Commit-Queue: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559539}
[modify] https://crrev.com/47d67ced179788afbbf6e1cd106194b5ba67c8ae/chrome/browser/password_manager/password_manager_browsertest.cc
[modify] https://crrev.com/47d67ced179788afbbf6e1cd106194b5ba67c8ae/chrome/browser/ui/passwords/manage_passwords_state.cc
[modify] https://crrev.com/47d67ced179788afbbf6e1cd106194b5ba67c8ae/chrome/browser/ui/passwords/manage_passwords_state.h

Status: Fixed (was: Available)
Project Member

Comment 6 by bugdroid1@chromium.org, May 17 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3a59c3eb8a2b87cfda23a6642de915c99bd3905f

commit 3a59c3eb8a2b87cfda23a6642de915c99bd3905f
Author: Chong Zhang <chongz@chromium.org>
Date: Thu May 17 17:28:17 2018

Revert "Update PasswordManager when a relevant credential is removed by the user."

This reverts commit 47d67ced179788afbbf6e1cd106194b5ba67c8ae.

Reason for revert: Seems to cause flakiness on interactive_ui_tests
http://chromium-try-flakes.appspot.com/search?q=PasswordGenerationInteractiveTest.PopupShownAndPasswordSelected

Original change's description:
> Update PasswordManager when a relevant credential is removed by the user.
> 
> Before this CL this behavior is possible:
> - a credential is autofilled.
> - user removes the credential via settings or "Manage passwords" bubble.
> - user clicks "Login" or reloads the page. In other words makes an action that we consider a successful form submission.
> - password manager updates the credential in the store by reviving it.
> 
> After this CL the password manager is informed as soon as the password is removed. Thus, the copy of the credential in the memory should go away.
> 
> Bug:  821763 , 841853 
> Change-Id: If6c371312a9ed55217f5998989dd0457296a538a
> Reviewed-on: https://chromium-review.googlesource.com/1064118
> Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
> Commit-Queue: Vasilii Sukhanov <vasilii@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#559539}

TBR=vasilii@chromium.org,dvadym@chromium.org

Change-Id: Ifbd0b85fd68399e01b58a8faaad29f677fb7ffe6
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  821763 ,  841853 ,  844077 
Reviewed-on: https://chromium-review.googlesource.com/1064473
Reviewed-by: Chong Zhang <chongz@chromium.org>
Commit-Queue: Chong Zhang <chongz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559590}
[modify] https://crrev.com/3a59c3eb8a2b87cfda23a6642de915c99bd3905f/chrome/browser/password_manager/password_manager_browsertest.cc
[modify] https://crrev.com/3a59c3eb8a2b87cfda23a6642de915c99bd3905f/chrome/browser/ui/passwords/manage_passwords_state.cc
[modify] https://crrev.com/3a59c3eb8a2b87cfda23a6642de915c99bd3905f/chrome/browser/ui/passwords/manage_passwords_state.h

I realize there's another ticket for the test, but now this ticket is not actually fixed if it has been reverted. :/
Status: Started (was: Fixed)
Project Member

Comment 9 by bugdroid1@chromium.org, May 18 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1fd384b65ca67a73e6766d754ca9b115d57d3686

commit 1fd384b65ca67a73e6766d754ca9b115d57d3686
Author: Vasilii Sukhanov <vasilii@chromium.org>
Date: Fri May 18 13:23:29 2018

Reland: Update PasswordManager when a relevant credential is removed by the user.

Before this CL this behavior is possible:
- a credential is autofilled.
- user removes the credential via settings or "Manage passwords" bubble.
- user clicks "Login" or reloads the page. In other words makes an action that we consider a successful form submission.
- password manager updates the credential in the store by reviving it.

After this CL the password manager is informed as soon as the password is removed. Thus, the copy of the credential in the memory should go away.

This is a reland of https://chromium-review.googlesource.com/c/chromium/src/+/1064118
The reason for original failure was a refetch on adding a credential:
- A password was generated and added to the store.
- Refetch was triggered.
- The generated password was autofilled everywhere including the original field.
- The password manager wasn't in the generation mode anymore.

Bug:  821763 , 841853 
Change-Id: Ia0f24ccdf8d53dd0c3c740e9672c291322a5fd63
Reviewed-on: https://chromium-review.googlesource.com/1065971
Reviewed-by: Vadym Doroshenko <dvadym@chromium.org>
Commit-Queue: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559875}
[modify] https://crrev.com/1fd384b65ca67a73e6766d754ca9b115d57d3686/chrome/browser/password_manager/password_manager_browsertest.cc
[modify] https://crrev.com/1fd384b65ca67a73e6766d754ca9b115d57d3686/chrome/browser/ui/passwords/manage_passwords_state.cc
[modify] https://crrev.com/1fd384b65ca67a73e6766d754ca9b115d57d3686/chrome/browser/ui/passwords/manage_passwords_state.h

Status: Fixed (was: Started)
 Issue 854168  has been merged into this issue.

Comment 12 by wfh@chromium.org, Jun 20 2018

Does this need a merge?
It's in M68 beta.
Cc: -vabr@chromium.org

Sign in to add a comment