Add ref-count on RenderViewHost for each new RenderFrameProxyHost.

Add ref-count on RenderViewHost for each new RenderFrameProxyHost.

Currently, only RenderFrameHosts are ref-counting the associated RenderViewHost.
Since it is possible to have a RenderViewHost with no associated RenderFrameHost
(for example a cross-process window opener/openee relationship), it should
be taken into account that the associated RenderFrameProxyHost is also
a ref-counting the RenderViewHost.
The goal of this CL is to allow for that, even though it isn't currently
the case. It is a small step towards removing swapped out RenderFrameHosts.

BUG=357747
Committed: https://crrev.com/8542185bbb9fd0c0e7d0240c64403d2e4cea39f2
Cr-Commit-Position: refs/heads/master@{#331016}

[nasko, 2015-05-20 21:20:30]

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

[nasko, 2015-05-20 21:20:31]

Hey Charlie,
Can you review this CL for me? It is implementing the proxy refcounting on
RenderViewHost. I broke this off of the bigger CL I'm working on, since it is
easier to review on its own.
Thanks in advance!
Nasko

[Charlie Reis, 2015-05-20 23:04:59]

Seems like it's on the right track.

Can we add some tests that show that RVHs are still deleted (e.g., with an
observer)?  It would be good to check cases like:
1) One RFH, no proxies involved.  Just a main frame swap kills the RVH.
2) Both proxies and RFHs exist (e.g., due to an OOPIF).  RVH goes away with a
main frame swap.
3) Only a proxy exists (e.g., in window.opener), no RFH in the tree.  RVH goes
away when the other window swaps.

Case 3 wouldn't pass without your CL.

https://codereview.chromium.org/1150793002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/1150793002/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_tree.cc:306:
render_view_host_map_.find(site_instance->GetId());
I'm trying to remember from our chat: did we decide it's ok to keep the map on
FrameTree, even though the RVH may not have any RFHs in the tree?  I think so,
since the RVH is still associated with the tree and will have an RFPH in the
tree in that case.

https://codereview.chromium.org/1150793002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/frame_tree.h (right):

https://codereview.chromium.org/1150793002/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_tree.h:128: void
UnregisterRenderViewHost(RenderViewHostImpl* render_view_host);
We should pick a better name. "Register" made sense when it referred to the
thing keeping RVH alive, but now the name is about the RVH itself.  Maybe
IncrementRenderViewHostRefCount?  That's a bit awkward, so I'm open to
alternatives.

https://codereview.chromium.org/1150793002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1150793002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:1576: instance,
frame_tree_node_->frame_tree()->GetRenderViewHost(instance),
Are we guaranteed to to have a RVH for |instance| here? 
FrameTree::GetRenderViewHost can return null.

https://codereview.chromium.org/1150793002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/1150793002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_proxy_host.cc:60:
frame_tree_node_->frame_tree()->RegisterRenderViewHost(render_view_host_);
Should we check that render_view_host is non-null, either here or in the
register call?  (I mention it because of the possible issue in RFHM.)

https://codereview.chromium.org/1150793002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_proxy_host.h (right):

https://codereview.chromium.org/1150793002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_proxy_host.h:154: // kept alive as long
as this object is alive.
nit: It is kept alive as long as any RenderFrameHosts or RenderFrameProxyHosts
are associated with it.

[nasko, 2015-05-21 16:22:13]

I've added tests for the first two cases. The last one will be with the bigger
CL.

https://codereview.chromium.org/1150793002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/1150793002/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_tree.cc:306:
render_view_host_map_.find(site_instance->GetId());
On 2015/05/20 23:05:00, Charlie Reis wrote:
> I'm trying to remember from our chat: did we decide it's ok to keep the map on
> FrameTree, even though the RVH may not have any RFHs in the tree?  I think so,
> since the RVH is still associated with the tree and will have an RFPH in the
> tree in that case.

I'm not sure if we discussed it explicitly, but I think the map should stay in
the FrameTree. All RVHs in that map are associated with objects in this
FrameTree, so it doesn't make sense to move it.
I'll leave it as is.

https://codereview.chromium.org/1150793002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/frame_tree.h (right):

https://codereview.chromium.org/1150793002/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_tree.h:128: void
UnregisterRenderViewHost(RenderViewHostImpl* render_view_host);
On 2015/05/20 23:05:00, Charlie Reis wrote:
> We should pick a better name. "Register" made sense when it referred to the
> thing keeping RVH alive, but now the name is about the RVH itself.  Maybe
> IncrementRenderViewHostRefCount?  That's a bit awkward, so I'm open to
> alternatives.

Renamed to keep the same pattern with RefCounted - AddRVHRef/ReleaseRVHRef.

https://codereview.chromium.org/1150793002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1150793002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:1576: instance,
frame_tree_node_->frame_tree()->GetRenderViewHost(instance),
On 2015/05/20 23:05:00, Charlie Reis wrote:
> Are we guaranteed to to have a RVH for |instance| here? 
> FrameTree::GetRenderViewHost can return null.

Yes, with the current code we are. With my follow up CL we aren't, but it
handles that case.

https://codereview.chromium.org/1150793002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/1150793002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_proxy_host.cc:60:
frame_tree_node_->frame_tree()->RegisterRenderViewHost(render_view_host_);
On 2015/05/20 23:05:00, Charlie Reis wrote:
> Should we check that render_view_host is non-null, either here or in the
> register call?  (I mention it because of the possible issue in RFHM.)

I can add a CHECK if you'd prefer, but we shouldn't be getting a null RVH.

https://codereview.chromium.org/1150793002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_proxy_host.h (right):

https://codereview.chromium.org/1150793002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_proxy_host.h:154: // kept alive as long
as this object is alive.
On 2015/05/20 23:05:00, Charlie Reis wrote:
> nit: It is kept alive as long as any RenderFrameHosts or RenderFrameProxyHosts
> are associated with it.

Done.

[Charlie Reis, 2015-05-21 18:48:17]

Great.  Looks good apart from one question about the third test.  A few other
nits below.

On 2015/05/21 16:22:13, nasko wrote:
> I've added tests for the first two cases. The last one will be with the bigger
> CL.

If I understand correctly, case 3 was why this CL was needed, right?  Is there a
reason not to include that test in this CL?

https://codereview.chromium.org/1150793002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/1150793002/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_tree.cc:306:
render_view_host_map_.find(site_instance->GetId());
On 2015/05/21 16:22:12, nasko wrote:
> On 2015/05/20 23:05:00, Charlie Reis wrote:
> > I'm trying to remember from our chat: did we decide it's ok to keep the map
on
> > FrameTree, even though the RVH may not have any RFHs in the tree?  I think
so,
> > since the RVH is still associated with the tree and will have an RFPH in the
> > tree in that case.
> 
> I'm not sure if we discussed it explicitly, but I think the map should stay in
> the FrameTree. All RVHs in that map are associated with objects in this
> FrameTree, so it doesn't make sense to move it.
> I'll leave it as is.

Acknowledged.

https://codereview.chromium.org/1150793002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1150793002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:1576: instance,
frame_tree_node_->frame_tree()->GetRenderViewHost(instance),
On 2015/05/21 16:22:12, nasko wrote:
> On 2015/05/20 23:05:00, Charlie Reis wrote:
> > Are we guaranteed to to have a RVH for |instance| here? 
> > FrameTree::GetRenderViewHost can return null.
> 
> Yes, with the current code we are. With my follow up CL we aren't, but it
> handles that case.

Acknowledged.

https://codereview.chromium.org/1150793002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/1150793002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_proxy_host.cc:60:
frame_tree_node_->frame_tree()->RegisterRenderViewHost(render_view_host_);
On 2015/05/21 16:22:12, nasko wrote:
> On 2015/05/20 23:05:00, Charlie Reis wrote:
> > Should we check that render_view_host is non-null, either here or in the
> > register call?  (I mention it because of the possible issue in RFHM.)
> 
> I can add a CHECK if you'd prefer, but we shouldn't be getting a null RVH.

Yeah, let's put a CHECK here.  I'd like to know if this invariant fails in
practice, especially since we store it in a map.

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

https://codereview.chromium.org/1150793002/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:469: int subframe_process =
child1_rfh->GetSiteInstance()->GetProcess()->GetID();
At the risk of spilling to the next line, I think subframe_process_id would be a
clearer name.

https://codereview.chromium.org/1150793002/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:486:
EXPECT_FALSE(RenderViewHost::FromID(subframe_process, subframe_rvh));
// Ensure the RVH for the subframe gets cleaned up when the frame goes away.

[nasko, 2015-05-21 21:28:08]

Fixed the comments.

Case 3 is impossible to do right now, without removing swapped-out RFHs, so I'll
include it in my follow up CLs.

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

https://codereview.chromium.org/1150793002/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:469: int subframe_process =
child1_rfh->GetSiteInstance()->GetProcess()->GetID();
On 2015/05/21 18:48:16, Charlie Reis wrote:
> At the risk of spilling to the next line, I think subframe_process_id would be
a
> clearer name.

Done.

https://codereview.chromium.org/1150793002/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:486:
EXPECT_FALSE(RenderViewHost::FromID(subframe_process, subframe_rvh));
On 2015/05/21 18:48:16, Charlie Reis wrote:
> // Ensure the RVH for the subframe gets cleaned up when the frame goes away.

Done.

[nasko, 2015-05-21 21:37:07]

Added the missed CHECK.

https://codereview.chromium.org/1150793002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/1150793002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_proxy_host.cc:60:
frame_tree_node_->frame_tree()->RegisterRenderViewHost(render_view_host_);
On 2015/05/21 18:48:16, Charlie Reis wrote:
> On 2015/05/21 16:22:12, nasko wrote:
> > On 2015/05/20 23:05:00, Charlie Reis wrote:
> > > Should we check that render_view_host is non-null, either here or in the
> > > register call?  (I mention it because of the possible issue in RFHM.)
> > 
> > I can add a CHECK if you'd prefer, but we shouldn't be getting a null RVH.
> 
> Yeah, let's put a CHECK here.  I'd like to know if this invariant fails in
> practice, especially since we store it in a map.

Done.

[Charlie Reis, 2015-05-21 21:39:18]

Ah yes, I forgot about the swapped out RFH in case 3.  Adding that test in the
next CL is fine.

LGTM!

[nasko, 2015-05-21 21:42:21]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2015-05-21 21:43:08]

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

[commit-bot: I haz the power, 2015-05-21 22:53:57]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-05-21 22:54:45]

Patchset 4 (id:??) landed as
https://crrev.com/8542185bbb9fd0c0e7d0240c64403d2e4cea39f2
Cr-Commit-Position: refs/heads/master@{#331016}

[Steven Holte, 2015-05-22 01:24:43]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1157563002/ by holte@chromium.org.

The reason for reverting is: It looks like the new browser test fails on Win8
aura
https://build.chromium.org/p/chromium.win/builders/Win8%20Aura/builds/30026/s....