Updating of all entries in PasswordManager of the same credentials on password update

components/password_manager/core/browser/password_form_manager.cc

Index: components/password_manager/core/browser/password_form_manager.cc
diff --git a/components/password_manager/core/browser/password_form_manager.cc b/components/password_manager/core/browser/password_form_manager.cc
index 1908ec566c1ae4fc494ba4d107980eaa77dbdd80..7d4fba42d1ee365fcf33afaf6b1dff13d2fabfd7 100644
--- a/components/password_manager/core/browser/password_form_manager.cc
+++ b/components/password_manager/core/browser/password_form_manager.cc
@@ -234,7 +234,7 @@ bool PasswordFormManager::IsNewLogin() const {
 }
 
 bool PasswordFormManager::IsPendingCredentialsPublicSuffixMatch() const {
-  return pending_credentials_.IsPublicSuffixMatch();
+  return pending_credentials_.is_public_suffix_match;
 }
 
 void PasswordFormManager::ProvisionallySave(
@@ -442,27 +442,22 @@ void PasswordFormManager::OnRequestDone(
 
       if (is_credential_protected)
         protected_credentials.push_back(login.Pass());
+      else
+        not_best_matches_.push_back(login.Pass());
       continue;
     }
 
-    // If there is another best-score match for the same username, replace it.
-    // TODO(vabr): Spare the replacing and keep the first instead of the last
-    // candidate.
+    // If there is another best-score match for the same username then leave it
+    // and add the current form to |not_best_matches_|.
     const base::string16& username = login->username_value;
     auto best_match_username = best_matches_.find(username);
-    if (best_match_username != best_matches_.end() &&
-        best_match_username->second == preferred_match_) {
-      preferred_match_ = nullptr;
+    if (best_match_username == best_matches_.end()) {

[engedy, 2015/09/29 16:20:45]

This change could be performed in its own, separate CL.

[dvadym, 2015/09/29 18:29:36]

Yeah, agree. But it's too late to split them :(

+      if (login->preferred)
+        preferred_match_ = login.get();
+      best_matches_.insert(username, login.Pass());
+    } else {
+      not_best_matches_.push_back(login.Pass());
     }
-    // Transfer ownership into the map.
-    const PasswordForm* best_match = login.get();
-    // TODO(mgiuca): Directly assign to |best_match_username|, instead of doing
-    // a second map traversal. This will only be possible once we have C++11
-    // library support (then |best_matches_| can be a map of scoped_ptrs instead
-    // of a ScopedPtrMap).
-    best_matches_.set(username, login.Pass());
-    if (best_match->preferred)
-      preferred_match_ = best_match;
   }
 
   // Add the protected results if we don't already have a result with the same
@@ -473,7 +468,10 @@ void PasswordFormManager::OnRequestDone(
     scoped_ptr<PasswordForm> protege(*it);
     *it = nullptr;
     const base::string16& username = protege->username_value;
-    best_matches_.insert(username, protege.Pass());
+    if (best_matches_.find(username) == best_matches_.end())
+      best_matches_.insert(username, protege.Pass());
+    else
+      not_best_matches_.push_back(protege.Pass());
   }
 
   UMA_HISTOGRAM_COUNTS("PasswordManager.NumPasswordsNotShown",
@@ -525,10 +523,10 @@ void PasswordFormManager::ProcessFrameInternal(
   // TODO(engedy): Clean this up. See: https://crbug.com/476519.
   bool wait_for_username =
       client_->IsOffTheRecord() ||
-      (!IsValidAndroidFacetURI(preferred_match_->original_signon_realm) &&
+      (!IsValidAndroidFacetURI(preferred_match_->signon_realm) &&

[engedy, 2015/09/29 16:20:45]

This line, at least, can now be removed.

[dvadym, 2015/09/29 18:29:36]

After offline discussion we understood that it should be left.

        (observed_form_.action.GetWithEmptyPath() !=
             preferred_match_->action.GetWithEmptyPath() ||
-        preferred_match_->IsPublicSuffixMatch() ||
+        preferred_match_->is_public_suffix_match ||
         observed_form_.IsPossibleChangePasswordForm()));
   if (wait_for_username)
     manager_action_ = kManagerActionNone;
@@ -664,6 +662,7 @@ void PasswordFormManager::UpdateLogin() {
 
   UpdatePreferredLoginState(password_store);
 
+  bool password_was_updated = false;
   // Update the new preferred login.
   if (!selected_username_.empty()) {
     // Username has changed. We set this selected username as the real
@@ -689,8 +688,28 @@ void PasswordFormManager::UpdateLogin() {
     pending_credentials_.submit_element = observed_form_.submit_element;
     password_store->UpdateLoginWithPrimaryKey(pending_credentials_,
                                               old_primary_key);
+    password_was_updated = true;
   } else {
     password_store->UpdateLogin(pending_credentials_);
+    password_was_updated = true;
+  }
+  // If this was password update then update all non-best matches entries with

[engedy, 2015/09/29 16:20:45]

Okay, I'd definitely split this into another CL too (or make the CL at hand
contain only this).

+  // the same username and the same old password.
+  if (password_was_updated) {
+    PasswordFormMap::const_iterator updated_password_it =
+        best_matches_.find(pending_credentials_.username_value);
+    DCHECK(best_matches_.end() != updated_password_it);
+    const base::string16& old_password =
+        updated_password_it->second->password_value;
+    for (size_t i = 0; i < not_best_matches_.size(); ++i) {
+      if (not_best_matches_[i]->username_value ==
+              pending_credentials_.username_value &&
+          not_best_matches_[i]->password_value == old_password) {
+        not_best_matches_[i]->password_value =
+            pending_credentials_.password_value;
+        password_store->UpdateLogin(*not_best_matches_[i]);
+      }
+    }
   }
 }
 
@@ -825,9 +844,9 @@ void PasswordFormManager::CreatePendingCredentials() {
       // that the autofilled credentials and |provisionally_saved_form_|
       // actually correspond  to two different accounts (see
       // http://crbug.com/385619). In that case the user should be asked again
-      // before saving the password. This is ensured by clearing
-      // |original_signon_realm| on |pending_credentials_|, which unmarks it as
-      // a PSL match.
+      // before saving the password. This is ensured by setting
+      // |password_overriden_| on |pending_credentials_| to false and setting
+      // |origin| and |signon_realm| to correct values.
       //
       // There is still the edge case when the autofilled credentials represent
       // the same account as |provisionally_saved_form_| but the stored password
@@ -839,8 +858,11 @@ void PasswordFormManager::CreatePendingCredentials() {
       // user by asking them is not significant, so we are fine with asking
       // here again.
       if (password_overridden_) {
-        pending_credentials_.original_signon_realm.clear();
-        DCHECK(!IsPendingCredentialsPublicSuffixMatch());
+        pending_credentials_.is_public_suffix_match = false;
+        password_overridden_ = false;

[engedy, 2015/09/29 16:20:45]

Hang on a minute, I think |password_overridden| is a positive signal that the
prompt should be shown, so I don't think we should set this to false.

[dvadym, 2015/09/29 18:29:36]

Settings |password_overridded| to false here makes credentils to be saved with
Save Bubble, not to show Update bubble.

[engedy, 2015/09/29 19:11:04]

That's true, but if I am not mistaken, in addition to that, in this special
case, it also controls whether *any* kind of bubble is shown in the first place
(or just the credential is saved without further ado), because this is not
classified as a new login.

See: PasswordManager::ShouldPromptUserToSavePassword().  

+        pending_credentials_.origin = provisionally_saved_form_->origin;
+        pending_credentials_.signon_realm =
+            provisionally_saved_form_->signon_realm;
       }
     } else {  // Not a PSL match.
       is_new_login_ = false;
@@ -896,6 +918,10 @@ void PasswordFormManager::CreatePendingCredentials() {
   }
 
   pending_credentials_.action = provisionally_saved_form_->action;
+  if (pending_credentials_.is_public_suffix_match) {

[engedy, 2015/09/29 16:20:45]

nit: What is the reason to have this here, and not after line 815? That way, it
looks like we could avoid duplication on lines 863--865.

[dvadym, 2015/09/29 18:29:36]

Yeah, it was moved there in PatchSet 816

[engedy, 2015/09/29 19:11:04]

Acknowledged.

+    pending_credentials_.origin = provisionally_saved_form_->origin;
+    pending_credentials_.signon_realm = provisionally_saved_form_->signon_realm;
+  }
   // If the user selected credentials we autofilled from a PasswordForm
   // that contained no action URL (IE6/7 imported passwords, for example),
   // bless it with the action URL from the observed form. See bug 1107719.
@@ -943,7 +969,7 @@ uint32_t PasswordFormManager::ScoreResult(const PasswordForm& candidate) const {
       std::min(form_path_segments_.size(), kSegmentCountCap);
 
   uint32_t score = 0u;
-  if (!candidate.IsPublicSuffixMatch()) {
+  if (!candidate.is_public_suffix_match) {
     score += 1u << 7;
   }
   if (candidate.origin == observed_form_.origin) {
@@ -986,7 +1012,7 @@ bool PasswordFormManager::IsBlacklistMatch(
     const autofill::PasswordForm& blacklisted_form) const {
   DCHECK(blacklisted_form.blacklisted_by_user);
 
-  if (blacklisted_form.IsPublicSuffixMatch())
+  if (blacklisted_form.is_public_suffix_match)
     return false;
   if (blacklisted_form.origin.GetOrigin() != observed_form_.origin.GetOrigin())
     return false;
@@ -1014,7 +1040,7 @@ void PasswordFormManager::DeleteEmptyUsernameCredentials() {
   }
   for (auto iter = best_matches_.begin(); iter != best_matches_.end(); ++iter) {
     PasswordForm* form = iter->second;
-    if (!form->IsPublicSuffixMatch() && form->username_value.empty() &&
+    if (!form->is_public_suffix_match && form->username_value.empty() &&
         form->password_value == pending_credentials_.password_value)
       password_store->RemoveLogin(*form);
   }