This prevents an occasional DCHECK seen during the SSL UI test that happened

because of the unresponsive tab logic.  If we are short-cutting a slow
beforeunload handler, we now avoid a redundant call to SetNavigationsSuspended.

Also clarified a few comments, removed what appeared to be an unnecessary call
to DidNavigateMainFrame, and re-enabled tests that were likely disabled due
to this issue.

BUG=6950
BUG=6698
TEST=SSLUITest.TestHTTPSExpiredCertAndDontProceed --single-process
TEST=SSLUITest.TestHTTPWithBrokenHTTPSResource
TEST=SSLUITest.TestOKHTTPS
TEST=SSLUITest.TestHTTPSExpiredCertAndProceed



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

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

4 files changed, 42 insertions, 17 deletions

diff --git a/chrome/browser/renderer_host/render_view_host.cc b/chrome/browser/renderer_host/render_view_host.cc
index 24a2a31..4a4a431 100644
--- a/chrome/browser/renderer_host/render_view_host.cc
+++ b/chrome/browser/renderer_host/render_view_host.cc
@@ -231,10 +231,13 @@ void RenderViewHost::LoadAlternateHTMLString(const std::string& html_text,
 }
 
 void RenderViewHost::SetNavigationsSuspended(bool suspend) {
+  // This should only be called to toggle the state.
   DCHECK(navigations_suspended_ != suspend);
+
   navigations_suspended_ = suspend;
   if (!suspend && suspended_nav_message_.get()) {
-    // Resume navigation
+    // There's a navigation message waiting to be sent.  Now that we're not
+    // suspended anymore, resume navigation by sending it.
     Send(suspended_nav_message_.release());
   }
 }
@@ -248,7 +251,7 @@ void RenderViewHost::FirePageBeforeUnload() {
   }
 
   // This may be called more than once (if the user clicks the tab close button
-  // several times, or if she clicks the tab close button than the browser close
+  // several times, or if she clicks the tab close button then the browser close
   // button), so this test makes sure we only send the message once.
   if (!is_waiting_for_unload_ack_) {
     // Start the hang monitor in case the renderer hangs in the beforeunload
diff --git a/chrome/browser/renderer_host/render_view_host.h b/chrome/browser/renderer_host/render_view_host.h
index 9529258..19bb424 100644
--- a/chrome/browser/renderer_host/render_view_host.h
+++ b/chrome/browser/renderer_host/render_view_host.h
@@ -120,13 +120,20 @@ class RenderViewHost : public RenderWidgetHost {
                                        const GURL& display_url,
                                        const std::string& security_info);
 
+  // Returns whether navigation messages are currently suspended for this
+  // RenderViewHost.  Only true during a cross-site navigation, while waiting
+  // for the onbeforeunload handler.
+  bool are_navigations_suspended() { return navigations_suspended_; }
+
   // Suspends (or unsuspends) any navigation messages from being sent from this
   // RenderViewHost.  This is called when a pending RenderViewHost is created
   // for a cross-site navigation, because we must suspend any navigations until
   // we hear back from the old renderer's onbeforeunload handler.  Note that it
   // is important that only one navigation event happen after calling this
   // method with |suspend| equal to true.  If |suspend| is false and there is
-  // a suspended_nav_message_, this will send the message.
+  // a suspended_nav_message_, this will send the message.  This function
+  // should only be called to toggle the state; callers should check
+  // are_navigations_suspended() first.
   void SetNavigationsSuspended(bool suspend);
 
   // Causes the renderer to invoke the onbeforeunload event handler.  The
diff --git a/chrome/browser/ssl/ssl_uitest.cc b/chrome/browser/ssl/ssl_uitest.cc
index 47cab12..2a84744 100644
--- a/chrome/browser/ssl/ssl_uitest.cc
+++ b/chrome/browser/ssl/ssl_uitest.cc
@@ -86,8 +86,7 @@ TEST_F(SSLUITest, TestHTTP) {
 
 // Visits a page over http which includes broken https resources (status should
 // be OK).
-// Disabled as test causes hangs, see bug 6698 for details.
-TEST_F(SSLUITest, DISABLED_TestHTTPWithBrokenHTTPSResource) {
+TEST_F(SSLUITest, TestHTTPWithBrokenHTTPSResource) {
   scoped_refptr<HTTPTestServer> http_server = PlainServer();
   scoped_refptr<HTTPSTestServer> bad_https_server = BadCertServer();
   scoped_ptr<TabProxy> tab(GetActiveTabProxy());
@@ -106,8 +105,7 @@ TEST_F(SSLUITest, DISABLED_TestHTTPWithBrokenHTTPSResource) {
 }
 
 // Visits a page over OK https:
-// Disabled as test causes hangs, see bug 6698 for details.
-TEST_F(SSLUITest, DISABLED_TestOKHTTPS) {
+TEST_F(SSLUITest, TestOKHTTPS) {
   scoped_refptr<HTTPSTestServer> https_server = GoodCertServer();
   scoped_ptr<TabProxy> tab(GetActiveTabProxy());
   NavigateTab(tab.get(),
@@ -128,8 +126,7 @@ TEST_F(SSLUITest, DISABLED_TestOKHTTPS) {
 }
 
 // Visits a page with https error and proceed:
-// Disabled as test causes hangs, see bug 6698 for details.
-TEST_F(SSLUITest, DISABLED_TestHTTPSExpiredCertAndProceed) {
+TEST_F(SSLUITest, TestHTTPSExpiredCertAndProceed) {
   scoped_refptr<HTTPSTestServer> bad_https_server = BadCertServer();
   scoped_ptr<TabProxy> tab(GetActiveTabProxy());
   NavigateTab(tab.get(),
diff --git a/chrome/browser/tab_contents/render_view_host_manager.cc b/chrome/browser/tab_contents/render_view_host_manager.cc
index 3e235e2..789bb57 100644
--- a/chrome/browser/tab_contents/render_view_host_manager.cc
+++ b/chrome/browser/tab_contents/render_view_host_manager.cc
@@ -124,16 +124,28 @@ bool RenderViewHostManager::ShouldCloseTabOnUnresponsiveRenderer() {
     return true;
 
   // If the tab becomes unresponsive during unload while doing a
-  // crosssite navigation, proceed with the navigation.
+  // cross-site navigation, proceed with the navigation.  (This assumes that
+  // the pending RenderViewHost is still responsive.)
   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);
+    // Haven't gotten around to starting the request, because we're still
+    // waiting for the beforeunload handler to finish.  We'll pretend that it
+    // did finish, to let the navigation proceed.  Note that there's a danger
+    // that the beforeunload handler will later finish and possibly return
+    // false (meaning the navigation should not proceed), but we'll ignore it
+    // in this case because it took too long.
+    if (pending_render_view_host_->are_navigations_suspended())
+      pending_render_view_host_->SetNavigationsSuspended(false);
   } else {
+    // The request has been started and paused, while we're waiting for the
+    // unload handler to finish.  We'll pretend that it did, by notifying the
+    // IO thread to let the response continue.  The pending renderer will then
+    // be swapped in as part of the usual DidNavigate logic.  (If the unload
+    // handler later finishes, this call will be ignored because the state in
+    // CrossSiteResourceHandler will already be cleaned up.)
     current_host()->process()->CrossSiteClosePageACK(
         pending_render_view_host_->site_instance()->process_host_id(), 
         pending_request_id);
-    DidNavigateMainFrame(pending_render_view_host_);
   }
   return false;
 }
@@ -212,8 +224,13 @@ void RenderViewHostManager::ShouldClosePage(bool proceed) {
   }
 
   if (proceed) {
-    // Ok to unload the current page, so proceed with the cross-site navigate.
-    pending_render_view_host_->SetNavigationsSuspended(false);
+    // Ok to unload the current page, so proceed with the cross-site
+    // navigation.  Note that if navigations are not currently suspended, it
+    // might be because the renderer was deemed unresponsive and this call was
+    // already made by ShouldCloseTabOnUnresponsiveRenderer.  In that case, it
+    // is ok to do nothing here.
+    if (pending_render_view_host_->are_navigations_suspended())
+      pending_render_view_host_->SetNavigationsSuspended(false);
   } else {
     // Current page says to cancel.
     CancelPendingRenderView();
@@ -462,8 +479,9 @@ RenderViewHost* RenderViewHostManager::UpdateRendererStateNavigate(
 
     // Suspend the new render view (i.e., don't let it send the cross-site
     // Navigate message) until we hear back from the old renderer's
-    // onbeforeunload handler.  If it returns false, we'll have to cancel the
-    // request.
+    // onbeforeunload handler.  If the handler returns false, we'll have to
+    // cancel the request.
+    DCHECK(!pending_render_view_host_->are_navigations_suspended());
     pending_render_view_host_->SetNavigationsSuspended(true);
 
     // Tell the CrossSiteRequestManager that this RVH has a pending cross-site