If a RemoteFrame asks to navigate, send an OpenURL IPC

If a RemoteFrame asks to navigate, send an OpenURL IPC

This is the first step toward making remote frames able to be navigated by their parent correctly, and to enable them to transition back into the same process as the parent.

This, combined with https://codereview.chromium.org/573353002/, "just work" to route a navigation request from a RemoteFrame back to its LocalFrame. However, we incorrectly commit the load into the existing LocalFrame (rather than transitioning processes again) and we assert during that commit.

BUG=

Committed: https://crrev.com/70ea134b950a3d5f016a05d6ea3fd336b2ba59f9
Cr-Commit-Position: refs/heads/master@{#297513}

[Nate Chapin, 2014-09-17 19:32:19]

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

[Nate Chapin, 2014-09-17 19:32:20]

This patch and https://codereview.chromium.org/573353002/ can land in either
order.

[Charlie Reis, 2014-09-17 21:31:41]

Yes, this seems like the right start once you settle on the navigate API in the
Blink CL.  It seems like this IPC wouldn't be handled by anyone at the moment,
though, so I'm surprised by the behavior you're describing.

https://codereview.chromium.org/574403002/diff/1/content/renderer/render_fram...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/574403002/diff/1/content/renderer/render_fram...
content/renderer/render_frame_proxy.cc:306: Send(new
FrameHostMsg_OpenURL(routing_id_, params));
I'm surprised this has an effect in the browser process.  Doesn't it get
delivered to RenderFrameProxyHost::OnMessageReceived when render_frame_host_ is
null?  (RenderFrameProxyHosts only have render_frame_host_'s for main frames.)

[Charlie Reis, 2014-09-17 21:32:55]

[+site-isolation-reviews@]

[Nate Chapin, 2014-09-17 21:54:41]

On 2014/09/17 21:31:41, Charlie Reis wrote:
> Yes, this seems like the right start once you settle on the navigate API in
the
> Blink CL.  It seems like this IPC wouldn't be handled by anyone at the moment,
> though, so I'm surprised by the behavior you're describing.
> 
>
https://codereview.chromium.org/574403002/diff/1/content/renderer/render_fram...
> File content/renderer/render_frame_proxy.cc (right):
> 
>
https://codereview.chromium.org/574403002/diff/1/content/renderer/render_fram...
> content/renderer/render_frame_proxy.cc:306: Send(new
> FrameHostMsg_OpenURL(routing_id_, params));
> I'm surprised this has an effect in the browser process.  Doesn't it get
> delivered to RenderFrameProxyHost::OnMessageReceived when render_frame_host_
is
> null?  (RenderFrameProxyHosts only have render_frame_host_'s for main frames.)

I haven't studied the browser process code well enough to know how it's being
routed, but I've verified that the navigation ends up happening, and does so in
a LocalFrame that claims to be a subframe and has a RemoteFrame for its main
frame.

[Nate Chapin, 2014-09-18 18:05:59]

This is the stack trace that the FrameHostMsg_OpenURL follows to get to a
FrameMsg_Navigate in the subframe is:
#2 0x7fdd22a8dca9 content::RenderFrameHostImpl::Navigate()
#3 0x7fdd22a83c05 content::NavigatorImpl::NavigateToEntry()
#4 0x7fdd22a83fde content::NavigatorImpl::NavigateToPendingEntry()
#5 0x7fdd2316150b content::WebContentsImpl::NavigateToPendingEntry()
#6 0x7fdd23161542 content::WebContentsImpl::NavigateToPendingEntry()
#7 0x7fdd22a68895 content::NavigationControllerImpl::NavigateToPendingEntry()
#8 0x7fdd22a68cba content::NavigationControllerImpl::LoadEntry()
#9 0x7fdd22a6a7a4 content::NavigationControllerImpl::LoadURLWithParams()
#10 0x7fdd34366934 (anonymous namespace)::LoadURLInContents()
#11 0x7fdd343656a1 chrome::Navigate()
#12 0x7fdd3453016d Browser::OpenURLFromTab()
#13 0x7fdd345301d7 Browser::OpenURLFromTab()
#14 0x7fdd231612d6 content::WebContentsImpl::OpenURL()
#15 0x7fdd2316491f content::WebContentsImpl::RequestOpenURL()
#16 0x7fdd23164a37 content::WebContentsImpl::RequestOpenURL()
#17 0x7fdd22a84ce2 content::NavigatorImpl::RequestTransferURL()
#18 0x7fdd22a848ba content::NavigatorImpl::RequestOpenURL()
#19 0x7fdd22a89f6f content::RenderFrameHostImpl::OnOpenURL()
#20 0x7fdd22a9bd97 DispatchToMethod<>()
#21 0x7fdd22a90b3f FrameHostMsg_OpenURL::Dispatch<>()
#22 0x7fdd22a8854c content::RenderFrameHostImpl::OnMessageReceived()
#23 0x7fdd22abc45e content::RenderFrameProxyHost::OnMessageReceived()
#24 0x7fdd22ebc0be content::RenderProcessHostImpl::OnMessageReceived()
#25 0x7fdd22ebc4cf content::RenderProcessHostImpl::OnMessageReceived()
#26 0x7fdd2bfd3fbd IPC::ChannelProxy::Context::OnDispatchMessage()

[Charlie Reis, 2014-09-18 18:09:05]

On 2014/09/18 18:05:59, Nate Chapin wrote:
> This is the stack trace that the FrameHostMsg_OpenURL follows to get to a
> FrameMsg_Navigate in the subframe is:
> #2 0x7fdd22a8dca9 content::RenderFrameHostImpl::Navigate()
> #3 0x7fdd22a83c05 content::NavigatorImpl::NavigateToEntry()
> #4 0x7fdd22a83fde content::NavigatorImpl::NavigateToPendingEntry()
> #5 0x7fdd2316150b content::WebContentsImpl::NavigateToPendingEntry()
> #6 0x7fdd23161542 content::WebContentsImpl::NavigateToPendingEntry()
> #7 0x7fdd22a68895 content::NavigationControllerImpl::NavigateToPendingEntry()
> #8 0x7fdd22a68cba content::NavigationControllerImpl::LoadEntry()
> #9 0x7fdd22a6a7a4 content::NavigationControllerImpl::LoadURLWithParams()
> #10 0x7fdd34366934 (anonymous namespace)::LoadURLInContents()
> #11 0x7fdd343656a1 chrome::Navigate()
> #12 0x7fdd3453016d Browser::OpenURLFromTab()
> #13 0x7fdd345301d7 Browser::OpenURLFromTab()
> #14 0x7fdd231612d6 content::WebContentsImpl::OpenURL()
> #15 0x7fdd2316491f content::WebContentsImpl::RequestOpenURL()
> #16 0x7fdd23164a37 content::WebContentsImpl::RequestOpenURL()
> #17 0x7fdd22a84ce2 content::NavigatorImpl::RequestTransferURL()
> #18 0x7fdd22a848ba content::NavigatorImpl::RequestOpenURL()
> #19 0x7fdd22a89f6f content::RenderFrameHostImpl::OnOpenURL()
> #20 0x7fdd22a9bd97 DispatchToMethod<>()
> #21 0x7fdd22a90b3f FrameHostMsg_OpenURL::Dispatch<>()
> #22 0x7fdd22a8854c content::RenderFrameHostImpl::OnMessageReceived()
> #23 0x7fdd22abc45e content::RenderFrameProxyHost::OnMessageReceived()
> #24 0x7fdd22ebc0be content::RenderProcessHostImpl::OnMessageReceived()
> #25 0x7fdd22ebc4cf content::RenderProcessHostImpl::OnMessageReceived()
> #26 0x7fdd2bfd3fbd IPC::ChannelProxy::Context::OnDispatchMessage()

On 2014/09/17 21:54:41, Nate Chapin wrote:
> On 2014/09/17 21:31:41, Charlie Reis wrote:
> > Yes, this seems like the right start once you settle on the navigate API in
> the
> > Blink CL.  It seems like this IPC wouldn't be handled by anyone at the
moment,
> > though, so I'm surprised by the behavior you're describing.
> > 
> >
>
https://codereview.chromium.org/574403002/diff/1/content/renderer/render_fram...
> > File content/renderer/render_frame_proxy.cc (right):
> > 
> >
>
https://codereview.chromium.org/574403002/diff/1/content/renderer/render_fram...
> > content/renderer/render_frame_proxy.cc:306: Send(new
> > FrameHostMsg_OpenURL(routing_id_, params));
> > I'm surprised this has an effect in the browser process.  Doesn't it get
> > delivered to RenderFrameProxyHost::OnMessageReceived when render_frame_host_
> is
> > null?  (RenderFrameProxyHosts only have render_frame_host_'s for main
frames.)
> 
> I haven't studied the browser process code well enough to know how it's being
> routed, but I've verified that the navigation ends up happening, and does so
in
> a LocalFrame that claims to be a subframe and has a RemoteFrame for its main
> frame.

Oh, I get it.  (I had to try it out locally to understand.)  With just your
patches the OpenURL call doesn't go anywhere, but it "works" with my
TakeOwnership hack in RFHM::CommitPending.  That's because my hack makes us keep
the RFH around longer than we would in practice.

This is a fine start, but we'll need to add a handler for OpenURL to
RenderFrameProxyHost to make it work in practice.  Did you just want to get the
initial support in and then make it work with a test later?  If so, LGTM.

[Nate Chapin, 2014-09-18 18:11:44]

On 2014/09/18 18:09:05, Charlie Reis wrote:
> On 2014/09/18 18:05:59, Nate Chapin wrote:
> > This is the stack trace that the FrameHostMsg_OpenURL follows to get to a
> > FrameMsg_Navigate in the subframe is:
> > #2 0x7fdd22a8dca9 content::RenderFrameHostImpl::Navigate()
> > #3 0x7fdd22a83c05 content::NavigatorImpl::NavigateToEntry()
> > #4 0x7fdd22a83fde content::NavigatorImpl::NavigateToPendingEntry()
> > #5 0x7fdd2316150b content::WebContentsImpl::NavigateToPendingEntry()
> > #6 0x7fdd23161542 content::WebContentsImpl::NavigateToPendingEntry()
> > #7 0x7fdd22a68895
content::NavigationControllerImpl::NavigateToPendingEntry()
> > #8 0x7fdd22a68cba content::NavigationControllerImpl::LoadEntry()
> > #9 0x7fdd22a6a7a4 content::NavigationControllerImpl::LoadURLWithParams()
> > #10 0x7fdd34366934 (anonymous namespace)::LoadURLInContents()
> > #11 0x7fdd343656a1 chrome::Navigate()
> > #12 0x7fdd3453016d Browser::OpenURLFromTab()
> > #13 0x7fdd345301d7 Browser::OpenURLFromTab()
> > #14 0x7fdd231612d6 content::WebContentsImpl::OpenURL()
> > #15 0x7fdd2316491f content::WebContentsImpl::RequestOpenURL()
> > #16 0x7fdd23164a37 content::WebContentsImpl::RequestOpenURL()
> > #17 0x7fdd22a84ce2 content::NavigatorImpl::RequestTransferURL()
> > #18 0x7fdd22a848ba content::NavigatorImpl::RequestOpenURL()
> > #19 0x7fdd22a89f6f content::RenderFrameHostImpl::OnOpenURL()
> > #20 0x7fdd22a9bd97 DispatchToMethod<>()
> > #21 0x7fdd22a90b3f FrameHostMsg_OpenURL::Dispatch<>()
> > #22 0x7fdd22a8854c content::RenderFrameHostImpl::OnMessageReceived()
> > #23 0x7fdd22abc45e content::RenderFrameProxyHost::OnMessageReceived()
> > #24 0x7fdd22ebc0be content::RenderProcessHostImpl::OnMessageReceived()
> > #25 0x7fdd22ebc4cf content::RenderProcessHostImpl::OnMessageReceived()
> > #26 0x7fdd2bfd3fbd IPC::ChannelProxy::Context::OnDispatchMessage()
> 
> On 2014/09/17 21:54:41, Nate Chapin wrote:
> > On 2014/09/17 21:31:41, Charlie Reis wrote:
> > > Yes, this seems like the right start once you settle on the navigate API
in
> > the
> > > Blink CL.  It seems like this IPC wouldn't be handled by anyone at the
> moment,
> > > though, so I'm surprised by the behavior you're describing.
> > > 
> > >
> >
>
https://codereview.chromium.org/574403002/diff/1/content/renderer/render_fram...
> > > File content/renderer/render_frame_proxy.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/574403002/diff/1/content/renderer/render_fram...
> > > content/renderer/render_frame_proxy.cc:306: Send(new
> > > FrameHostMsg_OpenURL(routing_id_, params));
> > > I'm surprised this has an effect in the browser process.  Doesn't it get
> > > delivered to RenderFrameProxyHost::OnMessageReceived when
render_frame_host_
> > is
> > > null?  (RenderFrameProxyHosts only have render_frame_host_'s for main
> frames.)
> > 
> > I haven't studied the browser process code well enough to know how it's
being
> > routed, but I've verified that the navigation ends up happening, and does so
> in
> > a LocalFrame that claims to be a subframe and has a RemoteFrame for its main
> > frame.
> 
> Oh, I get it.  (I had to try it out locally to understand.)  With just your
> patches the OpenURL call doesn't go anywhere, but it "works" with my
> TakeOwnership hack in RFHM::CommitPending.  That's because my hack makes us
keep
> the RFH around longer than we would in practice.
> 
> This is a fine start, but we'll need to add a handler for OpenURL to
> RenderFrameProxyHost to make it work in practice.  Did you just want to get
the
> initial support in and then make it work with a test later?  If so, LGTM.

I'm flexible. I'm assuming that other folks are better equipped to get the
browser process code right, but I'm willing to take a stab at it if you think
that makes sense.

[Nate Chapin, 2014-09-18 18:27:58]

On 2014/09/18 18:11:44, Nate Chapin wrote:
> On 2014/09/18 18:09:05, Charlie Reis wrote:
> > On 2014/09/18 18:05:59, Nate Chapin wrote:
> > > This is the stack trace that the FrameHostMsg_OpenURL follows to get to a
> > > FrameMsg_Navigate in the subframe is:
> > > #2 0x7fdd22a8dca9 content::RenderFrameHostImpl::Navigate()
> > > #3 0x7fdd22a83c05 content::NavigatorImpl::NavigateToEntry()
> > > #4 0x7fdd22a83fde content::NavigatorImpl::NavigateToPendingEntry()
> > > #5 0x7fdd2316150b content::WebContentsImpl::NavigateToPendingEntry()
> > > #6 0x7fdd23161542 content::WebContentsImpl::NavigateToPendingEntry()
> > > #7 0x7fdd22a68895
> content::NavigationControllerImpl::NavigateToPendingEntry()
> > > #8 0x7fdd22a68cba content::NavigationControllerImpl::LoadEntry()
> > > #9 0x7fdd22a6a7a4 content::NavigationControllerImpl::LoadURLWithParams()
> > > #10 0x7fdd34366934 (anonymous namespace)::LoadURLInContents()
> > > #11 0x7fdd343656a1 chrome::Navigate()
> > > #12 0x7fdd3453016d Browser::OpenURLFromTab()
> > > #13 0x7fdd345301d7 Browser::OpenURLFromTab()
> > > #14 0x7fdd231612d6 content::WebContentsImpl::OpenURL()
> > > #15 0x7fdd2316491f content::WebContentsImpl::RequestOpenURL()
> > > #16 0x7fdd23164a37 content::WebContentsImpl::RequestOpenURL()
> > > #17 0x7fdd22a84ce2 content::NavigatorImpl::RequestTransferURL()
> > > #18 0x7fdd22a848ba content::NavigatorImpl::RequestOpenURL()
> > > #19 0x7fdd22a89f6f content::RenderFrameHostImpl::OnOpenURL()
> > > #20 0x7fdd22a9bd97 DispatchToMethod<>()
> > > #21 0x7fdd22a90b3f FrameHostMsg_OpenURL::Dispatch<>()
> > > #22 0x7fdd22a8854c content::RenderFrameHostImpl::OnMessageReceived()
> > > #23 0x7fdd22abc45e content::RenderFrameProxyHost::OnMessageReceived()
> > > #24 0x7fdd22ebc0be content::RenderProcessHostImpl::OnMessageReceived()
> > > #25 0x7fdd22ebc4cf content::RenderProcessHostImpl::OnMessageReceived()
> > > #26 0x7fdd2bfd3fbd IPC::ChannelProxy::Context::OnDispatchMessage()
> > 
> > On 2014/09/17 21:54:41, Nate Chapin wrote:
> > > On 2014/09/17 21:31:41, Charlie Reis wrote:
> > > > Yes, this seems like the right start once you settle on the navigate API
> in
> > > the
> > > > Blink CL.  It seems like this IPC wouldn't be handled by anyone at the
> > moment,
> > > > though, so I'm surprised by the behavior you're describing.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/574403002/diff/1/content/renderer/render_fram...
> > > > File content/renderer/render_frame_proxy.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/574403002/diff/1/content/renderer/render_fram...
> > > > content/renderer/render_frame_proxy.cc:306: Send(new
> > > > FrameHostMsg_OpenURL(routing_id_, params));
> > > > I'm surprised this has an effect in the browser process.  Doesn't it get
> > > > delivered to RenderFrameProxyHost::OnMessageReceived when
> render_frame_host_
> > > is
> > > > null?  (RenderFrameProxyHosts only have render_frame_host_'s for main
> > frames.)
> > > 
> > > I haven't studied the browser process code well enough to know how it's
> being
> > > routed, but I've verified that the navigation ends up happening, and does
so
> > in
> > > a LocalFrame that claims to be a subframe and has a RemoteFrame for its
main
> > > frame.
> > 
> > Oh, I get it.  (I had to try it out locally to understand.)  With just your
> > patches the OpenURL call doesn't go anywhere, but it "works" with my
> > TakeOwnership hack in RFHM::CommitPending.  That's because my hack makes us
> keep
> > the RFH around longer than we would in practice.
> > 
> > This is a fine start, but we'll need to add a handler for OpenURL to
> > RenderFrameProxyHost to make it work in practice.  Did you just want to get
> the
> > initial support in and then make it work with a test later?  If so, LGTM.
> 
> I'm flexible. I'm assuming that other folks are better equipped to get the
> browser process code right, but I'm willing to take a stab at it if you think
> that makes sense.

It looks like this is as simple as RenderFrameProxyHost listening to
FrameHostMsg_OpenURL, converting the params to FrameMsg_Navigate_Params, and
calling Navigate() in the corresponding RenderFrameHostImpl.

Am I missing something? And how does one test this?

[Charlie Reis, 2014-09-18 18:36:59]

On 2014/09/18 18:27:58, Nate Chapin wrote:
> On 2014/09/18 18:11:44, Nate Chapin wrote:
> > On 2014/09/18 18:09:05, Charlie Reis wrote:
> > > On 2014/09/18 18:05:59, Nate Chapin wrote:
> > > > This is the stack trace that the FrameHostMsg_OpenURL follows to get to
a
> > > > FrameMsg_Navigate in the subframe is:
> > > > #2 0x7fdd22a8dca9 content::RenderFrameHostImpl::Navigate()
> > > > #3 0x7fdd22a83c05 content::NavigatorImpl::NavigateToEntry()
> > > > #4 0x7fdd22a83fde content::NavigatorImpl::NavigateToPendingEntry()
> > > > #5 0x7fdd2316150b content::WebContentsImpl::NavigateToPendingEntry()
> > > > #6 0x7fdd23161542 content::WebContentsImpl::NavigateToPendingEntry()
> > > > #7 0x7fdd22a68895
> > content::NavigationControllerImpl::NavigateToPendingEntry()
> > > > #8 0x7fdd22a68cba content::NavigationControllerImpl::LoadEntry()
> > > > #9 0x7fdd22a6a7a4 content::NavigationControllerImpl::LoadURLWithParams()
> > > > #10 0x7fdd34366934 (anonymous namespace)::LoadURLInContents()
> > > > #11 0x7fdd343656a1 chrome::Navigate()
> > > > #12 0x7fdd3453016d Browser::OpenURLFromTab()
> > > > #13 0x7fdd345301d7 Browser::OpenURLFromTab()
> > > > #14 0x7fdd231612d6 content::WebContentsImpl::OpenURL()
> > > > #15 0x7fdd2316491f content::WebContentsImpl::RequestOpenURL()
> > > > #16 0x7fdd23164a37 content::WebContentsImpl::RequestOpenURL()
> > > > #17 0x7fdd22a84ce2 content::NavigatorImpl::RequestTransferURL()
> > > > #18 0x7fdd22a848ba content::NavigatorImpl::RequestOpenURL()
> > > > #19 0x7fdd22a89f6f content::RenderFrameHostImpl::OnOpenURL()
> > > > #20 0x7fdd22a9bd97 DispatchToMethod<>()
> > > > #21 0x7fdd22a90b3f FrameHostMsg_OpenURL::Dispatch<>()
> > > > #22 0x7fdd22a8854c content::RenderFrameHostImpl::OnMessageReceived()
> > > > #23 0x7fdd22abc45e content::RenderFrameProxyHost::OnMessageReceived()
> > > > #24 0x7fdd22ebc0be content::RenderProcessHostImpl::OnMessageReceived()
> > > > #25 0x7fdd22ebc4cf content::RenderProcessHostImpl::OnMessageReceived()
> > > > #26 0x7fdd2bfd3fbd IPC::ChannelProxy::Context::OnDispatchMessage()
> > > 
> > > On 2014/09/17 21:54:41, Nate Chapin wrote:
> > > > On 2014/09/17 21:31:41, Charlie Reis wrote:
> > > > > Yes, this seems like the right start once you settle on the navigate
API
> > in
> > > > the
> > > > > Blink CL.  It seems like this IPC wouldn't be handled by anyone at the
> > > moment,
> > > > > though, so I'm surprised by the behavior you're describing.
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/574403002/diff/1/content/renderer/render_fram...
> > > > > File content/renderer/render_frame_proxy.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/574403002/diff/1/content/renderer/render_fram...
> > > > > content/renderer/render_frame_proxy.cc:306: Send(new
> > > > > FrameHostMsg_OpenURL(routing_id_, params));
> > > > > I'm surprised this has an effect in the browser process.  Doesn't it
get
> > > > > delivered to RenderFrameProxyHost::OnMessageReceived when
> > render_frame_host_
> > > > is
> > > > > null?  (RenderFrameProxyHosts only have render_frame_host_'s for main
> > > frames.)
> > > > 
> > > > I haven't studied the browser process code well enough to know how it's
> > being
> > > > routed, but I've verified that the navigation ends up happening, and
does
> so
> > > in
> > > > a LocalFrame that claims to be a subframe and has a RemoteFrame for its
> main
> > > > frame.
> > > 
> > > Oh, I get it.  (I had to try it out locally to understand.)  With just
your
> > > patches the OpenURL call doesn't go anywhere, but it "works" with my
> > > TakeOwnership hack in RFHM::CommitPending.  That's because my hack makes
us
> > keep
> > > the RFH around longer than we would in practice.
> > > 
> > > This is a fine start, but we'll need to add a handler for OpenURL to
> > > RenderFrameProxyHost to make it work in practice.  Did you just want to
get
> > the
> > > initial support in and then make it work with a test later?  If so, LGTM.
> > 
> > I'm flexible. I'm assuming that other folks are better equipped to get the
> > browser process code right, but I'm willing to take a stab at it if you
think
> > that makes sense.
> 
> It looks like this is as simple as RenderFrameProxyHost listening to
> FrameHostMsg_OpenURL, converting the params to FrameMsg_Navigate_Params, and
> calling Navigate() in the corresponding RenderFrameHostImpl.
> 
> Am I missing something? And how does one test this?

No, that's cutting out some steps.  The new URL may require a cross-site
navigation, so it might not be destined for the current RenderFrameHostImpl.

I think we want RFPH::OnOpenURL to pass it on to the OnOpenURL method of the
current RenderFrameHostImpl, which will then do a process swap if needed.  (For
example, suppose that a main frame on site A had a subframe on site B, then A
told the subframe to go to site C.)  This will require finding the current RFH
(via frame_tree_node()->current_frame_host()) and making its OnOpenURL call not
be private.

I would suggest testing it with a new test modeled on
SitePerProcessBrowserTest.CrossSiteIframe.  We can rip out most of the checks
and test the case above (A sends the B frame to C).

[Nate Chapin, 2014-09-18 20:04:23]

On 2014/09/18 18:36:59, Charlie Reis wrote:
> On 2014/09/18 18:27:58, Nate Chapin wrote:
> > On 2014/09/18 18:11:44, Nate Chapin wrote:
> > > On 2014/09/18 18:09:05, Charlie Reis wrote:
> > > > On 2014/09/18 18:05:59, Nate Chapin wrote:
> > > > > This is the stack trace that the FrameHostMsg_OpenURL follows to get
to
> a
> > > > > FrameMsg_Navigate in the subframe is:
> > > > > #2 0x7fdd22a8dca9 content::RenderFrameHostImpl::Navigate()
> > > > > #3 0x7fdd22a83c05 content::NavigatorImpl::NavigateToEntry()
> > > > > #4 0x7fdd22a83fde content::NavigatorImpl::NavigateToPendingEntry()
> > > > > #5 0x7fdd2316150b content::WebContentsImpl::NavigateToPendingEntry()
> > > > > #6 0x7fdd23161542 content::WebContentsImpl::NavigateToPendingEntry()
> > > > > #7 0x7fdd22a68895
> > > content::NavigationControllerImpl::NavigateToPendingEntry()
> > > > > #8 0x7fdd22a68cba content::NavigationControllerImpl::LoadEntry()
> > > > > #9 0x7fdd22a6a7a4
content::NavigationControllerImpl::LoadURLWithParams()
> > > > > #10 0x7fdd34366934 (anonymous namespace)::LoadURLInContents()
> > > > > #11 0x7fdd343656a1 chrome::Navigate()
> > > > > #12 0x7fdd3453016d Browser::OpenURLFromTab()
> > > > > #13 0x7fdd345301d7 Browser::OpenURLFromTab()
> > > > > #14 0x7fdd231612d6 content::WebContentsImpl::OpenURL()
> > > > > #15 0x7fdd2316491f content::WebContentsImpl::RequestOpenURL()
> > > > > #16 0x7fdd23164a37 content::WebContentsImpl::RequestOpenURL()
> > > > > #17 0x7fdd22a84ce2 content::NavigatorImpl::RequestTransferURL()
> > > > > #18 0x7fdd22a848ba content::NavigatorImpl::RequestOpenURL()
> > > > > #19 0x7fdd22a89f6f content::RenderFrameHostImpl::OnOpenURL()
> > > > > #20 0x7fdd22a9bd97 DispatchToMethod<>()
> > > > > #21 0x7fdd22a90b3f FrameHostMsg_OpenURL::Dispatch<>()
> > > > > #22 0x7fdd22a8854c content::RenderFrameHostImpl::OnMessageReceived()
> > > > > #23 0x7fdd22abc45e content::RenderFrameProxyHost::OnMessageReceived()
> > > > > #24 0x7fdd22ebc0be content::RenderProcessHostImpl::OnMessageReceived()
> > > > > #25 0x7fdd22ebc4cf content::RenderProcessHostImpl::OnMessageReceived()
> > > > > #26 0x7fdd2bfd3fbd IPC::ChannelProxy::Context::OnDispatchMessage()
> > > > 
> > > > On 2014/09/17 21:54:41, Nate Chapin wrote:
> > > > > On 2014/09/17 21:31:41, Charlie Reis wrote:
> > > > > > Yes, this seems like the right start once you settle on the navigate
> API
> > > in
> > > > > the
> > > > > > Blink CL.  It seems like this IPC wouldn't be handled by anyone at
the
> > > > moment,
> > > > > > though, so I'm surprised by the behavior you're describing.
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/574403002/diff/1/content/renderer/render_fram...
> > > > > > File content/renderer/render_frame_proxy.cc (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/574403002/diff/1/content/renderer/render_fram...
> > > > > > content/renderer/render_frame_proxy.cc:306: Send(new
> > > > > > FrameHostMsg_OpenURL(routing_id_, params));
> > > > > > I'm surprised this has an effect in the browser process.  Doesn't it
> get
> > > > > > delivered to RenderFrameProxyHost::OnMessageReceived when
> > > render_frame_host_
> > > > > is
> > > > > > null?  (RenderFrameProxyHosts only have render_frame_host_'s for
main
> > > > frames.)
> > > > > 
> > > > > I haven't studied the browser process code well enough to know how
it's
> > > being
> > > > > routed, but I've verified that the navigation ends up happening, and
> does
> > so
> > > > in
> > > > > a LocalFrame that claims to be a subframe and has a RemoteFrame for
its
> > main
> > > > > frame.
> > > > 
> > > > Oh, I get it.  (I had to try it out locally to understand.)  With just
> your
> > > > patches the OpenURL call doesn't go anywhere, but it "works" with my
> > > > TakeOwnership hack in RFHM::CommitPending.  That's because my hack makes
> us
> > > keep
> > > > the RFH around longer than we would in practice.
> > > > 
> > > > This is a fine start, but we'll need to add a handler for OpenURL to
> > > > RenderFrameProxyHost to make it work in practice.  Did you just want to
> get
> > > the
> > > > initial support in and then make it work with a test later?  If so,
LGTM.
> > > 
> > > I'm flexible. I'm assuming that other folks are better equipped to get the
> > > browser process code right, but I'm willing to take a stab at it if you
> think
> > > that makes sense.
> > 
> > It looks like this is as simple as RenderFrameProxyHost listening to
> > FrameHostMsg_OpenURL, converting the params to FrameMsg_Navigate_Params, and
> > calling Navigate() in the corresponding RenderFrameHostImpl.
> > 
> > Am I missing something? And how does one test this?
> 
> No, that's cutting out some steps.  The new URL may require a cross-site
> navigation, so it might not be destined for the current RenderFrameHostImpl.
> 
> I think we want RFPH::OnOpenURL to pass it on to the OnOpenURL method of the
> current RenderFrameHostImpl, which will then do a process swap if needed. 
(For
> example, suppose that a main frame on site A had a subframe on site B, then A
> told the subframe to go to site C.)  This will require finding the current RFH
> (via frame_tree_node()->current_frame_host()) and making its OnOpenURL call
not
> be private.
> 
> I would suggest testing it with a new test modeled on
> SitePerProcessBrowserTest.CrossSiteIframe.  We can rip out most of the checks
> and test the case above (A sends the B frame to C).

Test added, so this is now actually blocked on
https://codereview.chromium.org/573353002/

[Charlie Reis, 2014-09-18 20:22:28]

Looks great!  Just a few nits and some notes on the test.

https://codereview.chromium.org/574403002/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/574403002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.h:206: // a remote frmae.
nit: remote frmae -> RemoteFrame.

https://codereview.chromium.org/574403002/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/574403002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_proxy_host.cc:103: bool handled = true;
Let's move this new block to the end of the method.  We usually defer to other
potential handlers first, and then process the message.

(I realize this means render_frame_host_ will intercept it when the
CommitPending hack is in place, but it should work properly without that hack.)

https://codereview.chromium.org/574403002/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_proxy_host.h (right):

https://codereview.chromium.org/574403002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_proxy_host.h:112: void OnOpenURL(const
FrameHostMsg_OpenURL_Params& params);
Add:
// IPC Message handlers.

https://codereview.chromium.org/574403002/diff/20001/content/browser/site_per...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/574403002/diff/20001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:321: RenderFrameProxyHost*
proxy_to_parent =
We don't need the proxy_to_parent checks in this test.

https://codereview.chromium.org/574403002/diff/20001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:344:
EXPECT_NE(shell()->web_contents()->GetSiteInstance(), site_instance);
Let's keep this site_instance check (as a sanity check) and drop the
rvh/rph/proxy_to_parent checks, since other tests cover those.

https://codereview.chromium.org/574403002/diff/20001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:354: // navigates crosssite.
nit: cross-site

https://codereview.chromium.org/574403002/diff/20001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:373:
child->current_frame_host()->GetSiteInstance());
Again, these two site_instance checks are probably sufficient.

https://codereview.chromium.org/574403002/diff/20001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:382: }
Can we add a navigation back to the original site after this?  Or does that
break currently?  (If it doesn't work, let's put a TODO to add that here.)

Then the test would nicely cover A -> B -> C -> A.

[Nate Chapin, 2014-09-18 22:39:02]

https://codereview.chromium.org/574403002/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/574403002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.h:206: // a remote frmae.
On 2014/09/18 20:22:27, Charlie Reis wrote:
> nit: remote frmae -> RemoteFrame.

Done.

https://codereview.chromium.org/574403002/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/574403002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_proxy_host.cc:103: bool handled = true;
On 2014/09/18 20:22:27, Charlie Reis wrote:
> Let's move this new block to the end of the method.  We usually defer to other
> potential handlers first, and then process the message.
> 
> (I realize this means render_frame_host_ will intercept it when the
> CommitPending hack is in place, but it should work properly without that
hack.)

Done.

https://codereview.chromium.org/574403002/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_proxy_host.h (right):

https://codereview.chromium.org/574403002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_proxy_host.h:112: void OnOpenURL(const
FrameHostMsg_OpenURL_Params& params);
On 2014/09/18 20:22:27, Charlie Reis wrote:
> Add:
> // IPC Message handlers.

Done.

https://codereview.chromium.org/574403002/diff/20001/content/browser/site_per...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/574403002/diff/20001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:321: RenderFrameProxyHost*
proxy_to_parent =
On 2014/09/18 20:22:28, Charlie Reis wrote:
> We don't need the proxy_to_parent checks in this test.

Done.

https://codereview.chromium.org/574403002/diff/20001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:344:
EXPECT_NE(shell()->web_contents()->GetSiteInstance(), site_instance);
On 2014/09/18 20:22:28, Charlie Reis wrote:
> Let's keep this site_instance check (as a sanity check) and drop the
> rvh/rph/proxy_to_parent checks, since other tests cover those.

Done.

https://codereview.chromium.org/574403002/diff/20001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:354: // navigates crosssite.
On 2014/09/18 20:22:28, Charlie Reis wrote:
> nit: cross-site

Done.

https://codereview.chromium.org/574403002/diff/20001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:373:
child->current_frame_host()->GetSiteInstance());
On 2014/09/18 20:22:28, Charlie Reis wrote:
> Again, these two site_instance checks are probably sufficient.

Done.

[Charlie Reis, 2014-09-18 22:46:56]

LGTM.  Thanks for the test!

[Nate Chapin, 2014-09-29 16:21:32]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2014-09-29 16:22:37]

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

[commit-bot: I haz the power, 2014-09-29 16:25:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-29 16:25:41]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/61898)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Nate Chapin, 2014-09-29 17:06:45]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2014-09-29 17:08:01]

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

[Nate Chapin, 2014-09-29 17:43:38]

The CQ bit was unchecked by japhet@chromium.org

[Nate Chapin, 2014-09-29 17:46:16]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2014-09-29 17:46:45]

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

[commit-bot: I haz the power, 2014-09-29 18:54:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-29 18:54:08]

Try jobs failed on following builders:
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Nate Chapin, 2014-09-30 17:12:44]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2014-09-30 17:13:21]

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

[commit-bot: I haz the power, 2014-09-30 19:06:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-30 19:06:17]

Try jobs failed on following builders:
  win_chromium_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Nate Chapin, 2014-09-30 19:18:32]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2014-09-30 19:19:38]

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

[Nate Chapin, 2014-09-30 20:53:44]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2014-09-30 20:54:41]

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

[commit-bot: I haz the power, 2014-09-30 21:38:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-30 21:38:04]

Try jobs failed on following builders:
  linux_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu_trigger...)

[Nate Chapin, 2014-09-30 21:41:01]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2014-09-30 21:42:16]

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

[commit-bot: I haz the power, 2014-09-30 21:56:49]

Committed patchset #6 (id:100001) as b48649be1fa063e51aaae018b284fad3fd8356c8

[commit-bot: I haz the power, 2014-09-30 21:57:48]

Patchset 6 (id:??) landed as
https://crrev.com/70ea134b950a3d5f016a05d6ea3fd336b2ba59f9
Cr-Commit-Position: refs/heads/master@{#297513}