Password manager: improve the check for login failure

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/command_line.h"
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram.h"
 #include "base/prefs/pref_service.h"
@@ skipping 332 matching lines @@
          !provisional_save_manager_->IsPendingCredentialsPublicSuffixMatch();
 }
 
 void PasswordManager::OnPasswordFormsRendered(
     const std::vector<PasswordForm>& visible_forms) {
   if (!provisional_save_manager_.get())
     return;
 
   DCHECK(IsSavingEnabled());
 
-  // We now assume that if there is at least one visible password form
-  // that means that the previous login attempt failed.
-  if (!visible_forms.empty()) {
-    provisional_save_manager_->SubmitFailed();
-    provisional_save_manager_.reset();
-    return;
+  // If we see the login form again, then the login failed.
+  for (size_t i = 0; i < visible_forms.size(); ++i) {
+    if (visible_forms[i].action.is_valid() &&
+        provisional_save_manager_->pending_credentials().action ==
+            visible_forms[i].action) {

[Garrett Casto, 2014/01/15 02:28:27]

One thing to note is that it looks like at some point
PasswordFormManager::DoesManage() was used to check this (see
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pas...).
 This was before my time so I'm not sure why this was removed, but if it turns
out that your change makes matching too loose switching to DoesManage seems
reasonable.

[vabr (Chromium), 2014/01/15 08:37:53]

I actually started this CL with DoesManage + ACTION_MATCH_REQUIRED, but it
always returned false, because the names of the username or password elements
were not the same.

For now, I think just checking the actions is obvious enough to leave there
directly. If we end up with a more complex similarity check, it may make sense
to modify and use DoesManage for it. I'll put a comment saying this in the code.

On 2014/01/15 02:28:27, Garrett Casto wrote:

+      provisional_save_manager_->SubmitFailed();
+      provisional_save_manager_.reset();
+      return;
+    }
   }
 
   // Looks like a successful login attempt. Either show an infobar or
   // automatically save the login data. We prompt when the user hasn't already
   // given consent, either through previously accepting the infobar or by having
   // the browser generate the password.
   provisional_save_manager_->SubmitPassed();
   if (provisional_save_manager_->HasGeneratedPassword())
     UMA_HISTOGRAM_COUNTS("PasswordGeneration.Submitted", 1);
 
@@ skipping 90 matching lines @@
   }
 
   ManagePasswordsBubbleUIController* manage_passwords_bubble_ui_controller =
       ManagePasswordsBubbleUIController::FromWebContents(web_contents());
   if (manage_passwords_bubble_ui_controller &&
       CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kEnableSavePasswordBubble)) {
     manage_passwords_bubble_ui_controller->OnPasswordAutofilled(best_matches);
   }
 }