From 3596f90b2dffa20949870cd402833c3d5f05200e Mon Sep 17 00:00:00 2001 From: "creis@google.com" Date: Wed, 28 Jan 2009 21:15:49 +0000 Subject: 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 --- chrome/browser/renderer_host/render_view_host.cc | 7 +++-- chrome/browser/renderer_host/render_view_host.h | 9 +++++- chrome/browser/ssl/ssl_uitest.cc | 9 ++---- .../tab_contents/render_view_host_manager.cc | 34 +++++++++++++++++----- 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 http_server = PlainServer(); scoped_refptr bad_https_server = BadCertServer(); scoped_ptr 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 https_server = GoodCertServer(); scoped_ptr 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 bad_https_server = BadCertServer(); scoped_ptr 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 -- cgit v1.1