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

1 files changed, 17 insertions, 0 deletions

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