Support window.opener after a process swap.

Changes:
1. Swap out RVHs instead of closing them, so we can return to the same Frame object if we later come back.
2. Filter out messages from swapped out RVHs, in case any are in-flight.
3. Remove the workaround for navigating away from an app.
4. Update tests to reflect the new sequence of events.

BUG=65953
TEST=AppApiTest.AppProcess
TEST=ResourceDispatcherTest.CrossSiteImmediateLoadOnunloadCookie

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

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

14 files changed, 88 insertions, 31 deletions

diff --git a/chrome/browser/extensions/app_process_apitest.cc b/chrome/browser/extensions/app_process_apitest.cc
index c5be9c2..4172c0c 100644
--- a/chrome/browser/extensions/app_process_apitest.cc
+++ b/chrome/browser/extensions/app_process_apitest.cc
@@ -105,12 +105,9 @@ IN_PROC_BROWSER_TEST_F(AppApiTest, AppProcess) {
                    base_url.Resolve("path1/empty.html"), true);
   WindowOpenHelper(browser(), host,
                    base_url.Resolve("path2/empty.html"), true);
-  // TODO(creis): This should open in a new process (i.e., false for the last
-  // argument), but we temporarily avoid swapping processes away from an app
-  // until we're able to restore window.opener if the page later returns to an
-  // in-app URL.  See crbug.com/65953.
+  // This should open in a new process (i.e., false for the last argument).
   WindowOpenHelper(browser(), host,
-                   base_url.Resolve("path3/empty.html"), true);
+                   base_url.Resolve("path3/empty.html"), false);
 
   // Now let's have these pages navigate, into or out of the extension web
   // extent. They should switch processes.
@@ -118,10 +115,7 @@ IN_PROC_BROWSER_TEST_F(AppApiTest, AppProcess) {
   const GURL& non_app_url(base_url.Resolve("path3/empty.html"));
   NavigateTabHelper(browser()->GetTabContentsAt(2), non_app_url);
   NavigateTabHelper(browser()->GetTabContentsAt(3), app_url);
-  // TODO(creis): This should swap out of the app's process (i.e., EXPECT_NE),
-  // but we temporarily avoid swapping away from an app in case it needs to
-  // communicate with window.opener later.  See crbug.com/65953.
-  EXPECT_EQ(host->process(),
+  EXPECT_NE(host->process(),
             browser()->GetTabContentsAt(2)->render_view_host()->process());
   EXPECT_EQ(host->process(),
             browser()->GetTabContentsAt(3)->render_view_host()->process());
diff --git a/chrome/browser/extensions/extension_host.cc b/chrome/browser/extensions/extension_host.cc
index 7badebf..af3eda6 100644
--- a/chrome/browser/extensions/extension_host.cc
+++ b/chrome/browser/extensions/extension_host.cc
@@ -427,7 +427,8 @@ void ExtensionHost::DocumentOnLoadCompletedInMainFrame(RenderViewHost* rvh,
   }
 }
 
-void ExtensionHost::RunJavaScriptMessage(const std::wstring& message,
+void ExtensionHost::RunJavaScriptMessage(const RenderViewHost* rvh,
+                                         const std::wstring& message,
                                          const std::wstring& default_prompt,
                                          const GURL& frame_url,
                                          const int flags,
diff --git a/chrome/browser/extensions/extension_host.h b/chrome/browser/extensions/extension_host.h
index 311975d..116e37d 100644
--- a/chrome/browser/extensions/extension_host.h
+++ b/chrome/browser/extensions/extension_host.h
@@ -129,7 +129,8 @@ class ExtensionHost : public RenderViewHostDelegate,
   // RenderViewHostDelegate implementation.
   virtual RenderViewHostDelegate::View* GetViewDelegate();
   virtual WebPreferences GetWebkitPrefs();
-  virtual void RunJavaScriptMessage(const std::wstring& message,
+  virtual void RunJavaScriptMessage(const RenderViewHost* rvh,
+                                    const std::wstring& message,
                                     const std::wstring& default_prompt,
                                     const GURL& frame_url,
                                     const int flags,
diff --git a/chrome/browser/tab_contents/background_contents.cc b/chrome/browser/tab_contents/background_contents.cc
index e5f98b8..2d0dd69 100644
--- a/chrome/browser/tab_contents/background_contents.cc
+++ b/chrome/browser/tab_contents/background_contents.cc
@@ -103,6 +103,7 @@ void BackgroundContents::DidNavigate(
 }
 
 void BackgroundContents::RunJavaScriptMessage(
+    const RenderViewHost* rvh,
     const std::wstring& message,
     const std::wstring& default_prompt,
     const GURL& frame_url,
diff --git a/chrome/browser/tab_contents/background_contents.h b/chrome/browser/tab_contents/background_contents.h
index f4e8874..9cd4f81 100644
--- a/chrome/browser/tab_contents/background_contents.h
+++ b/chrome/browser/tab_contents/background_contents.h
@@ -67,7 +67,8 @@ class BackgroundContents : public RenderViewHostDelegate,
   virtual void DidNavigate(RenderViewHost* render_view_host,
                            const ViewHostMsg_FrameNavigate_Params& params);
   virtual WebPreferences GetWebkitPrefs();
-  virtual void RunJavaScriptMessage(const std::wstring& message,
+  virtual void RunJavaScriptMessage(const RenderViewHost* rvh,
+                                    const std::wstring& message,
                                     const std::wstring& default_prompt,
                                     const GURL& frame_url,
                                     const int flags,
diff --git a/chrome/browser/tab_contents/web_contents_unittest.cc b/chrome/browser/tab_contents/web_contents_unittest.cc
index 76f0ef0..411e693 100644
--- a/chrome/browser/tab_contents/web_contents_unittest.cc
+++ b/chrome/browser/tab_contents/web_contents_unittest.cc
@@ -300,6 +300,11 @@ TEST_F(TabContentsTest, CrossSiteBoundaries) {
   int pending_rvh_delete_count = 0;
   pending_rvh->set_delete_counter(&pending_rvh_delete_count);
 
+  // Navigations should be suspended in pending_rvh until ShouldCloseACK.
+  EXPECT_TRUE(pending_rvh->are_navigations_suspended());
+  orig_rvh->SendShouldCloseACK(true);
+  EXPECT_FALSE(pending_rvh->are_navigations_suspended());
+
   // DidNavigate from the pending page
   ViewHostMsg_FrameNavigate_Params params2;
   InitNavigateParams(&params2, 1, url2, PageTransition::TYPED);
@@ -310,20 +315,36 @@ TEST_F(TabContentsTest, CrossSiteBoundaries) {
   EXPECT_EQ(pending_rvh, contents()->render_view_host());
   EXPECT_NE(instance1, instance2);
   EXPECT_TRUE(contents()->pending_rvh() == NULL);
-  EXPECT_EQ(orig_rvh_delete_count, 1);
+  // We keep the original RVH around, swapped out.
+  EXPECT_TRUE(contents()->render_manager()->IsSwappedOut(orig_rvh));
+  EXPECT_EQ(orig_rvh_delete_count, 0);
 
   // Going back should switch SiteInstances again.  The first SiteInstance is
   // stored in the NavigationEntry, so it should be the same as at the start.
+  // We should use the same RVH as before, swapping it back in.
   controller().GoBack();
   TestRenderViewHost* goback_rvh = contents()->pending_rvh();
+  EXPECT_EQ(orig_rvh, goback_rvh);
   EXPECT_TRUE(contents()->cross_navigation_pending());
 
+  // Navigations should be suspended in goback_rvh until ShouldCloseACK.
+  EXPECT_TRUE(goback_rvh->are_navigations_suspended());
+  pending_rvh->SendShouldCloseACK(true);
+  EXPECT_FALSE(goback_rvh->are_navigations_suspended());
+
   // DidNavigate from the back action
   contents()->TestDidNavigate(goback_rvh, params1);
   EXPECT_FALSE(contents()->cross_navigation_pending());
   EXPECT_EQ(goback_rvh, contents()->render_view_host());
-  EXPECT_EQ(pending_rvh_delete_count, 1);
   EXPECT_EQ(instance1, contents()->GetSiteInstance());
+  // The pending RVH should now be swapped out, not deleted.
+  EXPECT_TRUE(contents()->render_manager()->IsSwappedOut(pending_rvh));
+  EXPECT_EQ(pending_rvh_delete_count, 0);
+
+  // Close tab and ensure RVHs are deleted.
+  DeleteContents();
+  EXPECT_EQ(orig_rvh_delete_count, 1);
+  EXPECT_EQ(pending_rvh_delete_count, 1);
 }
 
 // Test that navigating across a site boundary after a crash creates a new
@@ -355,6 +376,7 @@ TEST_F(TabContentsTest, CrossSiteBoundariesAfterCrash) {
   EXPECT_FALSE(contents()->cross_navigation_pending());
   EXPECT_TRUE(contents()->pending_rvh() == NULL);
   EXPECT_NE(orig_rvh, new_rvh);
+  EXPECT_FALSE(contents()->render_manager()->IsSwappedOut(orig_rvh));
   EXPECT_EQ(orig_rvh_delete_count, 1);
 
   // DidNavigate from the new page
@@ -367,6 +389,10 @@ TEST_F(TabContentsTest, CrossSiteBoundariesAfterCrash) {
   EXPECT_EQ(new_rvh, rvh());
   EXPECT_NE(instance1, instance2);
   EXPECT_TRUE(contents()->pending_rvh() == NULL);
+
+  // Close tab and ensure RVHs are deleted.
+  DeleteContents();
+  EXPECT_EQ(orig_rvh_delete_count, 1);
 }
 
 // Test that opening a new tab in the same SiteInstance and then navigating
@@ -395,6 +421,7 @@ TEST_F(TabContentsTest, NavigateTwoTabsCrossSite) {
   // Navigate first tab to a new site
   const GURL url2a("http://www.yahoo.com");
   controller().LoadURL(url2a, GURL(), PageTransition::TYPED);
+  orig_rvh->SendShouldCloseACK(true);
   TestRenderViewHost* pending_rvh_a = contents()->pending_rvh();
   ViewHostMsg_FrameNavigate_Params params2a;
   InitNavigateParams(&params2a, 1, url2a, PageTransition::TYPED);
@@ -405,6 +432,9 @@ TEST_F(TabContentsTest, NavigateTwoTabsCrossSite) {
   // Navigate second tab to the same site as the first tab
   const GURL url2b("http://mail.yahoo.com");
   contents2.controller().LoadURL(url2b, GURL(), PageTransition::TYPED);
+  TestRenderViewHost* rvh2 =
+      static_cast<TestRenderViewHost*>(contents2.render_view_host());
+  rvh2->SendShouldCloseACK(true);
   TestRenderViewHost* pending_rvh_b = contents2.pending_rvh();
   EXPECT_TRUE(pending_rvh_b != NULL);
   EXPECT_TRUE(contents2.cross_navigation_pending());
@@ -1602,7 +1632,8 @@ TEST_F(TabContentsTest, NoJSMessageOnInterstitials) {
   // attempting to show a JS message.
   IPC::Message* dummy_message = new IPC::Message;
   bool did_suppress_message = false;
-  contents()->RunJavaScriptMessage(L"This is an informative message", L"OK",
+  contents()->RunJavaScriptMessage(contents()->render_view_host(),
+      L"This is an informative message", L"OK",
       kGURL, ui::MessageBoxFlags::kIsJavascriptAlert, dummy_message,
       &did_suppress_message);
   EXPECT_TRUE(did_suppress_message);
diff --git a/chrome/browser/ui/browser.cc b/chrome/browser/ui/browser.cc
index 2095fcf..240bcf0 100644
--- a/chrome/browser/ui/browser.cc
+++ b/chrome/browser/ui/browser.cc
@@ -4201,7 +4201,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()->ClosePage(false, -1, -1);
+      tab->render_view_host()->ClosePage();
     } else {
       ClearUnloadState(tab, true);
     }
diff --git a/chrome/browser/ui/webui/web_ui_unittest.cc b/chrome/browser/ui/webui/web_ui_unittest.cc
index 0d4543c..a37b5cf 100644
--- a/chrome/browser/ui/webui/web_ui_unittest.cc
+++ b/chrome/browser/ui/webui/web_ui_unittest.cc
@@ -182,19 +182,28 @@ TEST_F(WebUITest, FocusOnNavigate) {
   GURL next_url("http://google.com/");
   int next_page_id = page_id + 1;
   controller().LoadURL(next_url, GURL(), PageTransition::LINK);
+  TestRenderViewHost* old_rvh = rvh();
+  old_rvh->SendShouldCloseACK(true);
   pending_rvh()->SendNavigate(next_page_id, next_url);
+  old_rvh->OnSwapOutACK();
 
   // Navigate back.  Should focus the location bar.
   int focus_called = tc->focus_called();
   ASSERT_TRUE(controller().CanGoBack());
   controller().GoBack();
+  old_rvh = rvh();
+  old_rvh->SendShouldCloseACK(true);
   pending_rvh()->SendNavigate(page_id, new_tab_url);
+  old_rvh->OnSwapOutACK();
   EXPECT_LT(focus_called, tc->focus_called());
 
   // Navigate forward.  Shouldn't focus the location bar.
   focus_called = tc->focus_called();
   ASSERT_TRUE(controller().CanGoForward());
   controller().GoForward();
+  old_rvh = rvh();
+  old_rvh->SendShouldCloseACK(true);
   pending_rvh()->SendNavigate(next_page_id, next_url);
+  old_rvh->OnSwapOutACK();
   EXPECT_EQ(focus_called, tc->focus_called());
 }
diff --git a/chrome/browser/visitedlink/visitedlink_unittest.cc b/chrome/browser/visitedlink/visitedlink_unittest.cc
index 6f58ad7..f5f1ace 100644
--- a/chrome/browser/visitedlink/visitedlink_unittest.cc
+++ b/chrome/browser/visitedlink/visitedlink_unittest.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -512,7 +512,7 @@ class VisitRelayingRenderProcessHost : public BrowserRenderProcessHost {
   virtual void CancelResourceRequests(int render_widget_id) {
   }
 
-  virtual void CrossSiteClosePageACK(const ViewMsg_ClosePage_Params& params) {
+  virtual void CrossSiteSwapOutACK(const ViewMsg_SwapOut_Params& params) {
   }
 
   virtual bool WaitForPaintMsg(int render_widget_id,
diff --git a/chrome/common/chrome_content_client.cc b/chrome/common/chrome_content_client.cc
index 84b5e5d..f5d0e90 100644
--- a/chrome/common/chrome_content_client.cc
+++ b/chrome/common/chrome_content_client.cc
@@ -12,6 +12,7 @@
 #include "chrome/common/child_process_logging.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/chrome_switches.h"
+#include "chrome/common/render_messages.h"
 #include "content/common/pepper_plugin_registry.h"
 #include "remoting/client/plugin/pepper_entrypoints.h"
 
@@ -189,4 +190,30 @@ void ChromeContentClient::AddPepperPlugins(
 #endif
 }
 
+bool ChromeContentClient::CanSendWhileSwappedOut(const IPC::Message* msg) {
+  // Any Chrome-specific messages that must be allowed to be sent from swapped
+  // out renderers.
+  switch (msg->type()) {
+    case ViewHostMsg_DomOperationResponse::ID:
+      return true;
+    default:
+      break;
+  }
+  return false;
+}
+
+bool ChromeContentClient::CanHandleWhileSwappedOut(
+    const IPC::Message& msg) {
+  // Any Chrome-specific messages (apart from those listed in
+  // CanSendWhileSwappedOut) that must be handled by the browser when sent from
+  // swapped out renderers.
+  switch (msg.type()) {
+    case ViewHostMsg_Snapshot::ID:
+      return true;
+    default:
+      break;
+  }
+  return false;
+}
+
 }  // namespace chrome
diff --git a/chrome/common/chrome_content_client.h b/chrome/common/chrome_content_client.h
index bd8d78c..b7316b6 100644
--- a/chrome/common/chrome_content_client.h
+++ b/chrome/common/chrome_content_client.h
@@ -17,6 +17,8 @@ class ChromeContentClient : public content::ContentClient {
   virtual void SetActiveURL(const GURL& url);
   virtual void SetGpuInfo(const GPUInfo& gpu_info);
   virtual void AddPepperPlugins(std::vector<PepperPluginInfo>* plugins);
+  virtual bool CanSendWhileSwappedOut(const IPC::Message* msg);
+  virtual bool CanHandleWhileSwappedOut(const IPC::Message& msg);
 };
 
 }  // namespace chrome
diff --git a/chrome/common/render_messages.h b/chrome/common/render_messages.h
index 3681e40..80534d8 100644
--- a/chrome/common/render_messages.h
+++ b/chrome/common/render_messages.h
@@ -29,13 +29,14 @@
 #include "ipc/ipc_message_macros.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebCache.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebConsoleMessage.h"
-#include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/gfx/rect.h"
 
 // Singly-included section for enums and custom IPC traits.
 #ifndef CHROME_COMMON_RENDER_MESSAGES_H_
 #define CHROME_COMMON_RENDER_MESSAGES_H_
 
+class SkBitmap;
+
 // Command values for the cmd parameter of the
 // ViewHost_JavaScriptStressTestControl message. For each command the parameter
 // passed has a different meaning:
diff --git a/chrome/renderer/chrome_content_renderer_client.cc b/chrome/renderer/chrome_content_renderer_client.cc
index f01769c..4d38a15 100644
--- a/chrome/renderer/chrome_content_renderer_client.cc
+++ b/chrome/renderer/chrome_content_renderer_client.cc
@@ -461,9 +461,6 @@ bool ChromeContentRendererClient::ShouldFork(WebFrame* frame,
   // TODO(erikkay) This is happening inside of a check to is_content_initiated
   // which means that things like the back button won't trigger it.  Is that
   // OK?
-  // TODO(creis): For hosted apps, we currently only swap processes to enter
-  // the app and not exit it, since we currently lose context (e.g.,
-  // window.opener) if the window navigates back.  See crbug.com/65953.
   if (!CrossesExtensionExtents(frame, url))
     return false;
 
@@ -559,10 +556,7 @@ bool ChromeContentRendererClient::CrossesExtensionExtents(WebFrame* frame,
   if (old_url.is_empty() && frame->opener())
     old_url = frame->opener()->url();
 
-  bool old_url_is_hosted_app = extensions->GetByURL(old_url) &&
-      !extensions->GetByURL(old_url)->web_extent().is_empty();
-  return !extensions->InSameExtent(old_url, new_url) &&
-         !old_url_is_hosted_app;
+  return !extensions->InSameExtent(old_url, new_url);
 }
 
 }  // namespace chrome
diff --git a/chrome/renderer/chrome_content_renderer_client.h b/chrome/renderer/chrome_content_renderer_client.h
index d408bab..26e4ccb 100644
--- a/chrome/renderer/chrome_content_renderer_client.h
+++ b/chrome/renderer/chrome_content_renderer_client.h
@@ -83,11 +83,6 @@ class ChromeContentRendererClient : public content::ContentRendererClient {
 
   // Returns true if the frame is navigating to an URL either into or out of an
   // extension app's extent.
-  // TODO(creis): Temporary workaround for crbug.com/65953: Only return true if
-  // we would enter an extension app's extent from a non-app, or if we leave an
-  // extension with no web extent.  We avoid swapping processes to exit a hosted
-  // app with a web extent for now, since we do not yet restore context (such
-  // as window.opener) if the window navigates back.
   bool CrossesExtensionExtents(WebKit::WebFrame* frame, const GURL& new_url);
 
   scoped_ptr<ChromeRenderProcessObserver> chrome_observer_;