Fixes two bugs in cookie prompting:

. In certain situations we could crash because we weren't letting
  CookiePromptView::Init complete and then attempting to reference
  some NULL fields. I've fixed this by adding AppModalDialog::IsValid
  and moving the logic that was in Init to IsValid. This way we only create
  CookiePromptView when needed.
. We were leaking AppModalDialogs. I've made CookiePromptView own the
  AppModalDialog and delete it when the CookiePromptView is deleted.

BUG=36079
TEST=enable prompting for cookies, go to a bunch of sites make sure
     you don't crash.

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

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

8 files changed, 57 insertions, 36 deletions

diff --git a/chrome/browser/app_modal_dialog.cc b/chrome/browser/app_modal_dialog.cc
index 815b25f..2eea602 100644
--- a/chrome/browser/app_modal_dialog.cc
+++ b/chrome/browser/app_modal_dialog.cc
@@ -17,16 +17,7 @@ AppModalDialog::AppModalDialog(TabContents* tab_contents,
       skip_this_dialog_(false) {
 }
 
-
 void AppModalDialog::ShowModalDialog() {
-  // If the TabContents or ExtensionHost that created this dialog navigated
-  // away or was destroyed before this dialog became visible, simply show the
-  // next dialog if any.
-  if (skip_this_dialog_) {
-    Singleton<AppModalDialogQueue>()->ShowNextDialog();
-    delete this;
-    return;
-  }
   if (tab_contents_)
     tab_contents_->Activate();
 
diff --git a/chrome/browser/app_modal_dialog.h b/chrome/browser/app_modal_dialog.h
index 715e12e..de87749 100644
--- a/chrome/browser/app_modal_dialog.h
+++ b/chrome/browser/app_modal_dialog.h
@@ -7,6 +7,7 @@
 
 #include <string>
 
+#include "base/basictypes.h"
 #include "build/build_config.h"
 #include "chrome/common/notification_observer.h"
 #include "chrome/common/notification_registrar.h"
@@ -46,6 +47,12 @@ class AppModalDialog {
   // Called by the app modal window queue when it is time to show this window.
   void ShowModalDialog();
 
+  // Returns true if the dialog is still valid. As dialogs are created they are
+  // added to the AppModalDialogQueue. When the current modal dialog finishes
+  // and it's time to show the next dialog in the queue IsValid is invoked.
+  // If IsValid returns false the dialog is deleted and not shown.
+  virtual bool IsValid() { return !skip_this_dialog_; }
+
   /////////////////////////////////////////////////////////////////////////////
   // The following methods are platform specific and should be implemented in
   // the platform specific .cc files.
@@ -101,6 +108,9 @@ class AppModalDialog {
   // True if the dialog should no longer be shown, e.g. because the underlying
   // tab navigated away while the dialog was queued.
   bool skip_this_dialog_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(AppModalDialog);
 };
 
 #endif  // CHROME_BROWSER_APP_MODAL_DIALOG_H_
diff --git a/chrome/browser/app_modal_dialog_queue.cc b/chrome/browser/app_modal_dialog_queue.cc
index 2ab1398..3d4f2da 100644
--- a/chrome/browser/app_modal_dialog_queue.cc
+++ b/chrome/browser/app_modal_dialog_queue.cc
@@ -15,13 +15,11 @@ void AppModalDialogQueue::AddDialog(AppModalDialog* dialog) {
 }
 
 void AppModalDialogQueue::ShowNextDialog() {
-  if (!app_modal_dialog_queue_.empty()) {
-    AppModalDialog* dialog = app_modal_dialog_queue_.front();
-    app_modal_dialog_queue_.pop();
+  AppModalDialog* dialog = GetNextDialog();
+  if (dialog)
     ShowModalDialog(dialog);
-  } else {
+  else
     active_dialog_ = NULL;
-  }
 }
 
 void AppModalDialogQueue::ActivateModalDialog() {
@@ -33,3 +31,14 @@ void AppModalDialogQueue::ShowModalDialog(AppModalDialog* dialog) {
   dialog->ShowModalDialog();
   active_dialog_ = dialog;
 }
+
+AppModalDialog* AppModalDialogQueue::GetNextDialog() {
+  while (!app_modal_dialog_queue_.empty()) {
+    AppModalDialog* dialog = app_modal_dialog_queue_.front();
+    app_modal_dialog_queue_.pop();
+    if (dialog->IsValid())
+      return dialog;
+    delete dialog;
+  }
+  return NULL;
+}
diff --git a/chrome/browser/app_modal_dialog_queue.h b/chrome/browser/app_modal_dialog_queue.h
index aa169c5..66ba3ff 100644
--- a/chrome/browser/app_modal_dialog_queue.h
+++ b/chrome/browser/app_modal_dialog_queue.h
@@ -54,11 +54,17 @@ class AppModalDialogQueue {
  private:
   friend struct DefaultSingletonTraits<AppModalDialogQueue>;
 
-  AppModalDialogQueue() : active_dialog_(NULL) { }
+  AppModalDialogQueue() : active_dialog_(NULL) {}
 
   // Shows |dialog| and notifies the BrowserList that a modal dialog is showing.
   void ShowModalDialog(AppModalDialog* dialog);
 
+  // Returns the next dialog to show. This removes entries from
+  // app_modal_dialog_queue_ until one is valid or the queue is empty. This
+  // returns NULL if there are no more dialogs, or all the dialogs in the queue
+  // are not valid.
+  AppModalDialog* GetNextDialog();
+
   // Contains all app modal dialogs which are waiting to be shown, with the
   // currently modal dialog at the front of the queue.
   std::queue<AppModalDialog*> app_modal_dialog_queue_;
diff --git a/chrome/browser/cookie_modal_dialog.cc b/chrome/browser/cookie_modal_dialog.cc
index 325378d..c953fb5 100644
--- a/chrome/browser/cookie_modal_dialog.cc
+++ b/chrome/browser/cookie_modal_dialog.cc
@@ -4,6 +4,9 @@
 
 #include "chrome/browser/cookie_modal_dialog.h"
 
+#include "chrome/browser/host_content_settings_map.h"
+#include "chrome/browser/profile.h"
+#include "chrome/browser/tab_contents/tab_contents.h"
 #include "chrome/browser/views/cookie_prompt_view.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/pref_service.h"
@@ -49,6 +52,25 @@ CookiePromptModalDialog::CookiePromptModalDialog(
       delegate_(delegate) {
 }
 
+bool CookiePromptModalDialog::IsValid() {
+  HostContentSettingsMap* host_content_settings_map =
+      tab_contents()->profile()->GetHostContentSettingsMap();
+  ContentSetting content_setting =
+      host_content_settings_map->GetContentSetting(
+          origin(),
+          CONTENT_SETTINGS_TYPE_COOKIES);
+  if (content_setting != CONTENT_SETTING_ASK) {
+    if (content_setting == CONTENT_SETTING_ALLOW) {
+      AllowSiteData(false, false);
+    } else {
+      DCHECK(content_setting == CONTENT_SETTING_BLOCK);
+      BlockSiteData(false);
+    }
+    return false;
+  }
+  return true;
+}
+
 void CookiePromptModalDialog::AllowSiteData(bool remember,
                                             bool session_expire) {
   delegate_->AllowSiteData(remember, session_expire);
diff --git a/chrome/browser/cookie_modal_dialog.h b/chrome/browser/cookie_modal_dialog.h
index c5cb8bd..05ed0e2 100644
--- a/chrome/browser/cookie_modal_dialog.h
+++ b/chrome/browser/cookie_modal_dialog.h
@@ -48,6 +48,7 @@ class CookiePromptModalDialog : public AppModalDialog {
   virtual int GetDialogButtons();
   virtual void AcceptWindow();
   virtual void CancelWindow();
+  virtual bool IsValid();
 
   DialogType dialog_type() const { return dialog_type_; }
   const GURL& origin() const { return origin_; }
diff --git a/chrome/browser/views/cookie_prompt_view.cc b/chrome/browser/views/cookie_prompt_view.cc
index 6cc526a..0cd93da 100644
--- a/chrome/browser/views/cookie_prompt_view.cc
+++ b/chrome/browser/views/cookie_prompt_view.cc
@@ -14,9 +14,7 @@
 #include "base/string_util.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/cookie_modal_dialog.h"
-#include "chrome/browser/host_content_settings_map.h"
 #include "chrome/browser/profile.h"
-#include "chrome/browser/tab_contents/tab_contents.h"
 #include "chrome/browser/views/browser_dialogs.h"
 #include "chrome/browser/views/cookie_info_view.h"
 #include "chrome/browser/views/database_open_info_view.h"
@@ -61,6 +59,7 @@ CookiePromptView::CookiePromptView(
 }
 
 CookiePromptView::~CookiePromptView() {
+  delete parent_;
 }
 
 ///////////////////////////////////////////////////////////////////////////////
@@ -137,24 +136,6 @@ void CookiePromptView::LinkActivated(views::Link* source, int event_flags) {
 // CookiePromptView, private:
 
 void CookiePromptView::Init() {
-  // See if there's still a need for this dialog.  Self destruct if not.
-  HostContentSettingsMap* host_content_settings_map =
-      parent_->tab_contents()->profile()->GetHostContentSettingsMap();
-  ContentSetting content_setting =
-      host_content_settings_map->GetContentSetting(
-          parent_->origin(), CONTENT_SETTINGS_TYPE_COOKIES);
-  if (content_setting != CONTENT_SETTING_ASK) {
-    if (content_setting == CONTENT_SETTING_ALLOW) {
-      parent_->AllowSiteData(false, false);
-    } else {
-      DCHECK(content_setting == CONTENT_SETTING_BLOCK);
-      parent_->BlockSiteData(false);
-    }
-    signaled_ = true;
-    GetWindow()->Close();
-    return;
-  }
-
   CookiePromptModalDialog::DialogType type = parent_->dialog_type();
   std::wstring display_host = UTF8ToWide(parent_->origin().host());
   views::Label* description_label = new views::Label(l10n_util::GetStringF(
diff --git a/chrome/browser/views/cookie_prompt_view.h b/chrome/browser/views/cookie_prompt_view.h
index 933ec0c..8f588e5 100644
--- a/chrome/browser/views/cookie_prompt_view.h
+++ b/chrome/browser/views/cookie_prompt_view.h
@@ -37,6 +37,7 @@ class CookiePromptView : public views::View,
                          public views::LinkController,
                          public CookieInfoViewDelegate {
  public:
+  // Creates a new CookiePromptView. We take ownership of |parent|.
   CookiePromptView(
       CookiePromptModalDialog* parent,
       gfx::NativeWindow root_window,
@@ -107,7 +108,7 @@ class CookiePromptView : public views::View,
   // Prompt window title.
   std::wstring title_;
 
-  // A pointer to the AppModalDialog that owns us.
+  // A pointer to the AppModalDialog that created us. We own this.
   CookiePromptModalDialog* parent_;
 
   gfx::NativeWindow root_window_;