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

5 files changed, 109 insertions, 66 deletions

diff --git a/chrome/browser/browser_init.cc b/chrome/browser/browser_init.cc
index 3ea9b88..eba5365 100644
--- a/chrome/browser/browser_init.cc
+++ b/chrome/browser/browser_init.cc
@@ -52,6 +52,8 @@ namespace {
 
 // A delegate for the InfoBar shown when the previous session has crashed. The
 // bar deletes itself automatically after it is closed.
+// TODO(timsteele): This delegate can leak when a tab is closed, see
+// http://crbug.com/6520
 class SessionCrashedInfoBarDelegate : public ConfirmInfoBarDelegate {
  public:
   explicit SessionCrashedInfoBarDelegate(TabContents* contents)
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_;
 
diff --git a/chrome/browser/ssl/ssl_manager.cc b/chrome/browser/ssl/ssl_manager.cc
index b2fd246..835e3f0 100644
--- a/chrome/browser/ssl/ssl_manager.cc
+++ b/chrome/browser/ssl/ssl_manager.cc
@@ -34,6 +34,8 @@
 #include "webkit/glue/resource_type.h"
 #include "generated_resources.h"
 
+// TODO(timsteele): SSLInfoBarDelegate can leak when a tab is closed, see
+// http://crbug.com/6520
 class SSLInfoBarDelegate : public ConfirmInfoBarDelegate {
  public:
    SSLInfoBarDelegate(TabContents* contents,
diff --git a/chrome/browser/tab_contents/infobar_delegate.h b/chrome/browser/tab_contents/infobar_delegate.h
index b1932d4..fa3f332 100644
--- a/chrome/browser/tab_contents/infobar_delegate.h
+++ b/chrome/browser/tab_contents/infobar_delegate.h
@@ -21,6 +21,23 @@ class LinkInfoBarDelegate;
 // does not map to a specific InfoBar type. Instead, you must implement either
 // AlertInfoBarDelegate or ConfirmInfoBarDelegate, or override with your own
 // delegate for your own InfoBar variety.
+//
+// --- WARNING ---
+// When creating your InfoBarDelegate subclass, it is recommended that you
+// design it such that you instantiate a brand new delegate for every call to
+// AddInfoBar, rather than re-using/sharing a delegate object. Otherwise,
+// you need to consider the fact that more than one InfoBar instance can exist
+// and reference the same delegate -- even though it is also true that we only
+// ever fully show one infobar (they don't stack). The dual-references occur
+// because a second InfoBar can be added while the first one is in the process
+// of closing (the animations). This can cause problems because when the first
+// one does finally fully close InfoBarDelegate::InfoBarClosed() is called,
+// and the delegate is free to clean itself up or reset state, which may have
+// fatal consequences for the InfoBar that was in the process of opening (or is
+// now fully opened) -- it is referencing a delegate that may not even exist
+// anymore. 
+// As such, it is generally much safer to dedicate a delegate instance to
+// AddInfoBar!
 class InfoBarDelegate {
  public:
   // Returns true if the supplied |delegate| is equal to this one. Equality is