Ensure that navigations are not ignored due to incomplete unload requests.

Also fix incorrect DCHECKs in decidePolicyForNavigation and UpdateTitle.

BUG=93427
TEST=See bug, comment 50.

Review URL: https://chromiumcodereview.appspot.com/9374003

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

8 files changed, 139 insertions, 39 deletions

diff --git a/content/browser/renderer_host/render_view_host.cc b/content/browser/renderer_host/render_view_host.cc
index 729fc68..24e09f5 100644
--- a/content/browser/renderer_host/render_view_host.cc
+++ b/content/browser/renderer_host/render_view_host.cc
@@ -241,13 +241,9 @@ void RenderViewHost::Navigate(const ViewMsg_Navigate_Params& params) {
     DCHECK(!suspended_nav_message_.get());
     suspended_nav_message_.reset(nav_message);
   } else {
-    // Unset this, otherwise if true and the hang monitor fires we'll
-    // incorrectly close the tab.
-    is_waiting_for_unload_ack_ = false;
-
-    // Unset this, in case we never finished committing a previous RVH swap.
-    // Otherwise we'll filter out the messages for this navigation.
-    is_swapped_out_ = false;
+    // Get back to a clean state, in case we start a new navigation without
+    // completing a RVH swap or unload handler.
+    SetSwappedOut(false);
 
     Send(nav_message);
   }
@@ -291,7 +287,8 @@ void RenderViewHost::SetNavigationsSuspended(bool suspend) {
     // There's a navigation message waiting to be sent.  Now that we're not
     // suspended anymore, resume navigation by sending it.  If we were swapped
     // out, we should also stop filtering out the IPC messages now.
-    is_swapped_out_ = false;
+    SetSwappedOut(false);
+
     Send(suspended_nav_message_.release());
   }
 }
@@ -375,7 +372,7 @@ void RenderViewHost::WasSwappedOut() {
   // for that message, and additional IPC messages may keep streaming in.
   // We filter them out, as long as that won't cause problems (e.g., we
   // still allow synchronous messages through).
-  is_swapped_out_ = true;
+  SetSwappedOut(true);
 
   // Inform the renderer that it can exit if no one else is using it.
   Send(new ViewMsg_WasSwappedOut(routing_id()));
@@ -1572,6 +1569,16 @@ void RenderViewHost::OnDomOperationResponse(
       content::Details<DomOperationNotificationDetails>(&details));
 }
 
+void RenderViewHost::SetSwappedOut(bool is_swapped_out) {
+  is_swapped_out_ = is_swapped_out;
+
+  // Whenever we change swap out state, we should not be waiting for
+  // beforeunload or unload acks.  We clear them here to be safe, since they
+  // can cause navigations to be ignored in OnMsgNavigate.
+  is_waiting_for_beforeunload_ack_ = false;
+  is_waiting_for_unload_ack_ = false;
+}
+
 void RenderViewHost::ClearPowerSaveBlockers() {
   STLDeleteValues(&power_save_blockers_);
 }
diff --git a/content/browser/renderer_host/render_view_host.h b/content/browser/renderer_host/render_view_host.h
index 9a17429..57e2c27 100644
--- a/content/browser/renderer_host/render_view_host.h
+++ b/content/browser/renderer_host/render_view_host.h
@@ -608,6 +608,10 @@ class CONTENT_EXPORT RenderViewHost : public RenderWidgetHost {
  private:
   friend class TestRenderViewHost;
 
+  // Sets whether this RenderViewHost is swapped out in favor of another,
+  // and clears any waiting state that is no longer relevant.
+  void SetSwappedOut(bool is_swapped_out);
+
   void ClearPowerSaveBlockers();
 
   // The SiteInstance associated with this RenderViewHost.  All pages drawn
diff --git a/content/browser/renderer_host/test_render_view_host.h b/content/browser/renderer_host/test_render_view_host.h
index 45c5285..c85fd03 100644
--- a/content/browser/renderer_host/test_render_view_host.h
+++ b/content/browser/renderer_host/test_render_view_host.h
@@ -223,6 +223,12 @@ class TestRenderViewHost : public RenderViewHost {
     return is_waiting_for_beforeunload_ack_;
   }
 
+  // Returns whether the RenderViewHost is currently waiting to hear the result
+  // of an unload handler from the renderer.
+  bool is_waiting_for_unload_ack() const {
+    return is_waiting_for_unload_ack_;
+  }
+
   // Sets whether the RenderViewHost is currently swapped out, and thus
   // filtering messages from the renderer.
   void set_is_swapped_out(bool is_swapped_out) {
diff --git a/content/browser/tab_contents/render_view_host_manager_unittest.cc b/content/browser/tab_contents/render_view_host_manager_unittest.cc
index 6c1f99a..68dc444 100644
--- a/content/browser/tab_contents/render_view_host_manager_unittest.cc
+++ b/content/browser/tab_contents/render_view_host_manager_unittest.cc
@@ -737,3 +737,42 @@ TEST_F(RenderViewHostManagerTest, PageDoesBackAndReload) {
   ASSERT_TRUE(entry != NULL);
   EXPECT_EQ(kUrl2, entry->GetURL());
 }
+
+// Ensure that we can go back and forward even if a SwapOut ACK isn't received.
+// See http://crbug.com/93427.
+TEST_F(RenderViewHostManagerTest, NavigateAfterMissingSwapOutACK) {
+  const GURL kUrl1("http://www.google.com/");
+  const GURL kUrl2("http://www.chromium.org/");
+
+  // Navigate to two pages.
+  contents()->NavigateAndCommit(kUrl1);
+  TestRenderViewHost* rvh1 = rvh();
+  contents()->NavigateAndCommit(kUrl2);
+  TestRenderViewHost* rvh2 = rvh();
+
+  // Now go back, but suppose the SwapOut_ACK isn't received.  This shouldn't
+  // happen, but we have seen it when going back quickly across many entries
+  // (http://crbug.com/93427).
+  contents()->GetController().GoBack();
+  EXPECT_TRUE(rvh2->is_waiting_for_beforeunload_ack());
+  contents()->ProceedWithCrossSiteNavigation();
+  EXPECT_FALSE(rvh2->is_waiting_for_beforeunload_ack());
+  rvh2->SwapOut(1, 1);
+  EXPECT_TRUE(rvh2->is_waiting_for_unload_ack());
+
+  // The back navigation commits.  We should proactively clear the
+  // is_waiting_for_unload_ack state to be safe.
+  const NavigationEntry* entry1 = contents()->GetController().GetPendingEntry();
+  rvh1->SendNavigate(entry1->GetPageID(), entry1->GetURL());
+  EXPECT_TRUE(rvh2->is_swapped_out());
+  EXPECT_FALSE(rvh2->is_waiting_for_unload_ack());
+
+  // We should be able to navigate forward.
+  contents()->GetController().GoForward();
+  contents()->ProceedWithCrossSiteNavigation();
+  const NavigationEntry* entry2 = contents()->GetController().GetPendingEntry();
+  rvh2->SendNavigate(entry2->GetPageID(), entry2->GetURL());
+  EXPECT_EQ(rvh2, rvh());
+  EXPECT_FALSE(rvh2->is_swapped_out());
+  EXPECT_TRUE(rvh1->is_swapped_out());
+}
diff --git a/content/browser/tab_contents/tab_contents.cc b/content/browser/tab_contents/tab_contents.cc
index f70a699..e1cd678 100644
--- a/content/browser/tab_contents/tab_contents.cc
+++ b/content/browser/tab_contents/tab_contents.cc
@@ -1747,7 +1747,7 @@ bool TabContents::UpdateTitleForEntry(NavigationEntryImpl* entry,
 
   // If a page is created via window.open and never navigated,
   // there will be no navigation entry. In this situation,
-  // |page_title_when_no_navigaiton_entry_| will be used for page title.
+  // |page_title_when_no_navigation_entry_| will be used for page title.
   if (entry) {
     if (final_title == entry->GetTitle())
       return false;  // Nothing changed, don't bother.
@@ -2003,10 +2003,16 @@ void TabContents::UpdateTitle(RenderViewHost* rvh,
   // getting useful data.
   SetNotWaitingForResponse();
 
-  DCHECK(rvh == GetRenderViewHost());
+  // Try to find the navigation entry, which might not be the current one.
+  // For example, it might be from a pending RVH for the pending entry.
   NavigationEntryImpl* entry = controller_.GetEntryWithPageID(
       rvh->site_instance(), page_id);
 
+  // We can handle title updates when we don't have an entry in
+  // UpdateTitleForEntry, but only if the update is from the current RVH.
+  if (!entry && rvh != GetRenderViewHost())
+    return;
+
   // TODO(evan): make use of title_direction.
   // http://code.google.com/p/chromium/issues/detail?id=27094
   if (!UpdateTitleForEntry(entry, title))
diff --git a/content/renderer/render_view_browsertest.cc b/content/renderer/render_view_browsertest.cc
index 1e9be79..746f1f1 100644
--- a/content/renderer/render_view_browsertest.cc
+++ b/content/renderer/render_view_browsertest.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -57,6 +57,38 @@ TEST_F(RenderViewImplTest, OnNavStateChanged) {
       ViewHostMsg_UpdateState::ID));
 }
 
+// Ensure the RenderViewImpl sends an ACK to a SwapOut request, even if it is
+// already swapped out.  http://crbug.com/93427.
+TEST_F(RenderViewImplTest, SendSwapOutACK) {
+  // Respond to a swap out request.
+  ViewMsg_SwapOut_Params params;
+  params.closing_process_id = 10;
+  params.closing_route_id = 11;
+  params.new_render_process_host_id = 12;
+  params.new_request_id = 13;
+  view()->OnSwapOut(params);
+
+  // Check for a valid OnSwapOutACK with echoed params.
+  const IPC::Message* msg = render_thread_->sink().GetUniqueMessageMatching(
+      ViewHostMsg_SwapOut_ACK::ID);
+  ASSERT_TRUE(msg);
+  ViewHostMsg_SwapOut_ACK::Param reply_params;
+  ViewHostMsg_SwapOut_ACK::Read(msg, &reply_params);
+  EXPECT_EQ(params.closing_process_id, reply_params.a.closing_process_id);
+  EXPECT_EQ(params.closing_route_id, reply_params.a.closing_route_id);
+  EXPECT_EQ(params.new_render_process_host_id,
+            reply_params.a.new_render_process_host_id);
+  EXPECT_EQ(params.new_request_id, reply_params.a.new_request_id);
+
+  // It is possible to get another swap out request.  Ensure that we send
+  // an ACK, even if we don't have to do anything else.
+  render_thread_->sink().ClearMessages();
+  view()->OnSwapOut(params);
+  const IPC::Message* msg2 = render_thread_->sink().GetUniqueMessageMatching(
+      ViewHostMsg_SwapOut_ACK::ID);
+  ASSERT_TRUE(msg2);
+}
+
 // Test that we get the correct UpdateState message when we go back twice
 // quickly without committing.  Regression test for http://crbug.com/58082.
 TEST_F(RenderViewImplTest, LastCommittedUpdateState) {
diff --git a/content/renderer/render_view_impl.cc b/content/renderer/render_view_impl.cc
index 639ff84..8240f49 100644
--- a/content/renderer/render_view_impl.cc
+++ b/content/renderer/render_view_impl.cc
@@ -2186,7 +2186,12 @@ WebNavigationPolicy RenderViewImpl::decidePolicyForNavigation(
     const WebNode&, WebNavigationPolicy default_policy, bool is_redirect) {
   // TODO(creis): Remove this when we fix OnSwapOut to not need a navigation.
   if (is_swapped_out_) {
-    DCHECK(request.url() == GURL("about:swappedout"));
+    // It is possible for in-progress navigations to arrive here just after we
+    // are swapped out, including iframes.  We should cancel them.
+    if (request.url() != GURL("about:swappedout"))
+      return WebKit::WebNavigationPolicyIgnore;
+
+    // Allow about:swappedout to complete.
     return default_policy;
   }
 
@@ -4228,31 +4233,31 @@ void RenderViewImpl::OnShouldClose() {
 }
 
 void RenderViewImpl::OnSwapOut(const ViewMsg_SwapOut_Params& params) {
-  if (is_swapped_out_)
-    return;
-
-  // Swap this RenderView out so the tab can navigate to a page rendered by a
-  // different process.  This involves running the unload handler and clearing
-  // the page.  Once WasSwappedOut is called, we also allow this process to exit
-  // if there are no other active RenderViews in it.
-
-  // Send an UpdateState message before we get swapped out.
-  SyncNavigationState();
-
-  // Synchronously run the unload handler before sending the ACK.
-  webview()->dispatchUnloadEvent();
-
-  // Swap out and stop sending any IPC messages that are not ACKs.
-  SetSwappedOut(true);
-
-  // Replace the page with a blank dummy URL.  The unload handler will not be
-  // run a second time, thanks to a check in FrameLoader::stopLoading.
-  // TODO(creis): Need to add a better way to do this that avoids running the
-  // beforeunload handler.  For now, we just run it a second time silently.
-  webview()->mainFrame()->loadHTMLString(std::string(),
-                                         GURL("about:swappedout"),
-                                         GURL("about:swappedout"),
-                                         false);
+  // Only run unload if we're not swapped out yet, but send the ack either way.
+  if (!is_swapped_out_) {
+    // Swap this RenderView out so the tab can navigate to a page rendered by a
+    // different process.  This involves running the unload handler and clearing
+    // the page.  Once WasSwappedOut is called, we also allow this process to
+    // exit if there are no other active RenderViews in it.
+
+    // Send an UpdateState message before we get swapped out.
+    SyncNavigationState();
+
+    // Synchronously run the unload handler before sending the ACK.
+    webview()->dispatchUnloadEvent();
+
+    // Swap out and stop sending any IPC messages that are not ACKs.
+    SetSwappedOut(true);
+
+    // Replace the page with a blank dummy URL.  The unload handler will not be
+    // run a second time, thanks to a check in FrameLoader::stopLoading.
+    // TODO(creis): Need to add a better way to do this that avoids running the
+    // beforeunload handler.  For now, we just run it a second time silently.
+    webview()->mainFrame()->loadHTMLString(std::string(),
+                                          GURL("about:swappedout"),
+                                          GURL("about:swappedout"),
+                                          false);
+  }
 
   // Just echo back the params in the ACK.
   Send(new ViewHostMsg_SwapOut_ACK(routing_id_, params));
diff --git a/content/renderer/render_view_impl.h b/content/renderer/render_view_impl.h
index 576f148..f0089b1 100644
--- a/content/renderer/render_view_impl.h
+++ b/content/renderer/render_view_impl.h
@@ -689,6 +689,7 @@ class RenderViewImpl : public RenderWidget,
   FRIEND_TEST_ALL_PREFIXES(RenderViewImplTest, OnNavStateChanged);
   FRIEND_TEST_ALL_PREFIXES(RenderViewImplTest, OnSetTextDirection);
   FRIEND_TEST_ALL_PREFIXES(RenderViewImplTest, OnUpdateWebPreferences);
+  FRIEND_TEST_ALL_PREFIXES(RenderViewImplTest, SendSwapOutACK);
   FRIEND_TEST_ALL_PREFIXES(RenderViewImplTest, StaleNavigationsIgnored);
   FRIEND_TEST_ALL_PREFIXES(RenderViewImplTest, UpdateTargetURLWithInvalidURL);
 #if defined(OS_MACOSX)
@@ -867,7 +868,7 @@ class RenderViewImpl : public RenderWidget,
   void OnShouldClose();
   void OnStop();
   void OnStopFinding(content::StopFindAction action);
-  void OnSwapOut(const ViewMsg_SwapOut_Params& params);
+  CONTENT_EXPORT void OnSwapOut(const ViewMsg_SwapOut_Params& params);
   void OnThemeChanged();
   void OnUndo();
   void OnUpdateTargetURLAck();