[Password Manager] Fix to recognise failed login attempt for sites where content server pushes new …

components/password_manager/core/browser/password_manager.cc

Index: components/password_manager/core/browser/password_manager.cc
diff --git a/components/password_manager/core/browser/password_manager.cc b/components/password_manager/core/browser/password_manager.cc
index e05dab3878ac5999bb8748d898a43b44507c6458..c0ac0b343055e293d2ceb07a24ca90dfb6e0fd84 100644
--- a/components/password_manager/core/browser/password_manager.cc
+++ b/components/password_manager/core/browser/password_manager.cc
@@ -69,6 +69,11 @@ bool ShouldDropSyncCredential() {
   return group_name != "Disabled";
 }
 
+bool CompareURLContentsEqualIgnoreCase(const GURL& src, const GURL& dst) {

[vabr (Chromium), 2014/08/21 14:54:12]

nit: "Compare" seems superfluous in the function name.

[vabr (Chromium), 2014/08/21 14:54:12]

Please spell out concretely that scheme is omitted, the notion of a URLContents
is not known widely enough to indicate that.

[Pritam Nikam, 2014/08/21 16:49:21]

Done.

[Pritam Nikam, 2014/08/21 16:49:21]

Done.

+  return (base::StringToLowerASCII(src.GetContent()) ==
+          base::StringToLowerASCII(dst.GetContent()));
+}
+
 }  // namespace
 
 const char PasswordManager::kOtherPossibleUsernamesExperiment[] =
@@ -437,12 +442,14 @@ void PasswordManager::OnPasswordFormsRendered(
   // If we see the login form again, then the login failed.
   if (did_stop_loading) {
     for (size_t i = 0; i < all_visible_forms_.size(); ++i) {
-      // TODO(vabr): The similarity check is just action equality for now. If it
-      // becomes more complex, it may make sense to consider modifying and using
-      // PasswordFormManager::DoesManage for it.
+      // TODO(vabr): The similarity check is just on case and scheme ignored

[vabr (Chromium), 2014/08/21 14:54:12]

I'm not a native English speaker, but the first sentence seems difficult to
parse to me. I'd suggest rephrasing, but before we do that, we should probably
clarify whether the case sensitivity is included or not.

[Pritam Nikam, 2014/08/21 16:49:21]

Done.

+      // action URL equality for now. If it becomes more complex, it may make
+      // sense to consider modifying and using PasswordFormManager::DoesManage
+      // for it.
       if (all_visible_forms_[i].action.is_valid() &&
-          provisional_save_manager_->pending_credentials().action ==
-              all_visible_forms_[i].action) {
+          CompareURLContentsEqualIgnoreCase(

[vabr (Chromium), 2014/08/21 14:54:12]

Please do not allow mixing of arbitrary schemes, just HTTP and HTTPS at this
point. Let's be conservative, because it is security relevant.

[Pritam Nikam, 2014/08/21 16:49:21]

Done.

[jww, 2014/08/21 19:19:13]

This lgtm from a security perspective but is certainly treading on very
dangerous territory. This seems fine because it leads to an *inaction*... but we
we need to be really, really careful. So if I am misunderstanding the fact that
this is an inaction, please correct me.

+              provisional_save_manager_->pending_credentials().action,
+              all_visible_forms_[i].action)) {
         if (logger) {
           logger->LogPasswordForm(Logger::STRING_PASSWORD_FORM_REAPPEARED,
                                   visible_forms[i]);