Set up Mojo connection when RenderFrameHost is reused for new RenderFrame

Set up Mojo connection when RenderFrameHost is reused for new RenderFrame

After a render process dies, the main RenderFrameHostImpl can be reused for a
new RenderFrame in a new render process. In this case, the Mojo connection
between the RFHI and the new RenderFrame was not being set up. This CL fixes
the glitch and additionally future-proofs for the case where sub-frames can be
reused as well:

- When the FrameTree resets process state, it instructs the RFHI's affected
  to invalidate their Mojo connections.
- When the RenderFrameHostManager creates a new RenderFrame for an RFHI,
  it instructs that RFHI to set up the Mojo connection if necessary.

BUG=421069
TEST=Navigate to about://omnibox. Kill the tab via the Task Manager and reload.
Observe that the page produces output upon submission of input.

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

[blundell, 2014-10-22 10:44:47]

blundell@chromium.org changed reviewers:
+ creis@chromium.org, sammc@chromium.org

[blundell, 2014-10-22 10:44:48]

Hi,

This CL as it is feels essentially like a hack. What I'd like to find is the
following: a point internal to RenderFrameHostImpl wherein it knows that a new
RenderFrame is being created for it to be associated with. This CL, while it
works, is unappealing for two reasons:
(1) The call to set up the new Mojo connection is made externally to RFHI.
(2) The hook point that I found in RenderFrameHostManager gets executed more
than once during the lifetime of a single RenderFrame, resulting in the need for
short-circuit logic in RFHI::SetUpMojo().

Charlie, could you advise on a better way to accomplish the goal of this CL?

Sam, alternatively, was it ever considered to have the RenderFrame set up the
connection rather than the RenderFrameHost? Are there compelling reasons not to
go that route?

Thanks,

Colin

[blundell, 2014-10-22 10:51:44]

On 2014/10/22 10:44:48, blundell wrote:
> Hi,
> 
> This CL as it is feels essentially like a hack. What I'd like to find is the
> following: a point internal to RenderFrameHostImpl wherein it knows that a new
> RenderFrame is being created for it to be associated with. This CL, while it
> works, is unappealing for two reasons:
> (1) The call to set up the new Mojo connection is made externally to RFHI.
> (2) The hook point that I found in RenderFrameHostManager gets executed more
> than once during the lifetime of a single RenderFrame, resulting in the need
for
> short-circuit logic in RFHI::SetUpMojo().
> 
> Charlie, could you advise on a better way to accomplish the goal of this CL?
> 
> Sam, alternatively, was it ever considered to have the RenderFrame set up the
> connection rather than the RenderFrameHost? Are there compelling reasons not
to
> go that route?
> 
> Thanks,
> 
> Colin

Correction: I think that what I said in (2) was not correct. I needed to insert
the short-circuit logic because SetUpMojo() is also called from the RFHI
constructor, and in some cases, it is called both from the constructor and from
RenderFrameHostManager for the same render frame. I need to keep the callsite in
the RFHI constructor because the flow in RenderFrameHostManager is not gone
through for every RenderFrame. Like I said, a hack.

[Sam McNally, 2014-10-22 11:25:05]

> Sam, alternatively, was it ever considered to have the RenderFrame set up the
> connection rather than the RenderFrameHost? Are there compelling reasons not
to
> go that route?
> 
I did look at both directions and I think the choice was fairly arbitrary (based
on thinking that a RenderFrameHost stuck with one RenderFrame). See
https://codereview.chromium.org/285333003/#ps670001 for a version that did setup
in that direction.

[Charlie Reis, 2014-10-22 16:07:05]

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

[Charlie Reis, 2014-10-22 16:07:06]

I'm out at an event today, so perhaps Nasko can take a look.  Otherwise I'm
happy to look when I get back.

[nasko, 2014-10-22 20:21:00]

On 2014/10/22 10:44:48, blundell wrote:
> Hi,
> 
> This CL as it is feels essentially like a hack. What I'd like to find is the
> following: a point internal to RenderFrameHostImpl wherein it knows that a new
> RenderFrame is being created for it to be associated with. This CL, while it
> works, is unappealing for two reasons:
> (1) The call to set up the new Mojo connection is made externally to RFHI.
> (2) The hook point that I found in RenderFrameHostManager gets executed more
> than once during the lifetime of a single RenderFrame, resulting in the need
for
> short-circuit logic in RFHI::SetUpMojo().
> 
> Charlie, could you advise on a better way to accomplish the goal of this CL?

RenderFrameHosts have two creation paths:
* The top-level frame is created as part of the view, which happens in
RenderViewHostImpl::CreateRenderView. RFH should already exist at this point.
* Any subframe RenderFrameHost creation is currently driven by the renderer,
when an iframe is added to the page. This codepath leads to the creation of new
RFH on the browser side, so the constructor call should take care of it.

We are in a transition period (as part of site isolation work) and the
interaction between RVH and RFH is a bit more complex than needed and the
top-level RenderFrameHost is constructed in a different way. Hopefully we will
simplify this codepath before long.

[blundell, 2014-10-22 20:29:58]

On 2014/10/22 20:21:00, nasko wrote:
> On 2014/10/22 10:44:48, blundell wrote:
> > Hi,
> > 
> > This CL as it is feels essentially like a hack. What I'd like to find is the
> > following: a point internal to RenderFrameHostImpl wherein it knows that a
new
> > RenderFrame is being created for it to be associated with. This CL, while it
> > works, is unappealing for two reasons:
> > (1) The call to set up the new Mojo connection is made externally to RFHI.
> > (2) The hook point that I found in RenderFrameHostManager gets executed more
> > than once during the lifetime of a single RenderFrame, resulting in the need
> for
> > short-circuit logic in RFHI::SetUpMojo().
> > 
> > Charlie, could you advise on a better way to accomplish the goal of this CL?
> 
> RenderFrameHosts have two creation paths:
> * The top-level frame is created as part of the view, which happens in
> RenderViewHostImpl::CreateRenderView. RFH should already exist at this point.
> * Any subframe RenderFrameHost creation is currently driven by the renderer,
> when an iframe is added to the page. This codepath leads to the creation of
new
> RFH on the browser side, so the constructor call should take care of it.
> 
> We are in a transition period (as part of site isolation work) and the
> interaction between RVH and RFH is a bit more complex than needed and the
> top-level RenderFrameHost is constructed in a different way. Hopefully we will
> simplify this codepath before long.

[blundell, 2014-10-22 20:29:58]

On 2014/10/22 20:21:00, nasko wrote:
> On 2014/10/22 10:44:48, blundell wrote:
> > Hi,
> > 
> > This CL as it is feels essentially like a hack. What I'd like to find is the
> > following: a point internal to RenderFrameHostImpl wherein it knows that a
new
> > RenderFrame is being created for it to be associated with. This CL, while it
> > works, is unappealing for two reasons:
> > (1) The call to set up the new Mojo connection is made externally to RFHI.
> > (2) The hook point that I found in RenderFrameHostManager gets executed more
> > than once during the lifetime of a single RenderFrame, resulting in the need
> for
> > short-circuit logic in RFHI::SetUpMojo().
> > 
> > Charlie, could you advise on a better way to accomplish the goal of this CL?
> 
> RenderFrameHosts have two creation paths:
> * The top-level frame is created as part of the view, which happens in
> RenderViewHostImpl::CreateRenderView. RFH should already exist at this point.
> * Any subframe RenderFrameHost creation is currently driven by the renderer,
> when an iframe is added to the page. This codepath leads to the creation of
new
> RFH on the browser side, so the constructor call should take care of it.
> 
> We are in a transition period (as part of site isolation work) and the
> interaction between RVH and RFH is a bit more complex than needed and the
> top-level RenderFrameHost is constructed in a different way. Hopefully we will
> simplify this codepath before long.

[blundell, 2014-10-22 20:31:04]

On 2014/10/22 20:21:00, nasko wrote:
> On 2014/10/22 10:44:48, blundell wrote:
> > Hi,
> > 
> > This CL as it is feels essentially like a hack. What I'd like to find is the
> > following: a point internal to RenderFrameHostImpl wherein it knows that a
new
> > RenderFrame is being created for it to be associated with. This CL, while it
> > works, is unappealing for two reasons:
> > (1) The call to set up the new Mojo connection is made externally to RFHI.
> > (2) The hook point that I found in RenderFrameHostManager gets executed more
> > than once during the lifetime of a single RenderFrame, resulting in the need
> for
> > short-circuit logic in RFHI::SetUpMojo().
> > 
> > Charlie, could you advise on a better way to accomplish the goal of this CL?
> 
> RenderFrameHosts have two creation paths:
> * The top-level frame is created as part of the view, which happens in
> RenderViewHostImpl::CreateRenderView. RFH should already exist at this point.
> * Any subframe RenderFrameHost creation is currently driven by the renderer,
> when an iframe is added to the page. This codepath leads to the creation of
new
> RFH on the browser side, so the constructor call should take care of it.
> 
> We are in a transition period (as part of site isolation work) and the
> interaction between RVH and RFH is a bit more complex than needed and the
> top-level RenderFrameHost is constructed in a different way. Hopefully we will
> simplify this codepath before long.

(sorry for the noise, my keyboard went crazy)

Thanks! Is there a point in RFHI where it is aware that a new RenderFrame is
being created for it to be its counterpoint? That's a flow I need to hook into,
and I can't see any super-clean way to do it currently.

[nasko, 2014-10-22 20:41:09]

On 2014/10/22 20:31:04, blundell wrote:
> On 2014/10/22 20:21:00, nasko wrote:
> > On 2014/10/22 10:44:48, blundell wrote:
> > > Hi,
> > > 
> > > This CL as it is feels essentially like a hack. What I'd like to find is
the
> > > following: a point internal to RenderFrameHostImpl wherein it knows that a
> new
> > > RenderFrame is being created for it to be associated with. This CL, while
it
> > > works, is unappealing for two reasons:
> > > (1) The call to set up the new Mojo connection is made externally to RFHI.
> > > (2) The hook point that I found in RenderFrameHostManager gets executed
more
> > > than once during the lifetime of a single RenderFrame, resulting in the
need
> > for
> > > short-circuit logic in RFHI::SetUpMojo().
> > > 
> > > Charlie, could you advise on a better way to accomplish the goal of this
CL?
> > 
> > RenderFrameHosts have two creation paths:
> > * The top-level frame is created as part of the view, which happens in
> > RenderViewHostImpl::CreateRenderView. RFH should already exist at this
point.
> > * Any subframe RenderFrameHost creation is currently driven by the renderer,
> > when an iframe is added to the page. This codepath leads to the creation of
> new
> > RFH on the browser side, so the constructor call should take care of it.
> > 
> > We are in a transition period (as part of site isolation work) and the
> > interaction between RVH and RFH is a bit more complex than needed and the
> > top-level RenderFrameHost is constructed in a different way. Hopefully we
will
> > simplify this codepath before long.
> 
> (sorry for the noise, my keyboard went crazy)
> 
> Thanks! Is there a point in RFHI where it is aware that a new RenderFrame is
> being created for it to be its counterpoint? That's a flow I need to hook
into,
> and I can't see any super-clean way to do it currently.

In the case of subframes, there exists RenderFrame, so that one is easy. For the
top-level unfortunately it isn't aware when the RenderFrame is created in the
child process. Maybe add the call to setup mojo in RVH::CreateRenderView after
the IPC is sent and put a comment to move this into RFH once we clean up this
codepath.

Also, when a process dies, we are guaranteed to not have any subframes, so you
technically need to worry only about top-level frames.

[blundell, 2014-10-23 11:56:00]

nasko@, thanks for the insights here! Realizing that this issue was only a
concern for the main frame enabled simplifying this CL. Charlie, this CL is
ready for review now.

Note: I did not put the new calls to RFHI inside RenderViewHostImpl because RVHI
only knows the main frame as a RVH, not a RVHI, and I assume that this is
intentional.

Sam, on reflection I'm going to leave the flow going from the browser to the
renderer because that means that the connection can be set up faster after the
creation of the RenderFrameImpl. I don't know if that matters in practice, but
it seems like it can't hurt.

[nasko, 2014-10-23 15:59:09]

Couple of small things.

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

https://codereview.chromium.org/666563005/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.h:360: void
SetUpMojoIfNeeded();
nit: I don't see "IfNeeded" suffix adding much useful information. It is totally
fine if the method returns early if setup isn't needed.

https://codereview.chromium.org/666563005/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/666563005/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:191:
dest_render_frame_host->SetUpMojoIfNeeded();
This should move below the InitRenderView call.

[Charlie Reis, 2014-10-23 18:38:32]

Hmm, I think I need to question the assumption about the main frame.  Won't the
same problem occur if a subframe crashes and then gets created again (in
--site-per-process mode)?

https://codereview.chromium.org/666563005/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/666563005/diff/80001/content/browser/frame_ho...
content/browser/frame_host/frame_tree.cc:236:
GetMainFrame()->InvalidateMojoConnection();
I don't think we want to do this on the main frame.  We get here for subframe
crashes (in --site-per-process mode) as well.  It should probably happen on the
frame itself in ResetNodesForNewProcess.

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

https://codereview.chromium.org/666563005/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.h:360: void
SetUpMojoIfNeeded();
On 2014/10/23 15:59:08, nasko wrote:
> nit: I don't see "IfNeeded" suffix adding much useful information. It is
totally
> fine if the method returns early if setup isn't needed.

I would be ok with keeping the IfNeeded part.  Not all setup methods can be
safely called multiple times, and there's precedent for it in content/ (even if
it's not universal).

[blundell, 2014-10-24 14:42:54]

Thanks!

https://codereview.chromium.org/666563005/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/666563005/diff/80001/content/browser/frame_ho...
content/browser/frame_host/frame_tree.cc:236:
GetMainFrame()->InvalidateMojoConnection();
On 2014/10/23 18:38:31, Charlie Reis wrote:
> I don't think we want to do this on the main frame.  We get here for subframe
> crashes (in --site-per-process mode) as well.  It should probably happen on
the
> frame itself in ResetNodesForNewProcess.

Done.

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

https://codereview.chromium.org/666563005/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.h:360: void
SetUpMojoIfNeeded();
On 2014/10/23 18:38:31, Charlie Reis wrote:
> On 2014/10/23 15:59:08, nasko wrote:
> > nit: I don't see "IfNeeded" suffix adding much useful information. It is
> totally
> > fine if the method returns early if setup isn't needed.
> 
> I would be ok with keeping the IfNeeded part.  Not all setup methods can be
> safely called multiple times, and there's precedent for it in content/ (even
if
> it's not universal).

Personally I like the |IfNeeded()|. Contrast this with RPHI::Init(), where every
callsite painstakingly comments that it is safe to call that method multiple
times.

https://codereview.chromium.org/666563005/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/666563005/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:191:
dest_render_frame_host->SetUpMojoIfNeeded();
On 2014/10/23 15:59:08, nasko wrote:
> This should move below the InitRenderView call.

It's actually important that this be before that call. When the render process
dies on a WebUI site and then a reload occurs, the call to InitRenderView()
indirectly ends up in the //chrome Mojo WebUI code adding the relevant
MojoWebUIController as a Mojo service to the RFH's ServiceRegistry. Thus, the
ServiceRegistry needs to be set up before that flow occurs.

[blundell, 2014-10-24 14:42:55]

Thanks!

https://codereview.chromium.org/666563005/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/666563005/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:191:
dest_render_frame_host->SetUpMojoIfNeeded();
On 2014/10/23 15:59:08, nasko wrote:
> This should move below the InitRenderView call.

It's actually important that this be before that call. When the render process
dies on a WebUI site and then a reload occurs, the call to InitRenderView()
indirectly ends up in the //chrome Mojo WebUI code adding the relevant
MojoWebUIController as a Mojo service to the RFH's ServiceRegistry. Thus, the
ServiceRegistry needs to be set up before that flow occurs.

[nasko, 2014-10-24 16:13:11]

https://codereview.chromium.org/666563005/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/666563005/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:191:
dest_render_frame_host->SetUpMojoIfNeeded();
On 2014/10/24 14:42:55, blundell wrote:
> On 2014/10/23 15:59:08, nasko wrote:
> > This should move below the InitRenderView call.
> 
> It's actually important that this be before that call. When the render process
> dies on a WebUI site and then a reload occurs, the call to InitRenderView()
> indirectly ends up in the //chrome Mojo WebUI code adding the relevant
> MojoWebUIController as a Mojo service to the RFH's ServiceRegistry. Thus, the
> ServiceRegistry needs to be set up before that flow occurs.

Ok. This seems to be important enough that some of the explanation would be good
to be in the comment before the call.

[Charlie Reis, 2014-10-24 16:30:45]

Thanks!  LGTM.

https://codereview.chromium.org/666563005/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/666563005/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:191:
dest_render_frame_host->SetUpMojoIfNeeded();
On 2014/10/24 16:13:11, nasko wrote:
> On 2014/10/24 14:42:55, blundell wrote:
> > On 2014/10/23 15:59:08, nasko wrote:
> > > This should move below the InitRenderView call.
> > 
> > It's actually important that this be before that call. When the render
process
> > dies on a WebUI site and then a reload occurs, the call to InitRenderView()
> > indirectly ends up in the //chrome Mojo WebUI code adding the relevant
> > MojoWebUIController as a Mojo service to the RFH's ServiceRegistry. Thus,
the
> > ServiceRegistry needs to be set up before that flow occurs.
> 
> Ok. This seems to be important enough that some of the explanation would be
good
> to be in the comment before the call.

+1.

[blundell, 2014-10-27 08:30:29]

Thanks! Comment added.

[blundell, 2014-10-27 08:30:35]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-10-27 08:30:56]

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

[commit-bot: I haz the power, 2014-10-27 09:21:25]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2014-10-27 09:21:59]

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

[qsr, 2014-10-27 11:45:40]

qsr@chromium.org changed reviewers:
+ qsr@chromium.org

[qsr, 2014-10-27 11:45:41]

https://codereview.chromium.org/666563005/diff/120001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/666563005/diff/120001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1457:
service_registry_android_.reset();
This is not that big a deal, but this should be resetted before the other one,
as it does have a reference to the service_registry. If it were to use it in its
destructor, this will break.