Refactor postMessage for out-of-process iframes.

Refactor postMessage for out-of-process iframes.

This CL moves cross-process postMessage plumbing from RenderView
to RenderFrame:

- The renderer-to-browser RouteMessageEvent message is now sent
  through RenderFrameProxy rather than a swapped-out RenderView.

- The bulk of WebContentsImpl::RouteMessageEvent is moved into
  RenderFrameProxyHost::RouteMessageEvent.  Unfortunately, we still
  need to call up to WebContents for dealing with <webview> and
  on-demand creation of swapped-out RVs for the opener chain.  This
  will be cleaned up as part of https://crbug.com/225940.

- the browser-to-renderer PostMessageEvent message is now handled in
  RenderFrameImpl::OnPostMessageEvent, instead of RenderViewImpl.

These changes make it possible to send messages between cross-process
subframes when in --site-per-process mode.  This is exercised in a new
test, SitePerProcessBrowserTest.SubframePostMessage.  More work will
be needed to support postMessages involving subframes in cross-process
openers (see https://crbug.com/225940 and https://crbug.com/423587).

With this change, the RFHM::SupportCrossProcessPostMessage test now
works in --site-per-process mode, so it is enabled on the Site
Isolation FYI bots.

BUG=389721
TBR=jam@chromium.org

Committed: https://crrev.com/e7da5a1508504fded712749adc154a69424afa9e
Cr-Commit-Position: refs/heads/master@{#324341}

[alexmos, 2015-04-02 23:16:59]

alexmos@chromium.org changed reviewers:
+ creis@chromium.org

[alexmos, 2015-04-02 23:17:00]

Charlie, could you please take a look?  Also some comments below.  Sorry about
this being long; there were a lot of mechanical RV-to-RF moves.  

I tried not to deviate from legacy postMessage behavior as much as possible and
leave cleanups for openers, <webview>, etc., to future CLs.  I had to put in
workarounds for Android WebView (in PS4) and subframes on cross-process openers
(in PS3), which I'm not entirely happy about, so open to suggestions if there
are cleaner ways to handle them.

https://codereview.chromium.org/1046933005/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_delegate.h (right):

https://codereview.chromium.org/1046933005/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_delegate.h:189: virtual int
EnsureOpenerRenderViewsExist(RenderFrameHost* render_frame_host);
We have a similar method in RFHM::Delegate,
CreateOpenerRenderViewsForRenderManager.  The difference here is that we need to
create swapped-out RVs starting from the current RFH rather than its opener, and
we need to not follow the opener chain for <webview> to match postMessage’s old
behavior.  This method will not be necessary once I clean up the opener stuff,
but in the meantime, do you think something like this is an acceptable
workaround?  I'm not sure whether it’s best to combine this with
CreateOpenerRenderViewsForRenderManager or keep it here, and whether its name
should indicate that it's intended for postMessage only.

https://codereview.chromium.org/1046933005/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/1046933005/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_proxy_host.cc:216:
new_params.source_routing_id = source_rvh->main_frame_routing_id();
The test that fails without this path is MimeHandlerViewTest.Iframe.

https://codereview.chromium.org/1046933005/diff/100001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1046933005/diff/100001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:4100: GetBrowserPluginGuest()
|| GetBrowserPluginEmbedder();
I really wanted to do the related SiteInstance check in RFPH::RouteMessageEvent,
but since we have to check for <webview> at the WebContents layer, I figured
it’d be clearer to just leave the whole check here.

https://codereview.chromium.org/1046933005/diff/100001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1046933005/diff/100001/content/renderer/rende...
content/renderer/render_frame_impl.cc:3553: CHECK(render_frame_proxy_);
Am I correct in assuming the proxy_ will always exist if the frame is swapped
out?  There is a window in OnSwapOut between setting is_swapped_out and calling
set_render_frame_proxy, but I’m not sure whether it’s possible for postMessage
to somehow happen in that window.  I could also route through the swapped-out RF
if that’s safer.

[Charlie Reis, 2015-04-06 16:18:56]

Nice job working through the issues here.  It'll be so nice to remove
swappedout:// and have this just go through RenderFrameProxy.

I think this is almost ready, apart from the postMessage-while-swapping-out
question.

https://codereview.chromium.org/1046933005/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_delegate.h (right):

https://codereview.chromium.org/1046933005/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_delegate.h:189: virtual int
EnsureOpenerRenderViewsExist(RenderFrameHost* render_frame_host);
On 2015/04/02 23:17:00, alexmos wrote:
> We have a similar method in RFHM::Delegate,
> CreateOpenerRenderViewsForRenderManager.  The difference here is that we need
to
> create swapped-out RVs starting from the current RFH rather than its opener,
and
> we need to not follow the opener chain for <webview> to match postMessage’s
old
> behavior.  This method will not be necessary once I clean up the opener stuff,
> but in the meantime, do you think something like this is an acceptable
> workaround?  I'm not sure whether it’s best to combine this with
> CreateOpenerRenderViewsForRenderManager or keep it here, and whether its name
> should indicate that it's intended for postMessage only.

Yes, I'm ok with this as a short term way to support swapped out RVs.  No need
to mention postMessage in the name, given the warning that it's going to be
removed.

https://codereview.chromium.org/1046933005/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/1046933005/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_proxy_host.cc:216:
new_params.source_routing_id = source_rvh->main_frame_routing_id();
On 2015/04/02 23:17:00, alexmos wrote:
> The test that fails without this path is MimeHandlerViewTest.Iframe.

Acknowledged.

https://codereview.chromium.org/1046933005/diff/100001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1046933005/diff/100001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:4100: GetBrowserPluginGuest()
|| GetBrowserPluginEmbedder();
On 2015/04/02 23:17:00, alexmos wrote:
> I really wanted to do the related SiteInstance check in
RFPH::RouteMessageEvent,
> but since we have to check for <webview> at the WebContents layer, I figured
> it’d be clearer to just leave the whole check here.

Actually, I think it would be better to move the related check to RFPH.  Then
it's clearer that we can remove this method as soon as we find a better way to
do the <webview> checks.

https://codereview.chromium.org/1046933005/diff/100001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1046933005/diff/100001/content/renderer/rende...
content/renderer/render_frame_impl.cc:3553: CHECK(render_frame_proxy_);
On 2015/04/02 23:17:00, alexmos wrote:
> Am I correct in assuming the proxy_ will always exist if the frame is swapped
> out?  There is a window in OnSwapOut between setting is_swapped_out and
calling
> set_render_frame_proxy, but I’m not sure whether it’s possible for postMessage
> to somehow happen in that window.  I could also route through the swapped-out
RF
> if that’s safer.

I think the only way it could happen is in a beforeunload handler.  (There's a
comment in OnSwapOut that says that the unload handler won't be run a second
time, but beforeunload will.)

I admit that it's nice not to have to process this message in
RenderFrameHostImpl, so sending it via render_frame_proxy_ would be a good
thing.  That said, it's probably worth testing the case of sending a postMessage
in beforeUnload while swapping out.

https://codereview.chromium.org/1046933005/diff/160001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1046933005/diff/160001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:1720: EXPECT_EQ(foo_url,
root->child_at(1)->current_url());
Let's also verify they're in different SiteInstances.

https://codereview.chromium.org/1046933005/diff/160001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1046933005/diff/160001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:4011: return
target_site_instance->IsRelatedSiteInstance(source_site_instance) ||
As in the earlier comment, let's try to put the related check in RFPH and just
do the guest check here.  Seems like that's the only part that needs to be here.

[alexmos, 2015-04-07 18:44:40]

https://codereview.chromium.org/1046933005/diff/100001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1046933005/diff/100001/content/renderer/rende...
content/renderer/render_frame_impl.cc:3553: CHECK(render_frame_proxy_);
On 2015/04/06 16:18:55, Charlie Reis wrote:
> On 2015/04/02 23:17:00, alexmos wrote:
> > Am I correct in assuming the proxy_ will always exist if the frame is
swapped
> > out?  There is a window in OnSwapOut between setting is_swapped_out and
> calling
> > set_render_frame_proxy, but I’m not sure whether it’s possible for
postMessage
> > to somehow happen in that window.  I could also route through the
swapped-out
> RF
> > if that’s safer.
> 
> I think the only way it could happen is in a beforeunload handler.  (There's a
> comment in OnSwapOut that says that the unload handler won't be run a second
> time, but beforeunload will.)
> 
> I admit that it's nice not to have to process this message in
> RenderFrameHostImpl, so sending it via render_frame_proxy_ would be a good
> thing.  That said, it's probably worth testing the case of sending a
postMessage
> in beforeUnload while swapping out.

Just to summarize what we discussed offline.  beforeunload should not be a
problem, because earlier in OnSwapOut, dispatchUnloadEvent() will clear all the
event handlers (as part of blink::Document::dispatchUnloadEvents), so while
NavigateToSwappedOutURL indeed dispatches beforeunload a second time, the event
handlers won't actually run.

However, I discovered a different problem, which is that calling OnStop() in
OnSwapOut can actually trigger onload event handlers to run.  An example is a
parent page which embeds a cross-site subframe, and the parent page has an
onload handler to send a postMessage to the subframe.  That onload handler gets
triggered deep inside OnStop (see stack at http://pastebin.com/gvqFGNRu), so it
needs the render_frame_proxy_ to already be set.  To fix this, I moved
OnSwapOut's call to set_render_frame_proxy() up to be done immediately after
is_swapped_out_ is set to true, and before OnStop.  This should be safe given
render_frame_proxy_'s current usage.  It also seemed safe to set the proxy
unconditionally, rather than for main frames only.

https://codereview.chromium.org/1046933005/diff/160001/content/browser/site_p...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1046933005/diff/160001/content/browser/site_p...
content/browser/site_per_process_browsertest.cc:1720: EXPECT_EQ(foo_url,
root->child_at(1)->current_url());
On 2015/04/06 16:18:56, Charlie Reis wrote:
> Let's also verify they're in different SiteInstances.

Done.

https://codereview.chromium.org/1046933005/diff/160001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1046933005/diff/160001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:4011: return
target_site_instance->IsRelatedSiteInstance(source_site_instance) ||
On 2015/04/06 16:18:56, Charlie Reis wrote:
> As in the earlier comment, let's try to put the related check in RFPH and just
> do the guest check here.  Seems like that's the only part that needs to be
here.

Done.

[Charlie Reis, 2015-04-07 21:14:32]

Great.  LGTM with nits.

https://codereview.chromium.org/1046933005/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_delegate.h (right):

https://codereview.chromium.org/1046933005/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_delegate.h:173: // |target_rfh|
from a source frame in the given SiteInstance.
Let's mention that this overrides the RFH's decision if true, and that it
defaults to false.

https://codereview.chromium.org/1046933005/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_delegate.h:178: // Ensure that
|render_frame_host| has swapped-out RenderViews and proxies
nit: source_rfh

https://codereview.chromium.org/1046933005/diff/180001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1046933005/diff/180001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:4013: RenderFrameHost*
render_frame_host) {
I think it might be worthwhile to keep "source_" in the name, to make this code
a little easier to follow.

https://codereview.chromium.org/1046933005/diff/180001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:4014: WebContentsImpl*
web_contents = NULL;
nit: source_web_contents
nit: nullptr

https://codereview.chromium.org/1046933005/diff/180001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:4015: if (render_frame_host) {
Looks like we don't expect this to be null.

https://codereview.chromium.org/1046933005/diff/180001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:4017:
static_cast<RenderFrameHostImpl*>(render_frame_host)
nit: We can remove at least one of these casts using
WebContents::FromRenderFrameHost instead.

https://codereview.chromium.org/1046933005/diff/180001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1046933005/diff/180001/content/common/frame_m...
content/common/frame_messages.h:212: // WebSerializedScriptValue in renderer.
nit: in the renderer process.

[alexmos, 2015-04-07 21:47:29]

alexmos@chromium.org changed reviewers:
+ kenrb@chromium.org

[alexmos, 2015-04-07 21:47:31]

Thanks!  Ken, do you mind doing an IPC owner review for this?

https://codereview.chromium.org/1046933005/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_delegate.h (right):

https://codereview.chromium.org/1046933005/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_delegate.h:173: // |target_rfh|
from a source frame in the given SiteInstance.
On 2015/04/07 21:14:32, Charlie Reis wrote:
> Let's mention that this overrides the RFH's decision if true, and that it
> defaults to false.

Done.

https://codereview.chromium.org/1046933005/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_delegate.h:178: // Ensure that
|render_frame_host| has swapped-out RenderViews and proxies
On 2015/04/07 21:14:32, Charlie Reis wrote:
> nit: source_rfh

Done.

https://codereview.chromium.org/1046933005/diff/180001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1046933005/diff/180001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:4013: RenderFrameHost*
render_frame_host) {
On 2015/04/07 21:14:32, Charlie Reis wrote:
> I think it might be worthwhile to keep "source_" in the name, to make this
code
> a little easier to follow.

Done.

https://codereview.chromium.org/1046933005/diff/180001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:4014: WebContentsImpl*
web_contents = NULL;
On 2015/04/07 21:14:32, Charlie Reis wrote:
> nit: source_web_contents
> nit: nullptr

Done.  nullptr is gone since the check below isn't actually needed.

https://codereview.chromium.org/1046933005/diff/180001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:4015: if (render_frame_host) {
On 2015/04/07 21:14:32, Charlie Reis wrote:
> Looks like we don't expect this to be null.

Done.

https://codereview.chromium.org/1046933005/diff/180001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:4017:
static_cast<RenderFrameHostImpl*>(render_frame_host)
On 2015/04/07 21:14:32, Charlie Reis wrote:
> nit: We can remove at least one of these casts using
> WebContents::FromRenderFrameHost instead.

Done.

https://codereview.chromium.org/1046933005/diff/180001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1046933005/diff/180001/content/common/frame_m...
content/common/frame_messages.h:212: // WebSerializedScriptValue in renderer.
On 2015/04/07 21:14:32, Charlie Reis wrote:
> nit: in the renderer process.

Done.

[kenrb, 2015-04-08 20:11:46]

ipc lgtm

[alexmos, 2015-04-08 20:41:10]

alexmos@chromium.org changed reviewers:
+ jam@chromium.org

[alexmos, 2015-04-08 20:41:10]

Thanks, TBR'ing jam@ for testing/buildbot/chromium.fyi.json (to re-enable the
SupportCrossProcessPostMessage test on site isolation FYI bots).

[alexmos, 2015-04-08 20:41:17]

The CQ bit was checked by alexmos@chromium.org

[alexmos, 2015-04-08 20:41:18]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1046933005/#ps200001
(title: "Charlie's nits")

[commit-bot: I haz the power, 2015-04-08 20:43:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1046933005/200001

[commit-bot: I haz the power, 2015-04-09 02:22:29]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-04-09 02:23:32]

Patchset 11 (id:??) landed as
https://crrev.com/e7da5a1508504fded712749adc154a69424afa9e
Cr-Commit-Position: refs/heads/master@{#324341}