Fix unresponsive tab closure when a navigation is in progress.

TBR=nasko@chromium.org
BUG=420557
TEST=Close a tab with an infinite unload event, then navigate right away.

Review URL: https://codereview.chromium.org/633323003

Cr-Commit-Position: refs/heads/master@{#298987}

content/browser/frame_host/render_frame_host_manager.cc

@@ -239,7 +239,13 @@ void RenderFrameHostManager::SetIsLoading(bool is_loading) {
 }
 
 bool RenderFrameHostManager::ShouldCloseTabOnUnresponsiveRenderer() {
-  if (!cross_navigation_pending_)
+  // If we're waiting for a close ACK, then the tab should close whether there's
+  // a navigation in progress or not.  Unfortunately, we also need to check for
+  // cases that we arrive here with no navigation in progress, since there are
+  // some tab closure paths that don't set is_waiting_for_close_ack to true.
+  // TODO(creis): Clean this up in http://crbug.com/418266.
+  if (!cross_navigation_pending_ ||
+      render_frame_host_->render_view_host()->is_waiting_for_close_ack())
     return true;
 
   // We should always have a pending RFH when there's a cross-process navigation

content/browser/frame_host/render_frame_host_manager_unittest.cc

@@ -101,6 +101,23 @@ class BeforeUnloadFiredWebContentsDelegate : public WebContentsDelegate {
   DISALLOW_COPY_AND_ASSIGN(BeforeUnloadFiredWebContentsDelegate);
 };
 
+class CloseWebContentsDelegate : public WebContentsDelegate {
+ public:
+  CloseWebContentsDelegate() : close_called_(false) {}
+  virtual ~CloseWebContentsDelegate() {}
+
+  virtual void CloseContents(WebContents* web_contents) override {
+    close_called_ = true;
+  }
+
+  bool is_closed() { return close_called_; }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(CloseWebContentsDelegate);
+
+  bool close_called_;
+};
+
 // This observer keeps track of the last deleted RenderViewHost to avoid
 // accessing it and causing use-after-free condition.
 class RenderViewHostDeletedObserver : public WebContentsObserver {
@@ -1515,6 +1532,31 @@ TEST_F(RenderFrameHostManagerTest, NavigateWithEarlyClose) {
   EXPECT_EQ(host, manager->current_frame_host());
 }
 
+TEST_F(RenderFrameHostManagerTest, CloseWithPendingWhileUnresponsive) {
+  const GURL kUrl1("http://www.google.com/");
+  const GURL kUrl2("http://www.chromium.org/");
+
+  CloseWebContentsDelegate close_delegate;
+  contents()->SetDelegate(&close_delegate);
+
+  // Navigate to the first page.
+  contents()->NavigateAndCommit(kUrl1);
+  TestRenderFrameHost* rfh1 = contents()->GetMainFrame();
+
+  // Start to close the tab, but assume it's unresponsive.
+  rfh1->render_view_host()->ClosePage();
+  EXPECT_TRUE(rfh1->render_view_host()->is_waiting_for_close_ack());
+
+  // Start a navigation to a new site.
+  controller().LoadURL(
+      kUrl2, Referrer(), ui::PAGE_TRANSITION_LINK, std::string());
+  EXPECT_TRUE(contents()->cross_navigation_pending());
+
+  // Simulate the unresponsiveness timer.  The tab should close.
+  contents()->RendererUnresponsive(rfh1->render_view_host());
+  EXPECT_TRUE(close_delegate.is_closed());
+}
+
 // Tests that the RenderFrameHost is properly deleted when the SwapOutACK is
 // received.  (SwapOut and the corresponding ACK always occur after commit.)
 // Also tests that an early SwapOutACK is properly ignored.

content/browser/renderer_host/render_view_host_impl.h

@@ -221,6 +221,9 @@ class CONTENT_EXPORT RenderViewHostImpl
     is_swapped_out_ = is_swapped_out;
   }
 
+  // TODO(creis): Remove as part of http://crbug.com/418265.
+  bool is_waiting_for_close_ack() const { return is_waiting_for_close_ack_; }
+
   // Tells the renderer that this RenderView will soon be swapped out, and thus
   // not to create any new modal dialogs until it happens.  This must be done
   // separately so that the PageGroupLoadDeferrers of any current dialogs are no