Destroy the old RenderWidgetHostView when swapping out a main frame.

Destroy the old RenderWidgetHostView when swapping out a main frame.

This fixes an issue where the old RenderView is reused by a new remote
subframe. We shouldn't be leaking resources, because now we are already
hiding the unused RenderView in the renderer, when the local main frame
gets detached and the WebViewFrameWidget is closed.

BUG=638375
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/9431efdc7b0146ef74f75ce160f2ba343ca2d550
Cr-Commit-Position: refs/heads/master@{#438516}

[lfg, 2016-11-14 16:01:07]

Description was changed from

==========
Destroy the old RenderWidgetHostView when swapping out a main frame.

This fixes an issue where the old RenderView is reused by a new remote
subframe. We shouldn't be leaking resources, because now we are already
hiding the unused RenderView in the renderer, when the local main frame
gets detached and the WebViewFrameWidget is closed.

BUG=638375
==========

to

==========
Destroy the old RenderWidgetHostView when swapping out a main frame.

This fixes an issue where the old RenderView is reused by a new remote
subframe. We shouldn't be leaking resources, because now we are already
hiding the unused RenderView in the renderer, when the local main frame
gets detached and the WebViewFrameWidget is closed.

BUG=638375
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[lfg, 2016-11-14 16:01:09]

The CQ bit was checked by lfg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-14 16:01:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-14 17:20:18]

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

[commit-bot: I haz the power, 2016-11-14 17:20:19]

Dry run: This issue passed the CQ dry run.

[lfg, 2016-11-15 04:37:33]

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

[lfg, 2016-11-15 04:37:34]

Charlie, can you take a look?

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

https://codereview.chromium.org/2496233003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1513: if
(frame_tree_node_->IsMainFrame()) {
This part I think it should be moved to RenderFrameHostManager::CommitPending,
but I didn't want to do this to avoid changing too many things in this patch.

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

https://codereview.chromium.org/2496233003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:1752: if
(!render_view_host->GetWidget()->GetView())
I moved this to CommitPending so it's easier to reason -- we create a
RenderWidgetHostView when a RenderViewHost transitions from swapped-out to
non-swapped-out, and the reverse is also true: we destroy the
RenderWidgetHostView when the RenderViewHost transitions from active to
swapped-out.

https://codereview.chromium.org/2496233003/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2496233003/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:4863: if
(!rvhi->is_swapped_out())
This is also a no-op, but it makes it easier to understand and associate the
creation of the RenderWidgetHostView with a non-swapped-out RenderViewHost.

[Charlie Reis, 2016-11-15 19:59:05]

[+alexmos, site-isolation-reviews]

Thanks for looking closer at this!  After figuring out where these pieces came
from, I think this makes a lot more sense.  I do wonder if we can go a bit
further (and the is_swapped_out() thing seems unfortunate), but I think you're
basically on the right track.

Adding Alex since I think he may have worked on some of this code for the recent
crashes.  Some thoughts and questions below.

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

https://codereview.chromium.org/2496233003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1513: if
(frame_tree_node_->IsMainFrame()) {
On 2016/11/15 04:37:34, lfg wrote:
> This part I think it should be moved to RenderFrameHostManager::CommitPending,
> but I didn't want to do this to avoid changing too many things in this patch.

Hmm, I see the dilemma.  We're basically doing the same thing in
RFHM::DiscardUnusedFrame right here, though we're missing the
set_main_frame_routing_id(MSG_ROUTING_NONE) call here.  (I guess that was taken
care of in CommitPending, along with redundant calls to set_is_active and
set_is_swapped_out?)

It's definitely odd to be doing the same work in 3 places.  If we don't need the
RWHV after CommitPending (given that this runs later after we've heard the swap
out ack), I'm inclined to move all this to CommitPending, perhaps using a shared
helper function with DiscardUnusedFrame to do the work?

If we're concerned about regressions, we should at least put a TODO here
explaining that.  I'd kind of like to do it all at once, but if you're aiming to
land this before the branch, I can understand the argument to do it separately.

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

https://codereview.chromium.org/2496233003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:1752: if
(!render_view_host->GetWidget()->GetView())
On 2016/11/15 04:37:34, lfg wrote:
> I moved this to CommitPending so it's easier to reason -- we create a
> RenderWidgetHostView when a RenderViewHost transitions from swapped-out to
> non-swapped-out, and the reverse is also true: we destroy the
> RenderWidgetHostView when the RenderViewHost transitions from active to
> swapped-out.

Sounds good-- I like the symmetry.  We don't actually need the RWHV until commit
time, right?

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

https://codereview.chromium.org/2496233003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:2164: if
(rvh->IsRenderViewLive() && rvh->is_swapped_out())
The is_swapped_out clause here is new.  If we keep it, can you update the
comment for why it's needed?

https://codereview.chromium.org/2496233003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:2177:
rvh->set_is_active(true);
I see this was just moved from below, but I'm noticing that it looks redundant
with RFHI::OnSwappedOut.  Are both needed, or is that why you though we might
want to move that code to here?

https://codereview.chromium.org/2496233003/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/2496233003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host_impl.h:173: bool is_swapped_out()
{ return is_swapped_out_; }
Hmm, I think we should be working to remove is_swapped_out_ (per the TODO on
line 330) rather than adding calls to it.  Is this turning into a concept we
need long term, or are there ways we can avoid using it?

https://codereview.chromium.org/2496233003/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2496233003/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:4863: if
(!rvhi->is_swapped_out())
On 2016/11/15 04:37:34, lfg wrote:
> This is also a no-op, but it makes it easier to understand and associate the
> creation of the RenderWidgetHostView with a non-swapped-out RenderViewHost.

When you say no-op, you mean is_swapped_out() and proxy_routing_id ==
MSG_ROUTING_NONE are equivalent?  Can you add a DCHECK for that if so?  I wonder
if it would also help to say something like:
// Only create a RenderWidgetHostView if the RVH's main frame is active.

And looking closer, it seems unfortunate that we're adding a call to
RVHI::is_swapped_out().  I was hoping to remove that member entirely, as the
TODO in render_view_host_impl.h indicates.  Basically, it's confusing to try to
track swapped out state on RVH.  Do you think there's a way around this, or do
we need to stick with this concept for now?

[alexmos, 2016-11-17 17:55:56]

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

[alexmos, 2016-11-17 17:55:57]

Some more thoughts below.  Sorry, I didn't end up on the reviewer list, so
didn't notice right away that Charlie called me out.

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

https://codereview.chromium.org/2496233003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1513: if
(frame_tree_node_->IsMainFrame()) {
On 2016/11/15 19:59:05, Charlie Reis (slow) wrote:
> On 2016/11/15 04:37:34, lfg wrote:
> > This part I think it should be moved to
RenderFrameHostManager::CommitPending,
> > but I didn't want to do this to avoid changing too many things in this
patch.

Do you mean all of the things in the if, or just some of them?  

My initial look is that:
- render_view_host_->set_is_active(false) looks redundant with the call in
RenderFrameHostImpl::SwapOut
- set_is_swapped_out(true) is done here intentionally and not in CommitPending,
as
we currently distinguish between a swapped-out RVH which has a RFH pending
deletion, and a swapped-out RVH where the
RFH is deleted.  For the former, is_active() -> false, is_swapped_out() ->
false.  For the latter, is_active() -> false, is_swapped_out() -> true.  The
only place where this matters is for SwappedOutMessages filtering.

The RWHV destruction, I agree, seems better in CommitPending, where it'll be
symmetrical with the CreateRenderWidgetHostViewForRenderManager call to create
the RWHV on the new active RVH.

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

https://codereview.chromium.org/2496233003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:2164: if
(rvh->IsRenderViewLive() && rvh->is_swapped_out())
Is the IsRenderViewLive check necessary?  How would we be committing a RFH where
its RV isn't live?

https://codereview.chromium.org/2496233003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:2164: if
(rvh->IsRenderViewLive() && rvh->is_swapped_out())
On 2016/11/15 19:59:05, Charlie Reis (slow) wrote:
> The is_swapped_out clause here is new.  If we keep it, can you update the
> comment for why it's needed?

Is the logic that if the RVH is reused and transitioning from swapped-out to
active, then we need to recreate the RWHV, but if we are committing a brand new
RFH/RVH, then we've already done it earlier (specifically, in
CreateRenderViewForRenderManager where the is_swapped_out check is reversed)?

If so, the is_swapped_out() looks correct as it matches the point at which the
reused RVH's RWHV is destroyed (which is what you added in OnSwappedOut, when
is_swapped_out() becomes true).  And we don't want to use is_active() instead,
since if we get here when reusing a pending delete RFH's RVH, its RWHV still
exists, and we don't want to recreate it - right?

I agree it's unfortunate to have to expose is_swapped_out() though.  Perhaps
it's better to just move the old RVH's RWHV destruction from OnSwappedOut to
here, in which case this could use is_active?

https://codereview.chromium.org/2496233003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:2177:
rvh->set_is_active(true);
On 2016/11/15 19:59:05, Charlie Reis (slow) wrote:
> I see this was just moved from below, but I'm noticing that it looks redundant
> with RFHI::OnSwappedOut.  Are both needed, or is that why you though we might
> want to move that code to here?

They are not redundant -- these are updating the new RVH's state, and
OnSwappedOut is updating the old (swapped-out) RVH's state.  (We also do
set_is_active(false) on the old RVH at commit time, in RFHI::SwapOut -- that
call should probably move here for clarity.)

https://codereview.chromium.org/2496233003/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2496233003/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:4863: if
(!rvhi->is_swapped_out())
On 2016/11/15 19:59:05, Charlie Reis (slow) wrote:
> On 2016/11/15 04:37:34, lfg wrote:
> > This is also a no-op, but it makes it easier to understand and associate the
> > creation of the RenderWidgetHostView with a non-swapped-out RenderViewHost.
> 
> When you say no-op, you mean is_swapped_out() and proxy_routing_id ==
> MSG_ROUTING_NONE are equivalent?  Can you add a DCHECK for that if so?  I
wonder
> if it would also help to say something like:
> // Only create a RenderWidgetHostView if the RVH's main frame is active.
> 
> And looking closer, it seems unfortunate that we're adding a call to
> RVHI::is_swapped_out().  I was hoping to remove that member entirely, as the
> TODO in render_view_host_impl.h indicates.  Basically, it's confusing to try
to
> track swapped out state on RVH.  Do you think there's a way around this, or do
> we need to stick with this concept for now?

See my earlier comment about moving the GetView() destruction to CommitPending,
which might allow using rvhi->is_active() here instead.

Also, let's add a comment here explaining why this is only done for active RVHs
and mention that for reused swapped-out RVHs we do this in CommitPending when
they become active.

[lfg, 2016-11-17 22:06:44]

The CQ bit was checked by lfg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-17 22:08:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lfg, 2016-11-17 22:11:05]

The CQ bit was checked by lfg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-17 22:11:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-17 23:20:21]

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

[commit-bot: I haz the power, 2016-11-17 23:20:22]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[lfg, 2016-11-18 20:04:49]

The CQ bit was checked by lfg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-18 20:05:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lfg, 2016-11-22 21:54:36]

The CQ bit was checked by lfg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-22 21:55:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lfg, 2016-11-22 22:32:40]

Patchset #7 (id:120001) has been deleted

[lfg, 2016-11-22 22:32:45]

Patchset #6 (id:100001) has been deleted

[lfg, 2016-11-22 22:36:14]

The CQ bit was checked by lfg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-22 22:36:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lfg, 2016-11-23 00:27:05]

Charlie/Alex, please take another look.

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

https://codereview.chromium.org/2496233003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1513: if
(frame_tree_node_->IsMainFrame()) {
I moved all the code to RFHM. I think it makes more sense to control the state
of the RenderViewHost in RFHM than in RFH.

I moved this particular code to DeleteFromPendingList, which should preserve the
today's behavior between active/swapped_out.

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

https://codereview.chromium.org/2496233003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:1752: if
(!render_view_host->GetWidget()->GetView())
On 2016/11/15 19:59:05, Charlie Reis (away) wrote:
> On 2016/11/15 04:37:34, lfg wrote:
> > I moved this to CommitPending so it's easier to reason -- we create a
> > RenderWidgetHostView when a RenderViewHost transitions from swapped-out to
> > non-swapped-out, and the reverse is also true: we destroy the
> > RenderWidgetHostView when the RenderViewHost transitions from active to
> > swapped-out.
> 
> Sounds good-- I like the symmetry.  We don't actually need the RWHV until
commit
> time, right?

Right.

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

https://codereview.chromium.org/2496233003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:2164: if
(rvh->IsRenderViewLive() && rvh->is_swapped_out())
On 2016/11/17 17:55:57, alexmos wrote:
> Is the IsRenderViewLive check necessary?  How would we be committing a RFH
where
> its RV isn't live?

I wasn't able to figure out when IsRenderViewLive is false, however, the code
bellow in line 2189 assumes this is possible, and has a comment about the
RenderViewHost dying while it was hidden. If this indeed isn't possible, then we
can do a bigger cleanup here.

https://codereview.chromium.org/2496233003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:2164: if
(rvh->IsRenderViewLive() && rvh->is_swapped_out())
On 2016/11/17 17:55:57, alexmos wrote:
> On 2016/11/15 19:59:05, Charlie Reis (slow) wrote:
> > The is_swapped_out clause here is new.  If we keep it, can you update the
> > comment for why it's needed?
> 
> Is the logic that if the RVH is reused and transitioning from swapped-out to
> active, then we need to recreate the RWHV, but if we are committing a brand
new
> RFH/RVH, then we've already done it earlier (specifically, in
> CreateRenderViewForRenderManager where the is_swapped_out check is reversed)?
> 
> If so, the is_swapped_out() looks correct as it matches the point at which the
> reused RVH's RWHV is destroyed (which is what you added in OnSwappedOut, when
> is_swapped_out() becomes true).  And we don't want to use is_active() instead,
> since if we get here when reusing a pending delete RFH's RVH, its RWHV still
> exists, and we don't want to recreate it - right?
> 
> I agree it's unfortunate to have to expose is_swapped_out() though.  Perhaps
> it's better to just move the old RVH's RWHV destruction from OnSwappedOut to
> here, in which case this could use is_active?

I've switched so that the lifetime of RWHV now tracks is_active, which should be
more correct -- we only want a RWHV when the RenderViewHost is being used for
rendering.

https://codereview.chromium.org/2496233003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:2177:
rvh->set_is_active(true);
On 2016/11/17 17:55:57, alexmos wrote:
> On 2016/11/15 19:59:05, Charlie Reis (slow) wrote:
> > I see this was just moved from below, but I'm noticing that it looks
redundant
> > with RFHI::OnSwappedOut.  Are both needed, or is that why you though we
might
> > want to move that code to here?
> 
> They are not redundant -- these are updating the new RVH's state, and
> OnSwappedOut is updating the old (swapped-out) RVH's state.  (We also do
> set_is_active(false) on the old RVH at commit time, in RFHI::SwapOut -- that
> call should probably move here for clarity.)

I've moved the code that sets the old RVH's active state here, as well as
deleting the old RWHV.

https://codereview.chromium.org/2496233003/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/2496233003/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host_impl.h:173: bool is_swapped_out()
{ return is_swapped_out_; }
On 2016/11/15 19:59:05, Charlie Reis (away) wrote:
> Hmm, I think we should be working to remove is_swapped_out_ (per the TODO on
> line 330) rather than adding calls to it.  Is this turning into a concept we
> need long term, or are there ways we can avoid using it?

Removed in favor of using is_active.

https://codereview.chromium.org/2496233003/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2496233003/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:4863: if
(!rvhi->is_swapped_out())
On 2016/11/15 19:59:05, Charlie Reis (away) wrote:
> On 2016/11/15 04:37:34, lfg wrote:
> > This is also a no-op, but it makes it easier to understand and associate the
> > creation of the RenderWidgetHostView with a non-swapped-out RenderViewHost.
> 
> When you say no-op, you mean is_swapped_out() and proxy_routing_id ==
> MSG_ROUTING_NONE are equivalent?  Can you add a DCHECK for that if so?  I
wonder
> if it would also help to say something like:
> // Only create a RenderWidgetHostView if the RVH's main frame is active.
> 
> And looking closer, it seems unfortunate that we're adding a call to
> RVHI::is_swapped_out().  I was hoping to remove that member entirely, as the
> TODO in render_view_host_impl.h indicates.  Basically, it's confusing to try
to
> track swapped out state on RVH.  Do you think there's a way around this, or do
> we need to stick with this concept for now?

Added the DCHECK and switched to is_active.

[commit-bot: I haz the power, 2016-11-23 01:09:17]

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

[commit-bot: I haz the power, 2016-11-23 01:09:18]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Charlie Reis, 2016-11-23 07:25:22]

Thanks-- the additional simplification seems nice.  LGTM, with a few requests
for explanations below.

https://codereview.chromium.org/2496233003/diff/160001/chrome/browser/search/...
File chrome/browser/search/search_unittest.cc (right):

https://codereview.chromium.org/2496233003/diff/160001/chrome/browser/search/...
chrome/browser/search/search_unittest.cc:67:
ui::WindowResizeHelperMac::Get()->Init(base::ThreadTaskRunnerHandle::Get());
Just for the record, what made this test change necessary?

https://codereview.chromium.org/2496233003/diff/160001/content/public/test/te...
File content/public/test/test_renderer_host.h (right):

https://codereview.chromium.org/2496233003/diff/160001/content/public/test/te...
content/public/test/test_renderer_host.h:179:
std::unique_ptr<MockGpuChannelEstablishFactory> gpu_channel_factory_;
Similarly, what made this change necessary?

[alexmos, 2016-11-23 17:54:45]

LGTM

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

https://codereview.chromium.org/2496233003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1513: if
(frame_tree_node_->IsMainFrame()) {
On 2016/11/23 00:27:04, lfg wrote:
> I moved all the code to RFHM. I think it makes more sense to control the state
> of the RenderViewHost in RFHM than in RFH.
> 
> I moved this particular code to DeleteFromPendingList, which should preserve
the
> today's behavior between active/swapped_out.

Acknowledged.

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

https://codereview.chromium.org/2496233003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:2164: if
(rvh->IsRenderViewLive() && rvh->is_swapped_out())
On 2016/11/23 00:27:04, lfg wrote:
> On 2016/11/17 17:55:57, alexmos wrote:
> > Is the IsRenderViewLive check necessary?  How would we be committing a RFH
> where
> > its RV isn't live?
> 
> I wasn't able to figure out when IsRenderViewLive is false, however, the code
> bellow in line 2189 assumes this is possible, and has a comment about the
> RenderViewHost dying while it was hidden. If this indeed isn't possible, then
we
> can do a bigger cleanup here.

Ack.  I'm not sure myself, but good point about line 2189.  Perhaps this has
something to do with how the sad tab is handled.  In any case, that's something
we can investigate later on.

[lfg, 2016-11-24 19:05:13]

The CQ bit was checked by lfg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-24 19:05:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-24 19:48:02]

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

[commit-bot: I haz the power, 2016-11-24 19:48:03]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lfg, 2016-11-24 21:30:32]

Patchset #9 (id:200001) has been deleted

[lfg, 2016-11-24 21:30:44]

Patchset #8 (id:180001) has been deleted

[lfg, 2016-11-28 21:04:26]

The CQ bit was checked by lfg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-28 21:05:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-28 23:28:53]

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

[commit-bot: I haz the power, 2016-11-28 23:28:54]

Dry run: This issue passed the CQ dry run.

[lfg, 2016-11-29 17:41:23]

Charlie, can you do one more pass in the changes from the last PS?

https://codereview.chromium.org/2496233003/diff/160001/chrome/browser/search/...
File chrome/browser/search/search_unittest.cc (right):

https://codereview.chromium.org/2496233003/diff/160001/chrome/browser/search/...
chrome/browser/search/search_unittest.cc:67:
ui::WindowResizeHelperMac::Get()->Init(base::ThreadTaskRunnerHandle::Get());
On 2016/11/23 07:25:22, Charlie Reis (slow) wrote:
> Just for the record, what made this test change necessary?

Search test performs multiple navigations, in particular, look at
kInstantNTPTestCases, where it navigates to foo.com, then bar.com, then back to
foo.com, which causes the RVH to become inactive and back to active again. This
causes the RWHV to be destroyed and recreated.

This change (and the other change) initializes the mocks necessary to allow the
creation of these RWHVs.

https://codereview.chromium.org/2496233003/diff/160001/content/public/test/te...
File content/public/test/test_renderer_host.h (right):

https://codereview.chromium.org/2496233003/diff/160001/content/public/test/te...
content/public/test/test_renderer_host.h:179:
std::unique_ptr<MockGpuChannelEstablishFactory> gpu_channel_factory_;
On 2016/11/23 07:25:22, Charlie Reis (slow) wrote:
> Similarly, what made this change necessary?

Same as the other change, this is necessary to allow creations of surfaces,
which are needed to create RWHVs.

https://codereview.chromium.org/2496233003/diff/240001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/2496233003/diff/240001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:725:
rvh->set_is_active(false);
I had to change this back from the DCHECK, since the DCHECK isn't always true.
The RenderViewHost can be active when the OnSwappedOut is called from the
swapout_event_monitor_timeout_.

[Charlie Reis, 2016-11-29 19:35:54]

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

[Charlie Reis, 2016-11-29 19:35:55]

Hmm, I glossed over the test code a bit before, but I'm a bit concerned by it. 
Part of the problem is that I don't know the interactions between these classes,
but I'm worried about introducing these view-related dependencies on tests that
do navigation.

If this is fundamental, then it's unclear to me why only search_unittest.cc
cares about the Mac-specific setup.  (Maybe the RVH enabler thing is ok if it's
hidden in the framework.)

Nick, do you know more about these test frameworks and whether this is an ok
change to how they're set up?

https://codereview.chromium.org/2496233003/diff/160001/chrome/browser/search/...
File chrome/browser/search/search_unittest.cc (right):

https://codereview.chromium.org/2496233003/diff/160001/chrome/browser/search/...
chrome/browser/search/search_unittest.cc:67:
ui::WindowResizeHelperMac::Get()->Init(base::ThreadTaskRunnerHandle::Get());
On 2016/11/29 17:41:22, lfg wrote:
> On 2016/11/23 07:25:22, Charlie Reis (slow) wrote:
> > Just for the record, what made this test change necessary?
> 
> Search test performs multiple navigations, in particular, look at
> kInstantNTPTestCases, where it navigates to http://foo.com, then
http://bar.com, then back to
> http://foo.com, which causes the RVH to become inactive and back to active
again. This
> causes the RWHV to be destroyed and recreated.
> 
> This change (and the other change) initializes the mocks necessary to allow
the
> creation of these RWHVs.

Hmm, I don't follow.  Lots of tests do multiple navigations between sites.  That
behavior shouldn't require window resize code in all such tests.  Is there
something else about this test in particular?

If we have to keep this, it needs a comment, but I'd really like to avoid the
need for it if we can.  Navigation tests shouldn't require this.

https://codereview.chromium.org/2496233003/diff/240001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/2496233003/diff/240001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:725:
rvh->set_is_active(false);
On 2016/11/29 17:41:22, lfg wrote:
> I had to change this back from the DCHECK, since the DCHECK isn't always true.
> The RenderViewHost can be active when the OnSwappedOut is called from the
> swapout_event_monitor_timeout_.

Ok.  Please mention that in a comment.

https://codereview.chromium.org/2496233003/diff/240001/content/public/test/te...
File content/public/test/test_renderer_host.h (right):

https://codereview.chromium.org/2496233003/diff/240001/content/public/test/te...
content/public/test/test_renderer_host.h:178: void SetUp();
This needs a comment.  Lots of classes instantiate one of these (e.g.,
ExtensionEventObserverTest), but it doesn't look like you call SetUp in all of
them?  Why not?

[alexmos, 2016-11-29 19:47:38]

https://codereview.chromium.org/2496233003/diff/240001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/2496233003/diff/240001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:725:
rvh->set_is_active(false);
On 2016/11/29 19:35:55, Charlie Reis (slow) wrote:
> On 2016/11/29 17:41:22, lfg wrote:
> > I had to change this back from the DCHECK, since the DCHECK isn't always
true.
> > The RenderViewHost can be active when the OnSwappedOut is called from the
> > swapout_event_monitor_timeout_.
> 
> Ok.  Please mention that in a comment.

I'm a bit confused by this actually.  We start the swapout timer in
CommitPending -> SwapOutOldFrame -> SwapOut, and that call in CommitPending
happens after we've done old_rvh->set_is_active(false).  How can we still get
here from OnSwappedOut with is_active being true?
Is this because some tests call OnSwappedOut directly, without going through the
timer and CommitPending?

[lfg, 2016-11-30 23:46:43]

On 2016/11/29 19:47:38, alexmos wrote:
> I'm a bit confused by this actually.  We start the swapout timer in
> CommitPending -> SwapOutOldFrame -> SwapOut, and that call in CommitPending
> happens after we've done old_rvh->set_is_active(false).  How can we still get
> here from OnSwappedOut with is_active being true?
> Is this because some tests call OnSwappedOut directly, without going through
the
> timer and CommitPending?

Ok, so I figured out the problem on tests, and I don't think it would happen in
real world usage.

Here's the problem: In unit tests, the SwapOut_ACK never really arrives, so
every time DeleteFromPendingList is called it is from the timeout monitor. If a
test commits a frame more than once, it's possible to be in a scenario where
there's a timeout monitor active, but the RenderViewHost has switched to an
active state as a result of the second commit.

I think we could look into clearing the swapout monitor in CommitPending, or
perhaps I could try modifying this test and repeatedly call
DisableSwapOutTimerForTesting (We'll need to expose some content internals
inside content/public/test/ to do this second option).

WDYT?

[lfg, 2016-11-30 23:50:04]

On 2016/11/29 19:35:55, Charlie Reis (slow) wrote:
> Hmm, I glossed over the test code a bit before, but I'm a bit concerned by it.

> Part of the problem is that I don't know the interactions between these
classes,
> but I'm worried about introducing these view-related dependencies on tests
that
> do navigation.
> 
> If this is fundamental, then it's unclear to me why only search_unittest.cc
> cares about the Mac-specific setup.  (Maybe the RVH enabler thing is ok if
it's
> hidden in the framework.)

RenderViewHostTestHarness initializes all of this.

BrowserWithTestWindowTest includes the RenderViewHostTestEnabler, but doesn't
inherit from RenderViewHostTestHarness. Perhaps we could look into switching the
inheritance to RenderViewHostTestHarness either directly or indirectly? I'm not
sure how much work would be necessary though.

This would affect a lot more tests, so I was trying to avoid something like
that.

[lfg, 2016-12-01 18:01:56]

The CQ bit was checked by lfg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 18:02:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lfg, 2016-12-01 18:05:28]

The CQ bit was checked by lfg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 18:05:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-01 18:48:05]

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

[commit-bot: I haz the power, 2016-12-01 18:48:06]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lfg, 2016-12-01 19:45:44]

On 2016/11/30 23:50:04, lfg wrote:
> On 2016/11/29 19:35:55, Charlie Reis (slow) wrote:
> > Hmm, I glossed over the test code a bit before, but I'm a bit concerned by
it.
> 
> > Part of the problem is that I don't know the interactions between these
> classes,
> > but I'm worried about introducing these view-related dependencies on tests
> that
> > do navigation.
> > 
> > If this is fundamental, then it's unclear to me why only search_unittest.cc
> > cares about the Mac-specific setup.  (Maybe the RVH enabler thing is ok if
> it's
> > hidden in the framework.)
> 
> RenderViewHostTestHarness initializes all of this.
> 
> BrowserWithTestWindowTest includes the RenderViewHostTestEnabler, but doesn't
> inherit from RenderViewHostTestHarness. Perhaps we could look into switching
the
> inheritance to RenderViewHostTestHarness either directly or indirectly? I'm
not
> sure how much work would be necessary though.
> 
> This would affect a lot more tests, so I was trying to avoid something like
> that.

Ok, just to update, this isn't very easy. Both RenderViewHostTestHarness and
BrowserWithTestWindowTest initializes a window in SetUp, and creates a
BrowserContext. Generalizing this concept to work across both cases is not
trivial.

[lfg, 2016-12-01 19:47:07]

Patchset #12 (id:300001) has been deleted

[lfg, 2016-12-01 19:47:13]

Patchset #11 (id:280001) has been deleted

[lfg, 2016-12-01 19:47:42]

The CQ bit was checked by lfg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 19:48:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-01 21:29:23]

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

[commit-bot: I haz the power, 2016-12-01 21:29:23]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2016-12-01 23:57:26]

Thanks-- I'm feeling better about it, though I still have questions about the
search_unittest.cc one.

Alex, are you ok with the RFHM change?

On 2016/12/01 19:45:44, lfg wrote:
> On 2016/11/30 23:50:04, lfg wrote:
> > On 2016/11/29 19:35:55, Charlie Reis (slow) wrote:
> > > Hmm, I glossed over the test code a bit before, but I'm a bit concerned by
> it.
> > 
> > > Part of the problem is that I don't know the interactions between these
> > classes,
> > > but I'm worried about introducing these view-related dependencies on tests
> > that
> > > do navigation.
> > > 
> > > If this is fundamental, then it's unclear to me why only
search_unittest.cc
> > > cares about the Mac-specific setup.  (Maybe the RVH enabler thing is ok if
> > it's
> > > hidden in the framework.)
> > 
> > RenderViewHostTestHarness initializes all of this.
> > 
> > BrowserWithTestWindowTest includes the RenderViewHostTestEnabler, but
doesn't
> > inherit from RenderViewHostTestHarness. Perhaps we could look into switching
> the
> > inheritance to RenderViewHostTestHarness either directly or indirectly? I'm
> not
> > sure how much work would be necessary though.
> > 
> > This would affect a lot more tests, so I was trying to avoid something like
> > that.
> 
> Ok, just to update, this isn't very easy. Both RenderViewHostTestHarness and
> BrowserWithTestWindowTest initializes a window in SetUp, and creates a
> BrowserContext. Generalizing this concept to work across both cases is not
> trivial.

Ok, thanks for giving the other approach a try.  I'm ok with this, if the other
uses of RenderViewHostTestEnabler don't need to call these new setup/teardown
methods.  (I haven't verified that they all fall under BrowserWithTestWindowTest
and get it for free.)

https://codereview.chromium.org/2496233003/diff/260001/chrome/browser/search/...
File chrome/browser/search/search_unittest.cc (right):

https://codereview.chromium.org/2496233003/diff/260001/chrome/browser/search/...
chrome/browser/search/search_unittest.cc:67:
ui::WindowResizeHelperMac::Get()->Init(base::ThreadTaskRunnerHandle::Get());
I'm still unclear on this one.  I think I follow the RenderViewHostTestEnabler
one, but I'm not aware of this Mac-specific code being necessary for other
navigation tests.  Is there a precedent for it?  Something specific about this
test that requires it?

If it's needed, please include a comment why.

https://codereview.chromium.org/2496233003/diff/260001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/2496233003/diff/260001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:725:
rvh->set_is_active(false);
Please add a comment about what you found with the unit tests, explaining why it
can't be a DCHECK.

https://codereview.chromium.org/2496233003/diff/260001/content/public/test/te...
File content/public/test/test_renderer_host.h (right):

https://codereview.chromium.org/2496233003/diff/260001/content/public/test/te...
content/public/test/test_renderer_host.h:178: void SetUp();
If only some tests need to call these, we should include a comment saying when
they're necessary.  (No need for that if it's always required.)

[alexmos, 2016-12-02 01:50:49]

On 2016/11/30 23:46:43, lfg wrote:
> On 2016/11/29 19:47:38, alexmos wrote:
> > I'm a bit confused by this actually.  We start the swapout timer in
> > CommitPending -> SwapOutOldFrame -> SwapOut, and that call in CommitPending
> > happens after we've done old_rvh->set_is_active(false).  How can we still
get
> > here from OnSwappedOut with is_active being true?
> > Is this because some tests call OnSwappedOut directly, without going through
> the
> > timer and CommitPending?
> 
> Ok, so I figured out the problem on tests, and I don't think it would happen
in
> real world usage.
> 
> Here's the problem: In unit tests, the SwapOut_ACK never really arrives, so
> every time DeleteFromPendingList is called it is from the timeout monitor. If
a
> test commits a frame more than once, it's possible to be in a scenario where
> there's a timeout monitor active, but the RenderViewHost has switched to an
> active state as a result of the second commit.
> 
> I think we could look into clearing the swapout monitor in CommitPending, or
> perhaps I could try modifying this test and repeatedly call
> DisableSwapOutTimerForTesting (We'll need to expose some content internals
> inside content/public/test/ to do this second option).
> 
> WDYT?

Thanks for the explanation.  I think this can happen on real pages too if a RFH
is pending deletion, and its RVH gets reused and becomes active on a quick A-B-A
navigation, all before either the swapout ack arrives or the swapout timer
fires.  Then when we get the ack or the timer, it seems that we'll hit this
case.  So I think leaving the code as-is (without the DCHECK) and adding a
comment explaining this as Charlie suggested seems like the best approach.

Out of curiosity, which test(s) were failing due to this?  If we did want to
prevent this from happening, I think we could just call OnSwappedOut() between
the first and second navigation, which stops the swapout timer.  But I'm
actually happy we have coverage for this case and so I'd say let's leave them
as-is.

[lfg, 2016-12-06 21:25:47]

Patchset #12 (id:340001) has been deleted

[lfg, 2016-12-06 21:27:32]

The CQ bit was checked by lfg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 21:28:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[lfg, 2016-12-06 22:47:04]

Please, take another look.

I've included Alex's test that tests that we correctly have the swapped-out bit
set.

https://codereview.chromium.org/2496233003/diff/260001/chrome/browser/search/...
File chrome/browser/search/search_unittest.cc (right):

https://codereview.chromium.org/2496233003/diff/260001/chrome/browser/search/...
chrome/browser/search/search_unittest.cc:67:
ui::WindowResizeHelperMac::Get()->Init(base::ThreadTaskRunnerHandle::Get());
On 2016/12/01 23:57:26, Charlie Reis wrote:
> I'm still unclear on this one.  I think I follow the RenderViewHostTestEnabler
> one, but I'm not aware of this Mac-specific code being necessary for other
> navigation tests.  Is there a precedent for it?  Something specific about this
> test that requires it?
> 
> If it's needed, please include a comment why.

I've moved this to test enabler class, since it's needed for navigation.

https://codereview.chromium.org/2496233003/diff/260001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/2496233003/diff/260001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:725:
rvh->set_is_active(false);
On 2016/12/01 23:57:26, Charlie Reis wrote:
> Please add a comment about what you found with the unit tests, explaining why
it
> can't be a DCHECK.

I've moved this to an if check, and added Alex's test.

https://codereview.chromium.org/2496233003/diff/260001/content/public/test/te...
File content/public/test/test_renderer_host.h (right):

https://codereview.chromium.org/2496233003/diff/260001/content/public/test/te...
content/public/test/test_renderer_host.h:178: void SetUp();
On 2016/12/01 23:57:26, Charlie Reis wrote:
> If only some tests need to call these, we should include a comment saying when
> they're necessary.  (No need for that if it's always required.)

I've removed this and moved it to the constructor. It should be cleaner and used
by all tests that include the RenderViewHostTestEnabler.

[commit-bot: I haz the power, 2016-12-06 23:01:36]

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

[commit-bot: I haz the power, 2016-12-06 23:01:37]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2016-12-08 18:26:04]

Thanks for the test framework cleanup and extra test!  LGTM, if Alex is happy
with it.

[alexmos, 2016-12-09 19:35:20]

LGTM, thanks!

[lfg, 2016-12-10 04:42:16]

The CQ bit was checked by lfg@chromium.org

[commit-bot: I haz the power, 2016-12-10 04:42:28]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-10 06:23:04]

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

[commit-bot: I haz the power, 2016-12-10 06:23:05]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[lfg, 2016-12-10 06:38:02]

The CQ bit was checked by lfg@chromium.org

[commit-bot: I haz the power, 2016-12-10 06:38:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-10 08:11:00]

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

[commit-bot: I haz the power, 2016-12-10 08:11:02]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[lfg, 2016-12-12 21:22:38]

The CQ bit was checked by lfg@chromium.org

[commit-bot: I haz the power, 2016-12-12 21:23:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-13 02:06:37]

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

[commit-bot: I haz the power, 2016-12-13 02:06:38]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lfg, 2016-12-13 20:04:50]

The CQ bit was checked by lfg@chromium.org

[commit-bot: I haz the power, 2016-12-13 20:05:44]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-13 20:14:36]

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

[commit-bot: I haz the power, 2016-12-13 20:14:40]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[lfg, 2016-12-14 01:42:48]

The CQ bit was checked by lfg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-14 01:43:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-14 03:32:35]

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

[commit-bot: I haz the power, 2016-12-14 03:32:36]

Dry run: This issue passed the CQ dry run.

[lfg, 2016-12-14 14:41:30]

The CQ bit was checked by lfg@chromium.org

[lfg, 2016-12-14 14:41:31]

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/2496233003/#ps380001
(title: "rebase")

[commit-bot: I haz the power, 2016-12-14 14:41:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-14 14:45:35]

CQ is committing da patch.

Bot data: {"patchset_id": 380001, "attempt_start_ts": 1481726490753090,
"parent_rev": "6b4efb93082da6182f2d645daf285e745524fc9e", "commit_rev":
"dc6cc1b009d356d0b18b05d50aa77d7bf554e15a"}

[commit-bot: I haz the power, 2016-12-14 14:46:03]

Description was changed from

==========
Destroy the old RenderWidgetHostView when swapping out a main frame.

This fixes an issue where the old RenderView is reused by a new remote
subframe. We shouldn't be leaking resources, because now we are already
hiding the unused RenderView in the renderer, when the local main frame
gets detached and the WebViewFrameWidget is closed.

BUG=638375
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Destroy the old RenderWidgetHostView when swapping out a main frame.

This fixes an issue where the old RenderView is reused by a new remote
subframe. We shouldn't be leaking resources, because now we are already
hiding the unused RenderView in the renderer, when the local main frame
gets detached and the WebViewFrameWidget is closed.

BUG=638375
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2496233003
==========

[commit-bot: I haz the power, 2016-12-14 14:46:04]

Committed patchset #13 (id:380001)

[commit-bot: I haz the power, 2016-12-14 14:48:18]

Description was changed from

==========
Destroy the old RenderWidgetHostView when swapping out a main frame.

This fixes an issue where the old RenderView is reused by a new remote
subframe. We shouldn't be leaking resources, because now we are already
hiding the unused RenderView in the renderer, when the local main frame
gets detached and the WebViewFrameWidget is closed.

BUG=638375
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2496233003
==========

to

==========
Destroy the old RenderWidgetHostView when swapping out a main frame.

This fixes an issue where the old RenderView is reused by a new remote
subframe. We shouldn't be leaking resources, because now we are already
hiding the unused RenderView in the renderer, when the local main frame
gets detached and the WebViewFrameWidget is closed.

BUG=638375
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/9431efdc7b0146ef74f75ce160f2ba343ca2d550
Cr-Commit-Position: refs/heads/master@{#438516}
==========

[commit-bot: I haz the power, 2016-12-14 14:48:20]

Patchset 13 (id:??) landed as
https://crrev.com/9431efdc7b0146ef74f75ce160f2ba343ca2d550
Cr-Commit-Position: refs/heads/master@{#438516}

[piman, 2017-03-28 20:57:54]

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

[piman, 2017-03-28 20:57:56]

https://codereview.chromium.org/2496233003/diff/380001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2496233003/diff/380001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:4927: // reused, the RWHV is
created in RenderFrameHostManager::CommitPending.
Belated drive-by...

This seems to be assuming it's ok to destroy and then recreate a RWHV on the
same RWHI. I don't think it's true at all - some state is distributed between
RW, RWHI and RWHV and so RWHI must be 1:1 with RWHV, at least without a fair
amount of refactoring. Among others, the RWHV is holding on to and manages
graphics resources sent by the RW, and it leads to a lot of confusion (and
possibly leaks) if the RW ends up talking to different RWHV without knowing
anything about it.


Why are RVH "reused"? Could we not destroy the RWHV if they are?

[dcheng, 2017-03-28 21:04:25]

On 2017/03/28 20:57:56, piman wrote:
>
https://codereview.chromium.org/2496233003/diff/380001/content/browser/web_co...
> File content/browser/web_contents/web_contents_impl.cc (right):
> 
>
https://codereview.chromium.org/2496233003/diff/380001/content/browser/web_co...
> content/browser/web_contents/web_contents_impl.cc:4927: // reused, the RWHV is
> created in RenderFrameHostManager::CommitPending.
> Belated drive-by...
> 
> This seems to be assuming it's ok to destroy and then recreate a RWHV on the
> same RWHI. I don't think it's true at all - some state is distributed between
> RW, RWHI and RWHV and so RWHI must be 1:1 with RWHV, at least without a fair
> amount of refactoring. Among others, the RWHV is holding on to and manages
> graphics resources sent by the RW, and it leads to a lot of confusion (and
> possibly leaks) if the RW ends up talking to different RWHV without knowing
> anything about it.
> 
> 
> Why are RVH "reused"? Could we not destroy the RWHV if they are?

RVH is reused because the RV in the renderer persists as well. It's non-trivial
to recreate the RV: RV holds blink::WebView which holds the blink::Page, etc.
However, it looks like reusing the RVH/RV is quite problematic as well... while
the long-term answer here is to split the compositing bits out of RenderView
completely, can we get away with a hack in the short-term? e.g. some sort of
"reset compositor" method on RV to bring it back to a clean-enough slate that
things won't explode?

[piman, 2017-03-28 21:25:17]

On 2017/03/28 21:04:25, dcheng wrote:
> On 2017/03/28 20:57:56, piman wrote:
> >
>
https://codereview.chromium.org/2496233003/diff/380001/content/browser/web_co...
> > File content/browser/web_contents/web_contents_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/2496233003/diff/380001/content/browser/web_co...
> > content/browser/web_contents/web_contents_impl.cc:4927: // reused, the RWHV
is
> > created in RenderFrameHostManager::CommitPending.
> > Belated drive-by...
> > 
> > This seems to be assuming it's ok to destroy and then recreate a RWHV on the
> > same RWHI. I don't think it's true at all - some state is distributed
between
> > RW, RWHI and RWHV and so RWHI must be 1:1 with RWHV, at least without a fair
> > amount of refactoring. Among others, the RWHV is holding on to and manages
> > graphics resources sent by the RW, and it leads to a lot of confusion (and
> > possibly leaks) if the RW ends up talking to different RWHV without knowing
> > anything about it.
> > 
> > 
> > Why are RVH "reused"? Could we not destroy the RWHV if they are?
> 
> RVH is reused because the RV in the renderer persists as well. It's
non-trivial
> to recreate the RV: RV holds blink::WebView which holds the blink::Page, etc.

How did it work before? RV and RW have been the same thing forever, why is it
suddenly an issue?

> However, it looks like reusing the RVH/RV is quite problematic as well...
while
> the long-term answer here is to split the compositing bits out of RenderView
> completely, can we get away with a hack in the short-term? e.g. some sort of
> "reset compositor" method on RV to bring it back to a clean-enough slate that
> things won't explode?

I'm not sure. There's more than just the compositor, there are a bunch of things
such as sizing, input events, IME, accessibility, focus, mouse capture, cursor,
etc. where the RWHV is what keeps the state, not the RWHI. Can we recreate all
that state? Maybe? Sounds like a lot of work! Should we? I'm not sure...