Add ref-count on RenderViewHost for each new RenderFrameProxyHost.

Add ref-count on RenderViewHost for each new RenderFrameProxyHost.

This CL is a reland of https://codereview.chromium.org/1150793002/ with improved version of the test.

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

[nasko, 2015-05-22 19:05:47]

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

[nasko, 2015-05-22 19:05:47]

Hey Nick,
Can you review this CL for me? PatchSet 1 is identical to my previous CL. The
latter PS is improving the test to ensure the RFH is destructed before ensuring
cleanup is complete.
Thanks in advance!
Nasko

[ncarter (slow), 2015-05-22 23:03:14]

Just nits.

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

https://codereview.chromium.org/1151393003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1574: instance,
frame_tree_node_->frame_tree()->GetRenderViewHost(instance),
Would it be worth adding a comment here about why it's guaranteed here that the
RVH already exists (and isn't on the pending delete list)? I think the
invariants guaranteeing that are pretty spread out, and certainly not obvious.

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

https://codereview.chromium.org/1151393003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:1738: // is
expected.
If you wanted to make it robust against this, you could probably do it by making
sure that either FrameDeleted(|rfh|) or RenderFrameHostChanged(|rfh|, _) had
occurred before RenderFrameDeleted(|rfh|).

https://codereview.chromium.org/1151393003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:1739: class
RenderFrameHostDestructionObserver : public WebContentsObserver {
This is pretty similar to
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...

If you want to pull it out into a test util, I'd support that.

https://codereview.chromium.org/1151393003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:1747: void
Observe(RenderFrameHost* rfh) {
This shadows WebContentsObserver::Observe, which is kind of weird.

Consider dropping this method and just passing the rfh in the ctor? You can get
the WebContents via WebContents::FromRenderFrameHost(rfh).

https://codereview.chromium.org/1151393003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:1760: void
RenderFrameDeleted(RenderFrameHost* rfh) override {
This should be public.

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

https://codereview.chromium.org/1151393003/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:467:
EXPECT_EQ(child1_rfh->GetSiteInstance(), child2_rfh->GetSiteInstance());
Lines 463-467 could be replaced with a DepictFrameTree(root), but you'd still
want child1_rfh so that you could cache its ID for the check on line 488.

[ncarter (slow), 2015-05-22 23:04:29]

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

https://codereview.chromium.org/1151393003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:1738: // is
expected.
On 2015/05/22 23:03:14, ncarter wrote:
> If you wanted to make it robust against this, you could probably do it by
making
> sure that either FrameDeleted(|rfh|) or RenderFrameHostChanged(|rfh|, _) had
> occurred before RenderFrameDeleted(|rfh|).

[but I'm okay with just leaving the "Note:" too]

[nasko, 2015-05-22 23:46:50]

Fixes!

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

https://codereview.chromium.org/1151393003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1574: instance,
frame_tree_node_->frame_tree()->GetRenderViewHost(instance),
On 2015/05/22 23:03:13, ncarter wrote:
> Would it be worth adding a comment here about why it's guaranteed here that
the
> RVH already exists (and isn't on the pending delete list)? I think the
> invariants guaranteeing that are pretty spread out, and certainly not obvious.

This is just temporary. My bigger CL to remove swapped out will create the RVH
in this method if it didn't already exist and pass it in.

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

https://codereview.chromium.org/1151393003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:1739: class
RenderFrameHostDestructionObserver : public WebContentsObserver {
On 2015/05/22 23:03:14, ncarter wrote:
> This is pretty similar to
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
> 
> If you want to pull it out into a test util, I'd support that.

Might be a good idea. I'd leave that for another CL though. I even thought of
pulling it into a common test file, as I imagine it might be used in other
places, but wanted to see this being stable for a bit before doing that.

Pulling it into common code and combining it with the other observer will be
also good to incorporate your note (above) about making it more robust.

https://codereview.chromium.org/1151393003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:1747: void
Observe(RenderFrameHost* rfh) {
On 2015/05/22 23:03:14, ncarter wrote:
> This shadows WebContentsObserver::Observe, which is kind of weird.
> 
> Consider dropping this method and just passing the rfh in the ctor? You can
get
> the WebContents via WebContents::FromRenderFrameHost(rfh).

Done.

https://codereview.chromium.org/1151393003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:1760: void
RenderFrameDeleted(RenderFrameHost* rfh) override {
On 2015/05/22 23:03:14, ncarter wrote:
> This should be public.

Why should it? I've changed it, but curious about the rationale.

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

https://codereview.chromium.org/1151393003/diff/60001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:467:
EXPECT_EQ(child1_rfh->GetSiteInstance(), child2_rfh->GetSiteInstance());
On 2015/05/22 23:03:14, ncarter wrote:
> Lines 463-467 could be replaced with a DepictFrameTree(root), but you'd still
> want child1_rfh so that you could cache its ID for the check on line 488.

Done.

[ncarter (slow), 2015-05-23 00:33:36]

lgtm

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

https://codereview.chromium.org/1151393003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1574: instance,
frame_tree_node_->frame_tree()->GetRenderViewHost(instance),
On 2015/05/22 23:46:49, nasko wrote:
> On 2015/05/22 23:03:13, ncarter wrote:
> > Would it be worth adding a comment here about why it's guaranteed here that
> the
> > RVH already exists (and isn't on the pending delete list)? I think the
> > invariants guaranteeing that are pretty spread out, and certainly not
obvious.
> 
> This is just temporary. My bigger CL to remove swapped out will create the RVH
> in this method if it didn't already exist and pass it in.

Acknowledged.

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

https://codereview.chromium.org/1151393003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:1739: class
RenderFrameHostDestructionObserver : public WebContentsObserver {
On 2015/05/22 23:46:49, nasko wrote:
> On 2015/05/22 23:03:14, ncarter wrote:
> > This is pretty similar to
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...
> > 
> > If you want to pull it out into a test util, I'd support that.
> 
> Might be a good idea. I'd leave that for another CL though. I even thought of
> pulling it into a common test file, as I imagine it might be used in other
> places, but wanted to see this being stable for a bit before doing that.
> 
> Pulling it into common code and combining it with the other observer will be
> also good to incorporate your note (above) about making it more robust.

Acknowledged.

https://codereview.chromium.org/1151393003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:1760: void
RenderFrameDeleted(RenderFrameHost* rfh) override {
On 2015/05/22 23:46:49, nasko wrote:
> On 2015/05/22 23:03:14, ncarter wrote:
> > This should be public.
> 
> Why should it? I've changed it, but curious about the rationale.

1. When you have something that's private in a derived class but public in the
base class (and you're using public inheritance), callers can still access it by
casting to the base class, so it's not really hidden.

2. Marking one method private does nothing to prevent users of
RenderFrameHostDestructionObserver from calling the other two dozen public
methods of WebContentsObserver.

3. A private method ought to be a signal that "it's an implementation detail and
all callers should be found right here in this class", but that's not the case
here.

https://isocpp.org/wiki/faq/proper-inheritance#hiding-inherited-public

If you really wanted to hide the WCO guts from end-users of
RenderFrameHostDestructionObserver, the options are private inheritance (which
google style doesn't allow) or composition (move the WCO implementation to a
nested class and have a member variable of that type).

[nasko, 2015-05-23 02:23:09]

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

https://codereview.chromium.org/1151393003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:1760: void
RenderFrameDeleted(RenderFrameHost* rfh) override {
On 2015/05/23 00:33:36, ncarter wrote:
> On 2015/05/22 23:46:49, nasko wrote:
> > On 2015/05/22 23:03:14, ncarter wrote:
> > > This should be public.
> > 
> > Why should it? I've changed it, but curious about the rationale.
> 
> 1. When you have something that's private in a derived class but public in the
> base class (and you're using public inheritance), callers can still access it
by
> casting to the base class, so it's not really hidden.
> 
> 2. Marking one method private does nothing to prevent users of
> RenderFrameHostDestructionObserver from calling the other two dozen public
> methods of WebContentsObserver.
> 
> 3. A private method ought to be a signal that "it's an implementation detail
and
> all callers should be found right here in this class", but that's not the case
> here.
> 
> https://isocpp.org/wiki/faq/proper-inheritance#hiding-inherited-public
> 
> If you really wanted to hide the WCO guts from end-users of
> RenderFrameHostDestructionObserver, the options are private inheritance (which
> google style doesn't allow) or composition (move the WCO implementation to a
> nested class and have a member variable of that type).

Acknowledged.

[nasko, 2015-05-23 02:23:20]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2015-05-23 02:23:27]

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

[commit-bot: I haz the power, 2015-05-23 02:27:36]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-05-23 02:28:31]

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