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 17317c61aa7cc99bb65a3f4755ebdbc2046a46ed..f89e0f088d65cb9bb352fcd753714d3808da2e3f 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;
 }
 
 bool PasswordFormManager::HasValidPasswordForm() const {
@@ -452,27 +452,26 @@ 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;
-    }
-    // Transfer ownership into the map.
-    const PasswordForm* best_match = login.get();
+    if (best_match_username == best_matches_.end()) {
+      if (login->preferred)
+        preferred_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;
+    best_matches_.insert(username, login.Pass());

[vasilii, 2015/09/25 14:16:15]

Indent. The comment is irrelevant.

[dvadym, 2015/09/28 13:58:40]

Done.

+    } else {
+      not_best_matches_.push_back(login.Pass());
+    }
   }
 
   // Add the protected results if we don't already have a result with the same
@@ -483,7 +482,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",
@@ -535,10 +537,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) &&
        (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;
@@ -674,6 +676,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
@@ -699,8 +702,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
+  // 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]);
+      }
+    }
   }
 }
 
@@ -835,9 +858,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
@@ -849,8 +872,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;
+        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;
@@ -906,6 +932,10 @@ void PasswordFormManager::CreatePendingCredentials() {
   }
 
   pending_credentials_.action = provisionally_saved_form_->action;
+  if (pending_credentials_.is_public_suffix_match) {
+    pending_credentials_.origin = provisionally_saved_form_->origin;
+    pending_credentials_.signon_realm = provisionally_saved_form_->signon_realm;

[vasilii, 2015/09/25 14:16:15]

I lost the track in this function. When is at possible that
pending_credentials_.origin != provisionally_saved_form_->origin /
pending_credentials_.signon_realm != provisionally_saved_form_->signon_realm by
the end of the method?

[dvadym, 2015/09/29 13:51:40]

In two places:
1.PSL match, not overridden
2.PSL change password form

[vasilii, 2015/09/29 14:35:22]

That is, the answer to my question is "Never" because the new block assigns
exactly for PSL.
In this case, remove the "if" condition.

[dvadym, 2015/09/29 15:43:40]

After offline discussion I've moved setting |origin| and |signon_realm| to if
(IsPendingCredentialsPublicSuffixMatch()) { block, since it's the only case
where it's needed

+  }
   // 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.
@@ -953,7 +983,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) {
@@ -996,7 +1026,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;
@@ -1024,7 +1054,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);
   }