Fix the RenderFrameProxyHost mop-up logic in RenderFrameHostManager

Fix the RenderFrameProxyHost mop-up logic in RenderFrameHostManager.

RenderFrameProxyHosts need to exist in a SiteInstance so long as
at least one active frame exists: when we're about to
trigger the (1 -> 0) transition in |active_frame_count|,
we need to destroy any lingering proxies.

Bug 444955 occurs because we don't check for the (1 -> 0) transition
when the last active frame happens to be a pending frame when it's
deleted.

Also fixed here is bug 473714, found by inspection,
where deletion of a not-live frame would incorrectly rambo
the RenderFrameProxyHosts that could potentially be needed by
another live RenderFrameHost. A new test demonstrates this
and verifies the fix.

TEST=content_unittests
BUG=444955, 473714
Committed: https://crrev.com/dfb21875dd6279d0a973eb24028205620d160cfe
Cr-Commit-Position: refs/heads/master@{#323921}

[ncarter (slow), 2015-04-03 18:20:42]

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

[ncarter (slow), 2015-04-03 18:20:43]

Charlie, Alex: Please have a look

[Charlie Reis, 2015-04-03 19:55:09]

LGTM with nits.

My one question is whether it would make more sense to do the proxy shutdown
from RenderFrameHostImpl's destructor.  I do like that RFHM is currently the one
responsible for managing the relationship between RFHs and RFPHs, but it seems
like that forces us to catch all the ways that a RFH might go away.

Then again, I guess we need to do this cleanup earlier than RFH's destructor,
such as when the RFH is put on the pending delete list.  So maybe that wouldn't
work.

https://codereview.chromium.org/1055203002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1055203002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1768: // on behalf of
frames anywhere in the browsing instance.
nit: s/browsing instance/BrowsingInstance/

https://codereview.chromium.org/1055203002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/1055203002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:581: // SiteInstance,
this will shutdown all the RenderFrameProxyHosts in the site
nit: s/site instance/SiteInstance/

https://codereview.chromium.org/1055203002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/1055203002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1414: // Test
that we clean up swapped out RenderFrameHosts when a process hosting
The test itself still seems to be about swapped out RenderViewHosts, rather than
using GetRenderFrameProxyHost.  Does it make more sense to update the test or
leave the comment as it was?

https://codereview.chromium.org/1055203002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1415: // those
associated RenderFrame crashes. http://crbug.com/258993
nit: RenderFrames

https://codereview.chromium.org/1055203002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1928: // mess
with the first tab's content.
Can you add https://crbug.com/473714 to the comment?

https://codereview.chromium.org/1055203002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1968: // A
proxy to the iframe should now exist in the site instance of the main
s/site instance/SiteInstance/

[alexmos, 2015-04-03 20:55:55]

LGTM.  Thanks for cleaning this up!

https://codereview.chromium.org/1055203002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1055203002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1771: // First remove
any swapped out RFH for this SiteInstance from our own list.
While we're here, does this comment need an update?  Seems like this function is
mainly about clearing proxies and only handles swapped-out RFH for main frames.

[ncarter (slow), 2015-04-03 22:03:34]

https://codereview.chromium.org/1055203002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1055203002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1768: // on behalf of
frames anywhere in the browsing instance.
On 2015/04/03 19:55:08, Charlie Reis wrote:
> nit: s/browsing instance/BrowsingInstance/

Done.

https://codereview.chromium.org/1055203002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1771: // First remove
any swapped out RFH for this SiteInstance from our own list.
On 2015/04/03 20:55:55, alexmos wrote:
> While we're here, does this comment need an update?  Seems like this function
is
> mainly about clearing proxies and only handles swapped-out RFH for main
frames.

Done.

But honestly the comment is the least goofy part of this function right now. For
one, the explicit call to ClearProxiesInSiteInstance on |this| seems unnecessary
-- the loop below should either traverse |this| too, unless we're in the middle
of being detached from the tree, in which case our proxies ought to go away when
|this| is deleted.

More puzzlingly, iterating over the RenderWidgetHosts will, I think, cause frame
trees to be traversed multiple times -- the traversal done on behalf of subframe
widgets is unnecessary?

https://codereview.chromium.org/1055203002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/1055203002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:581: // SiteInstance,
this will shutdown all the RenderFrameProxyHosts in the site
On 2015/04/03 19:55:09, Charlie Reis wrote:
> nit: s/site instance/SiteInstance/

Done.

https://codereview.chromium.org/1055203002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/1055203002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1414: // Test
that we clean up swapped out RenderFrameHosts when a process hosting
On 2015/04/03 19:55:09, Charlie Reis wrote:
> The test itself still seems to be about swapped out RenderViewHosts, rather
than
> using GetRenderFrameProxyHost.  Does it make more sense to update the test or
> leave the comment as it was?

Keen observation. I restored the comments and test name because I don't want to
disturb the test more than I need to now. This test will need to be updated
again when swappedout goes away.

https://codereview.chromium.org/1055203002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1415: // those
associated RenderFrame crashes. http://crbug.com/258993
On 2015/04/03 19:55:09, Charlie Reis wrote:
> nit: RenderFrames

Done.

https://codereview.chromium.org/1055203002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1928: // mess
with the first tab's content.
On 2015/04/03 19:55:09, Charlie Reis wrote:
> Can you add https://crbug.com/473714 to the comment?

Done.

https://codereview.chromium.org/1055203002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1968: // A
proxy to the iframe should now exist in the site instance of the main
On 2015/04/03 19:55:09, Charlie Reis wrote:
> s/site instance/SiteInstance/

Done.

https://codereview.chromium.org/1055203002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (right):

https://codereview.chromium.org/1055203002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1846:
TEST_F(RenderFrameHostManagerTest, DetachPendingChild) {
Note that in the latest patch set I've changed this test significantly. I
realized that in making it test bug 444955 it no longer was a valid test for
441357 -- I solved this by adding two frames in the same site_instance, and
detaching them both. That causes us to exercise both the "is last active frame"
case and the "is not last active frame" case.

[ncarter (slow), 2015-04-06 16:34:55]

The CQ bit was checked by nick@chromium.org

[ncarter (slow), 2015-04-06 16:34:55]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
alexmos@chromium.org
Link to the patchset: https://codereview.chromium.org/1055203002/#ps100001
(title: "Charlie fixes.")

[commit-bot: I haz the power, 2015-04-06 16:35:08]

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

[Charlie Reis, 2015-04-06 16:35:34]

Still LGTM with some thoughts below.

https://codereview.chromium.org/1055203002/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1055203002/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1771: // First remove
any swapped out RFH for this SiteInstance from our own list.
On 2015/04/03 22:03:33, ncarter wrote:
> On 2015/04/03 20:55:55, alexmos wrote:
> > While we're here, does this comment need an update?  Seems like this
function
> is
> > mainly about clearing proxies and only handles swapped-out RFH for main
> frames.
> 
> Done.
> 
> But honestly the comment is the least goofy part of this function right now.
For
> one, the explicit call to ClearProxiesInSiteInstance on |this| seems
unnecessary
> -- the loop below should either traverse |this| too, unless we're in the
middle
> of being detached from the tree, in which case our proxies ought to go away
when
> |this| is deleted.
> 
> More puzzlingly, iterating over the RenderWidgetHosts will, I think, cause
frame
> trees to be traversed multiple times -- the traversal done on behalf of
subframe
> widgets is unnecessary?

Agreed.  You'd mentioned that it might be worth tracking the active and inactive
frames for a SiteInstance.  Are you still considering that for a followup CL?

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

https://codereview.chromium.org/1055203002/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1898:
ASSERT_EQ(host1, GetPendingFrameHost(iframe1));
Why ASSERT here and EXPECT above?  I tend to use EXPECT for most things, unless
it's an invariant that would prevent the test from proceeding.  That way you get
as many failures in the output as you can.

[ncarter (slow), 2015-04-06 16:38:18]

The CQ bit was unchecked by nick@chromium.org

[ncarter (slow), 2015-04-06 16:48:55]

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

https://codereview.chromium.org/1055203002/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1898:
ASSERT_EQ(host1, GetPendingFrameHost(iframe1));
On 2015/04/06 16:35:34, Charlie Reis wrote:
> Why ASSERT here and EXPECT above?  I tend to use EXPECT for most things,
unless
> it's an invariant that would prevent the test from proceeding.  That way you
get
> as many failures in the output as you can.

Changed these to EXPECTS_.

[ncarter (slow), 2015-04-06 16:48:59]

The CQ bit was checked by nick@chromium.org

[ncarter (slow), 2015-04-06 16:48:59]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
alexmos@chromium.org
Link to the patchset: https://codereview.chromium.org/1055203002/#ps120001
(title: "EXPECT_ instead of ASSERT_")

[commit-bot: I haz the power, 2015-04-06 16:49:18]

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

[commit-bot: I haz the power, 2015-04-06 18:36:04]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-04-06 18:38:14]

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