Bandaid patch so that we continue with crosssite navigations instead of closing the tab if the beforeunload /unload handler hangs. This patch does the right user-visible behavior, but I'm not a huge fan of the plumbing necessary to make it work. Totally open to cleanup suggestions.

There's also currently one bug that I haven't been able to pinpoint in the UI test. It only treats the first UI test of the four that I run as a cross-site navigation. No matter which test I run first. I wonder if there is some state I should be setting/clearing before/after each test run?

Also there's a DHECK that we hit that the UI test exposed. I 'm not sure it's a case that a user could actually hit though and it's not new with this code, so I added a TODO.

Can I get help from a mac person on adding the UI test to the xcode project?

BUG=3198
Review URL: http://codereview.chromium.org/8920

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

17 files changed, 233 insertions, 25 deletions

diff --git a/chrome/browser/render_view_host.cc b/chrome/browser/render_view_host.cc
index 84f5a7d..8f866ec 100644
--- a/chrome/browser/render_view_host.cc
+++ b/chrome/browser/render_view_host.cc
@@ -243,9 +243,6 @@ void RenderViewHost::FirePageBeforeUnload() {
 }
 
 void RenderViewHost::FirePageUnload() {
-  // Start the hang monitor in case the renderer hangs in the unload handler.
-  is_waiting_for_unload_ack_ = true;
-  StartHangMonitorTimeout(TimeDelta::FromMilliseconds(kUnloadTimeoutMS));
   ClosePage(site_instance()->process_host_id(),
             routing_id());
 }
@@ -267,6 +264,10 @@ void RenderViewHost::ClosePageIgnoringUnloadEvents(int render_process_host_id,
 
 void RenderViewHost::ClosePage(int new_render_process_host_id,
                                int new_request_id) {
+  // Start the hang monitor in case the renderer hangs in the unload handler.
+  is_waiting_for_unload_ack_ = true;
+  StartHangMonitorTimeout(TimeDelta::FromMilliseconds(kUnloadTimeoutMS));
+
   if (IsRenderViewLive()) {
     Send(new ViewMsg_ClosePage(routing_id_,
                                new_render_process_host_id,
@@ -280,9 +281,15 @@ void RenderViewHost::ClosePage(int new_render_process_host_id,
   }
 }
 
-void RenderViewHost::SetHasPendingCrossSiteRequest(bool has_pending_request) {
+void RenderViewHost::SetHasPendingCrossSiteRequest(bool has_pending_request, 
+                                                   int request_id) {
   Singleton<CrossSiteRequestManager>()->SetHasPendingCrossSiteRequest(
       process()->host_id(), routing_id_, has_pending_request);
+  pending_request_id_ = request_id;
+}
+
+int RenderViewHost::GetPendingRequestId() {
+  return pending_request_id_;
 }
 
 void RenderViewHost::OnCrossSiteResponse(int new_render_process_host_id,
@@ -1041,6 +1048,7 @@ void RenderViewHost::OnMsgRunJavaScriptMessage(
   StopHangMonitorTimeout();
   if (modal_dialog_count_++ == 0)
     SetEvent(modal_dialog_event_.Get());
+  bool did_suppress_message = false;
   delegate_->RunJavaScriptMessage(message, default_prompt, flags, reply_msg);
 }
 
@@ -1251,22 +1259,11 @@ void RenderViewHost::OnQueryFormFieldAutofill(const std::wstring& field_name,
 }
 
 void RenderViewHost::NotifyRendererUnresponsive() {
-  if (is_waiting_for_unload_ack_ &&
-      !Singleton<CrossSiteRequestManager>()->HasPendingCrossSiteRequest(
-          process()->host_id(), routing_id_)) {
-    // If the tab hangs in the beforeunload/unload handler there's really
-    // nothing we can do to recover. Pretend the unload listeners have
-    // all fired and close the tab. If the hang is in the beforeunload handler
-    // then the user will not have the option of cancelling the close.
-    UnloadListenerHasFired();
-    delegate_->Close(this);
-    return;
-  }
-
   // If the debugger is attached, we're going to be unresponsive anytime it's
   // stopped at a breakpoint.
-  if (!debugger_attached_)
-    delegate_->RendererUnresponsive(this);
+  if (!debugger_attached_) {
+    delegate_->RendererUnresponsive(this, is_waiting_for_unload_ack_);
+  }
 }
 
 void RenderViewHost::NotifyRendererResponsive() {
diff --git a/chrome/browser/render_view_host.h b/chrome/browser/render_view_host.h
index 7f7d298..d35c0b3 100644
--- a/chrome/browser/render_view_host.h
+++ b/chrome/browser/render_view_host.h
@@ -155,7 +155,12 @@ class RenderViewHost : public RenderWidgetHost {
   // for which another renderer will need to run an onunload event handler.
   // This is called before the first navigation event for this RenderViewHost,
   // and again after the corresponding OnCrossSiteResponse.
-  void SetHasPendingCrossSiteRequest(bool has_pending_request);
+  void SetHasPendingCrossSiteRequest(bool has_pending_request, int request_id);
+
+  // Returns the request_id for the pending cross-site request.
+  // This is just needed in case the unload of the current page
+  // hangs, in which case we need to swap to the pending RenderViewHost.
+  int GetPendingRequestId();
 
   // Called by ResourceDispatcherHost when a response for a pending cross-site
   // request is received.  The ResourceDispatcherHost will pause the response
@@ -540,6 +545,13 @@ class RenderViewHost : public RenderWidgetHost {
   // sending messages back to the browser.
   bool enable_dom_ui_bindings_;
 
+  // The request_id for the pending cross-site request. Set to -1 if
+  // there is a pending request, but we have not yet started the unload
+  // for the current page. Set to the request_id value of the pending
+  // request once we have gotten the some data for the pending page
+  // and thus started the unload process.
+  int pending_request_id_;
+
   // True if javascript access to the external host (through
   // automation) is allowed.
   bool enable_external_host_bindings_;
diff --git a/chrome/browser/render_view_host_delegate.h b/chrome/browser/render_view_host_delegate.h
index e4aefb1..376287f 100644
--- a/chrome/browser/render_view_host_delegate.h
+++ b/chrome/browser/render_view_host_delegate.h
@@ -348,7 +348,8 @@ class RenderViewHostDelegate {
 
   // Notification that the renderer has become unresponsive. The
   // delegate can use this notification to show a warning to the user.
-  virtual void RendererUnresponsive(RenderViewHost* render_view_host) { }
+  virtual void RendererUnresponsive(RenderViewHost* render_view_host, 
+                                    bool is_during_unload) { }
 
   // Notification that a previously unresponsive renderer has become
   // responsive again. The delegate can use this notification to end the
diff --git a/chrome/browser/render_view_host_manager.cc b/chrome/browser/render_view_host_manager.cc
index 96a4653..81e95aa 100644
--- a/chrome/browser/render_view_host_manager.cc
+++ b/chrome/browser/render_view_host_manager.cc
@@ -157,6 +157,25 @@ void RenderViewHostManager::SetIsLoading(bool is_loading) {
     original_render_view_host_->SetIsLoading(is_loading);
 }
 
+bool RenderViewHostManager::ShouldCloseTabOnUnresponsiveRenderer() {
+  if (renderer_state_ != PENDING)
+    return true;
+
+  // If the tab becomes unresponsive during unload while doing a
+  // crosssite navigation, proceed with the navigation.
+  int pending_request_id = pending_render_view_host_->GetPendingRequestId();
+  if (pending_request_id == -1) {
+    // Haven't gotten around to starting the request. 
+    pending_render_view_host_->SetNavigationsSuspended(false);
+  } else {
+    current_host()->process()->CrossSiteClosePageACK(
+        pending_render_view_host_->site_instance()->process_host_id(), 
+        pending_request_id);
+    DidNavigateMainFrame(pending_render_view_host_);
+  }
+  return false;
+}
+
 void RenderViewHostManager::DidNavigateMainFrame(
     RenderViewHost* render_view_host) {
   if (renderer_state_ == NORMAL) {
@@ -292,7 +311,8 @@ void RenderViewHostManager::OnCrossSiteResponse(int new_render_process_host_id,
   // means it is not a download or unsafe page, and we are going to perform the
   // navigation.  Thus, we no longer need to remember that the RenderViewHost
   // is part of a pending cross-site request.
-  pending_render_view_host_->SetHasPendingCrossSiteRequest(false);
+  pending_render_view_host_->SetHasPendingCrossSiteRequest(false, 
+                                                           new_request_id);
 }
 
 void RenderViewHostManager::RendererAbortedProvisionalLoad(
@@ -873,7 +893,7 @@ RenderViewHost* RenderViewHostManager::UpdateRendererStateNavigate(
     // Tell the CrossSiteRequestManager that this RVH has a pending cross-site
     // request, so that ResourceDispatcherHost will know to tell us to run the
     // old page's onunload handler before it sends the response.
-    pending_render_view_host_->SetHasPendingCrossSiteRequest(true);
+    pending_render_view_host_->SetHasPendingCrossSiteRequest(true, -1);
 
     // We now have a pending RVH.  If we were in NORMAL, we should now be in
     // PENDING.  If we were in LEAVING_INTERSTITIAL, we should stay there.
diff --git a/chrome/browser/render_view_host_manager.h b/chrome/browser/render_view_host_manager.h
index 45a5b69..371c398 100644
--- a/chrome/browser/render_view_host_manager.h
+++ b/chrome/browser/render_view_host_manager.h
@@ -106,6 +106,11 @@ class RenderViewHostManager {
   // don't know which one so we tell them all.
   void SetIsLoading(bool is_loading);
 
+  // Whether to close the tab or not when there is a hang during an unload
+  // handler. If we are mid-crosssite navigation, then we should proceed
+  // with the navigation instead of closing the tab.
+  bool ShouldCloseTabOnUnresponsiveRenderer();
+
   // Called when a renderer's main frame navigates. This handles all the logic
   // associated with interstitial management.
   void DidNavigateMainFrame(RenderViewHost* render_view_host);
diff --git a/chrome/browser/unload_uitest.cc b/chrome/browser/unload_uitest.cc
new file mode 100644
index 0000000..61f9024
--- /dev/null
+++ b/chrome/browser/unload_uitest.cc
@@ -0,0 +1,102 @@
+// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/file_util.h"
+
+#include "chrome/browser/automation/url_request_mock_http_job.h"
+#include "chrome/test/automation/browser_proxy.h"
+#include "chrome/test/ui/ui_test.h"
+#include "net/url_request/url_request_unittest.h"
+
+class UnloadTest : public UITest {
+ public:
+  void CheckTitle(const std::wstring& expected_title) {
+    const int kCheckDelayMs = 100;
+    int max_wait_time = 5000;
+    while (max_wait_time > 0) {
+      max_wait_time -= kCheckDelayMs;
+      Sleep(kCheckDelayMs);
+      if (expected_title == GetActiveTabTitle())
+        break;
+    }
+
+    EXPECT_EQ(expected_title, GetActiveTabTitle());
+  }
+
+  void NavigateToUnloadFileUsingTestServer(const std::wstring& test_filename,
+                                           const std::wstring& expected_title) {
+    const wchar_t kDocRoot[] = L"chrome/test/data";
+    TestServer server(kDocRoot);
+
+    std::wstring test_file = L"files/unload/";
+    file_util::AppendToPath(&test_file, test_filename);
+
+    GURL url(server.TestServerPageW(test_file));
+    NavigateToURL(url);
+
+    CheckTitle(expected_title);
+  }
+
+  void NavigateToNolistenersFileTwice() {
+    NavigateToURL(
+        URLRequestMockHTTPJob::GetMockUrl(L"unload/nolisteners.html"));
+    CheckTitle(L"nolisteners");
+    NavigateToURL(
+        URLRequestMockHTTPJob::GetMockUrl(L"unload/nolisteners.html"));
+    CheckTitle(L"nolisteners");
+  }
+
+  void NavigateToNolistenersFileTwiceAsync() {
+    // TODO(ojan): We hit a DCHECK in RenderViewHost::OnMsgShouldCloseACK
+    // if we don't sleep here.
+    Sleep(400);
+    NavigateToURLAsync(
+        URLRequestMockHTTPJob::GetMockUrl(L"unload/nolisteners.html"));
+    Sleep(400);
+    NavigateToURLAsync(
+        URLRequestMockHTTPJob::GetMockUrl(L"unload/nolisteners.html"));
+
+    Sleep(2000);
+
+    CheckTitle(L"nolisteners");
+  }
+};
+
+// Navigate to a page with an infinite unload handler.
+// Then two two async crosssite requests to ensure
+// we don't get confused and think we're closing the tab.
+TEST_F(UnloadTest, CrossSiteInfiniteUnloadAsync) {
+  NavigateToUnloadFileUsingTestServer(L"unloadlooping.html", L"unloadlooping");
+  NavigateToNolistenersFileTwiceAsync();
+  ASSERT_TRUE(IsBrowserRunning());
+}
+
+// Navigate to a page with an infinite unload handler.
+// Then two two sync crosssite requests to ensure
+// we correctly nav to each one. 
+TEST_F(UnloadTest, CrossSiteInfiniteUnloadSync) {
+  NavigateToUnloadFileUsingTestServer(L"unloadlooping.html", L"unloadlooping");
+  NavigateToNolistenersFileTwice();
+  ASSERT_TRUE(IsBrowserRunning());
+}
+
+// Navigate to a page with an infinite beforeunload handler.
+// Then two two async crosssite requests to ensure
+// we don't get confused and think we're closing the tab.
+TEST_F(UnloadTest, CrossSiteInfiniteBeforeUnloadAsync) {
+  NavigateToUnloadFileUsingTestServer(L"beforeunloadlooping.html", 
+                                      L"beforeunloadlooping");
+  NavigateToNolistenersFileTwiceAsync();
+  ASSERT_TRUE(IsBrowserRunning());
+}
+
+// Navigate to a page with an infinite beforeunload handler.
+// Then two two sync crosssite requests to ensure
+// we correctly nav to each one. 
+TEST_F(UnloadTest, CrossSiteInfiniteBeforeUnloadSync) {
+  NavigateToUnloadFileUsingTestServer(L"beforeunloadlooping.html", 
+                                      L"beforeunloadlooping");
+  NavigateToNolistenersFileTwice();
+  ASSERT_TRUE(IsBrowserRunning());
+}
diff --git a/chrome/browser/web_contents.cc b/chrome/browser/web_contents.cc
index e454275..cedb58c 100644
--- a/chrome/browser/web_contents.cc
+++ b/chrome/browser/web_contents.cc
@@ -1333,7 +1333,25 @@ bool WebContents::CanBlur() const {
   return delegate() ? delegate()->CanBlur() : true;
 }
 
-void WebContents::RendererUnresponsive(RenderViewHost* rvh) {
+void WebContents::RendererUnresponsive(RenderViewHost* rvh, 
+                                       bool is_during_unload) {
+  if (is_during_unload) {
+    // Hang occurred while firing the beforeunload/unload handler.
+    // Pretend the handler fired so tab closing continues as if it had.
+    rvh->UnloadListenerHasFired();
+
+    if (!render_manager_.ShouldCloseTabOnUnresponsiveRenderer()) {
+      return;
+    }
+
+    // If the tab hangs in the beforeunload/unload handler there's really
+    // nothing we can do to recover. Pretend the unload listeners have
+    // all fired and close the tab. If the hang is in the beforeunload handler
+    // then the user will not have the option of cancelling the close.
+    Close(rvh);
+    return;
+  }
+
   if (render_view_host() && render_view_host()->IsRenderViewLive())
     HungRendererWarning::ShowForWebContents(this);
 }
diff --git a/chrome/browser/web_contents.h b/chrome/browser/web_contents.h
index 9804ac6..267e546 100644
--- a/chrome/browser/web_contents.h
+++ b/chrome/browser/web_contents.h
@@ -311,7 +311,8 @@ class WebContents : public TabContents,
                                         new_request_id);
   }
   virtual bool CanBlur() const;
-  virtual void RendererUnresponsive(RenderViewHost* render_view_host);
+  virtual void RendererUnresponsive(RenderViewHost* render_view_host, 
+                                    bool is_during_unload);
   virtual void RendererResponsive(RenderViewHost* render_view_host);
   virtual void LoadStateChanged(const GURL& url, net::LoadState load_state);
   virtual void OnDidGetApplicationInfo(
diff --git a/chrome/test/data/unload/beforeunloadlooping.html b/chrome/test/data/unload/beforeunloadlooping.html
new file mode 100644
index 0000000..ca82c87
--- /dev/null
+++ b/chrome/test/data/unload/beforeunloadlooping.html
@@ -0,0 +1,12 @@
+<html>
+<head>
+<title>beforeunloadlooping</title>
+</head>
+<body>
+<script>
+window.onbeforeunload = function(e) {
+  while(true) {}
+}
+</script>
+</body>
+</html>
\ No newline at end of file
diff --git a/chrome/test/data/unload/nolisteners.html b/chrome/test/data/unload/nolisteners.html
index 42682b4..ea59143 100644
--- a/chrome/test/data/unload/nolisteners.html
+++ b/chrome/test/data/unload/nolisteners.html
@@ -1 +1,7 @@
-<html><body></body></html>
\ No newline at end of file
+<html>
+<head>
+<title>nolisteners</title>
+</head>
+<body>
+</body>
+</html>
\ No newline at end of file
diff --git a/chrome/test/data/unload/unload.html b/chrome/test/data/unload/unload.html
index 4ce7f78..eb34941 100644
--- a/chrome/test/data/unload/unload.html
+++ b/chrome/test/data/unload/unload.html
@@ -1,4 +1,7 @@
 <html>
+<head>
+<title>unload</title>
+</head>
 <body>
 <script>
 window.onunload = function(e) {
diff --git a/chrome/test/data/unload/unloadlooping.html b/chrome/test/data/unload/unloadlooping.html
index 134ce30..443b99a 100644
--- a/chrome/test/data/unload/unloadlooping.html
+++ b/chrome/test/data/unload/unloadlooping.html
@@ -1,4 +1,7 @@
 <html>
+<head>
+<title>unloadlooping</title>
+</head>
 <body>
 <script>
 window.onunload = function(e) {
diff --git a/chrome/test/data/unload/unloadloopingalert.html b/chrome/test/data/unload/unloadloopingalert.html
index dacd9b5..c43e295 100644
--- a/chrome/test/data/unload/unloadloopingalert.html
+++ b/chrome/test/data/unload/unloadloopingalert.html
@@ -1,4 +1,7 @@
 <html>
+<head>
+<title>unloadloopingalert</title>
+</head>
 <body>
 <script>
 window.onunload = function(e) {
diff --git a/chrome/test/data/unload/unloadloopingtwosecondsalert.html b/chrome/test/data/unload/unloadloopingtwosecondsalert.html
index ec796f9..8a1aa36 100644
--- a/chrome/test/data/unload/unloadloopingtwosecondsalert.html
+++ b/chrome/test/data/unload/unloadloopingtwosecondsalert.html
@@ -1,4 +1,7 @@
 <html>
+<head>
+<title>unloadloopingtwosecondsalert</title>
+</head>
 <body>
 <script>
 window.onunload = function(e) {
diff --git a/chrome/test/ui/ui_test.cc b/chrome/test/ui/ui_test.cc
index 322f02c..f4371d2 100644
--- a/chrome/test/ui/ui_test.cc
+++ b/chrome/test/ui/ui_test.cc
@@ -342,6 +342,15 @@ TabProxy* UITest::GetActiveTab() {
   return window_proxy->GetTab(active_tab_index);
 }
 
+void UITest::NavigateToURLAsync(const GURL& url) {
+  scoped_ptr<TabProxy> tab_proxy(GetActiveTab());
+  ASSERT_TRUE(tab_proxy.get());
+  if (!tab_proxy.get())
+    return;
+
+  tab_proxy->NavigateToURLAsync(url);
+}
+
 void UITest::NavigateToURL(const GURL& url) {
   scoped_ptr<TabProxy> tab_proxy(GetActiveTab());
   ASSERT_TRUE(tab_proxy.get());
diff --git a/chrome/test/ui/ui_test.h b/chrome/test/ui/ui_test.h
index f575496..66a103c 100644
--- a/chrome/test/ui/ui_test.h
+++ b/chrome/test/ui/ui_test.h
@@ -67,6 +67,11 @@ class UITest : public testing::Test {
   // Exits out browser instance.
   void QuitBrowser();
 
+  // Tells the browser to navigato to the givne URL in the active tab
+  // of the first app window.
+  // Does not wait for the navigation to complete to return.
+  void NavigateToURLAsync(const GURL& url);
+
   // Tells the browser to navigate to the given URL in the active tab
   // of the first app window.
   // This method doesn't return until the navigation is complete.
diff --git a/chrome/test/ui/ui_tests.vcproj b/chrome/test/ui/ui_tests.vcproj
index 5bfe40e..067b232 100644
--- a/chrome/test/ui/ui_tests.vcproj
+++ b/chrome/test/ui/ui_tests.vcproj
@@ -323,6 +323,14 @@
 			</File>
 		</Filter>
 		<Filter
+			Name="TestUnload"
+			>
+			<File
+				RelativePath="..\..\browser\unload_uitest.cc"
+				>
+			</File>
+		</Filter>
+		<Filter
 			Name="TestAuthentication"
 			>
 			<File