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..dfb2116efe2db29a83377ce79daf4f45c79b412b 100644
--- a/components/password_manager/core/browser/password_form_manager.cc
+++ b/components/password_manager/core/browser/password_form_manager.cc
@@ -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());
+    } 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,7 +537,7 @@ 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() ||
@@ -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]);
+      }
+    }
   }
 }
 
@@ -837,7 +860,8 @@ void PasswordFormManager::CreatePendingCredentials() {
       // 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

[vabr (Chromium), 2015/09/24 15:17:33]

This comment is out of date, it references original_signon_realm.

[dvadym, 2015/09/25 10:19:52]

Done.

-      // a PSL match.
+      // a PSL match, setting |password_overriden_| 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 +873,12 @@ 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();
+        pending_credentials_.is_public_suffix_match = false;
         DCHECK(!IsPendingCredentialsPublicSuffixMatch());

[vabr (Chromium), 2015/09/24 15:17:33]

I don't think this DCHECK is meaningful any more. It checks that the flag set to
false above is still false one line below.

[dvadym, 2015/09/25 10:19:52]

Done.

+        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 +934,10 @@ void PasswordFormManager::CreatePendingCredentials() {
   }
 
   pending_credentials_.action = provisionally_saved_form_->action;
+  if (pending_credentials_.IsPublicSuffixMatch()) {
+    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.