Allow RenderFrames to swap out for subframe navigations.

Allow RenderFrames to swap out for subframe navigations.

BUG=314791
TEST=Cross-site subframes get swapped out when using --site-per-process.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243148

[Charlie Reis, 2013-12-20 21:21:28]

Nasko: here's the CL to swap out subframe RenderFrames.  I don't think it has
any dependencies on the CL to navigate subframe RFHs 
(https://codereview.chromium.org/118443008/), but it won't ever be called until
that CL lands.

This one's ready for review at any rate, and I'm cleaning up the other one.

[nasko, 2013-12-20 22:59:50]

Few nits.

https://codereview.chromium.org/109653014/diff/30001/content/common/swapped_o...
File content/common/swapped_out_messages.cc (right):

https://codereview.chromium.org/109653014/diff/30001/content/common/swapped_o...
content/common/swapped_out_messages.cc:39: // Handled by RenderFrame.
nit: Technically it is RenderFrameHost, isn't it? It is sent by RenderFrame.

https://codereview.chromium.org/109653014/diff/30001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/109653014/diff/30001/content/renderer/render_...
content/renderer/render_frame_impl.cc:484: IPC_MESSAGE_HANDLER(FrameMsg_SwapOut,
OnSwapOut)
nit: this is 3 spaces indented, should be 2. Also, no need for empty line after.

https://codereview.chromium.org/109653014/diff/30001/content/renderer/render_...
content/renderer/render_frame_impl.cc:517: frame_->stopLoading();
This seems to be missing the StopAltErrorPageFetcher calls that are in
RVH::OnStop. Is this intentional?

https://codereview.chromium.org/109653014/diff/30001/content/renderer/render_...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/109653014/diff/30001/content/renderer/render_...
content/renderer/render_frame_impl.h:84: void SetWebFrame(blink::WebFrame*
web_frame);
nit: Since this is not in the public interface, it should be hacker_style, no?

https://codereview.chromium.org/109653014/diff/30001/content/renderer/render_...
content/renderer/render_frame_impl.h:351: // In --site-per-process mode, stores
the WebFrame we are associated with.
nit: I think when we remove the flag requirement for RF per core::Frame, we will
always have the WebFrame pointer stored, so the comment might not be needed.

[Charlie Reis, 2013-12-21 00:26:41]

Thanks!  New patch is ready.

https://codereview.chromium.org/109653014/diff/30001/content/common/swapped_o...
File content/common/swapped_out_messages.cc (right):

https://codereview.chromium.org/109653014/diff/30001/content/common/swapped_o...
content/common/swapped_out_messages.cc:39: // Handled by RenderFrame.
On 2013/12/20 22:59:51, nasko wrote:
> nit: Technically it is RenderFrameHost, isn't it? It is sent by RenderFrame.

Yes, that's right.  Fixed for the others above as well.

https://codereview.chromium.org/109653014/diff/30001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/109653014/diff/30001/content/renderer/render_...
content/renderer/render_frame_impl.cc:484: IPC_MESSAGE_HANDLER(FrameMsg_SwapOut,
OnSwapOut)
On 2013/12/20 22:59:51, nasko wrote:
> nit: this is 3 spaces indented, should be 2. Also, no need for empty line
after.

Oops!  That was a copy paste error from the other patch.  Thanks.

https://codereview.chromium.org/109653014/diff/30001/content/renderer/render_...
content/renderer/render_frame_impl.cc:517: frame_->stopLoading();
On 2013/12/20 22:59:51, nasko wrote:
> This seems to be missing the StopAltErrorPageFetcher calls that are in
> RVH::OnStop. Is this intentional?

Yeah, though I'm not 100% sure it's right.  I did this to stop loading on this
particular frame (and its children) rather than from the top-level page down. 
I'm not certain what the right semantics are here, but since there's other
issues with this hack (e.g., lack of dispatchUnloadEvent), I didn't investigate.

I've added a TODO.

https://codereview.chromium.org/109653014/diff/30001/content/renderer/render_...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/109653014/diff/30001/content/renderer/render_...
content/renderer/render_frame_impl.h:84: void SetWebFrame(blink::WebFrame*
web_frame);
On 2013/12/20 22:59:51, nasko wrote:
> nit: Since this is not in the public interface, it should be hacker_style, no?

No, only one-liners that can be inlined should be hacker_style:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

https://codereview.chromium.org/109653014/diff/30001/content/renderer/render_...
content/renderer/render_frame_impl.h:351: // In --site-per-process mode, stores
the WebFrame we are associated with.
On 2013/12/20 22:59:51, nasko wrote:
> nit: I think when we remove the flag requirement for RF per core::Frame, we
will
> always have the WebFrame pointer stored, so the comment might not be needed.

Good thought, but let's remove the comment when that change lands.  For the
moment, this CL doesn't depend on having a RenderFrame per frame and could land
first.

https://codereview.chromium.org/109653014/diff/140001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/109653014/diff/140001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:995:
old_render_frame_host->set_swapped_out(true);
I also forgot to add this part from the draft CL in the earlier patch set.

[commit-bot: I haz the power, 2014-01-02 15:51:35]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[nasko, 2014-01-02 15:53:11]

LGTM

[commit-bot: I haz the power, 2014-01-02 15:53:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/109653014/140001

[commit-bot: I haz the power, 2014-01-02 15:53:59]

Failed to apply patch for
content/browser/frame_host/render_frame_host_manager.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/browser/frame_host/render_frame_host_manager.cc
  Hunk #1 succeeded at 256 (offset 2 lines).
  Hunk #2 succeeded at 303 (offset 2 lines).
  Hunk #3 FAILED at 530.
  Hunk #4 FAILED at 984.
  Hunk #5 FAILED at 1016.
  3 out of 5 hunks FAILED -- saving rejects to file
content/browser/frame_host/render_frame_host_manager.cc.rej

Patch:       content/browser/frame_host/render_frame_host_manager.cc
Index: content/browser/frame_host/render_frame_host_manager.cc
diff --git a/content/browser/frame_host/render_frame_host_manager.cc
b/content/browser/frame_host/render_frame_host_manager.cc
index
b5e1d4885562508e36d629412c352f8bd1815012..d1a380da396e75ce333ece4cafd6544e610f1d68
100644
--- a/content/browser/frame_host/render_frame_host_manager.cc
+++ b/content/browser/frame_host/render_frame_host_manager.cc
@@ -254,7 +254,7 @@ bool
RenderFrameHostManager::ShouldCloseTabOnUnresponsiveRenderer() {
   return false;
 }
 
-// TODO(creis): This should take in a RenderFrameHost.
+// TODO(creis): Remove this in favor of SwappedOutFrame.
 void RenderFrameHostManager::SwappedOut(RenderViewHost* render_view_host) {
   // Make sure this is from our current RVH, and that we have a pending
   // navigation from OnCrossSiteResponse.  (There may be no pending navigation
@@ -301,6 +301,56 @@ void RenderFrameHostManager::SwappedOut(RenderViewHost*
render_view_host) {
   pending_nav_params_.reset();
 }
 
+void RenderFrameHostManager::SwappedOutFrame(
+    RenderFrameHostImpl* render_frame_host) {
+  // Make sure this is from our current RFH, and that we have a pending
+  // navigation from OnCrossSiteResponse.  (There may be no pending navigation
+  // for data URLs that don't make network requests, for example.)   If not,
+  // just return early and ignore.
+  if (render_frame_host != render_frame_host_ || !pending_nav_params_.get()) {
+    pending_nav_params_.reset();
+    return;
+  }
+
+  // Sanity check that this is for the correct frame.
+  DCHECK_EQ(frame_tree_node_->frame_id(), pending_nav_params_->frame_id);
+
+  // Now that the unload handler has run, we need to either initiate the
+  // pending transfer (if there is one) or resume the paused response (if not).
+  // TODO(creis): The blank swapped out page is visible during this time, but
+  // we can shorten this by delivering the response directly, rather than
+  // forcing an identical request to be made.
+  if (pending_nav_params_->is_transfer) {
+    // Treat the last URL in the chain as the destination and the remainder as
+    // the redirect chain.
+    CHECK(pending_nav_params_->transfer_url_chain.size());
+    GURL transfer_url = pending_nav_params_->transfer_url_chain.back();
+    pending_nav_params_->transfer_url_chain.pop_back();
+
+    // We don't know whether the original request had |user_action| set to
true.
+    // However, since we force the navigation to be in the current tab, it
+    // doesn't matter.
+    // TODO(creis): Move RequestTransferURL to RenderFrameHost's navigator.
+    render_frame_host->render_view_host()->GetDelegate()->RequestTransferURL(
+        transfer_url,
+        pending_nav_params_->transfer_url_chain,
+        pending_nav_params_->referrer,
+        pending_nav_params_->page_transition,
+        CURRENT_TAB,
+        pending_nav_params_->frame_id,
+        pending_nav_params_->global_request_id,
+        false,
+        true);
+  } else if (pending_render_frame_host_) {
+    RenderProcessHostImpl* pending_process =
+        static_cast<RenderProcessHostImpl*>(
+            pending_render_frame_host_->GetProcess());
+    pending_process->ResumeDeferredNavigation(
+        pending_nav_params_->global_request_id);
+  }
+  pending_nav_params_.reset();
+}
+
 // TODO(creis): This should take in a RenderFrameHost.
 void RenderFrameHostManager::DidNavigateMainFrame(
     RenderViewHost* render_view_host) {
@@ -480,13 +530,16 @@ void RenderFrameHostManager::SwapOutOldPage() {
   // unload handler finishes and the navigation completes, we will send a
   // message to the ResourceDispatcherHost, allowing the pending RVH's response
   // to resume.
-  // TODO(creis): We should do this on the RFH or else we'll swap out the
-  // top-level page when subframes navigate.
-  render_frame_host_->render_view_host()->SwapOut();
+  // Note: This must be done on the RFH or else we'll swap out the top-level
+  // page when subframes navigate.
+  if (frame_tree_node_->IsMainFrame())
+    render_frame_host_->render_view_host()->SwapOut();
+  else
+    render_frame_host_->SwapOut();
 
   // ResourceDispatcherHost has told us to run the onunload handler, which
   // means it is not a download or unsafe page, and we are going to perform the
-  // navigation.  Thus, we no longer need to remember that the RenderViewHost
+  // navigation.  Thus, we no longer need to remember that the RenderFrameHost
   // is part of a pending cross-site request.
   if (pending_render_frame_host_) {
     pending_render_frame_host_->render_view_host()->
@@ -934,8 +987,13 @@ void RenderFrameHostManager::CommitPending() {
 
   // Hide the old view now that the new one is visible.
   if (old_render_frame_host->render_view_host()->GetView()) {
-    old_render_frame_host->render_view_host()->GetView()->Hide();
-    old_render_frame_host->render_view_host()->WasSwappedOut();
+    if (is_main_frame) {
+      old_render_frame_host->render_view_host()->GetView()->Hide();
+      old_render_frame_host->render_view_host()->WasSwappedOut();
+    } else {
+      // TODO(creis): We'll need to set this back to false if we navigate back.
+      old_render_frame_host->set_swapped_out(true);
+    }
   }
 
   // Make sure the size is up to date.  (Fix for bug 1079768.)
@@ -966,7 +1024,8 @@ void RenderFrameHostManager::CommitPending() {
   if (old_render_frame_host->render_view_host()->IsRenderViewLive()) {
     // If the old RFH is live, we are swapping it out and should keep track of
     // it in case we navigate back to it.
-    DCHECK(old_render_frame_host->render_view_host()->is_swapped_out());
+    DCHECK(old_render_frame_host->is_swapped_out() ||
+           old_render_frame_host->render_view_host()->is_swapped_out());
     // Temp fix for http://crbug.com/90867 until we do a better cleanup to make
     // sure we don't get different rvh instances for the same site instance
     // in the same rvhmgr.

[commit-bot: I haz the power, 2014-01-03 18:41:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/109653014/400001

[commit-bot: I haz the power, 2014-01-03 19:19:00]

Retried try job too often on android_aosp for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_ao...

[commit-bot: I haz the power, 2014-01-03 19:23:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/109653014/400001

[commit-bot: I haz the power, 2014-01-04 00:15:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/109653014/750001

[commit-bot: I haz the power, 2014-01-04 00:37:51]

Retried try job too often on linux_rel for step(s) base_unittests,
browser_tests, interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2014-01-06 16:42:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/109653014/970001

[commit-bot: I haz the power, 2014-01-06 19:57:17]

Change committed as 243148