Send replies to sync IPCs from swapped out renderers.

BUG=93427
TEST=Go back/forward quickly with cross-process navigations.

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

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

9 files changed, 129 insertions, 26 deletions

diff --git a/content/browser/javascript_dialogs.h b/content/browser/javascript_dialogs.h
index 4b1af55..31e13c42 100644
--- a/content/browser/javascript_dialogs.h
+++ b/content/browser/javascript_dialogs.h
@@ -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.
 
@@ -7,6 +7,7 @@
 #pragma once
 
 #include "base/string16.h"
+#include "content/browser/renderer_host/render_view_host.h"
 #include "content/common/content_export.h"
 #include "ui/base/javascript_message_type.h"
 #include "ui/gfx/native_widget_types.h"
@@ -34,7 +35,8 @@ class CONTENT_EXPORT DialogDelegate {
 class CONTENT_EXPORT JavaScriptDialogDelegate : public DialogDelegate {
  public:
   // This callback is invoked when the dialog is closed.
-  virtual void OnDialogClosed(IPC::Message* reply_msg,
+  virtual void OnDialogClosed(RenderViewHost* rvh,
+                              IPC::Message* reply_msg,
                               bool success,
                               const string16& user_input) = 0;
 
diff --git a/content/browser/renderer_host/render_view_host.cc b/content/browser/renderer_host/render_view_host.cc
index 22d9b89..3b6ba75 100644
--- a/content/browser/renderer_host/render_view_host.cc
+++ b/content/browser/renderer_host/render_view_host.cc
@@ -666,10 +666,21 @@ bool RenderViewHost::OnMessageReceived(const IPC::Message& msg) {
     return true;
 
   // Filter out most IPC messages if this renderer is swapped out.
-  // We still want to certain ACKs to keep our state consistent.
-  if (is_swapped_out_)
-    if (!content::SwappedOutMessages::CanHandleWhileSwappedOut(msg))
+  // We still want to handle certain ACKs to keep our state consistent.
+  if (is_swapped_out_) {
+    if (!content::SwappedOutMessages::CanHandleWhileSwappedOut(msg)) {
+      // If this is a synchronous message and we decided not to handle it,
+      // we must send an error reply, or else the renderer will be stuck
+      // and won't respond to future requests.
+      if (msg.is_sync()) {
+        IPC::Message* reply = IPC::SyncMessage::GenerateReply(&msg);
+        reply->set_reply_error();
+        Send(reply);
+      }
+      // Don't continue looking for someone to handle it.
       return true;
+    }
+  }
 
   ObserverListBase<content::RenderViewHostObserver>::Iterator it(observers_);
   content::RenderViewHostObserver* observer;
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 978fe90..6c1f99a 100644
--- a/content/browser/tab_contents/render_view_host_manager_unittest.cc
+++ b/content/browser/tab_contents/render_view_host_manager_unittest.cc
@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/utf_string_conversions.h"
 #include "content/browser/browser_thread_impl.h"
 #include "content/browser/browser_url_handler.h"
 #include "content/browser/mock_content_browser_client.h"
@@ -24,6 +25,7 @@
 #include "content/test/test_notification_tracker.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "googleurl/src/url_util.h"
+#include "ui/base/javascript_message_type.h"
 #include "webkit/glue/webkit_glue.h"
 
 using content::BrowserContext;
@@ -233,6 +235,85 @@ TEST_F(RenderViewHostManagerTest, NewTabPageProcesses) {
       contents2.GetRenderViewHost()->site_instance());
 }
 
+// Ensure that the browser ignores most IPC messages that arrive from a
+// RenderViewHost that has been swapped out.  We do not want to take
+// action on requests from a non-active renderer.  The main exception is
+// for synchronous messages, which cannot be ignored without leaving the
+// renderer in a stuck state.  See http://crbug.com/93427.
+TEST_F(RenderViewHostManagerTest, FilterMessagesWhileSwappedOut) {
+  BrowserThreadImpl ui_thread(BrowserThread::UI, MessageLoop::current());
+  const GURL kNtpUrl(chrome::kTestNewTabURL);
+  const GURL kDestUrl("http://www.google.com/");
+
+  // Navigate our first tab to the new tab page and then to the destination.
+  NavigateActiveAndCommit(kNtpUrl);
+  TestRenderViewHost* ntp_rvh = static_cast<TestRenderViewHost*>(
+      contents()->GetRenderManagerForTesting()->current_host());
+
+  // Send an update title message and make sure it works.
+  const string16 ntp_title = ASCIIToUTF16("NTP Title");
+  WebKit::WebTextDirection direction = WebKit::WebTextDirectionLeftToRight;
+  EXPECT_TRUE(ntp_rvh->TestOnMessageReceived(
+      ViewHostMsg_UpdateTitle(rvh()->routing_id(), 0, ntp_title, direction)));
+  EXPECT_EQ(ntp_title, contents()->GetTitle());
+
+  // Navigate to a cross-site URL.
+  contents()->GetController().LoadURL(
+      kDestUrl, content::Referrer(), content::PAGE_TRANSITION_LINK,
+      std::string());
+  EXPECT_TRUE(contents()->cross_navigation_pending());
+  TestRenderViewHost* dest_rvh = static_cast<TestRenderViewHost*>(
+      contents()->GetRenderManagerForTesting()->pending_render_view_host());
+  ASSERT_TRUE(dest_rvh);
+  EXPECT_NE(ntp_rvh, dest_rvh);
+
+  // BeforeUnload finishes.
+  ntp_rvh->SendShouldCloseACK(true);
+
+  // Assume SwapOutACK times out, so the dest_rvh proceeds and commits.
+  dest_rvh->SendNavigate(101, kDestUrl);
+
+  // The new RVH should be able to update its title.
+  const string16 dest_title = ASCIIToUTF16("Google");
+  EXPECT_TRUE(dest_rvh->TestOnMessageReceived(
+      ViewHostMsg_UpdateTitle(rvh()->routing_id(), 101, dest_title,
+                              direction)));
+  EXPECT_EQ(dest_title, contents()->GetTitle());
+
+  // The old renderer, being slow, now updates the title. It should be filtered
+  // out and not take effect.
+  EXPECT_TRUE(ntp_rvh->is_swapped_out());
+  EXPECT_TRUE(ntp_rvh->TestOnMessageReceived(
+      ViewHostMsg_UpdateTitle(rvh()->routing_id(), 0, ntp_title, direction)));
+  EXPECT_EQ(dest_title, contents()->GetTitle());
+
+  // We cannot filter out synchronous IPC messages, because the renderer would
+  // be left waiting for a reply.  We pick RunBeforeUnloadConfirm as an example
+  // that can run easily within a unit test, and that needs to receive a reply
+  // without showing an actual dialog.
+  MockRenderProcessHost* ntp_process_host =
+      static_cast<MockRenderProcessHost*>(ntp_rvh->process());
+  ntp_process_host->sink().ClearMessages();
+  const string16 msg = ASCIIToUTF16("Message");
+  bool result = false;
+  string16 unused;
+  ViewHostMsg_RunBeforeUnloadConfirm before_unload_msg(
+      rvh()->routing_id(), kNtpUrl, msg, &result, &unused);
+  // Enable pumping for check in BrowserMessageFilter::CheckCanDispatchOnUI.
+  before_unload_msg.EnableMessagePumping();
+  EXPECT_TRUE(ntp_rvh->TestOnMessageReceived(before_unload_msg));
+  EXPECT_TRUE(ntp_process_host->sink().GetUniqueMessageMatching(IPC_REPLY_ID));
+
+  // Also test RunJavaScriptMessage.
+  ntp_process_host->sink().ClearMessages();
+  ViewHostMsg_RunJavaScriptMessage js_msg(
+      rvh()->routing_id(), msg, msg, kNtpUrl,
+      ui::JAVASCRIPT_MESSAGE_TYPE_CONFIRM, &result, &unused);
+  js_msg.EnableMessagePumping();
+  EXPECT_TRUE(ntp_rvh->TestOnMessageReceived(js_msg));
+  EXPECT_TRUE(ntp_process_host->sink().GetUniqueMessageMatching(IPC_REPLY_ID));
+}
+
 // When there is an error with the specified page, renderer exits view-source
 // mode. See WebFrameImpl::DidFail(). We check by this test that
 // EnableViewSourceMode message is sent on every navigation regardless
diff --git a/content/browser/tab_contents/tab_contents.cc b/content/browser/tab_contents/tab_contents.cc
index accc564..6e2780a 100644
--- a/content/browser/tab_contents/tab_contents.cc
+++ b/content/browser/tab_contents/tab_contents.cc
@@ -2017,7 +2017,7 @@ void TabContents::RequestTransferURL(const GURL& url,
 }
 
 void TabContents::RunJavaScriptMessage(
-    const RenderViewHost* rvh,
+    RenderViewHost* rvh,
     const string16& message,
     const string16& default_prompt,
     const GURL& frame_url,
@@ -2061,13 +2061,13 @@ void TabContents::RunJavaScriptMessage(
   if (suppress_this_message) {
     // If we are suppressing messages, just reply as if the user immediately
     // pressed "Cancel".
-    OnDialogClosed(reply_msg, false, string16());
+    OnDialogClosed(rvh, reply_msg, false, string16());
   }
 
   *did_suppress_message = suppress_this_message;
 }
 
-void TabContents::RunBeforeUnloadConfirm(const RenderViewHost* rvh,
+void TabContents::RunBeforeUnloadConfirm(RenderViewHost* rvh,
                                          const string16& message,
                                          IPC::Message* reply_msg) {
   if (delegate_)
@@ -2078,7 +2078,8 @@ void TabContents::RunBeforeUnloadConfirm(const RenderViewHost* rvh,
       !delegate_ ||
       delegate_->ShouldSuppressDialogs();
   if (suppress_this_message) {
-    GetRenderViewHost()->JavaScriptDialogClosed(reply_msg, true, string16());
+    // The reply must be sent to the RVH that sent the request.
+    rvh->JavaScriptDialogClosed(reply_msg, true, string16());
     return;
   }
 
@@ -2264,7 +2265,8 @@ bool TabContents::CreateRenderViewForRenderManager(
   return true;
 }
 
-void TabContents::OnDialogClosed(IPC::Message* reply_msg,
+void TabContents::OnDialogClosed(RenderViewHost* rvh,
+                                 IPC::Message* reply_msg,
                                  bool success,
                                  const string16& user_input) {
   if (is_showing_before_unload_dialog_ && !success) {
@@ -2275,7 +2277,11 @@ void TabContents::OnDialogClosed(IPC::Message* reply_msg,
     tab_close_start_time_ = base::TimeTicks();
   }
   is_showing_before_unload_dialog_ = false;
-  GetRenderViewHost()->JavaScriptDialogClosed(reply_msg, success, user_input);
+  // The reply must be sent to the RVH that sent the request.
+  // TODO(creis): Eliminate cases where we pass in null.
+  if (!rvh)
+    rvh = GetRenderViewHost();
+  rvh->JavaScriptDialogClosed(reply_msg, success, user_input);
 }
 
 gfx::NativeWindow TabContents::GetDialogRootWindow() const {
diff --git a/content/browser/tab_contents/tab_contents.h b/content/browser/tab_contents/tab_contents.h
index 5dda23e..263883c 100644
--- a/content/browser/tab_contents/tab_contents.h
+++ b/content/browser/tab_contents/tab_contents.h
@@ -268,14 +268,14 @@ class CONTENT_EXPORT TabContents
       WindowOpenDisposition disposition,
       int64 source_frame_id,
       const content::GlobalRequestID& transferred_global_request_id) OVERRIDE;
-  virtual void RunJavaScriptMessage(const RenderViewHost* rvh,
+  virtual void RunJavaScriptMessage(RenderViewHost* rvh,
                                     const string16& message,
                                     const string16& default_prompt,
                                     const GURL& frame_url,
                                     ui::JavascriptMessageType type,
                                     IPC::Message* reply_msg,
                                     bool* did_suppress_message) OVERRIDE;
-  virtual void RunBeforeUnloadConfirm(const RenderViewHost* rvh,
+  virtual void RunBeforeUnloadConfirm(RenderViewHost* rvh,
                                       const string16& message,
                                       IPC::Message* reply_msg) OVERRIDE;
   virtual content::RendererPreferences GetRendererPrefs(
@@ -336,7 +336,8 @@ class CONTENT_EXPORT TabContents
   virtual void CreateViewAndSetSizeForRVH(RenderViewHost* rvh) OVERRIDE;
 
   // Overridden from JavaScriptDialogDelegate:
-  virtual void OnDialogClosed(IPC::Message* reply_msg,
+  virtual void OnDialogClosed(RenderViewHost* rvh,
+                              IPC::Message* reply_msg,
                               bool success,
                               const string16& user_input) OVERRIDE;
   virtual gfx::NativeWindow GetDialogRootWindow() const OVERRIDE;
diff --git a/content/common/swapped_out_messages.cc b/content/common/swapped_out_messages.cc
index 3189d19..16c58e1 100644
--- a/content/common/swapped_out_messages.cc
+++ b/content/common/swapped_out_messages.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.
 
@@ -43,6 +43,8 @@ bool SwappedOutMessages::CanHandleWhileSwappedOut(
   // We drop most other messages that arrive from a swapped out renderer.
   // However, some are important (e.g., ACKs) for keeping the browser and
   // renderer state consistent in case we later return to the renderer.
+  // Note that synchronous messages that are not handled will receive an
+  // error reply instead, to avoid leaving the renderer in a stuck state.
   switch (msg.type()) {
     // Sends an ACK.
     case ViewHostMsg_ShowView::ID:
@@ -60,14 +62,11 @@ bool SwappedOutMessages::CanHandleWhileSwappedOut(
     case ViewHostMsg_Close::ID:
     // Sends an ACK.
     case ViewHostMsg_RequestMove::ID:
-    // Suppresses dialog and sends an ACK.
-    case ViewHostMsg_RunJavaScriptMessage::ID:
-    // Suppresses dialog and sends an ACK.
-    case ViewHostMsg_RunBeforeUnloadConfirm::ID:
     // Sends an ACK.
     case ViewHostMsg_AccessibilityNotifications::ID:
 #if defined(USE_X11)
-    // Synchronous message when leaving a page with plugin.
+    // Synchronous message when leaving a page with plugin.  In this case,
+    // we want to destroy the plugin rather than return an error message.
     case ViewHostMsg_DestroyPluginContainer::ID:
 #endif
       return true;
diff --git a/content/public/browser/render_view_host_delegate.h b/content/public/browser/render_view_host_delegate.h
index 2699ea4..5a7ab88 100644
--- a/content/public/browser/render_view_host_delegate.h
+++ b/content/public/browser/render_view_host_delegate.h
@@ -294,7 +294,7 @@ class CONTENT_EXPORT RenderViewHostDelegate : public IPC::Channel::Listener {
       const content::GlobalRequestID& old_request_id) {}
 
   // A javascript message, confirmation or prompt should be shown.
-  virtual void RunJavaScriptMessage(const RenderViewHost* rvh,
+  virtual void RunJavaScriptMessage(RenderViewHost* rvh,
                                     const string16& message,
                                     const string16& default_prompt,
                                     const GURL& frame_url,
@@ -302,7 +302,7 @@ class CONTENT_EXPORT RenderViewHostDelegate : public IPC::Channel::Listener {
                                     IPC::Message* reply_msg,
                                     bool* did_suppress_message) {}
 
-  virtual void RunBeforeUnloadConfirm(const RenderViewHost* rvh,
+  virtual void RunBeforeUnloadConfirm(RenderViewHost* rvh,
                                       const string16& message,
                                       IPC::Message* reply_msg) {}
 
diff --git a/content/public/browser/web_contents_delegate.cc b/content/public/browser/web_contents_delegate.cc
index 2a9bc4f..4edadc2 100644
--- a/content/public/browser/web_contents_delegate.cc
+++ b/content/public/browser/web_contents_delegate.cc
@@ -146,7 +146,8 @@ class JavaScriptDialogCreatorStub : public JavaScriptDialogCreator {
       JavaScriptDialogDelegate* delegate,
       const string16& message_text,
       IPC::Message* reply_message) OVERRIDE {
-    delegate->OnDialogClosed(reply_message, true, string16());
+    // TODO(creis): Pass in the RVH that sent the request.
+    delegate->OnDialogClosed(NULL, reply_message, true, string16());
   }
 
   virtual void ResetJavaScriptState(
diff --git a/content/test/test_content_client.cc b/content/test/test_content_client.cc
index eeb1df3..954958f 100644
--- a/content/test/test_content_client.cc
+++ b/content/test/test_content_client.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.
 
@@ -36,11 +36,13 @@ void TestContentClient::AddNPAPIPlugins(
 }
 
 bool TestContentClient::CanSendWhileSwappedOut(const IPC::Message* msg) {
-  return true;
+  // TestContentClient does not need to send any additional messages.
+  return false;
 }
 
 bool TestContentClient::CanHandleWhileSwappedOut(const IPC::Message& msg) {
-  return true;
+  // TestContentClient does not need to handle any additional messages.
+  return false;
 }
 
 std::string TestContentClient::GetUserAgent(bool* overriding) const {