ChromeFrame tabs would hang at times while closing. This would randomly occur if the page had an unload handler.

We execute unload handlers in the WM_DESTROY message in the external tab and spin a nested loop waiting for the
unload handlers to finish. This causes a deadlock at times if a windows message is dispatched to IE which is blocked
in DestroyWindow.

The fix is to remove the nested loop mess from the external tab and instead send over a special automation message
to Chrome in which context we execute the unload handlers. The message contains the notification window and the
actual window message to be posted back when the unload handlers finish executing. 

The active document/activex spin a GetMessage loop waiting for this message to arrive. To ensure that we don't wait
indefinitely we have a 1 second timer and exit the loop if this timer is received.

Fixes bug http://code.google.com/p/chromium/issues/detail?id=49132

Bug=49132
Test=Covered by existing unload event test.

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

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

13 files changed, 131 insertions, 40 deletions

diff --git a/chrome/browser/automation/automation_provider.cc b/chrome/browser/automation/automation_provider.cc
index fb8c7a1..cbc6cd4 100644
--- a/chrome/browser/automation/automation_provider.cc
+++ b/chrome/browser/automation/automation_provider.cc
@@ -560,6 +560,7 @@ void AutomationProvider::OnMessageReceived(const IPC::Message& message) {
     IPC_MESSAGE_HANDLER(AutomationMsg_HandleMessageFromExternalHost,
                         OnMessageFromExternalHost)
     IPC_MESSAGE_HANDLER(AutomationMsg_BrowserMove, OnBrowserMoved)
+    IPC_MESSAGE_HANDLER(AutomationMsg_RunUnloadHandlers, OnRunUnloadHandlers)
 #endif  // defined(OS_WIN)
 #if defined(OS_CHROMEOS)
     IPC_MESSAGE_HANDLER_DELAY_REPLY(AutomationMsg_LoginWithUserAndPass,
diff --git a/chrome/browser/automation/automation_provider.h b/chrome/browser/automation/automation_provider.h
index 45c8abd..a0d2dd7 100644
--- a/chrome/browser/automation/automation_provider.h
+++ b/chrome/browser/automation/automation_provider.h
@@ -842,6 +842,9 @@ class AutomationProvider : public base::RefCounted<AutomationProvider>,
 
   void OnBrowserMoved(int handle);
 
+  void OnRunUnloadHandlers(int handle, gfx::NativeWindow notification_window,
+                           int notification_message);
+
   ExternalTabContainer* GetExternalTabForHandle(int handle);
 #endif  // defined(OS_WIN)
 
diff --git a/chrome/browser/automation/automation_provider_win.cc b/chrome/browser/automation/automation_provider_win.cc
index dbc8c48..b661d37 100644
--- a/chrome/browser/automation/automation_provider_win.cc
+++ b/chrome/browser/automation/automation_provider_win.cc
@@ -590,3 +590,13 @@ void AutomationProvider::NavigateExternalTabAtIndex(
     *status = AUTOMATION_MSG_NAVIGATION_SUCCESS;
   }
 }
+
+void AutomationProvider::OnRunUnloadHandlers(
+    int handle, gfx::NativeWindow notification_window,
+    int notification_message) {
+  ExternalTabContainer* external_tab = GetExternalTabForHandle(handle);
+  if (external_tab) {
+    external_tab->RunUnloadHandlers(notification_window, notification_message);
+  }
+}
+
diff --git a/chrome/browser/automation/chrome_frame_automation_provider.cc b/chrome/browser/automation/chrome_frame_automation_provider.cc
index 112f2fc..52f8ded 100644
--- a/chrome/browser/automation/chrome_frame_automation_provider.cc
+++ b/chrome/browser/automation/chrome_frame_automation_provider.cc
@@ -64,7 +64,8 @@ bool ChromeFrameAutomationProvider::IsValidMessage(uint32 type) {
     case AutomationMsg_RequestEnd::ID:
     case AutomationMsg_SaveAsAsync::ID:
     case AutomationMsg_RemoveBrowsingData::ID:
-    case AutomationMsg_OverrideEncoding::ID: {
+    case AutomationMsg_OverrideEncoding::ID:
+    case AutomationMsg_RunUnloadHandlers::ID: {
       is_valid_message = true;
       break;
     }
diff --git a/chrome/browser/external_tab_container_win.cc b/chrome/browser/external_tab_container_win.cc
index 8c08ec4..7d12555 100644
--- a/chrome/browser/external_tab_container_win.cc
+++ b/chrome/browser/external_tab_container_win.cc
@@ -40,10 +40,6 @@ static const wchar_t kWindowObjectKey[] = L"ChromeWindowObject";
 base::LazyInstance<ExternalTabContainer::PendingTabs>
     ExternalTabContainer::pending_tabs_(base::LINKER_INITIALIZED);
 
-// ExternalTabContainer::PendingTabs ExternalTabContainer::pending_tabs_;
-ExternalTabContainer* ExternalTabContainer::innermost_tab_for_unload_event_
-    = NULL;
-
 ExternalTabContainer::ExternalTabContainer(
     AutomationProvider* automation, AutomationResourceMessageFilter* filter)
     : automation_(automation),
@@ -60,7 +56,9 @@ ExternalTabContainer::ExternalTabContainer(
       pending_(false),
       infobars_enabled_(true),
       focus_manager_(NULL),
-      external_tab_view_(NULL) {
+      external_tab_view_(NULL),
+      notification_window_(NULL),
+      notification_message_(NULL) {
 }
 
 ExternalTabContainer::~ExternalTabContainer() {
@@ -394,17 +392,8 @@ void ExternalTabContainer::CloseContents(TabContents* source) {
   static const int kExternalTabCloseContentsDelayMS = 100;
 
   if (waiting_for_unload_event_) {
-    // If we are not the innermost tab waiting for the unload event to return
-    // then don't handle this notification right away as we need the inner
-    // message loop to terminate.
-    if (this != innermost_tab_for_unload_event_) {
-      ChromeThread::PostDelayedTask(
-          ChromeThread::UI, FROM_HERE,
-          NewRunnableMethod(this, &ExternalTabContainer::CloseContents,
-                            source), kExternalTabCloseContentsDelayMS);
-      return;
-    }
-    MessageLoop::current()->Quit();
+    PostMessage(notification_window_, notification_message_, 0, 0);
+    waiting_for_unload_event_ = false;
   } else {
     if (automation_) {
       automation_->Send(new AutomationMsg_CloseExternalTab(0, tab_handle_));
@@ -690,22 +679,6 @@ LRESULT ExternalTabContainer::OnCreate(LPCREATESTRUCT create_struct) {
 }
 
 void ExternalTabContainer::OnDestroy() {
-  if (tab_contents_) {
-    waiting_for_unload_event_ = true;
-    if (Browser::RunUnloadEventsHelper(tab_contents_)) {
-      // Maintain a local global stack of Externa;TabCotainers waiting for the
-      // unload event listeners to finish. We need this as we only want to
-      // handle the CloseContents call from the TabContents when the current
-      // ExternalTabContainers message loop is active. This ensures that nested
-      // ExternalTabContainer message loops terminate correctly.
-      ExternalTabContainer* current_tab = innermost_tab_for_unload_event_;
-      innermost_tab_for_unload_event_ = this;
-      MessageLoop::current()->Run();
-      innermost_tab_for_unload_event_ = current_tab;
-    }
-    waiting_for_unload_event_ = false;
-  }
-
   Uninitialize();
   WidgetWin::OnDestroy();
   if (browser_.get()) {
@@ -718,6 +691,23 @@ void ExternalTabContainer::OnFinalMessage(HWND window) {
   Release();
 }
 
+void ExternalTabContainer::RunUnloadHandlers(
+    gfx::NativeWindow notification_window,
+    int notification_message) {
+  DCHECK(::IsWindow(notification_window));
+  if (tab_contents_) {
+    notification_window_ = notification_window;
+    notification_message_ = notification_message;
+
+    if (Browser::RunUnloadEventsHelper(tab_contents_)) {
+      waiting_for_unload_event_ = true;
+    }
+  }
+  if (!waiting_for_unload_event_) {
+    PostMessage(notification_window, notification_message, 0, 0);
+  }
+}
+
 ////////////////////////////////////////////////////////////////////////////////
 // ExternalTabContainer, private:
 bool ExternalTabContainer::ProcessUnhandledKeyStroke(HWND window,
diff --git a/chrome/browser/external_tab_container_win.h b/chrome/browser/external_tab_container_win.h
index f9a51d0..1d92f49 100644
--- a/chrome/browser/external_tab_container_win.h
+++ b/chrome/browser/external_tab_container_win.h
@@ -206,6 +206,9 @@ class ExternalTabContainer : public TabContentsDelegate,
 
   virtual bool infobars_enabled();
 
+  void RunUnloadHandlers(gfx::NativeWindow notification_window,
+                         int notification_message);
+
  protected:
   // Overridden from views::WidgetWin:
   virtual LRESULT OnCreate(LPCREATESTRUCT create_struct);
@@ -313,11 +316,6 @@ class ExternalTabContainer : public TabContentsDelegate,
   // Set to true if the tab is waiting for the unload event to complete.
   bool waiting_for_unload_event_;
 
-  // Pointer to the innermost ExternalTabContainer instance which is waiting
-  // for the unload event listeners to finish.
-  // Used to maintain a local global stack of containers.
-  static ExternalTabContainer* innermost_tab_for_unload_event_;
-
   // Contains the list of URL requests which are pending waiting for an ack
   // from the external host.
   std::vector<PendingTopLevelNavigation> pending_open_url_requests_;
@@ -333,6 +331,9 @@ class ExternalTabContainer : public TabContentsDelegate,
 
   views::View* external_tab_view_;
 
+  gfx::NativeWindow notification_window_;
+  int notification_message_;
+
   DISALLOW_COPY_AND_ASSIGN(ExternalTabContainer);
 };
 
diff --git a/chrome/test/automation/automation_messages_internal.h b/chrome/test/automation/automation_messages_internal.h
index bdb0d07..d72e6d3 100644
--- a/chrome/test/automation/automation_messages_internal.h
+++ b/chrome/test/automation/automation_messages_internal.h
@@ -1404,4 +1404,15 @@ IPC_BEGIN_MESSAGES(Automation)
   //   None expected
   IPC_MESSAGE_ROUTED1(AutomationMsg_CloseExternalTab, int)
 
+  // This message requests that the external tab identified by the tab handle
+  // runs unload handlers if any on the current page.
+  // Request:
+  //   -int: Tab handle
+  //   -gfx::NativeWindow: notification window
+  //   -int: notification message.
+  // Response:
+  //   None expected
+  IPC_MESSAGE_ROUTED3(AutomationMsg_RunUnloadHandlers, int, gfx::NativeWindow,
+                      int)
+
 IPC_END_MESSAGES(Automation)
diff --git a/chrome_frame/chrome_frame_activex_base.h b/chrome_frame/chrome_frame_activex_base.h
index dc0a955..be8553a 100644
--- a/chrome_frame/chrome_frame_activex_base.h
+++ b/chrome_frame/chrome_frame_activex_base.h
@@ -361,6 +361,43 @@ END_MSG_MAP()
     return true;
   }
 
+  // IOleInPlaceObject overrides.
+  STDMETHOD(InPlaceDeactivate)(void) {
+    static UINT onload_handlers_done_msg =
+        RegisterWindowMessage(L"ChromeFrame_OnloadHandlersDone");
+
+    if (m_bInPlaceActive && IsWindow() && IsValid()) {
+      static const int kChromeFrameUnloadEventTimerId = 0xdeadbeef;
+      static const int kChromeFrameUnloadEventTimeout = 1000;
+
+      // To prevent us from indefinitely waiting for an acknowledgement from
+      // Chrome indicating that unload handlers have been run, we set a 1
+      // second timer and exit the loop when it fires.
+      ::SetTimer(m_hWnd, kChromeFrameUnloadEventTimerId,
+                 kChromeFrameUnloadEventTimeout, NULL);
+
+      automation_client_->RunUnloadHandlers(m_hWnd, onload_handlers_done_msg);
+
+      MSG msg = {0};
+      while (GetMessage(&msg, NULL, 0, 0)) {
+        if (msg.message == onload_handlers_done_msg &&
+            msg.hwnd == m_hWnd) {
+          break;
+        }
+
+        if (msg.message == WM_TIMER &&
+            msg.wParam == kChromeFrameUnloadEventTimerId) {
+          break;
+        }
+        TranslateMessage(&msg);
+        DispatchMessage(&msg);
+      }
+
+      ::KillTimer(m_hWnd, kChromeFrameUnloadEventTimerId);
+    }
+    return IOleInPlaceObjectWindowlessImpl<T>::InPlaceDeactivate();
+  }
+
  protected:
   virtual void GetProfilePath(const std::wstring& profile_name,
                               FilePath* profile_path) {
diff --git a/chrome_frame/chrome_frame_automation.cc b/chrome_frame/chrome_frame_automation.cc
index c6d43d7..681f055 100644
--- a/chrome_frame/chrome_frame_automation.cc
+++ b/chrome_frame/chrome_frame_automation.cc
@@ -1233,6 +1233,13 @@ void ChromeFrameAutomationClient::RemoveBrowsingData(int remove_mask) {
       new AutomationMsg_RemoveBrowsingData(0, remove_mask));
 }
 
+void ChromeFrameAutomationClient::RunUnloadHandlers(HWND notification_window,
+                                                    int notification_message) {
+  automation_server_->Send(
+      new AutomationMsg_RunUnloadHandlers(0, tab_handle_, notification_window,
+                                          notification_message));
+}
+
 //////////////////////////////////////////////////////////////////////////
 // PluginUrlRequestDelegate implementation.
 // Forward network related responses to Chrome.
diff --git a/chrome_frame/chrome_frame_automation.h b/chrome_frame/chrome_frame_automation.h
index 7b9b774..77c82a3 100644
--- a/chrome_frame/chrome_frame_automation.h
+++ b/chrome_frame/chrome_frame_automation.h
@@ -273,6 +273,10 @@ class ChromeFrameAutomationClient
   // For IDeleteBrowsingHistorySupport
   void RemoveBrowsingData(int remove_mask);
 
+  // Sends an IPC message to the external tab container requesting it to run
+  // unload handlers on the page.
+  void RunUnloadHandlers(HWND notification_window, int notification_message);
+
  protected:
   // ChromeFrameAutomationProxy::LaunchDelegate implementation.
   virtual void LaunchComplete(ChromeFrameAutomationProxy* proxy,
diff --git a/chrome_frame/test/data/fulltab_before_unload_event_main.html b/chrome_frame/test/data/fulltab_before_unload_event_main.html
index d4bbc47..51b50b6 100644
--- a/chrome_frame/test/data/fulltab_before_unload_event_main.html
+++ b/chrome_frame/test/data/fulltab_before_unload_event_main.html
@@ -1,6 +1,5 @@
 <html>
   <head>
-    <meta http-equiv="x-ua-compatible" content="chrome=1" />
     <title>ChromeFrame fulltab mode unload event final page</title>
     <script type="text/javascript"
         src="chrome_frame_tester_helpers.js"></script>
diff --git a/chrome_frame/test/data/fulltab_before_unload_event_test.html b/chrome_frame/test/data/fulltab_before_unload_event_test.html
index 562312a..c10aaea 100644
--- a/chrome_frame/test/data/fulltab_before_unload_event_test.html
+++ b/chrome_frame/test/data/fulltab_before_unload_event_test.html
@@ -1,6 +1,5 @@
 <html>
   <head>
-    <meta http-equiv="x-ua-compatible" content="chrome=1" />
     <title>ChromeFrame fulltab mode unload event test</title>
     <script type="text/javascript"
         src="chrome_frame_tester_helpers.js"></script>
diff --git a/chrome_frame/test/navigation_test.cc b/chrome_frame/test/navigation_test.cc
index 50da567..2324f86 100644
--- a/chrome_frame/test/navigation_test.cc
+++ b/chrome_frame/test/navigation_test.cc
@@ -676,5 +676,33 @@ TEST_P(FullTabNavigationTest, FLAKY_FormPostBackForward) {
   LaunchIEAndNavigate(kFormPostUrl);
 }
 
+TEST_P(FullTabNavigationTest, CF_UnloadEventTest) {
+  bool in_cf = GetParam().invokes_cf();
+  if (!in_cf) {
+    LOG(ERROR) << "Test not yet implemented.";
+    return;
+  }
+
+  std::wstring kUnloadEventTestUrl =
+      GetTestUrl(L"fulltab_before_unload_event_test.html");
+
+  std::wstring kUnloadEventMainUrl =
+      GetTestUrl(L"fulltab_before_unload_event_main.html");
+
+  server_mock_.ExpectAndServeAnyRequests(GetParam());
+  InSequence expect_in_sequence_for_scope;
+
+  ie_mock_.ExpectNavigation(in_cf, kUnloadEventTestUrl);
+  EXPECT_CALL(ie_mock_, OnLoad(in_cf, StrEq(kUnloadEventTestUrl)));
+
+  ie_mock_.ExpectNavigation(in_cf, kUnloadEventMainUrl);
+  EXPECT_CALL(ie_mock_, OnLoad(in_cf, StrEq(kUnloadEventMainUrl)));
+
+  EXPECT_CALL(ie_mock_, OnMessage(_, _, _))
+      .WillOnce(CloseBrowserMock(&ie_mock_));
+
+  LaunchIEAndNavigate(kUnloadEventTestUrl);
+}
+
 }  // namespace chrome_frame_test