Fix for no password save when action URL is modified

chrome/browser/password_manager/password_manager.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/password_manager/password_manager.h"
 
 #include "base/metrics/histogram.h"
 #include "base/threading/platform_thread.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/password_manager/password_form_manager.h"
@@ skipping 62 matching lines @@
   ReportMetrics(*password_manager_enabled_);
 }
 
 PasswordManager::~PasswordManager() {
 }
 
 void PasswordManager::SetFormHasGeneratedPassword(const PasswordForm& form) {
   for (ScopedVector<PasswordFormManager>::iterator iter =
            pending_login_managers_.begin();
        iter != pending_login_managers_.end(); ++iter) {
-    if ((*iter)->DoesManage(form)) {
+    if ((*iter)->DoesManage(
+        form, PasswordFormManager::kActionMatchRequired)) {
       (*iter)->SetHasGeneratedPassword();
       return;
     }
   }
   // If there is no corresponding PasswordFormManager, we create one. This is
   // not the common case, and should only happen when there is a bug in our
   // ability to detect forms.
   bool ssl_valid = (form.origin.SchemeIsSecure() &&
                     !delegate_->DidLastPageLoadEncounterSSLErrors());
   PasswordFormManager* manager =
@@ skipping 13 matching lines @@
 
 void PasswordManager::ProvisionallySavePassword(const PasswordForm& form) {
   if (!IsSavingEnabled())
     return;
 
   // No password to save? Then don't.
   if (form.password_value.empty())
     return;
 
   scoped_ptr<PasswordFormManager> manager;
+  ScopedVector<PasswordFormManager>::iterator matched_manager_it =
+      pending_login_managers_.end();
   for (ScopedVector<PasswordFormManager>::iterator iter =
            pending_login_managers_.begin();
        iter != pending_login_managers_.end(); ++iter) {
-    if ((*iter)->DoesManage(form)) {
-      // Transfer ownership of the manager from |pending_login_managers_| to
-      // |manager|.
-      manager.reset(*iter);
-      pending_login_managers_.weak_erase(iter);
+    // If we find a manager that exactly matches the submitted form including
+    // the action URL, exit the loop.
+    if ((*iter)->DoesManage(
+        form, PasswordFormManager::kActionMatchRequired)) {
+      matched_manager_it = iter;
       break;
+    // If the current manager matches the submitted form excluding the action
+    // URL, remember it as a candidate and continue searching for an exact
+    // match.
+    } else if ((*iter)->DoesManage(
+        form, PasswordFormManager::kActionMatchNotRequired)) {

[tim (not reviewing), 2012/09/05 21:40:59]

Can we add a PasswordManagerTest that verifies matches with action mismatch?

[guohui, 2012/09/17 13:28:55]

Done.

+      matched_manager_it = iter;
     }
   }
   // If we didn't find a manager, this means a form was submitted without
   // first loading the page containing the form. Don't offer to save
   // passwords in this case.
-  if (!manager.get())
+  if (matched_manager_it != pending_login_managers_.end()) {
+    // Transfer ownership of the manager from |pending_login_managers_| to
+    // |manager|.
+    manager.reset(*matched_manager_it);
+    pending_login_managers_.weak_erase(matched_manager_it);
+  } else {
     return;
+  }
 
   // If we found a manager but it didn't finish matching yet, the user has
   // tried to submit credentials before we had time to even find matching
   // results for the given form and autofill. If this is the case, we just
   // give up.
   if (!manager->HasCompletedMatching())
     return;
 
   // Also get out of here if the user told us to 'never remember' passwords for
   // this form.
@@ skipping 69 matching lines @@
 void PasswordManager::OnPasswordFormsRendered(
     const std::vector<PasswordForm>& visible_forms) {
   if (!provisional_save_manager_.get())
     return;
 
   DCHECK(IsSavingEnabled());
 
   // First, check for a failed login attempt.
   for (std::vector<PasswordForm>::const_iterator iter = visible_forms.begin();
        iter != visible_forms.end(); ++iter) {
-    if (provisional_save_manager_->DoesManage(*iter)) {
+    if (provisional_save_manager_->DoesManage(
+        *iter, PasswordFormManager::kActionMatchRequired)) {
       // The form trying to be saved has immediately re-appeared. Assume login
       // failure and abort this save, by clearing provisional_save_manager_.
       provisional_save_manager_->SubmitFailed();
       provisional_save_manager_.reset();
       return;
     }
   }
 
   if (!provisional_save_manager_->HasValidPasswordForm()) {
     // Form is not completely valid - we do not support it.
@@ skipping 41 matching lines @@
       if (observer_) {
         observer_->OnAutofillDataAvailable(preferred_match.username_value,
                                            preferred_match.password_value);
       }
   }
 }
 
 bool PasswordManager::IsFillingEnabled() const {
   return delegate_->GetProfile() && *password_manager_enabled_;
 }