summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
Diffstat
-rw-r--r--chrome/browser/browser_init.cc2
-rw-r--r--chrome/browser/password_manager/password_manager.cc128
-rw-r--r--chrome/browser/password_manager/password_manager.h26
-rw-r--r--chrome/browser/ssl/ssl_manager.cc2
-rw-r--r--chrome/browser/tab_contents/infobar_delegate.h17
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
generated by cgit v1.1 at 2025-02-08 15:05:46 +0000