Displaying consecutive alerts from plugins should not hang the browser. The plugins display alerts via the NPN_Evaluate API. The browser signals an event handle to ensure that the plugin starts peeking for messages while waiting for the NPN_Evaluate call to return. When the dialog is dismissed by the user, windows does send some messages to the plugin window underneath, like activation messages, etc. These don't get dispatched as the event is reset when the dialog is dismissed, i.e. much before the window is actually

destroyed.

The fix is to reset the event handle after the window is actually destroyed. To achieve this I added an OnClose virtual function to the DialogDelegate interface, which is overridden by the JavascriptMessageBoxDialog class which eventually ensures that the event is reset.

This fixes http://code.google.com/p/chromium/issues/detail?id=10799

I updated the AlertInWindowMessage npapi test to display two alerts instead of one.

Bug=10799

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

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

15 files changed, 61 insertions, 5 deletions

diff --git a/chrome/browser/app_modal_dialog.cc b/chrome/browser/app_modal_dialog.cc
index fb16ac8..3cfe47f 100644
--- a/chrome/browser/app_modal_dialog.cc
+++ b/chrome/browser/app_modal_dialog.cc
@@ -98,3 +98,9 @@ void AppModalDialog::OnAccept(const std::wstring& prompt_text,
       tab_contents()->set_suppress_javascript_messages(true);
   }
 }
+
+void AppModalDialog::OnClose() {
+ if (tab_contents_) {
+   tab_contents_->OnJavaScriptMessageBoxWindowDestroyed();
+ }
+}
diff --git a/chrome/browser/app_modal_dialog.h b/chrome/browser/app_modal_dialog.h
index c7e2dc9..325c222 100644
--- a/chrome/browser/app_modal_dialog.h
+++ b/chrome/browser/app_modal_dialog.h
@@ -79,6 +79,7 @@ class AppModalDialog : public NotificationObserver {
   // Callbacks from NativeDialog when the user accepts or cancels the dialog.
   void OnCancel();
   void OnAccept(const std::wstring& prompt_text, bool suppress_js_messages);
+  void OnClose();
 
   // Helper methods used to query or control the dialog. This is used by
   // automation.
diff --git a/chrome/browser/renderer_host/render_view_host.cc b/chrome/browser/renderer_host/render_view_host.cc
index 09737bb..5fee2fb 100644
--- a/chrome/browser/renderer_host/render_view_host.cc
+++ b/chrome/browser/renderer_host/render_view_host.cc
@@ -536,13 +536,16 @@ void RenderViewHost::JavaScriptMessageBoxClosed(IPC::Message* reply_msg,
     StartHangMonitorTimeout(TimeDelta::FromMilliseconds(kUnloadTimeoutMS));
   }
 
-  if (--modal_dialog_count_ == 0)
-    modal_dialog_event_->Reset();
   ViewHostMsg_RunJavaScriptMessage::WriteReplyParams(reply_msg,
                                                      success, prompt);
   Send(reply_msg);
 }
 
+void RenderViewHost::JavaScriptMessageBoxWindowDestroyed() {
+ if (--modal_dialog_count_ == 0)
+   modal_dialog_event_->Reset();
+}
+
 void RenderViewHost::ModalHTMLDialogClosed(IPC::Message* reply_msg,
                                            const std::string& json_retval) {
   if (is_waiting_for_unload_ack_)
diff --git a/chrome/browser/renderer_host/render_view_host.h b/chrome/browser/renderer_host/render_view_host.h
index 4421aab..b23d63b 100644
--- a/chrome/browser/renderer_host/render_view_host.h
+++ b/chrome/browser/renderer_host/render_view_host.h
@@ -283,6 +283,10 @@ class RenderViewHost : public RenderWidgetHost {
                                   bool success,
                                   const std::wstring& prompt);
 
+  // This function is called when the JavaScript message box window has been
+  // destroyed.
+  void JavaScriptMessageBoxWindowDestroyed();
+
   // Notifies the RenderView that the modal html dialog has been closed.
   void ModalHTMLDialogClosed(IPC::Message* reply_msg,
                              const std::string& json_retval);
diff --git a/chrome/browser/tab_contents/render_view_host_manager.cc b/chrome/browser/tab_contents/render_view_host_manager.cc
index 07e4277..4a55dbb 100644
--- a/chrome/browser/tab_contents/render_view_host_manager.cc
+++ b/chrome/browser/tab_contents/render_view_host_manager.cc
@@ -253,6 +253,10 @@ void RenderViewHostManager::OnJavaScriptMessageBoxClosed(
   render_view_host_->JavaScriptMessageBoxClosed(reply_msg, success, prompt);
 }
 
+void RenderViewHostManager::OnJavaScriptMessageBoxWindowDestroyed() {
+  render_view_host_->JavaScriptMessageBoxWindowDestroyed();
+}
+
 void RenderViewHostManager::Observe(NotificationType type,
                                     const NotificationSource& source,
                                     const NotificationDetails& details) {
diff --git a/chrome/browser/tab_contents/render_view_host_manager.h b/chrome/browser/tab_contents/render_view_host_manager.h
index d77e649..11ec445 100644
--- a/chrome/browser/tab_contents/render_view_host_manager.h
+++ b/chrome/browser/tab_contents/render_view_host_manager.h
@@ -148,6 +148,9 @@ class RenderViewHostManager : public NotificationObserver {
                                     bool success,
                                     const std::wstring& prompt);
 
+  // Forwards this message to the RenderViewHost.
+  void OnJavaScriptMessageBoxWindowDestroyed();
+
   // Sets the passed passed interstitial as the currently showing interstitial.
   // |interstitial_page| should be non NULL (use the remove_interstitial_page
   // method to unset the interstitial) and no interstitial page should be set
diff --git a/chrome/browser/tab_contents/tab_contents.cc b/chrome/browser/tab_contents/tab_contents.cc
index d253dc9..9042518 100644
--- a/chrome/browser/tab_contents/tab_contents.cc
+++ b/chrome/browser/tab_contents/tab_contents.cc
@@ -1062,6 +1062,10 @@ void TabContents::OnJavaScriptMessageBoxClosed(IPC::Message* reply_msg,
   render_manager_.OnJavaScriptMessageBoxClosed(reply_msg, success, prompt);
 }
 
+void TabContents::OnJavaScriptMessageBoxWindowDestroyed() {
+  render_manager_.OnJavaScriptMessageBoxWindowDestroyed();
+}
+
 void TabContents::OnSavePage() {
   // If we can not save the page, try to download it.
   if (!SavePackage::IsSavableContents(contents_mime_type())) {
diff --git a/chrome/browser/tab_contents/tab_contents.h b/chrome/browser/tab_contents/tab_contents.h
index 4f938a2..cf25aba 100644
--- a/chrome/browser/tab_contents/tab_contents.h
+++ b/chrome/browser/tab_contents/tab_contents.h
@@ -516,6 +516,9 @@ class TabContents : public PageNavigator,
                                     bool success,
                                     const std::wstring& prompt);
 
+  // AppModalDialog calls this when the javascript dialog has been destroyed.
+  void OnJavaScriptMessageBoxWindowDestroyed();
+
   // Prepare for saving the current web page to disk.
   void OnSavePage();
 
diff --git a/chrome/browser/views/jsmessage_box_dialog.cc b/chrome/browser/views/jsmessage_box_dialog.cc
index 336a427..df60f0f9 100644
--- a/chrome/browser/views/jsmessage_box_dialog.cc
+++ b/chrome/browser/views/jsmessage_box_dialog.cc
@@ -96,6 +96,10 @@ bool JavascriptMessageBoxDialog::Accept() {
   return true;
 }
 
+void JavascriptMessageBoxDialog::OnClose() {
+  parent_->OnClose();
+}
+
 std::wstring JavascriptMessageBoxDialog::GetDialogButtonLabel(
     MessageBoxFlags::DialogButton button) const {
   if (parent_->is_before_unload_dialog()) {
diff --git a/chrome/browser/views/jsmessage_box_dialog.h b/chrome/browser/views/jsmessage_box_dialog.h
index 41f7e31..f709a50 100644
--- a/chrome/browser/views/jsmessage_box_dialog.h
+++ b/chrome/browser/views/jsmessage_box_dialog.h
@@ -44,6 +44,7 @@ class JavascriptMessageBoxDialog : public views::DialogDelegate {
   virtual bool IsModal() const { return true; }
   virtual views::View* GetContentsView();
   virtual views::View* GetInitiallyFocusedView();
+  virtual void OnClose();
 
  private:
   TabContents* tab_contents() {
diff --git a/chrome/test/ui/npapi_uitest.cc b/chrome/test/ui/npapi_uitest.cc
index d1ad73e..f8d5f47 100644
--- a/chrome/test/ui/npapi_uitest.cc
+++ b/chrome/test/ui/npapi_uitest.cc
@@ -210,6 +210,15 @@ TEST_F(NPAPIVisiblePluginTester, AlertInWindowMessage) {
   ASSERT_TRUE((MessageBoxFlags::DIALOGBUTTON_OK & available_buttons) != 0);
   ASSERT_TRUE(automation()->ClickAppModalDialogButton(
       MessageBoxFlags::DIALOGBUTTON_OK));
+
+  modal_dialog_showing = false;
+  ASSERT_TRUE(automation()->WaitForAppModalDialog(kShortWaitTimeout));
+  ASSERT_TRUE(automation()->GetShowingAppModalDialog(&modal_dialog_showing,
+      &available_buttons));
+  ASSERT_TRUE(modal_dialog_showing);
+  ASSERT_TRUE((MessageBoxFlags::DIALOGBUTTON_OK & available_buttons) != 0);
+  ASSERT_TRUE(automation()->ClickAppModalDialogButton(
+      MessageBoxFlags::DIALOGBUTTON_OK));
 }
 
 #endif
diff --git a/views/window/dialog_client_view.cc b/views/window/dialog_client_view.cc
index 978d45a..5a36830 100644
--- a/views/window/dialog_client_view.cc
+++ b/views/window/dialog_client_view.cc
@@ -207,7 +207,7 @@ void DialogClientView::AcceptWindow() {
   }
   if (GetDialogDelegate()->Accept(false)) {
     accepted_ = true;
-    window()->Close();
+    Close();
   }
 }
 
@@ -215,7 +215,7 @@ void DialogClientView::CancelWindow() {
   // Call the standard Close handler, which checks with the delegate before
   // proceeding. This checking _isn't_ done here, but in the WM_CLOSE handler,
   // so that the close box on the window also shares this code path.
-  window()->Close();
+  Close();
 }
 
 ///////////////////////////////////////////////////////////////////////////////
@@ -327,7 +327,7 @@ gfx::Size DialogClientView::GetPreferredSize() {
 
 bool DialogClientView::AcceleratorPressed(const Accelerator& accelerator) {
   DCHECK(accelerator.GetKeyCode() == VK_ESCAPE);  // We only expect Escape key.
-  window()->Close();
+  Close();
   return true;
 }
 
@@ -459,4 +459,9 @@ void DialogClientView::InitClass() {
   }
 }
 
+void DialogClientView::Close() {
+  window()->Close();
+  GetDialogDelegate()->OnClose();
+}
+
 }  // namespace views
diff --git a/views/window/dialog_client_view.h b/views/window/dialog_client_view.h
index ecb9449..5d1fa33 100644
--- a/views/window/dialog_client_view.h
+++ b/views/window/dialog_client_view.h
@@ -95,6 +95,9 @@ class DialogClientView : public ClientView,
   // Returns the DialogDelegate for the window.
   DialogDelegate* GetDialogDelegate() const;
 
+  // Closes the window.
+  void Close();
+
   // The dialog buttons.
   NativeButton* ok_button_;
   NativeButton* cancel_button_;
diff --git a/views/window/dialog_delegate.h b/views/window/dialog_delegate.h
index 4418e61f..0d66956 100644
--- a/views/window/dialog_delegate.h
+++ b/views/window/dialog_delegate.h
@@ -103,6 +103,9 @@ class DialogDelegate : public WindowDelegate {
   // Overridden from WindowDelegate:
   virtual ClientView* CreateClientView(Window* window);
 
+  // Called when the window has been closed.
+  virtual void OnClose() {};
+
   // A helper for accessing the DialogClientView object contained by this
   // delegate's Window.
   DialogClientView* GetDialogClientView() const;
diff --git a/webkit/glue/plugins/test/plugin_windowed_test.cc b/webkit/glue/plugins/test/plugin_windowed_test.cc
index 79e662b..3a15ae0 100644
--- a/webkit/glue/plugins/test/plugin_windowed_test.cc
+++ b/webkit/glue/plugins/test/plugin_windowed_test.cc
@@ -92,6 +92,9 @@ LRESULT CALLBACK WindowedPluginTest::WindowProc(
     } else if (this_ptr->test_name() == "alert_in_window_message" &&
                message == WM_PAINT) {
       this_ptr->done_ = true;
+      // We call this function twice as we want to display two alerts
+      // and verify that we don't hang the browser.
+      CallJSFunction(this_ptr, "CallAlert");
       CallJSFunction(this_ptr, "CallAlert");
     }
   }