Start using RenderFrameProxyHost objects.

content/renderer/render_frame_impl.cc

Index: content/renderer/render_frame_impl.cc
diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc
index c7d721dbee6d9e60d95a36bfbf80344d5a6d3fd6..0d3d03a75383cf3dc3698866a9b422ebc39a7dca 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -74,6 +74,7 @@
 #include "content/renderer/notification_provider.h"
 #include "content/renderer/npapi/plugin_channel_host.h"
 #include "content/renderer/push_messaging_dispatcher.h"
+#include "content/renderer/render_frame_proxy.h"
 #include "content/renderer/render_process.h"
 #include "content/renderer/render_thread_impl.h"
 #include "content/renderer/render_view_impl.h"
@@ -368,10 +369,31 @@ RenderFrameImpl* RenderFrameImpl::FromRoutingID(int32 routing_id) {
 }
 
 // static
+void RenderFrameImpl::CreateFrame(int routing_id, int parent_routing_id) {
+  CHECK_NE(MSG_ROUTING_NONE, parent_routing_id);

[Charlie Reis, 2014/07/24 22:36:30]

Wait, the comments on this function and the IPC message said this is allowed. 
Which one is wrong?

[kenrb, 2014/07/25 23:42:06]

I don't know what is intended here, so I left a TODO for nasko.

+
+  RenderFrameProxy* proxy = RenderFrameProxy::FromRoutingID(parent_routing_id);
+
+  // If the browser is sending a valid parent routing id, it better be created

[Charlie Reis, 2014/07/24 22:36:31]

it better be -> it should already be

[kenrb, 2014/07/25 23:42:06]

Done.

+  // and registered.
+  CHECK(proxy);
+  blink::WebRemoteFrame* parent_web_frame = proxy->web_frame();
+
+  // Create the RenderFrame and WebLocalFrame, linking the two.
+  RenderFrameImpl* render_frame =
+      RenderFrameImpl::Create(proxy->render_view(), routing_id);
+  blink::WebLocalFrame* web_frame =
+      parent_web_frame->createLocalChild("", render_frame);
+  render_frame->SetWebFrame(web_frame);
+  render_frame->Initialize();
+}
+
+// static
 RenderFrame* RenderFrame::FromWebFrame(blink::WebFrame* web_frame) {
   return RenderFrameImpl::FromWebFrame(web_frame);
 }
 
+// static
 RenderFrameImpl* RenderFrameImpl::FromWebFrame(blink::WebFrame* web_frame) {
   FrameMap::iterator iter = g_frame_map.Get().find(web_frame);
   if (iter != g_frame_map.Get().end())
@@ -668,11 +690,13 @@ bool RenderFrameImpl::Send(IPC::Message* message) {
     delete message;
     return false;
   }
-  if (is_swapped_out_ || render_view_->is_swapped_out()) {
+  if (frame_->parent() == NULL &&
+      (is_swapped_out_ || render_view_->is_swapped_out())) {
     if (!SwappedOutMessages::CanSendWhileSwappedOut(message)) {
       delete message;
       return false;
     }
+
     // In most cases, send IPCs through the proxy when swapped out. In some
     // calls the associated RenderViewImpl routing id is used to send
     // messages, so don't use the proxy.
@@ -684,7 +708,8 @@ bool RenderFrameImpl::Send(IPC::Message* message) {
 }
 
 bool RenderFrameImpl::OnMessageReceived(const IPC::Message& msg) {
-  GetContentClient()->SetActiveURL(frame_->document().url());
+  if (!frame_->document().isNull())

[dcheng, 2014/07/24 18:23:28]

I don't understand why we need this check. Under what circumstances are we going
to end up with a null WebDocument for a WebLocalFrame? It doesn't really seem
like this should ever happen, so I'm curious if it's because of our transition
code or something else.

[kenrb, 2014/07/24 21:06:17]

Right now we have RenderFrameProxy wrapping a RenderFrameImpl, and in that
situation there is a null document. This is only temporary, if I understand all
of this correctly. I will add a comment for that.

I rebased today, and now there is a crash bug that looks like a similar problem
to that.

+    GetContentClient()->SetActiveURL(frame_->document().url());
 
   ObserverListBase<RenderFrameObserver>::Iterator it(observers_);
   RenderFrameObserver* observer;
@@ -755,7 +780,8 @@ void RenderFrameImpl::OnNavigate(const FrameMsg_Navigate_Params& params) {
     return;
 
   // Swap this renderer back in if necessary.
-  if (render_view_->is_swapped_out_) {
+  if (render_view_->is_swapped_out_ &&
+      GetWebFrame() == render_view_->webview()->mainFrame()) {
     // We marked the view as hidden when swapping the view out, so be sure to
     // reset the visibility state before navigating to the new URL.
     render_view_->webview()->setVisibilityState(
@@ -948,6 +974,8 @@ void RenderFrameImpl::OnBeforeUnload() {
 
 void RenderFrameImpl::OnSwapOut(int proxy_routing_id) {
   RenderFrameProxy* proxy = NULL;
+  bool is_site_per_process =
+      CommandLine::ForCurrentProcess()->HasSwitch(switches::kSitePerProcess);
 
   // Only run unload if we're not swapped out yet, but send the ack either way.
   if (!is_swapped_out_ || !render_view_->is_swapped_out_) {
@@ -994,7 +1022,8 @@ void RenderFrameImpl::OnSwapOut(int proxy_routing_id) {
     // run a second time, thanks to a check in FrameLoader::stopLoading.
     // TODO(creis): Need to add a better way to do this that avoids running the
     // beforeunload handler. For now, we just run it a second time silently.
-    render_view_->NavigateToSwappedOutURL(frame_);
+    if (!is_site_per_process || frame_->parent() == NULL)
+      render_view_->NavigateToSwappedOutURL(frame_);
 
     // Let WebKit know that this view is hidden so it can drop resources and
     // stop compositing.
@@ -1014,8 +1043,17 @@ void RenderFrameImpl::OnSwapOut(int proxy_routing_id) {
 
   // Now that all of the cleanup is complete and the browser side is notified,
   // start using the RenderFrameProxy, if one is created.
-  if (proxy)
-    set_render_frame_proxy(proxy);
+  if (proxy) {
+    if (frame_->parent()) {
+      frame_->swap(proxy->web_frame());
+      if (is_site_per_process) {

[Charlie Reis, 2014/07/24 22:36:31]

When would this be false?  We don't swap out subframes unless --site-per-process
is passed.

[nasko, 2014/07/25 07:13:21]

It shouldn't be false. We will only have proxies for subframes with
--site-per-process.

[kenrb, 2014/07/25 23:42:06]

I will leave these for Nasko's follow-up CL.

+        // TODO(nasko): delete the frame here, since we've replaced it with a

[Charlie Reis, 2014/07/24 22:36:30]

Can we do this now?  (Is it a leak if we don't?)

[nasko, 2014/07/25 07:13:21]

It likely is a leak. I left this piece for a follow up CL, since
--site-per-process is not the default and I didn't want to make this even more
complex.

+        // proxy.
+      }
+    } else {
+      set_render_frame_proxy(proxy);
+    }
+  }
 }
 
 void RenderFrameImpl::OnContextMenuClosed(
@@ -1685,6 +1723,7 @@ void RenderFrameImpl::loadURLExternally(
     blink::WebNavigationPolicy policy,
     const blink::WebString& suggested_name) {
   DCHECK(!frame_ || frame_ == frame);
+
   Referrer referrer(RenderViewImpl::GetReferrerFromRequest(frame, request));
   if (policy == blink::WebNavigationPolicyDownload) {
     render_view_->Send(new ViewHostMsg_DownloadUrl(render_view_->GetRoutingID(),
@@ -1707,8 +1746,9 @@ blink::WebNavigationPolicy RenderFrameImpl::decidePolicyForNavigation(
     blink::WebNavigationPolicy default_policy,
     bool is_redirect) {
   DCHECK(!frame_ || frame_ == frame);
-  return DecidePolicyForNavigation(
+  WebNavigationPolicy value = DecidePolicyForNavigation(

[Charlie Reis, 2014/07/24 22:36:31]

Why is this change needed?  (Leftover from an earlier change?)

[nasko, 2014/07/25 07:13:21]

Most likely leftover from debugging statements using the return value.

[kenrb, 2014/07/25 23:42:06]

Done.

       this, frame, extra_data, request, type, default_policy, is_redirect);
+  return value;
 }
 
 blink::WebHistoryItem RenderFrameImpl::historyItemForNewChildFrame(
@@ -1813,8 +1853,8 @@ void RenderFrameImpl::didStartProvisionalLoad(blink::WebLocalFrame* frame) {
                     DidStartProvisionalLoad(frame));
   FOR_EACH_OBSERVER(RenderFrameObserver, observers_, DidStartProvisionalLoad());
 
-  Send(new FrameHostMsg_DidStartProvisionalLoadForFrame(routing_id_,
-                                                        ds->request().url()));
+  Send(new FrameHostMsg_DidStartProvisionalLoadForFrame(
+       routing_id_, ds->request().url()));

[Charlie Reis, 2014/07/24 22:36:31]

This indent looks wrong.  I think it was fine before, or it should need another
4 spaces.

[kenrb, 2014/07/25 23:42:06]

Done.

 }
 
 void RenderFrameImpl::didReceiveServerRedirectForProvisionalLoad(
@@ -2383,13 +2423,19 @@ void RenderFrameImpl::willSendRequest(
     if (request.targetType() == blink::WebURLRequest::TargetIsMainFrame) {
       request.setFirstPartyForCookies(request.url());
     } else {
-      request.setFirstPartyForCookies(
-          frame->top()->document().firstPartyForCookies());
+      // TODO(nasko): When the top-level frame is remote, there is no document.
+      // This is broken and should be fixed to propagate the URL.

[Charlie Reis, 2014/07/24 22:36:31]

URL -> first party

[kenrb, 2014/07/25 23:42:06]

Done.

+      WebFrame* top = frame->top();
+      if (top->isWebLocalFrame()) {
+        request.setFirstPartyForCookies(
+            frame->top()->document().firstPartyForCookies());
+      }
     }
   }
 
   WebFrame* top_frame = frame->top();
-  if (!top_frame)
+  // TODO(nasko): Hack around asking about top-frame data source.

[Charlie Reis, 2014/07/24 22:36:31]

This could lead to some nasty bugs.  We should elaborate a bit here (e.g.,
mention site-per-process or something) to make sure we don't forget about this.

[kenrb, 2014/07/25 23:42:06]

Done.

+  if (!top_frame || top_frame->isWebRemoteFrame())
     top_frame = frame;
   WebDataSource* provisional_data_source = top_frame->provisionalDataSource();
   WebDataSource* top_data_source = top_frame->dataSource();
@@ -2486,8 +2532,16 @@ void RenderFrameImpl::willSendRequest(
     provider_id = provider->provider_id();
   }
 
-  int parent_routing_id = frame->parent() ?
-      FromWebFrame(frame->parent())->GetRoutingID() : -1;
+  WebFrame* parent = frame->parent();
+  int parent_routing_id = MSG_ROUTING_NONE;
+  if (!parent) {
+    parent_routing_id = -1;
+  } else if (parent->isWebLocalFrame()) {
+    parent_routing_id = FromWebFrame(parent)->GetRoutingID();
+  } else {
+    parent_routing_id = RenderFrameProxy::FromWebFrame(parent)->routing_id();
+  }
+
   RequestExtraData* extra_data = new RequestExtraData();
   extra_data->set_visibility_state(render_view_->visibilityState());
   extra_data->set_custom_user_agent(custom_user_agent);
@@ -3198,28 +3252,35 @@ WebNavigationPolicy RenderFrameImpl::DecidePolicyForNavigation(
 #endif
 
   Referrer referrer(RenderViewImpl::GetReferrerFromRequest(frame, request));
+  const CommandLine& command_line = *CommandLine::ForCurrentProcess();
 
-  if (is_swapped_out_ || render_view_->is_swapped_out()) {
-    if (request.url() != GURL(kSwappedOutURL)) {
-      // Targeted links may try to navigate a swapped out frame.  Allow the
-      // browser process to navigate the tab instead.  Note that it is also
-      // possible for non-targeted navigations (from this view) to arrive
-      // here just after we are swapped out.  It's ok to send them to the
-      // browser, as long as they're for the top level frame.
-      // TODO(creis): Ensure this supports targeted form submissions when
-      // fixing http://crbug.com/101395.
-      if (frame->parent() == NULL) {
-        OpenURL(frame, request.url(), referrer, default_policy);
-        return blink::WebNavigationPolicyIgnore;  // Suppress the load here.
+  bool is_subframe = !!frame->parent();
+
+  if (command_line.HasSwitch(switches::kSitePerProcess) && is_subframe) {
+    // There's no reason to ignore navigations on subframes (why again?)

[Charlie Reis, 2014/07/24 22:36:31]

"(why again?)"... ?

Perhaps this is because subframes get deleted rather than swapped out?

[nasko, 2014/07/25 07:13:21]

Yes, subframes don't use swapout logic, so the existing code block doesn't
apply. The code can be written a bit better, this was just to unblock things.

[kenrb, 2014/07/25 23:42:06]

Done.

+  } else {
+    if (is_swapped_out_ || render_view_->is_swapped_out()) {
+      if (request.url() != GURL(kSwappedOutURL)) {
+        // Targeted links may try to navigate a swapped out frame.  Allow the
+        // browser process to navigate the tab instead.  Note that it is also
+        // possible for non-targeted navigations (from this view) to arrive
+        // here just after we are swapped out.  It's ok to send them to the
+        // browser, as long as they're for the top level frame.
+        // TODO(creis): Ensure this supports targeted form submissions when
+        // fixing http://crbug.com/101395.
+        if (frame->parent() == NULL) {
+          OpenURL(frame, request.url(), referrer, default_policy);
+          return blink::WebNavigationPolicyIgnore;  // Suppress the load here.
+        }
+
+        // We should otherwise ignore in-process iframe navigations, if they
+        // arrive just after we are swapped out.
+        return blink::WebNavigationPolicyIgnore;
       }
 
-      // We should otherwise ignore in-process iframe navigations, if they
-      // arrive just after we are swapped out.
-      return blink::WebNavigationPolicyIgnore;
+      // Allow kSwappedOutURL to complete.
+      return default_policy;
     }
-
-    // Allow kSwappedOutURL to complete.
-    return default_policy;
   }
 
   // Webkit is asking whether to navigate to a new URL.
@@ -3237,7 +3298,6 @@ WebNavigationPolicy RenderFrameImpl::DecidePolicyForNavigation(
   // all top-level navigations to the browser to let it swap processes when
   // crossing site boundaries.  This is currently expected to break some script
   // calls and navigations, such as form submissions.
-  const CommandLine& command_line = *CommandLine::ForCurrentProcess();
   bool force_swap_due_to_flag =
       command_line.HasSwitch(switches::kEnableStrictSiteIsolation) ||
       command_line.HasSwitch(switches::kSitePerProcess);