Fixing a problem, where a hung renderer process is not killed when navigating away

Fixing a problem, where a hung renderer process is not killed when navigating away

BUG=104346
TEST=Steps to reproduce listed in the bug.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=132407

[nasko, 2012-04-12 22:52:59]

sky, avi, in your area of review, this is identical to the previous change you
reviewed at http://codereview.chromium.org/10008015.

This is the same fix as before, with a small change to fix an uninitialized
variable and improve a check in the browser test. The diff between this new
patch and the previously reviewed change is:


--- a/content/browser/renderer_host/render_view_host_impl.cc
+++ b/content/browser/renderer_host/render_view_host_impl.cc
@@ -143,6 +143,7 @@ RenderViewHostImpl::RenderViewHostImpl(SiteInstance*
instance,
       run_modal_reply_msg_(NULL),
       is_waiting_for_beforeunload_ack_(false),
       is_waiting_for_unload_ack_(false),
+      has_timed_out_on_unload_(false),
       unload_ack_is_for_cross_site_transition_(false),
       are_javascript_messages_suppressed_(false),
       sudden_termination_allowed_(false),


--- a/content/browser/renderer_host/render_view_host_browsertest.cc
+++ b/content/browser/renderer_host/render_view_host_browsertest.cc
@@ -278,12 +278,11 @@ IN_PROC_BROWSER_TEST_F(RenderViewHostTest,
UnresponsiveCrossSiteNavigation) {
     ui_test_utils::NavigateToURL(browser(), new_process_url);
     process_hang_observer.Wait();
     process_exit_observer.Wait();
-  }
 
-  // This should fail, because the navigation to the new URL results 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));
+    // Since the process is killed during the navigation, we don't expect the
+    // renderer host to have a connection to it.
+    EXPECT_FALSE(rph->HasConnection());
+  }
 
   ui_test_utils::NavigateToURL(browser(), same_process_url);
   tab = browser()->GetWebContentsAt(0);

[Charlie Reis, 2012-04-12 23:06:04]

Still LGTM.

[Charlie Reis, 2012-04-12 23:09:29]

Just to be explicit for Scott and Avi, we'd like to re-land essentially the same
change after having it reverted for a test flake that is now fixed.

Scott, can you re-approve content/browser and content/test?

Avi, can you re-approve render_process_host.h (unchanged since last time)?

Thanks!

[sky, 2012-04-12 23:41:23]

LGTM

[nasko, 2012-04-13 16:15:54]

Avi, can you take a look at this today? There has been no change in the part you
have reviewed already, so it won't take more than a minute.
Thanks in advance,
Nasko

[Avi (use Gerrit), 2012-04-14 00:49:16]

SLGTM

[commit-bot: I haz the power, 2012-04-16 14:06:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/10065028/1

[commit-bot: I haz the power, 2012-04-16 15:58:29]

Change committed as 132407