Re-landing r21673 without re-enabling the BrowserTest, which apparently is

still failing.

Make downloads not prevent tabs from closing. If a download creates a
cross-site transition (for example, if you click a link in Gmail that results
in a download in a new tab), that tab will be stuck and you can't close it or
the browser. This is the opposite problem with a similar cause as bug 16246. In
both cases we were using some secondary signal to tell us if we're closing for
a cross site transition or closing the tab, and that signal was wrong. In this
case, we were running the onunload handler, but because there was a pending
RenderViewHost, the RenderManager would think that the close was a cross-site
one, and not forward the close message to actually close the tab. This patch
adds a flag to the on unload handlers that indicates whether it's for a tab
closure or a cross-site transition, so we can do the right thing unambiguously
when the message returns. In this case I keep this information in the
RenderView in case we send multiple close requests, we'll close the tab if any
of them were for the entire tab, even if that particular one was dropped
because we don't want to have more than one in flight at once. BUG=17560
TEST=none.
Review URL: http://codereview.chromium.org/160122
Review URL: http://codereview.chromium.org/159426

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

8 files changed, 62 insertions, 38 deletions

diff --git a/chrome/browser/browser.cc b/chrome/browser/browser.cc
index c400d48..3f54397 100644
--- a/chrome/browser/browser.cc
+++ b/chrome/browser/browser.cc
@@ -1563,7 +1563,7 @@ bool Browser::RunUnloadListenerBeforeClosing(TabContents* contents) {
     // them. Once they have fired, we'll get a message back saying whether
     // to proceed closing the page or not, which sends us back to this method
     // with the HasUnloadListener bit cleared.
-    contents->render_view_host()->FirePageBeforeUnload();
+    contents->render_view_host()->FirePageBeforeUnload(false);
     return true;
   }
   return false;
@@ -2476,7 +2476,7 @@ void Browser::ProcessPendingTabs() {
     // Null check render_view_host here as this gets called on a PostTask and
     // the tab's render_view_host may have been nulled out.
     if (tab->render_view_host()) {
-      tab->render_view_host()->FirePageBeforeUnload();
+      tab->render_view_host()->FirePageBeforeUnload(false);
     } else {
       ClearUnloadState(tab);
     }
diff --git a/chrome/browser/renderer_host/render_view_host.cc b/chrome/browser/renderer_host/render_view_host.cc
index 070bd55..9a97eb0 100644
--- a/chrome/browser/renderer_host/render_view_host.cc
+++ b/chrome/browser/renderer_host/render_view_host.cc
@@ -108,6 +108,7 @@ RenderViewHost::RenderViewHost(SiteInstance* instance,
       suspended_nav_message_(NULL),
       run_modal_reply_msg_(NULL),
       is_waiting_for_unload_ack_(false),
+      unload_ack_is_for_cross_site_transition_(false),
       are_javascript_messages_suppressed_(false),
       sudden_termination_allowed_(false),
       in_inspect_element_mode_(false) {
@@ -300,21 +301,33 @@ void RenderViewHost::SetNavigationsSuspended(bool suspend) {
   }
 }
 
-void RenderViewHost::FirePageBeforeUnload() {
+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.
+    unload_ack_is_for_cross_site_transition_ = for_cross_site_transition;
     OnMsgShouldCloseACK(true);
     return;
   }
 
   // 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), so this test makes sure we only send the message once.
-  if (!is_waiting_for_unload_ack_) {
+  // button), and we only send the message once.
+  if (is_waiting_for_unload_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
+    // (if there was a cross-site "close" request pending when the user clicked
+    // the close button). We want to keep the "for cross site" flag only if
+    // both the old and the new ones are also for cross site.
+    unload_ack_is_for_cross_site_transition_ =
+        unload_ack_is_for_cross_site_transition_ && for_cross_site_transition;
+  } else {
     // Start the hang monitor in case the renderer hangs in the beforeunload
     // handler.
     is_waiting_for_unload_ack_ = true;
+    unload_ack_is_for_cross_site_transition_ = for_cross_site_transition;
     StartHangMonitorTimeout(TimeDelta::FromMilliseconds(kUnloadTimeoutMS));
     Send(new ViewMsg_ShouldClose(routing_id()));
   }
@@ -1439,8 +1452,10 @@ void RenderViewHost::OnMsgShouldCloseACK(bool proceed) {
 
   RenderViewHostDelegate::RendererManagement* management_delegate =
       delegate_->GetRendererManagementDelegate();
-  if (management_delegate)
-    management_delegate->ShouldClosePage(proceed);
+  if (management_delegate) {
+    management_delegate->ShouldClosePage(
+        unload_ack_is_for_cross_site_transition_, proceed);
+  }
 }
 
 void RenderViewHost::OnQueryFormFieldAutofill(const std::wstring& field_name,
diff --git a/chrome/browser/renderer_host/render_view_host.h b/chrome/browser/renderer_host/render_view_host.h
index 6d196f4e..d0836bc 100644
--- a/chrome/browser/renderer_host/render_view_host.h
+++ b/chrome/browser/renderer_host/render_view_host.h
@@ -151,7 +151,11 @@ class RenderViewHost : public RenderWidgetHost,
   // Causes the renderer to invoke the onbeforeunload event handler.  The
   // result will be returned via ViewMsg_ShouldClose. See also ClosePage which
   // will fire the PageUnload event.
-  void FirePageBeforeUnload();
+  //
+  // Set bool for_cross_site_transition when this close is just for the current
+  // RenderView in the case of a cross-site transition. False means we're
+  // closing the entire tab.
+  void FirePageBeforeUnload(bool for_cross_site_transition);
 
   // Causes the renderer to close the current page, including running its
   // onunload event handler.  A ClosePage_ACK message will be sent to the
@@ -619,8 +623,17 @@ 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.
   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).
+  bool unload_ack_is_for_cross_site_transition_;
+
   bool are_javascript_messages_suppressed_;
 
   // True if the render view can be shut down suddenly.
diff --git a/chrome/browser/renderer_host/render_view_host_delegate.h b/chrome/browser/renderer_host/render_view_host_delegate.h
index e33d64b..8ea6e16 100644
--- a/chrome/browser/renderer_host/render_view_host_delegate.h
+++ b/chrome/browser/renderer_host/render_view_host_delegate.h
@@ -145,9 +145,11 @@ class RenderViewHostDelegate {
    public:
     // Notification whether we should close the page, after an explicit call to
     // AttemptToClosePage.  This is called before a cross-site request or before
-    // a tab/window is closed, to allow the appropriate renderer to approve or
-    // deny the request.  |proceed| indicates whether the user chose to proceed.
-    virtual void ShouldClosePage(bool proceed) = 0;
+    // a tab/window is closed (as indicated by the first parameter) to allow the
+    // appropriate renderer to approve or deny the request.  |proceed| indicates
+    // whether the user chose to proceed.
+    virtual void ShouldClosePage(bool for_cross_site_transition,
+                                 bool proceed) = 0;
 
     // Called by ResourceDispatcherHost when a response for a pending cross-site
     // request is received.  The ResourceDispatcherHost will pause the response
diff --git a/chrome/browser/tab_contents/render_view_host_manager.cc b/chrome/browser/tab_contents/render_view_host_manager.cc
index 4146f862..959db7f 100644
--- a/chrome/browser/tab_contents/render_view_host_manager.cc
+++ b/chrome/browser/tab_contents/render_view_host_manager.cc
@@ -211,33 +211,32 @@ void RenderViewHostManager::OnJavaScriptMessageBoxWindowDestroyed() {
   render_view_host_->JavaScriptMessageBoxWindowDestroyed();
 }
 
-void RenderViewHostManager::ShouldClosePage(bool proceed) {
-  // Should only see this while we have a pending renderer.  Otherwise, we
-  // should ignore.
-  if (!pending_render_view_host_) {
+void RenderViewHostManager::ShouldClosePage(bool for_cross_site_transition,
+                                            bool proceed) {
+  if (for_cross_site_transition) {
+    if (proceed) {
+      // Ok to unload the current page, so proceed with the cross-site
+      // navigation.  Note that if navigations are not currently suspended, it
+      // might be because the renderer was deemed unresponsive and this call was
+      // already made by ShouldCloseTabOnUnresponsiveRenderer.  In that case, it
+      // is ok to do nothing here.
+      if (pending_render_view_host_->are_navigations_suspended())
+        pending_render_view_host_->SetNavigationsSuspended(false);
+    } else {
+      // Current page says to cancel.
+      CancelPending();
+      cross_navigation_pending_ = false;
+    }
+  } else {
+    // Non-cross site transition means closing the entire tab.
     bool proceed_to_fire_unload;
     delegate_->BeforeUnloadFiredFromRenderManager(proceed,
                                                   &proceed_to_fire_unload);
 
     if (proceed_to_fire_unload) {
       // This is not a cross-site navigation, the tab is being closed.
-      render_view_host_->ClosePage(true, -1, -1);
+      render_view_host_->ClosePage(false, -1, -1);
     }
-    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
-    // might be because the renderer was deemed unresponsive and this call was
-    // already made by ShouldCloseTabOnUnresponsiveRenderer.  In that case, it
-    // is ok to do nothing here.
-    if (pending_render_view_host_->are_navigations_suspended())
-      pending_render_view_host_->SetNavigationsSuspended(false);
-  } else {
-    // Current page says to cancel.
-    CancelPending();
-    cross_navigation_pending_ = false;
   }
 }
 
@@ -567,7 +566,7 @@ RenderViewHost* RenderViewHostManager::UpdateRendererStateForNavigate(
     // Tell the old render view to run its onbeforeunload handler, since it
     // doesn't otherwise know that the cross-site request is happening.  This
     // will trigger a call to ShouldClosePage with the reply.
-    render_view_host_->FirePageBeforeUnload();
+    render_view_host_->FirePageBeforeUnload(true);
 
     return pending_render_view_host_;
   } else {
diff --git a/chrome/browser/tab_contents/render_view_host_manager.h b/chrome/browser/tab_contents/render_view_host_manager.h
index 0b2460b..8c8dde2 100644
--- a/chrome/browser/tab_contents/render_view_host_manager.h
+++ b/chrome/browser/tab_contents/render_view_host_manager.h
@@ -160,7 +160,7 @@ class RenderViewHostManager
   }
 
   // RenderViewHostDelegate::RendererManagement implementation.
-  virtual void ShouldClosePage(bool proceed);
+  virtual void ShouldClosePage(bool for_cross_site_transition, bool proceed);
   virtual void OnCrossSiteResponse(int new_render_process_host_id,
                                    int new_request_id);
   virtual void OnCrossSiteNavigationCanceled();
diff --git a/chrome/browser/tab_contents/tab_contents.cc b/chrome/browser/tab_contents/tab_contents.cc
index 9b5098f..4546161 100644
--- a/chrome/browser/tab_contents/tab_contents.cc
+++ b/chrome/browser/tab_contents/tab_contents.cc
@@ -2204,10 +2204,6 @@ void TabContents::OnJSOutOfMemory() {
       this, l10n_util::GetString(IDS_JS_OUT_OF_MEMORY_PROMPT), NULL));
 }
 
-void TabContents::ShouldClosePage(bool proceed) {
-  render_manager_.ShouldClosePage(proceed);
-}
-
 void TabContents::OnCrossSiteResponse(int new_render_process_host_id,
                                       int new_request_id) {
   // Allows the TabContents to react when a cross-site response is ready to be
diff --git a/chrome/browser/tab_contents/tab_contents.h b/chrome/browser/tab_contents/tab_contents.h
index fc6f76f..5d987a2 100644
--- a/chrome/browser/tab_contents/tab_contents.h
+++ b/chrome/browser/tab_contents/tab_contents.h
@@ -850,7 +850,6 @@ class TabContents : public PageNavigator,
   virtual RendererPreferences GetRendererPrefs() const;
   virtual WebPreferences GetWebkitPrefs();
   virtual void OnJSOutOfMemory();
-  virtual void ShouldClosePage(bool proceed);
   virtual void OnCrossSiteResponse(int new_render_process_host_id,
                                    int new_request_id);
   virtual bool CanBlur() const;