diff options
author | creis@chromium.org <creis@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-03-12 20:12:58 +0000 |
---|---|---|
committer | creis@chromium.org <creis@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-03-12 20:12:58 +0000 |
commit | 8c0eb70b0f944127475e420765286ac082645dd4 (patch) | |
tree | 447fc228b9657e2c759a93b4b2e4c1ad42a2cf50 /content | |
parent | 95102f6e9af498512938d4853fe0a22426ab47b1 (diff) | |
download | chromium_src-8c0eb70b0f944127475e420765286ac082645dd4.zip chromium_src-8c0eb70b0f944127475e420765286ac082645dd4.tar.gz chromium_src-8c0eb70b0f944127475e420765286ac082645dd4.tar.bz2 |
Fixing a problem, where a hung renderer process is not killed when navigating away
BUG=104346
TEST=Steps to reproduce listed in the bug.
Review URL: https://chromiumcodereview.appspot.com/9514016
Patch from Nasko Oskov <nasko@chromium.org>.
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@126199 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'content')
-rw-r--r-- | content/browser/renderer_host/render_process_host_browsertest.cc | 68 | ||||
-rw-r--r-- | content/browser/renderer_host/render_view_host_impl.cc | 40 | ||||
-rw-r--r-- | content/test/data/english_page.html | 7 | ||||
-rw-r--r-- | content/test/data/infinite_beforeunload.html | 14 | ||||
-rw-r--r-- | content/test/data/infinite_unload.html | 14 |
5 files changed, 143 insertions, 0 deletions
diff --git a/content/browser/renderer_host/render_process_host_browsertest.cc b/content/browser/renderer_host/render_process_host_browsertest.cc index 50de340..4ea27e9 100644 --- a/content/browser/renderer_host/render_process_host_browsertest.cc +++ b/content/browser/renderer_host/render_process_host_browsertest.cc @@ -15,6 +15,7 @@ #include "content/common/test_url_constants.h" #include "content/public/browser/browser_thread.h" #include "content/public/common/content_switches.h" +#include "net/test/test_server.h" using content::WebContents; @@ -252,3 +253,70 @@ IN_PROC_BROWSER_TEST_F(RenderProcessHostTest, ProcessOverflow) { IN_PROC_BROWSER_TEST_F(RenderProcessHostTestWithCommandLine, ProcessOverflow) { TestProcessOverflow(); } + +IN_PROC_BROWSER_TEST_F(RenderProcessHostTest, UnresponsiveCrossSiteNavigation) { + WebContents* tab = NULL; + WebContents* tab2 = NULL; + content::RenderProcessHost* rph = NULL; + base::ProcessHandle process; + FilePath doc_root; + + doc_root = doc_root.Append(FILE_PATH_LITERAL("content")); + doc_root = doc_root.Append(FILE_PATH_LITERAL("test")); + doc_root = doc_root.Append(FILE_PATH_LITERAL("data")); + + // Start two servers to enable cross-site navigations. + net::TestServer server(net::TestServer::TYPE_HTTP, + net::TestServer::kLocalhost, doc_root); + ASSERT_TRUE(server.Start()); + net::TestServer https_server(net::TestServer::TYPE_HTTPS, + net::TestServer::kLocalhost, doc_root); + ASSERT_TRUE(https_server.Start()); + + GURL infinite_beforeunload_url( + server.GetURL("files/infinite_beforeunload.html")); + GURL infinite_unload_url(server.GetURL("files/infinite_unload.html")); + GURL same_process_url(server.GetURL("files/english_page.html")); + GURL new_process_url(https_server.GetURL("files/english_page.html")); + + // Navigate the tab to the page which will lock up the process when we + // navigate away from it. + ui_test_utils::NavigateToURL(browser(), infinite_unload_url); + tab = browser()->GetWebContentsAt(0); + rph = tab->GetRenderProcessHost(); + EXPECT_EQ(tab->GetURL(), infinite_unload_url); + + // Remember the process prior to navigation, as we expect it to get killed. + process = rph->GetHandle(); + ASSERT_TRUE(process); + + ui_test_utils::NavigateToURL(browser(), new_process_url); + // This should fail, because the navigation to the new URL would result in + // the process getting killed. This is an indirect way to check for the + // process having been terminated. + EXPECT_FALSE(base::KillProcess(process, 1, false)); + + // Now, let's load the unresponsive page in one tab, then open another tab + // which will use the same process. + ui_test_utils::NavigateToURL(browser(), infinite_beforeunload_url); + tab = browser()->GetWebContentsAt(0); + rph = tab->GetRenderProcessHost(); + EXPECT_EQ(tab->GetURL(), infinite_beforeunload_url); + + ui_test_utils::NavigateToURLWithDisposition(browser(), same_process_url, + NEW_BACKGROUND_TAB, ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION); + EXPECT_EQ(browser()->tab_count(), 2); + tab2 = browser()->GetWebContentsAt(1); + ASSERT_TRUE(tab2 != NULL); + EXPECT_EQ(tab2->GetURL(), same_process_url); + EXPECT_EQ(rph, tab2->GetRenderProcessHost()); + + process = rph->GetHandle(); + ASSERT_TRUE(process); + + // Navigating to the cross site URL will not kill the process, since it will + // have more than one tab using it. Kill it to confirm that it is still there, + // as well as finish the test faster. + ui_test_utils::NavigateToURL(browser(), new_process_url); + EXPECT_TRUE(base::KillProcess(process, 1, false)); +} diff --git a/content/browser/renderer_host/render_view_host_impl.cc b/content/browser/renderer_host/render_view_host_impl.cc index daf17c7..cf2593f 100644 --- a/content/browser/renderer_host/render_view_host_impl.cc +++ b/content/browser/renderer_host/render_view_host_impl.cc @@ -41,6 +41,7 @@ #include "content/public/browser/notification_details.h" #include "content/public/browser/notification_service.h" #include "content/public/browser/notification_types.h" +#include "content/public/browser/render_process_host.h" #include "content/public/browser/render_view_host_delegate.h" #include "content/public/browser/render_view_host_observer.h" #include "content/public/browser/user_metrics.h" @@ -418,6 +419,12 @@ void RenderViewHostImpl::WasSwappedOut() { // Don't bother reporting hung state anymore. StopHangMonitorTimeout(); + // If we are still waiting on the unload handler to be run, we consider + // the process hung and we should terminate it if there are no other tabs + // using the process. If there are other views using this process, the + // unresponsive renderer timeout will catch it. + bool hung = is_waiting_for_unload_ack_; + // Now that we're no longer the active RVH in the tab, start filtering out // most IPC messages. Usually the renderer will have stopped sending // messages as of OnSwapOutACK. However, we may have timed out waiting @@ -426,6 +433,39 @@ void RenderViewHostImpl::WasSwappedOut() { // still allow synchronous messages through). SetSwappedOut(true); + // If we are not running the renderer in process and no other tab is using + // the hung process, kill it, assuming it is a real process (unit tests don't + // have real processes). + if (hung) { + base::ProcessHandle process_handle = GetProcess()->GetHandle(); + int views = 0; + + // Count the number of listeners for the process, which is equivalent to + // views using the process as of this writing. + content::RenderProcessHost::listeners_iterator iter( + GetProcess()->ListenersIterator()); + for (; !iter.IsAtEnd(); iter.Advance()) + ++views; + + if (!content::RenderProcessHost::run_renderer_in_process() && + process_handle && views <= 1) { + // We expect the delegate for this RVH to be TabContents, as it is the + // only class that swaps out render view hosts on navigation. + DCHECK(delegate_->GetRenderViewType() == content::VIEW_TYPE_TAB_CONTENTS); + + // Kill the process only if TabContents sets SuddenTerminationAllowed, + // which indicates that the timer has expired. + // This is not the case if we load data URLs or about:blank. The reason + // is that there is no network requests and this code is hit without + // setting the unresponsiveness timer. This allows a corner case where a + // navigation to a data URL will leave a process running, if the + // beforeunload handler completes fine, but the unload handler hangs. + // At this time, the complexity to solve this edge case is not worthwhile. + if (SuddenTerminationAllowed()) + base::KillProcess(process_handle, content::RESULT_CODE_HUNG, false); + } + } + // Inform the renderer that it can exit if no one else is using it. Send(new ViewMsg_WasSwappedOut(GetRoutingID())); } diff --git a/content/test/data/english_page.html b/content/test/data/english_page.html new file mode 100644 index 0000000..30b0d42 --- /dev/null +++ b/content/test/data/english_page.html @@ -0,0 +1,7 @@ +<html> +<head><title>This page is in English</title></head> +<body> +No joke! This is a page written in English. Awesome don't you think? +It has to be more than 100 bytes long for the CLD to detect the language correctly. +</body> +</html> diff --git a/content/test/data/infinite_beforeunload.html b/content/test/data/infinite_beforeunload.html new file mode 100644 index 0000000..785ad0f --- /dev/null +++ b/content/test/data/infinite_beforeunload.html @@ -0,0 +1,14 @@ +<html> +<head> + <title>Infinite beforeunload</title> +</head> + +<body> +<script> + window.onbeforeunload = function(e) { + while(true){} + } +</script> +</body> + +</html> diff --git a/content/test/data/infinite_unload.html b/content/test/data/infinite_unload.html new file mode 100644 index 0000000..671250d --- /dev/null +++ b/content/test/data/infinite_unload.html @@ -0,0 +1,14 @@ +<html> +<head> + <title>Infinite unload</title> +</head> + +<body> +<script> + window.onunload = function(e) { + while(true){} + } +</script> +</body> + +</html> |