Fix issue 6296 by using a dedicated delegate instance for each infobar that the PasswordManager opens, rather than always using the PasswordManager itself as the delegate.

The crash was occuring because before one infobar completely finished closing (i.e during its close animation), another password infobar would be opened, and for a moment in time two infobars are pointing to the same delegate. When the initial closing animation completes, it would null-out a field (pending_decision_manager_), which the now visible infobar would depend on when the user clicks one of the native buttons. This click would dereference a null member, hence causing the crash.

Other clients of infobars seem to be immune to this problem because they either all create dedicated delegate instances per infobar, or they don't reset any state on InfoBarClosed.

I think (at the very least) the infobar code should document that it can be risky to use a "shared" delegate due to the possibility of two simultaneously visible infobars referring to the same delegate.
Review URL: http://codereview.chromium.org/18065

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

2 files changed, 88 insertions, 66 deletions

diff --git a/chrome/browser/password_manager/password_manager.cc b/chrome/browser/password_manager/password_manager.cc
index 4187151..8756a34 100644
--- a/chrome/browser/password_manager/password_manager.cc
+++ b/chrome/browser/password_manager/password_manager.cc
@@ -9,6 +9,8 @@
 #include "chrome/browser/profile.h"
 #include "chrome/browser/tab_contents/web_contents.h"
 #include "chrome/common/l10n_util.h"
+#include "chrome/common/notification_registrar.h"
+#include "chrome/common/notification_service.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/pref_service.h"
 #include "chrome/common/resource_bundle.h"
@@ -17,14 +19,91 @@
 #include "chromium_strings.h"
 #include "generated_resources.h"
 
+// After a successful *new* login attempt, we take the PasswordFormManager in
+// provisional_save_manager_ and move it to a SavePasswordInfoBarDelegate 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 an infobar.
+class SavePasswordInfoBarDelegate : public ConfirmInfoBarDelegate,
+                                    public NotificationObserver {
+ public:
+  SavePasswordInfoBarDelegate(TabContents* tab_contents,
+                              PasswordFormManager* form_to_save) :
+      ConfirmInfoBarDelegate(tab_contents),
+      form_to_save_(form_to_save) {
+    registrar_.Add(this, NOTIFY_TAB_CLOSED,
+                   Source<NavigationController>(tab_contents->controller()));
+  }
+
+   virtual ~SavePasswordInfoBarDelegate() { }  
+
+  // Overridden from ConfirmInfoBarDelegate:
+  virtual void InfoBarClosed() {
+    delete this;
+  }
+
+  virtual std::wstring GetMessageText() const {
+    return l10n_util::GetString(IDS_PASSWORD_MANAGER_SAVE_PASSWORD_PROMPT);
+  }
+
+  virtual SkBitmap* GetIcon() const {
+    return ResourceBundle::GetSharedInstance().GetBitmapNamed(
+        IDR_INFOBAR_SAVE_PASSWORD);
+  }
+
+  virtual int GetButtons() const {
+    return BUTTON_OK | BUTTON_CANCEL;
+  }
+
+  virtual std::wstring GetButtonLabel(InfoBarButton button) const {
+    if (button == BUTTON_OK)
+      return l10n_util::GetString(IDS_PASSWORD_MANAGER_SAVE_BUTTON);
+    if (button == BUTTON_CANCEL)
+      return l10n_util::GetString(IDS_PASSWORD_MANAGER_BLACKLIST_BUTTON);
+    NOTREACHED();
+    return std::wstring();
+  }
+
+  virtual bool Accept() {
+    DCHECK(form_to_save_.get());
+    form_to_save_->Save();
+    return true;
+  }
+
+  virtual bool Cancel() {
+    DCHECK(form_to_save_.get());
+    form_to_save_->PermanentlyBlacklist();
+    return true;
+  }
+
+  // NotificationObserver
+  virtual void Observe(NotificationType type, const NotificationSource& source,
+                       const NotificationDetails& details) {
+    DCHECK(type == NOTIFY_TAB_CLOSED);
+    // We don't get InfoBarClosed notification when a tab is closed, so we need
+    // to clean up after ourselves now.
+    // TODO(timsteele): This should not be necessary; see http://crbug.com/6520
+    delete this;
+  }
+ 
+ private:
+  // The PasswordFormManager managing the form we're asking the user about,
+  // and should update as per her decision.
+  scoped_ptr<PasswordFormManager> form_to_save_;
+
+  NotificationRegistrar registrar_;
+
+  DISALLOW_COPY_AND_ASSIGN(SavePasswordInfoBarDelegate);
+};
+
 // static
 void PasswordManager::RegisterUserPrefs(PrefService* prefs) {
   prefs->RegisterBooleanPref(prefs::kPasswordManagerEnabled, true);
 }
 
 PasswordManager::PasswordManager(WebContents* web_contents)
-    : ConfirmInfoBarDelegate(web_contents),
-      web_contents_(web_contents),
+    : web_contents_(web_contents),
       observer_(NULL),
       login_managers_deleter_(&pending_login_managers_) {
   password_manager_enabled_.Init(prefs::kPasswordManagerEnabled,
@@ -32,8 +111,6 @@ PasswordManager::PasswordManager(WebContents* web_contents)
 }
 
 PasswordManager::~PasswordManager() {
-  // Remove any InfoBars we may be showing.
-  web_contents_->RemoveInfoBar(this);
 }
 
 void PasswordManager::ProvisionallySavePassword(PasswordForm form) {
@@ -109,8 +186,9 @@ void PasswordManager::DidStopLoading() {
     return;
 
   if (provisional_save_manager_->IsNewLogin()) {
-    web_contents_->AddInfoBar(this);
-    pending_decision_manager_.reset(provisional_save_manager_.release());
+    web_contents_->AddInfoBar(
+        new SavePasswordInfoBarDelegate(web_contents_,
+                                        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.
@@ -179,41 +257,3 @@ void PasswordManager::Autofill(const PasswordForm& form_for_autofill,
   }
 }
 
-// PasswordManager, ConfirmInfoBarDelegate implementation: ---------------------
-
-void PasswordManager::InfoBarClosed() {
-  pending_decision_manager_.reset(NULL);
-}
-
-std::wstring PasswordManager::GetMessageText() const {
-  return l10n_util::GetString(IDS_PASSWORD_MANAGER_SAVE_PASSWORD_PROMPT);
-}
-
-SkBitmap* PasswordManager::GetIcon() const {
-  return ResourceBundle::GetSharedInstance().GetBitmapNamed(
-      IDR_INFOBAR_SAVE_PASSWORD);
-}
-
-int PasswordManager::GetButtons() const {
-  return BUTTON_OK | BUTTON_CANCEL;
-}
-
-std::wstring PasswordManager::GetButtonLabel(InfoBarButton button) const {
-  if (button == BUTTON_OK)
-    return l10n_util::GetString(IDS_PASSWORD_MANAGER_SAVE_BUTTON);
-  if (button == BUTTON_CANCEL)
-    return l10n_util::GetString(IDS_PASSWORD_MANAGER_BLACKLIST_BUTTON);
-  NOTREACHED();
-  return std::wstring();
-}
-
-bool PasswordManager::Accept() {
-  pending_decision_manager_->Save();
-  return true;
-}
-
-bool PasswordManager::Cancel() {
-  pending_decision_manager_->PermanentlyBlacklist();
-  return true;
-}
-
diff --git a/chrome/browser/password_manager/password_manager.h b/chrome/browser/password_manager/password_manager.h
index 74c5554..96171fe 100644
--- a/chrome/browser/password_manager/password_manager.h
+++ b/chrome/browser/password_manager/password_manager.h
@@ -21,8 +21,7 @@ class WebContents;
 // receiving password form data from the renderer and managing the password
 // database through the WebDataService. The PasswordManager is a LoginModel
 // for purposes of supporting HTTP authentication dialogs.
-class PasswordManager : public views::LoginModel,
-                        public ConfirmInfoBarDelegate {
+class PasswordManager : public views::LoginModel {
  public:
   static void RegisterUserPrefs(PrefService* prefs);
 
@@ -60,22 +59,13 @@ class PasswordManager : public views::LoginModel,
     observer_ = observer;
   }
 
- private:
-  // Overridden from ConfirmInfoBarDelegate:
-  virtual void InfoBarClosed();
-  virtual std::wstring GetMessageText() const;
-  virtual SkBitmap* GetIcon() const;
-  virtual int GetButtons() const;
-  virtual std::wstring GetButtonLabel(InfoBarButton button) const;
-  virtual bool Accept();
-  virtual bool Cancel();
-  
+ private:  
   // Note about how a PasswordFormManager can transition from
-  // pending_login_managers_ to {provisional_save, pending_decision_}manager_.
+  // pending_login_managers_ to provisional_save_manager_ and the infobar.
   // 
   // 1. form "seen"
   //       |                                             new
-  //       |                                               ___ pending_decision
+  //       |                                               ___ Infobar
   // pending_login -- form submit --> provisional_save ___/
   //             ^                            |           \___ (update DB)
   //             |                           fail
@@ -98,14 +88,6 @@ class PasswordManager : public views::LoginModel,
   // time a user submits a login form and gets to the next page.
   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_;