Fix for crbug.com/5471 (browser crash regression).

The "pending_save_manager_" used to only be non-null
between the time the user hits 'Submit' on a form and we either 1) show an 
infobar, or 2) update an existing login. Now, instead, the member only gets reset on 
InfoBarClosed(), and so this ordering of events was causing a crash:

--ExpireInfoBar, removes delegate from infobar_delegates_
--WebContents::DidStopLoading, causes PasswordManager to call AddInfoBar because it
has a pending_save_manager_ (and AddInfoBar succeeds because there is no "equal" 
delegate already in the set)
--Animation terminates, which calls InfoBarClosed(), and resets the 
pending_save_manager_
--Now we have an infobar and a null pending_save_manager_, so clicking Accept/Cancel 
will deref 0.

Renamed |pending_save_manager_| to |provisional_save_manager_| to be more consistent,
and added |pending_decision_manager_| to take responsibility of the PasswordFormManager
in |provisional_save_manager_| when the infobar comes in to play, so that we have
the same ownership model as before the new infobar changes.


Review URL: http://codereview.chromium.org/14464

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@7094 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 40 insertions, 21 deletions

diff --git a/chrome/browser/password_manager.cc b/chrome/browser/password_manager.cc
index 97c8ea7..8779209 100644
--- a/chrome/browser/password_manager.cc
+++ b/chrome/browser/password_manager.cc
@@ -77,33 +77,30 @@ void PasswordManager::ProvisionallySavePassword(PasswordForm form) {
           ProcessedSSLErrorFromRequest();
   form.preferred = true;
   manager->ProvisionallySave(form);
-  pending_save_manager_.reset(manager);
+  provisional_save_manager_.reset(manager);
   pending_login_managers_.erase(iter);
   // We don't care about the rest of the forms on the page now that one
   // was selected.
   STLDeleteElements(&pending_login_managers_);
-  pending_login_managers_.clear();
 }
 
 void PasswordManager::DidNavigate() {
   // As long as this navigation isn't due to a currently pending
   // password form submit, we're ready to reset and move on.
-  if (!pending_save_manager_.get() && !pending_login_managers_.empty()) {
+  if (!provisional_save_manager_.get() && !pending_login_managers_.empty())
     STLDeleteElements(&pending_login_managers_);
-    pending_login_managers_.clear();
-  }
 }
 
 void PasswordManager::ClearProvisionalSave() {
-  pending_save_manager_.reset();
+  provisional_save_manager_.reset();
 }
 
 void PasswordManager::DidStopLoading() {
-  if (!pending_save_manager_.get())
+  if (!provisional_save_manager_.get())
     return;
 
   DCHECK(!web_contents_->profile()->IsOffTheRecord());
-  DCHECK(!pending_save_manager_->IsBlacklisted());
+  DCHECK(!provisional_save_manager_->IsBlacklisted());
 
   if (!web_contents_->profile() ||
       !web_contents_->profile()->GetWebDataService(Profile::IMPLICIT_ACCESS))
@@ -111,13 +108,14 @@ void PasswordManager::DidStopLoading() {
   if (!web_contents_->controller())
     return;
 
-  if (pending_save_manager_->IsNewLogin()) {
+  if (provisional_save_manager_->IsNewLogin()) {
     web_contents_->AddInfoBar(this);
+    pending_decision_manager_.reset(provisional_save_manager_.release());
   } else {
     // If the save is not a new username entry, then we just want to save this
     // data (since the user already has related data saved), so don't prompt.
-    pending_save_manager_->Save();
-    pending_save_manager_.reset();
+    provisional_save_manager_->Save();
+    provisional_save_manager_.reset();
   }
 }
 
@@ -136,12 +134,12 @@ void PasswordManager::PasswordFormsSeen(const std::vector<PasswordForm>& forms)
 
   std::vector<PasswordForm>::const_iterator iter;
   for (iter = forms.begin(); iter != forms.end(); iter++) {
-    if (pending_save_manager_.get() &&
-        pending_save_manager_->DoesManage(*iter)) {
+    if (provisional_save_manager_.get() &&
+        provisional_save_manager_->DoesManage(*iter)) {
       // The form trying to be saved has immediately re-appeared. Assume
       // login failure and abort this save.  Fallback to pending login state
       // since the user may try again.
-      pending_login_managers_.push_back(pending_save_manager_.release());
+      pending_login_managers_.push_back(provisional_save_manager_.release());
       // Don't delete the login managers since the user may try again
       // and we want to be able to save in that case.
       break;
@@ -184,7 +182,7 @@ void PasswordManager::Autofill(const PasswordForm& form_for_autofill,
 // PasswordManager, ConfirmInfoBarDelegate implementation: ---------------------
 
 void PasswordManager::InfoBarClosed() {
-  pending_save_manager_.reset(NULL);
+  pending_decision_manager_.reset(NULL);
 }
 
 std::wstring PasswordManager::GetMessageText() const {
@@ -210,12 +208,12 @@ std::wstring PasswordManager::GetButtonLabel(InfoBarButton button) const {
 }
 
 bool PasswordManager::Accept() {
-  pending_save_manager_->Save();
+  pending_decision_manager_->Save();
   return true;
 }
 
 bool PasswordManager::Cancel() {
-  pending_save_manager_->PermanentlyBlacklist();
+  pending_decision_manager_->PermanentlyBlacklist();
   return true;
 }
 
diff --git a/chrome/browser/password_manager.h b/chrome/browser/password_manager.h
index 1b57082..a1e28eb 100644
--- a/chrome/browser/password_manager.h
+++ b/chrome/browser/password_manager.h
@@ -70,6 +70,17 @@ class PasswordManager : public views::LoginModel,
   virtual bool Accept();
   virtual bool Cancel();
   
+  // Note about how a PasswordFormManager can transition from
+  // pending_login_managers_ to {provisional_save, pending_decision_}manager_.
+  // 
+  // 1. form "seen"
+  //       |                                             new
+  //       |                                               ___ pending_decision
+  // pending_login -- form submit --> provisional_save ___/
+  //             ^                            |           \___ (update DB)
+  //             |                           fail
+  //             |-----------<------<---------|          !new
+  //
   // When a form is "seen" on a page, a PasswordFormManager is created
   // and stored in this collection until user navigates away from page.
   typedef std::vector<PasswordFormManager*> LoginManagers;
@@ -79,11 +90,21 @@ class PasswordManager : public views::LoginModel,
   // tab closes) on a page with a password form, thus containing login managers.
   STLElementDeleter<LoginManagers> login_managers_deleter_;
 
-  // When the user opts to save a password, this contains the
-  // PasswordFormManager for the form in question.
-  // Scoped incase PasswordManager gets deleted (e.g tab closes) between the
+  // When the user submits a password/credential, this contains the
+  // PasswordFormManager for the form in question until we deem the login
+  // attempt to have succeeded (as in valid credentials). If it fails, we
+  // send the PasswordFormManager back to the pending_login_managers_ set.
+  // Scoped in case PasswordManager gets deleted (e.g tab closes) between the
   // time a user submits a login form and gets to the next page.
-  scoped_ptr<PasswordFormManager> pending_save_manager_;
+  scoped_ptr<PasswordFormManager> provisional_save_manager_;
+
+  // After a successful *new* login attempt, we take the PasswordFormManager in
+  // provisional_save_manager_ and move it here while the user makes up their
+  // mind with the "save password" infobar. Note if the login is one we already
+  // know about, the end of the line is provisional_save_manager_ because we
+  // just update it on success and so such forms never end up in
+  // pending_decision_manager_.
+  scoped_ptr<PasswordFormManager> pending_decision_manager_;
 
   // The containing WebContents
   WebContents* web_contents_;