Fix crash when an extension popup shows a JS alert. Showing the alert takes...

Landing this on for pam. Original issue: http://codereview.chromium.org/425011/show.

Disable showing JS alerts from popups, because doing so makes the popup
disappear immediately, which has all sorts of unfortunate side effects for the
poor orphaned alert (see bug for details).

BUG=27758
TEST=create extension with popup, put link with |onclick="alert('test');"| in
it; install extension, open popup, and click link; see no crash (nor popup)

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

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

5 files changed, 76 insertions, 39 deletions

diff --git a/chrome/browser/app_modal_dialog.cc b/chrome/browser/app_modal_dialog.cc
index 258eb19..b96b863 100644
--- a/chrome/browser/app_modal_dialog.cc
+++ b/chrome/browser/app_modal_dialog.cc
@@ -5,6 +5,7 @@
 #include "chrome/browser/app_modal_dialog.h"
 
 #include "chrome/browser/app_modal_dialog_queue.h"
+#include "chrome/browser/extensions/extension_host.h"
 #include "chrome/browser/tab_contents/tab_contents.h"
 #include "chrome/common/notification_service.h"
 #include "chrome/common/notification_type.h"
@@ -20,6 +21,8 @@ AppModalDialog::AppModalDialog(JavaScriptMessageBoxClient* client,
                                IPC::Message* reply_msg)
     : dialog_(NULL),
       client_(client),
+      tab_contents_(client_->AsTabContents()),
+      extension_host_(client_->AsExtensionHost()),
       skip_this_dialog_(false),
       title_(title),
       dialog_flags_(dialog_flags),
@@ -29,6 +32,7 @@ AppModalDialog::AppModalDialog(JavaScriptMessageBoxClient* client,
       is_before_unload_dialog_(is_before_unload_dialog),
       reply_msg_(reply_msg) {
   InitNotifications();
+  DCHECK((tab_contents_ != NULL) != (extension_host_ != NULL));
 }
 
 void AppModalDialog::Observe(NotificationType type,
@@ -37,45 +41,48 @@ void AppModalDialog::Observe(NotificationType type,
   if (skip_this_dialog_)
     return;
 
-  // We only observe our NavigationController for NAV_ENTRY_COMMITTED and our
-  // TabContents for TAB_CONTENTS_DESTROYED, both of which indicate that we
-  // should ignore this dialog. Also clear the client for good measure, since
-  // it's now invalid.
+  if (NotificationType::EXTENSION_HOST_DESTROYED == type &&
+      Details<ExtensionHost>(extension_host_) != details)
+    return;
+
+  // If we reach here, we know the notification is relevant to us, either
+  // because we're only observing applicable sources or because we passed the
+  // check above. Both of those indicate that we should ignore this dialog.
+  // Also clear the client, since it's now invalid.
   skip_this_dialog_ = true;
   client_ = NULL;
   CloseModalDialog();
 }
 
-void AppModalDialog::SendCloseNotification() {
-  NotificationService::current()->Notify(
-      NotificationType::APP_MODAL_DIALOG_CLOSED,
-      Source<AppModalDialog>(this),
-      NotificationService::NoDetails());
-}
-
 void AppModalDialog::InitNotifications() {
   // Make sure we get relevant navigation notifications so we know when our
   // parent contents will disappear or navigate to a different page.
-  TabContents* tab_contents = client_->AsTabContents();
-  if (tab_contents) {
+  if (tab_contents_) {
     registrar_.Add(this, NotificationType::NAV_ENTRY_COMMITTED,
-                   Source<NavigationController>(&tab_contents->controller()));
+                   Source<NavigationController>(&tab_contents_->controller()));
     registrar_.Add(this, NotificationType::TAB_CONTENTS_DESTROYED,
-                   Source<TabContents>(tab_contents));
+                   Source<TabContents>(tab_contents_));
+  } else if (extension_host_) {
+    // EXTENSION_HOST_DESTROYED uses the Profile as its source, but we care
+    // about the ExtensionHost (which is passed in the details).
+    registrar_.Add(this, NotificationType::EXTENSION_HOST_DESTROYED,
+                   NotificationService::AllSources());
+  } else {
+    NOTREACHED();
   }
 }
 
 void AppModalDialog::ShowModalDialog() {
-  // If the TabContents that created this dialog navigated away before this
-  // dialog became visible, simply show the next dialog if any.
+  // 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;
   }
-  TabContents* tab_contents = client_->AsTabContents();
-  if (tab_contents)
-    tab_contents->Activate();
+  if (tab_contents_)
+    tab_contents_->Activate();
 
   CreateAndShowDialog();
 
@@ -98,7 +105,7 @@ void AppModalDialog::OnCancel() {
     client_->OnMessageBoxClosed(reply_msg_, false, std::wstring());
   }
 
-  SendCloseNotification();
+  Cleanup();
 }
 
 void AppModalDialog::OnAccept(const std::wstring& prompt_text,
@@ -111,9 +118,26 @@ void AppModalDialog::OnAccept(const std::wstring& prompt_text,
       client_->SetSuppressMessageBoxes(true);
   }
 
-  SendCloseNotification();
+  Cleanup();
 }
 
 void AppModalDialog::OnClose() {
-  SendCloseNotification();
+  Cleanup();
+}
+
+void AppModalDialog::Cleanup() {
+  if (skip_this_dialog_) {
+    // We can't use the client_, because we might be in the process of
+    // destroying it.
+    if (tab_contents_)
+      tab_contents_->OnMessageBoxClosed(reply_msg_, false, L"");
+    else if (extension_host_)
+      extension_host_->OnMessageBoxClosed(reply_msg_, false, L"");
+    else
+      NOTREACHED();
+  }
+  NotificationService::current()->Notify(
+    NotificationType::APP_MODAL_DIALOG_CLOSED,
+    Source<AppModalDialog>(this),
+    NotificationService::NoDetails());
 }
diff --git a/chrome/browser/app_modal_dialog.h b/chrome/browser/app_modal_dialog.h
index a58981c..b3885c3 100644
--- a/chrome/browser/app_modal_dialog.h
+++ b/chrome/browser/app_modal_dialog.h
@@ -21,6 +21,7 @@ typedef GtkWidget* NativeDialog;
 typedef void* NativeDialog;
 #endif
 
+class ExtensionHost;
 class JavaScriptMessageBoxClient;
 class TabContents;
 namespace IPC {
@@ -96,8 +97,9 @@ class AppModalDialog : public NotificationObserver {
                        const NotificationSource& source,
                        const NotificationDetails& details);
 
-  // Sends APP_MODAL_DIALOG_CLOSED notification.
-  void SendCloseNotification();
+  // Cleans up after the dialog closes for any reason: sends close
+  // notification and re-enables input events.
+  void Cleanup();
 
   NotificationRegistrar registrar_;
 
@@ -108,6 +110,13 @@ class AppModalDialog : public NotificationObserver {
   // and receive results.
   JavaScriptMessageBoxClient* client_;
 
+  // The client_ as an ExtensionHost, cached for use during notifications that
+  // may arrive after the client has entered its destructor (and is thus
+  // treated as a base JavaScriptMessageBoxClient). This will be NULL if the
+  // client is not an ExtensionHost.
+  TabContents* tab_contents_;
+  ExtensionHost* extension_host_;
+
   // 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_;
diff --git a/chrome/browser/extensions/extension_host.h b/chrome/browser/extensions/extension_host.h
index 9407b85..e3ae680 100644
--- a/chrome/browser/extensions/extension_host.h
+++ b/chrome/browser/extensions/extension_host.h
@@ -156,6 +156,7 @@ class ExtensionHost : public ExtensionPopupHost::PopupDelegate,
                                   const std::wstring& prompt);
   virtual void SetSuppressMessageBoxes(bool suppress_message_boxes) {}
   virtual TabContents* AsTabContents() { return NULL; }
+  virtual ExtensionHost* AsExtensionHost() { return this; }
 
  private:
   friend class ProcessCreationQueue;
diff --git a/chrome/browser/jsmessage_box_client.h b/chrome/browser/jsmessage_box_client.h
index a370445..169805c 100644
--- a/chrome/browser/jsmessage_box_client.h
+++ b/chrome/browser/jsmessage_box_client.h
@@ -15,6 +15,7 @@
 
 #include "app/gfx/native_widget_types.h"
 
+class ExtensionHost;
 class GURL;
 class Profile;
 class TabContents;
@@ -41,10 +42,12 @@ class JavaScriptMessageBoxClient {
   // Indicates whether additional message boxes should be suppressed.
   virtual void SetSuppressMessageBoxes(bool suppress_message_boxes) = 0;
 
-  // Returns the TabContents associated with this message box -- in practice,
-  // the TabContents implementing this interface -- or NULL if it has no
-  // TabContents (e.g., it's an ExtensionHost).
+  // Returns the TabContents or ExtensionHost associated with this message
+  // box -- in practice, the object implementing this interface. Exactly one of
+  // these must be non-NULL; behavior is undefined (read: it'll probably crash)
+  // if that is not the case.
   virtual TabContents* AsTabContents() = 0;
+  virtual ExtensionHost* AsExtensionHost() = 0;
 };
 
 #endif  // CHROME_BROWSER_JSMESSAGE_BOX_CLIENT_H_
diff --git a/chrome/browser/tab_contents/tab_contents.h b/chrome/browser/tab_contents/tab_contents.h
index 33dae8a..50aae3f 100644
--- a/chrome/browser/tab_contents/tab_contents.h
+++ b/chrome/browser/tab_contents/tab_contents.h
@@ -631,6 +631,17 @@ class TabContents : public PageNavigator,
     return request_context_;
   }
 
+  // JavaScriptMessageBoxClient ------------------------------------------------
+  virtual std::wstring GetMessageBoxTitle(const GURL& frame_url,
+                                          bool is_alert);
+  virtual gfx::NativeWindow GetMessageBoxRootWindow();
+  virtual void OnMessageBoxClosed(IPC::Message* reply_msg,
+                                  bool success,
+                                  const std::wstring& prompt);
+  virtual void SetSuppressMessageBoxes(bool suppress_message_boxes);
+  virtual TabContents* AsTabContents() { return this; }
+  virtual ExtensionHost* AsExtensionHost() { return NULL; }
+
  private:
   friend class NavigationController;
   // Used to access the child_windows_ (ConstrainedWindowList) for testing
@@ -951,17 +962,6 @@ class TabContents : public PageNavigator,
                        const NotificationSource& source,
                        const NotificationDetails& details);
 
-
-  // JavaScriptMessageBoxClient ------------------------------------------------
-  virtual std::wstring GetMessageBoxTitle(const GURL& frame_url,
-                                          bool is_alert);
-  virtual gfx::NativeWindow GetMessageBoxRootWindow();
-  virtual void OnMessageBoxClosed(IPC::Message* reply_msg,
-                                  bool success,
-                                  const std::wstring& prompt);
-  virtual void SetSuppressMessageBoxes(bool suppress_message_boxes);
-  virtual TabContents* AsTabContents() { return this; }
-
   // Data for core operation ---------------------------------------------------
 
   // Delegate for notifying our owner about stuff. Not owned by us.