Prevents an old RenderViewHost from preempting a cross-site navigation once the unload request has been made.

This fixes a bug where competing navigations could either cause the tab to close unexpectedly or all future cross-site navigations (and possibly tab close attempts) to fail.

BUG=23942
BUG=26839
TEST=TabContentsTest.CrossSiteCantPreemptAfterUnload

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

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

5 files changed, 190 insertions, 17 deletions

diff --git a/chrome/browser/renderer_host/render_view_host.cc b/chrome/browser/renderer_host/render_view_host.cc
index 0f61b0f..368a9ad 100644
--- a/chrome/browser/renderer_host/render_view_host.cc
+++ b/chrome/browser/renderer_host/render_view_host.cc
@@ -114,6 +114,7 @@ RenderViewHost::RenderViewHost(SiteInstance* instance,
       navigations_suspended_(false),
       suspended_nav_message_(NULL),
       run_modal_reply_msg_(NULL),
+      is_waiting_for_beforeunload_ack_(false),
       is_waiting_for_unload_ack_(false),
       unload_ack_is_for_cross_site_transition_(false),
       are_javascript_messages_suppressed_(false),
@@ -302,7 +303,7 @@ void RenderViewHost::FirePageBeforeUnload(bool for_cross_site_transition) {
   if (!IsRenderViewLive()) {
     // This RenderViewHost doesn't have a live renderer, so just skip running
     // the onbeforeunload handler.
-    is_waiting_for_unload_ack_ = true;  // Prevent check in OnMsgShouldCloseACK.
+    is_waiting_for_beforeunload_ack_ = true;  // Checked by OnMsgShouldCloseACK.
     unload_ack_is_for_cross_site_transition_ = for_cross_site_transition;
     OnMsgShouldCloseACK(true);
     return;
@@ -311,7 +312,7 @@ void RenderViewHost::FirePageBeforeUnload(bool for_cross_site_transition) {
   // 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 then the browser close
   // button), and we only send the message once.
-  if (is_waiting_for_unload_ack_) {
+  if (is_waiting_for_beforeunload_ack_) {
     // Some of our close messages could be for the tab, others for cross-site
     // transitions. We always want to think it's for closing the tab if any
     // of the messages were, since otherwise it might be impossible to close
@@ -323,7 +324,7 @@ void RenderViewHost::FirePageBeforeUnload(bool for_cross_site_transition) {
   } else {
     // Start the hang monitor in case the renderer hangs in the beforeunload
     // handler.
-    is_waiting_for_unload_ack_ = true;
+    is_waiting_for_beforeunload_ack_ = true;
     unload_ack_is_for_cross_site_transition_ = for_cross_site_transition;
     StartHangMonitorTimeout(TimeDelta::FromMilliseconds(kUnloadTimeoutMS));
     Send(new ViewMsg_ShouldClose(routing_id()));
@@ -333,8 +334,10 @@ void RenderViewHost::FirePageBeforeUnload(bool for_cross_site_transition) {
 void RenderViewHost::ClosePage(bool for_cross_site_transition,
                                int new_render_process_host_id,
                                int new_request_id) {
-  // Start the hang monitor in case the renderer hangs in the unload handler.
+  // In most cases, this will not be set to false afterward.  Either the tab
+  // will be closed, or a pending RenderViewHost will replace this one.
   is_waiting_for_unload_ack_ = true;
+  // Start the hang monitor in case the renderer hangs in the unload handler.
   StartHangMonitorTimeout(TimeDelta::FromMilliseconds(kUnloadTimeoutMS));
 
   ViewMsg_ClosePage_Params params;
@@ -355,6 +358,7 @@ void RenderViewHost::ClosePage(bool for_cross_site_transition,
 
 void RenderViewHost::ClosePageIgnoringUnloadEvents() {
   StopHangMonitorTimeout();
+  is_waiting_for_beforeunload_ack_ = false;
   is_waiting_for_unload_ack_ = false;
 
   sudden_termination_allowed_ = true;
@@ -575,9 +579,11 @@ void RenderViewHost::JavaScriptMessageBoxClosed(IPC::Message* reply_msg,
                                                 bool success,
                                                 const std::wstring& prompt) {
   process()->set_ignore_input_events(false);
-  if (is_waiting_for_unload_ack_) {
+  bool is_waiting =
+      is_waiting_for_beforeunload_ack_ || is_waiting_for_unload_ack_;
+  if (is_waiting) {
     if (are_javascript_messages_suppressed_) {
-      delegate_->RendererUnresponsive(this, is_waiting_for_unload_ack_);
+      delegate_->RendererUnresponsive(this, is_waiting);
       return;
     }
 
@@ -591,7 +597,7 @@ void RenderViewHost::JavaScriptMessageBoxClosed(IPC::Message* reply_msg,
 
 void RenderViewHost::ModalHTMLDialogClosed(IPC::Message* reply_msg,
                                            const std::string& json_retval) {
-  if (is_waiting_for_unload_ack_)
+  if (is_waiting_for_beforeunload_ack_ || is_waiting_for_unload_ack_)
     StartHangMonitorTimeout(TimeDelta::FromMilliseconds(kUnloadTimeoutMS));
 
   ViewHostMsg_ShowModalHTMLDialog::WriteReplyParams(reply_msg, json_retval);
@@ -924,6 +930,33 @@ void RenderViewHost::OnMsgRenderViewGone() {
 // get a new page_id because we need to create a new navigation entry for that
 // action.
 void RenderViewHost::OnMsgNavigate(const IPC::Message& msg) {
+  // If we're waiting for a beforeunload ack from this renderer and we receive a
+  // Navigate message, then the renderer was navigating before it received the
+  // request.  If it is during a cross-site navigation, then we should forget
+  // about the beforeunload, because the navigation will now be canceled.  (If
+  // it is instead during an attempt to close the page, we should be sure to
+  // keep waiting for the ack, which the new page will send.)
+  //
+  // If we did not clear this state, an unresponsiveness timer might think we
+  // are waiting for an ack but are not in a cross-site navigation, and would
+  // close the tab.  TODO(creis): That timer code should be refactored to only
+  // close the tab if we explicitly know the user tried to close the tab, and
+  // not just check for the absence of a cross-site navigation.  Once that's
+  // fixed, this check can go away.
+  if (is_waiting_for_beforeunload_ack_ &&
+      unload_ack_is_for_cross_site_transition_) {
+    is_waiting_for_beforeunload_ack_ = false;
+    StopHangMonitorTimeout();
+  }
+
+  // If we're waiting for an unload ack from this renderer and we receive a
+  // Navigate message, then the renderer was navigating before it received the
+  // unload request.  It will either respond to the unload request soon or our
+  // timer will expire.  Either way, we should ignore this message, because we
+  // have already committed to closing this renderer.
+  if (is_waiting_for_unload_ack_)
+    return;
+
   // Read the parameters out of the IPC message directly to avoid making another
   // copy when we filter the URLs.
   void* iter = NULL;
@@ -1539,8 +1572,13 @@ void RenderViewHost::OnReceivedSerializedHtmlData(const GURL& frame_url,
 
 void RenderViewHost::OnMsgShouldCloseACK(bool proceed) {
   StopHangMonitorTimeout();
-  DCHECK(is_waiting_for_unload_ack_);
-  is_waiting_for_unload_ack_ = false;
+  // If this renderer navigated while the beforeunload request was in flight, we
+  // may have cleared this state in OnMsgNavigate, in which case we can ignore
+  // this message.
+  if (!is_waiting_for_beforeunload_ack_)
+    return;
+
+  is_waiting_for_beforeunload_ack_ = false;
 
   RenderViewHostDelegate::RendererManagement* management_delegate =
       delegate_->GetRendererManagementDelegate();
@@ -1585,7 +1623,8 @@ void RenderViewHost::WindowMoveOrResizeStarted() {
 }
 
 void RenderViewHost::NotifyRendererUnresponsive() {
-  delegate_->RendererUnresponsive(this, is_waiting_for_unload_ack_);
+  delegate_->RendererUnresponsive(
+      this, is_waiting_for_beforeunload_ack_ || is_waiting_for_unload_ack_);
 }
 
 void RenderViewHost::NotifyRendererResponsive() {
diff --git a/chrome/browser/renderer_host/render_view_host.h b/chrome/browser/renderer_host/render_view_host.h
index 2f52c05..9ac9191 100644
--- a/chrome/browser/renderer_host/render_view_host.h
+++ b/chrome/browser/renderer_host/render_view_host.h
@@ -638,15 +638,21 @@ class RenderViewHost : public RenderWidgetHost,
   // must return to the renderer to unblock it.
   IPC::Message* run_modal_reply_msg_;
 
-  // Set to true when there is a pending ViewMsg_ShouldClose message pending.
-  // This ensures we don't spam the renderer many times to close. When true,
-  // the value of unload_ack_is_for_cross_site_transition_ indicates which type
-  // of unload this is for.
+  // Set to true when there is a pending ViewMsg_ShouldClose message.  This
+  // ensures we don't spam the renderer with multiple beforeunload requests.
+  // When either this value or is_waiting_for_unload_ack_ is true, the value of
+  // unload_ack_is_for_cross_site_transition_ indicates whether this is for a
+  // cross-site transition or a tab close attempt.
+  bool is_waiting_for_beforeunload_ack_;
+
+  // Set to true when there is a pending ViewMsg_Close message.  Also see
+  // is_waiting_for_beforeunload_ack_, unload_ack_is_for_cross_site_transition_.
   bool is_waiting_for_unload_ack_;
 
-  // Valid only when is_waiting_for_unload_ack_ is true, this tells us if the
-  // unload request is for closing the entire tab ( = false), or only this
-  // RenderViewHost in the case of a cross-site transition ( = true).
+  // Valid only when is_waiting_for_beforeunload_ack_ or
+  // is_waiting_for_unload_ack_ is true.  This tells us if the unload request
+  // is for closing the entire tab ( = false), or only this RenderViewHost in
+  // the case of a cross-site transition ( = true).
   bool unload_ack_is_for_cross_site_transition_;
 
   bool are_javascript_messages_suppressed_;
diff --git a/chrome/browser/tab_contents/render_view_host_manager.cc b/chrome/browser/tab_contents/render_view_host_manager.cc
index 52b0813..8622f39 100644
--- a/chrome/browser/tab_contents/render_view_host_manager.cc
+++ b/chrome/browser/tab_contents/render_view_host_manager.cc
@@ -208,6 +208,10 @@ void RenderViewHostManager::RendererAbortedProvisionalLoad(
 void RenderViewHostManager::ShouldClosePage(bool for_cross_site_transition,
                                             bool proceed) {
   if (for_cross_site_transition) {
+    // Ignore if we're not in a cross-site navigation.
+    if (!cross_navigation_pending_)
+      return;
+
     if (proceed) {
       // Ok to unload the current page, so proceed with the cross-site
       // navigation.  Note that if navigations are not currently suspended, it
diff --git a/chrome/browser/tab_contents/tab_contents.h b/chrome/browser/tab_contents/tab_contents.h
index 79bfab8..ad5de2f 100644
--- a/chrome/browser/tab_contents/tab_contents.h
+++ b/chrome/browser/tab_contents/tab_contents.h
@@ -629,6 +629,7 @@ class TabContents : public PageNavigator,
   FRIEND_TEST(BlockedPopupContainerTest, TestReposition);
   FRIEND_TEST(TabContentsTest, NoJSMessageOnInterstitials);
   FRIEND_TEST(TabContentsTest, UpdateTitle);
+  FRIEND_TEST(TabContentsTest, CrossSiteCantPreemptAfterUnload);
 
   // Temporary until the view/contents separation is complete.
   friend class TabContentsView;
diff --git a/chrome/browser/tab_contents/web_contents_unittest.cc b/chrome/browser/tab_contents/web_contents_unittest.cc
index 0021691..9b59406 100644
--- a/chrome/browser/tab_contents/web_contents_unittest.cc
+++ b/chrome/browser/tab_contents/web_contents_unittest.cc
@@ -519,6 +519,129 @@ TEST_F(TabContentsTest, CrossSiteUnloadHandlers) {
   EXPECT_TRUE(contents()->pending_rvh() == NULL);
 }
 
+// Test that during a slow cross-site navigation, the original renderer can
+// navigate to a different URL and have it displayed, canceling the slow
+// navigation.
+TEST_F(TabContentsTest, CrossSiteNavigationPreempted) {
+  contents()->transition_cross_site = true;
+  TestRenderViewHost* orig_rvh = rvh();
+  SiteInstance* instance1 = contents()->GetSiteInstance();
+
+  // Navigate to URL.  First URL should use first RenderViewHost.
+  const GURL url("http://www.google.com");
+  controller().LoadURL(url, GURL(), PageTransition::TYPED);
+  ViewHostMsg_FrameNavigate_Params params1;
+  InitNavigateParams(&params1, 1, url);
+  contents()->TestDidNavigate(orig_rvh, params1);
+  EXPECT_FALSE(contents()->cross_navigation_pending());
+  EXPECT_EQ(orig_rvh, contents()->render_view_host());
+
+  // Navigate to new site, simulating an onbeforeunload approval.
+  const GURL url2("http://www.yahoo.com");
+  controller().LoadURL(url2, GURL(), PageTransition::TYPED);
+  orig_rvh->TestOnMessageReceived(ViewHostMsg_ShouldClose_ACK(0, true));
+  EXPECT_TRUE(contents()->cross_navigation_pending());
+
+  // Suppose the original renderer navigates before the new one is ready.
+  orig_rvh->SendNavigate(2, GURL("http://www.google.com/foo"));
+
+  // Verify that the pending navigation is cancelled.
+  SiteInstance* instance2 = contents()->GetSiteInstance();
+  EXPECT_FALSE(contents()->cross_navigation_pending());
+  EXPECT_EQ(orig_rvh, rvh());
+  EXPECT_EQ(instance1, instance2);
+  EXPECT_TRUE(contents()->pending_rvh() == NULL);
+}
+
+// Test that the original renderer can preempt a cross-site navigation while the
+// beforeunload request is in flight.
+TEST_F(TabContentsTest, CrossSitePreemptDuringBeforeUnload) {
+  contents()->transition_cross_site = true;
+  TestRenderViewHost* orig_rvh = rvh();
+  SiteInstance* instance1 = contents()->GetSiteInstance();
+
+  // Navigate to URL.  First URL should use first RenderViewHost.
+  const GURL url("http://www.google.com");
+  controller().LoadURL(url, GURL(), PageTransition::TYPED);
+  ViewHostMsg_FrameNavigate_Params params1;
+  InitNavigateParams(&params1, 1, url);
+  contents()->TestDidNavigate(orig_rvh, params1);
+  EXPECT_FALSE(contents()->cross_navigation_pending());
+  EXPECT_EQ(orig_rvh, contents()->render_view_host());
+
+  // Navigate to new site, with the befureunload request in flight.
+  const GURL url2("http://www.yahoo.com");
+  controller().LoadURL(url2, GURL(), PageTransition::TYPED);
+
+  // Suppose the original renderer navigates now, while the beforeunload request
+  // is in flight.  We must cancel the pending navigation and show this new
+  // page, because the beforeunload handler might return false.
+  orig_rvh->SendNavigate(2, GURL("http://www.google.com/foo"));
+
+  // Verify that the pending navigation is cancelled.
+  SiteInstance* instance2 = contents()->GetSiteInstance();
+  EXPECT_FALSE(contents()->cross_navigation_pending());
+  EXPECT_EQ(orig_rvh, rvh());
+  EXPECT_EQ(instance1, instance2);
+  EXPECT_TRUE(contents()->pending_rvh() == NULL);
+
+  // Make sure the beforeunload ack doesn't cause problems if it arrives here.
+  orig_rvh->TestOnMessageReceived(ViewHostMsg_ShouldClose_ACK(0, true));
+}
+
+// Test that the original renderer cannot preempt a cross-site navigation once
+// the unload request has been made.  At this point, the cross-site navigation
+// is almost ready to be displayed, and the original renderer is only given a
+// short chance to run an unload handler.  Prevents regression of bug 23942.
+TEST_F(TabContentsTest, CrossSiteCantPreemptAfterUnload) {
+  contents()->transition_cross_site = true;
+  TestRenderViewHost* orig_rvh = rvh();
+  SiteInstance* instance1 = contents()->GetSiteInstance();
+
+  // Navigate to URL.  First URL should use first RenderViewHost.
+  const GURL url("http://www.google.com");
+  controller().LoadURL(url, GURL(), PageTransition::TYPED);
+  ViewHostMsg_FrameNavigate_Params params1;
+  InitNavigateParams(&params1, 1, url);
+  contents()->TestDidNavigate(orig_rvh, params1);
+  EXPECT_FALSE(contents()->cross_navigation_pending());
+  EXPECT_EQ(orig_rvh, contents()->render_view_host());
+
+  // Navigate to new site, simulating an onbeforeunload approval.
+  const GURL url2("http://www.yahoo.com");
+  controller().LoadURL(url2, GURL(), PageTransition::TYPED);
+  orig_rvh->TestOnMessageReceived(ViewHostMsg_ShouldClose_ACK(0, true));
+  EXPECT_TRUE(contents()->cross_navigation_pending());
+  TestRenderViewHost* pending_rvh = static_cast<TestRenderViewHost*>(
+      contents()->pending_rvh());
+
+  // Simulate the pending renderer's response, which leads to an unload request
+  // being sent to orig_rvh.
+  contents()->OnCrossSiteResponse(0, 0);
+
+  // Suppose the original renderer navigates now, while the unload request is in
+  // flight.  We should ignore it, wait for the unload ack, and let the pending
+  // request continue.  Otherwise, the tab may close spontaneously or stop
+  // responding to navigation requests.  (See bug 23942.)
+  ViewHostMsg_FrameNavigate_Params params1a;
+  InitNavigateParams(&params1a, 2, GURL("http://www.google.com/foo"));
+  orig_rvh->SendNavigate(2, GURL("http://www.google.com/foo"));
+
+  // Verify that the pending navigation is still in progress.
+  EXPECT_TRUE(contents()->cross_navigation_pending());
+  EXPECT_TRUE(contents()->pending_rvh() != NULL);
+
+  // DidNavigate from the pending page should commit it.
+  ViewHostMsg_FrameNavigate_Params params2;
+  InitNavigateParams(&params2, 1, url2);
+  contents()->TestDidNavigate(pending_rvh, params2);
+  SiteInstance* instance2 = contents()->GetSiteInstance();
+  EXPECT_FALSE(contents()->cross_navigation_pending());
+  EXPECT_EQ(pending_rvh, rvh());
+  EXPECT_NE(instance1, instance2);
+  EXPECT_TRUE(contents()->pending_rvh() == NULL);
+}
+
 // Test that NavigationEntries have the correct content state after going
 // forward and back.  Prevents regression for bug 1116137.
 TEST_F(TabContentsTest, NavigationEntryContentState) {