Make the RenderViewHostImpl update its visibility after a swap.

Make the RenderViewHostImpl update its visibility after a swap, and bring
consistency across other platform specific implementations of Hide/Swap

This is a contiuation of a previous review (
https://codereview.chromium.org/23866013/) which I've re-uploaded here since something happened to break the diffs in the previous one.

BUG=286259
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=224790

[jamesr, 2013-09-19 01:21:47]

lgtm

[jamesr, 2013-09-19 01:21:58]

On 2013/09/19 01:21:47, jamesr wrote:
> lgtm

(although I'm not an OWNER)

[piman, 2013-09-19 02:26:51]

LGTM, but you accidentally a word in the issue subject.

[zturner1, 2013-09-19 03:43:50]

Looks like DCHECK() on the host_ isn't the right thing to do, since all the
tests are failing.  This makes the logic a bit more complicated.  Is a straight
early-out at the beginning of the function acceptable, or does some code need to
execute even in the presence of a null host_?  I'm not very familiar with this
system.

An example would be RWHVAndroid::WasHidden().  Still call RunAckCallbacks() or
not?  Or in RWHVAura::WasHidden(), still call SetFrameVisibility in the presence
of a null host_, or just early out?

An early-out is equivalent to the existing behavior (since we know it was never
happening otherwise all these functions would have crashed), but some
implementations like RWHVWin::WasShown(), for example, does do some logic before
the null check.

[no sievers, 2013-09-19 19:49:08]

https://codereview.chromium.org/24101003/diff/11001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/24101003/diff/11001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:964: WasShown();
I thought this somehow happened already when I followed the code:
window_->Show() -> SetVisible() -> delegate_->OnWindowTargetVisibilityChanged()
-> WebContentsViewAura::OnWindowTargetVisibilityChanged() ->
web_contents_->WasShown() ->  rwhv->WasShown()

[no sievers, 2013-09-19 19:51:31]

Also what happens if you start the navigation and then the application hides the
tab:

- Navigate creates new RWHV for the WebContents as visible
- the app hides the webcontents (WasHidden()) which goes to the old RWHV
- RWHV swap happens and we make the renderer visible although it's in the
background

[jamesr, 2013-09-19 20:32:14]

On 2013/09/19 19:51:31, sievers wrote:
> Also what happens if you start the navigation and then the application hides
the
> tab:
> 
> - Navigate creates new RWHV for the WebContents as visible
> - the app hides the webcontents (WasHidden()) which goes to the old RWHV
> - RWHV swap happens and we make the renderer visible although it's in the
> background

That shouldn't happen - in the RWHV swap we should update the visibility to the
current state (i.e. hidden in this example).

[zturner, 2013-09-19 20:46:45]

https://codereview.chromium.org/24101003/diff/11001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/24101003/diff/11001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:964: WasShown();
On 2013/09/19 19:49:08, sievers wrote:
> I thought this somehow happened already when I followed the code:
> window_->Show() -> SetVisible() ->
delegate_->OnWindowTargetVisibilityChanged()
> -> WebContentsViewAura::OnWindowTargetVisibilityChanged() ->
> web_contents_->WasShown() ->  rwhv->WasShown()

The delegate is a RWHVA.  So it goes into
RWHVA::OnWindowTargetVisibilityChanged(), which does nothing.

[no sievers, 2013-09-19 20:48:57]

On 2013/09/19 20:32:14, jamesr wrote:
> On 2013/09/19 19:51:31, sievers wrote:
> > Also what happens if you start the navigation and then the application hides
> the
> > tab:
> > 
> > - Navigate creates new RWHV for the WebContents as visible
> > - the app hides the webcontents (WasHidden()) which goes to the old RWHV
> > - RWHV swap happens and we make the renderer visible although it's in the
> > background
> 
> That shouldn't happen - in the RWHV swap we should update the visibility to
the
> current state (i.e. hidden in this example).

CommitPending does not check WebContents::IsHidden() but calls Show()
regardless. Should it check?

[no sievers, 2013-09-19 20:51:01]

On 2013/09/19 20:46:45, zturner wrote:
>
https://codereview.chromium.org/24101003/diff/11001/content/browser/renderer_...
> File content/browser/renderer_host/render_widget_host_view_aura.cc (right):
> 
>
https://codereview.chromium.org/24101003/diff/11001/content/browser/renderer_...
> content/browser/renderer_host/render_widget_host_view_aura.cc:964: WasShown();
> On 2013/09/19 19:49:08, sievers wrote:
> > I thought this somehow happened already when I followed the code:
> > window_->Show() -> SetVisible() ->
> delegate_->OnWindowTargetVisibilityChanged()
> > -> WebContentsViewAura::OnWindowTargetVisibilityChanged() ->
> > web_contents_->WasShown() ->  rwhv->WasShown()
> 
> The delegate is a RWHVA.  So it goes into
> RWHVA::OnWindowTargetVisibilityChanged(), which does nothing.

Ok, I believe you :)
WebContentsViewAura also derives from aura::WindowDelegate, but I guess that is
something else.

[cpu_(ooo_6.6-7.5), 2013-09-20 15:44:06]

reviewer folks, how are we feeling about this change? we got little time until
branch. Thanks.

[piman, 2013-09-20 16:51:29]

https://codereview.chromium.org/24101003/diff/31001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/24101003/diff/31001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_android.cc:202: if (!host_
|| !host_->is_hidden())
host_ is only reset right before |this| is destroyed. Is this really called in
the destructor?

https://codereview.chromium.org/24101003/diff/31001/content/browser/renderer_...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/24101003/diff/31001/content/browser/renderer_...
content/browser/renderer_host/render_widget_host_view_aura.cc:693: if (!host_ ||
!host_->is_hidden())
Where is host_ reset? I can't find the code that does that.

[no sievers, 2013-09-20 17:24:43]

I'm still wondering about the case described in #8.

Unless I'm missing something there are two visibility states: one set by the
embedder when it shows or hides a tab etc. and calls WasShown()/WasHidden(), the
other one set from the view manager when it swaps out the view.

1) delegate_->IsHidden() == false and navigation starts which creates a new RWHV
(!hidden)
2) the user switches tabs, the app calls WasHidden()
3) the manager swaps out the RWHV (the old one was hidden, the new one is
!hidden)

[zturner, 2013-09-20 18:23:23]

On 2013/09/20 17:24:43, sievers wrote:
> I'm still wondering about the case described in #8.
> 
> Unless I'm missing something there are two visibility states: one set by the
> embedder when it shows or hides a tab etc. and calls WasShown()/WasHidden(),
the
> other one set from the view manager when it swaps out the view.
> 
> 1) delegate_->IsHidden() == false and navigation starts which creates a new
RWHV
> (!hidden)
> 2) the user switches tabs, the app calls WasHidden()
> 3) the manager swaps out the RWHV (the old one was hidden, the new one is
> !hidden)

All of this code is brand new to me, so I can't say for sure without testing
this case.
Can I easily create a unit test for this scenario?

[no sievers, 2013-09-20 18:26:09]

On 2013/09/20 18:23:23, zturner wrote:
> On 2013/09/20 17:24:43, sievers wrote:
> > I'm still wondering about the case described in #8.
> > 
> > Unless I'm missing something there are two visibility states: one set by the
> > embedder when it shows or hides a tab etc. and calls WasShown()/WasHidden(),
> the
> > other one set from the view manager when it swaps out the view.
> > 
> > 1) delegate_->IsHidden() == false and navigation starts which creates a new
> RWHV
> > (!hidden)
> > 2) the user switches tabs, the app calls WasHidden()
> > 3) the manager swaps out the RWHV (the old one was hidden, the new one is
> > !hidden)
> 
> All of this code is brand new to me, so I can't say for sure without testing
> this case.
> Can I easily create a unit test for this scenario?

That would be great, but I don't know if it'd be easy. I'd think maybe with a
content_browsertest you could simulate different cases?

Had a quick chat with James and we *think* that it needs a change here:
RenderViewHostManager::CommitPending() {
....
- if (render_view_host_->GetView())
+ if (render_view_host_->GetView() && !delegate_->IsHidden())
    render_view_host_->GetView()->Show();
  else
    delegate_->RenderProcessGoneFromRenderManager(render_view_host_);

[no sievers, 2013-09-20 18:27:07]

On 2013/09/20 18:26:09, sievers wrote:
> On 2013/09/20 18:23:23, zturner wrote:
> > On 2013/09/20 17:24:43, sievers wrote:
> > > I'm still wondering about the case described in #8.
> > > 
> > > Unless I'm missing something there are two visibility states: one set by
the
> > > embedder when it shows or hides a tab etc. and calls
WasShown()/WasHidden(),
> > the
> > > other one set from the view manager when it swaps out the view.
> > > 
> > > 1) delegate_->IsHidden() == false and navigation starts which creates a
new
> > RWHV
> > > (!hidden)
> > > 2) the user switches tabs, the app calls WasHidden()
> > > 3) the manager swaps out the RWHV (the old one was hidden, the new one is
> > > !hidden)
> > 
> > All of this code is brand new to me, so I can't say for sure without testing
> > this case.
> > Can I easily create a unit test for this scenario?
> 
> That would be great, but I don't know if it'd be easy. I'd think maybe with a
> content_browsertest you could simulate different cases?
> 
> Had a quick chat with James and we *think* that it needs a change here:
> RenderViewHostManager::CommitPending() {
> ....
> - if (render_view_host_->GetView())
> + if (render_view_host_->GetView() && !delegate_->IsHidden())
>     render_view_host_->GetView()->Show();
>   else
>     delegate_->RenderProcessGoneFromRenderManager(render_view_host_);

Actually, that's not quite right, you probably don't want to fall into the
else-path if it was hidden in the meantime.

[jamesr, 2013-09-20 18:28:04]

On 2013/09/20 16:51:29, piman wrote:
>
https://codereview.chromium.org/24101003/diff/31001/content/browser/renderer_...
> File content/browser/renderer_host/render_widget_host_view_android.cc (right):
> 
>
https://codereview.chromium.org/24101003/diff/31001/content/browser/renderer_...
> content/browser/renderer_host/render_widget_host_view_android.cc:202: if
(!host_
> || !host_->is_hidden())
> host_ is only reset right before |this| is destroyed. Is this really called in
> the destructor?
> 
>
https://codereview.chromium.org/24101003/diff/31001/content/browser/renderer_...
> File content/browser/renderer_host/render_widget_host_view_aura.cc (right):
> 
>
https://codereview.chromium.org/24101003/diff/31001/content/browser/renderer_...
> content/browser/renderer_host/render_widget_host_view_aura.cc:693: if (!host_
||
> !host_->is_hidden())
> Where is host_ reset? I can't find the code that does that.

render_widget_host_view_win.cc has code to NULL its RenderWidgetHostImpl* out
before destruction (it's called render_widget_host_ on that class) in
RWHVWin::Destroy().  As a result, a random subset of functions in
render_widget_host_view_win.cc null-check and as a probable result of that a
random subset of render_widget_host_view_aura.cc functions also null-check the
host_ pointer.  Looking at the other subclasses:

rwhv_android.cc has a path to NULL out its host_, inconsistent null checks
rwhv_guest.cc has a path to NULL out, inconsistent null checks
rwhv_mac.mm has a path to NULL out, inconsistent null checks


do you know enough about the lifetime of these objects to know why they're all
different and why some functions get null checks and some don't?  I do not and
it's incredibly confusing.

[jamesr, 2013-09-20 18:28:55]

On 2013/09/20 18:27:07, sievers wrote:
> > Had a quick chat with James and we *think* that it needs a change here:
> > RenderViewHostManager::CommitPending() {
> > ....
> > - if (render_view_host_->GetView())
> > + if (render_view_host_->GetView() && !delegate_->IsHidden())
> >     render_view_host_->GetView()->Show();
> >   else
> >     delegate_->RenderProcessGoneFromRenderManager(render_view_host_);
> 
> Actually, that's not quite right, you probably don't want to fall into the
> else-path if it was hidden in the meantime.

Yes, but I also think this is something that's reasonable to do as a separate
patch.  The RenderViewHostManager code is buggy with or without this patch.

[piman, 2013-09-20 18:38:21]

On Fri, Sep 20, 2013 at 11:28 AM, <jamesr@chromium.org> wrote:

> On 2013/09/20 16:51:29, piman wrote:
>
> https://codereview.chromium.**org/24101003/diff/31001/**
>
content/browser/renderer_host/**render_widget_host_view_**android.cc<https://codereview.chromium.org/24101003/diff/31001/content/browser/renderer_host/render_widget_host_view_android.cc>
>
>> File content/browser/renderer_host/**render_widget_host_view_**android.cc
>> (right):
>>
>
>
> https://codereview.chromium.**org/24101003/diff/31001/**
> content/browser/renderer_host/**render_widget_host_view_**
>
android.cc#newcode202<https://codereview.chromium.org/24101003/diff/31001/content/browser/renderer_host/render_widget_host_view_android.cc#newcode202>
>
>> content/browser/renderer_host/**render_widget_host_view_**android.cc:202:
>> if
>>
> (!host_
>
>> || !host_->is_hidden())
>> host_ is only reset right before |this| is destroyed. Is this really
>> called in
>> the destructor?
>>
>
>
> https://codereview.chromium.**org/24101003/diff/31001/**
>
content/browser/renderer_host/**render_widget_host_view_aura.**cc<https://codereview.chromium.org/24101003/diff/31001/content/browser/renderer_host/render_widget_host_view_aura.cc>
>
>> File content/browser/renderer_host/**render_widget_host_view_aura.**cc
>> (right):
>>
>
>
> https://codereview.chromium.**org/24101003/diff/31001/**
> content/browser/renderer_host/**render_widget_host_view_aura.**
>
cc#newcode693<https://codereview.chromium.org/24101003/diff/31001/content/browser/renderer_host/render_widget_host_view_aura.cc#newcode693>
>
>> content/browser/renderer_host/**render_widget_host_view_aura.**cc:693:
>> if (!host_
>>
> ||
>
>> !host_->is_hidden())
>> Where is host_ reset? I can't find the code that does that.
>>
>
> render_widget_host_view_win.cc has code to NULL its RenderWidgetHostImpl*
> out
> before destruction (it's called render_widget_host_ on that class) in
> RWHVWin::Destroy().  As a result, a random subset of functions in
> render_widget_host_view_win.cc null-check and as a probable result of that
> a
> random subset of render_widget_host_view_aura.**cc functions also
> null-check the
> host_ pointer.  Looking at the other subclasses:
>
> rwhv_android.cc has a path to NULL out its host_, inconsistent null checks
> rwhv_guest.cc has a path to NULL out, inconsistent null checks
> rwhv_mac.mm has a path to NULL out, inconsistent null checks
>
>
> do you know enough about the lifetime of these objects to know why they're
> all
> different and why some functions get null checks and some don't?  I do not
> and
> it's incredibly confusing.
>

I don't in the general case. John might. At least on Aura, AFAICT the RWH
outlives the RWHVA. I don't know whether that is inconsistent with other
platforms.
My point is that there's things to do when we get hidden (e.g. release
compositor locks). If it were that the RWH is destroy before we get here,
that's when we need to do them. Testing for NULL here means that it could
happen, but there's no code to deal with it.


>
https://codereview.chromium.**org/24101003/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jamesr, 2013-09-20 18:42:10]

OK, sounds like all the NULL checks in RWHVAura should go away.

[zturner, 2013-09-20 19:17:59]

On 2013/09/20 18:42:10, jamesr wrote:
> OK, sounds like all the NULL checks in RWHVAura should go away.

The null checks were added because without them none of the tests pass.  For
example, it's possible to have a window with no root window, which will cause a
crash, so that requires a check around the RootWindow.  And if there is no root
window, then the window_->GetCompositor() will also be null etc, so a check
around the compositor is necessary.  I don't remember the specific case that
forced me to add a null check around host_, but it definitely was necessary to
prevent a crash.

[zturner, 2013-09-20 19:26:51]

On 2013/09/20 19:17:59, zturner wrote:
> On 2013/09/20 18:42:10, jamesr wrote:
> > OK, sounds like all the NULL checks in RWHVAura should go away.
> 
> The null checks were added because without them none of the tests pass.  For
> example, it's possible to have a window with no root window, which will cause
a
> crash, so that requires a check around the RootWindow.  And if there is no
root
> window, then the window_->GetCompositor() will also be null etc, so a check
> around the compositor is necessary.  I don't remember the specific case that
> forced me to add a null check around host_, but it definitely was necessary to
> prevent a crash.

Actually it looks like I've taken the null checks off of the host, so this means
that piman's statement that the RWH outlives the RWHVA is currently reflected in
the code.  The other null checks are necessary even in the presence of a valid
host though.

[zturner, 2013-09-20 19:29:34]

On 2013/09/20 19:26:51, zturner wrote:
> On 2013/09/20 19:17:59, zturner wrote:
> > On 2013/09/20 18:42:10, jamesr wrote:
> > > OK, sounds like all the NULL checks in RWHVAura should go away.
> > 
> > The null checks were added because without them none of the tests pass.  For
> > example, it's possible to have a window with no root window, which will
cause
> a
> > crash, so that requires a check around the RootWindow.  And if there is no
> root
> > window, then the window_->GetCompositor() will also be null etc, so a check
> > around the compositor is necessary.  I don't remember the specific case that
> > forced me to add a null check around host_, but it definitely was necessary
to
> > prevent a crash.
> 
> Actually it looks like I've taken the null checks off of the host, so this
means
> that piman's statement that the RWH outlives the RWHVA is currently reflected
in
> the code.  The other null checks are necessary even in the presence of a valid
> host though.

Gah, sorry for the spam, I was looking in my editor at a different version of
the
source, so ignore that last message.  I will run tests without the null check
for 
the host_, but leave the rest of the null checks and additionally add the check
in 
the SwapPending for IsHidden(), and hopefully update in a bit.

[zturner, 2013-09-20 22:14:17]

Check whether the web contents are hidden before showing the view, and remove
the null check from RWHVA

[jamesr, 2013-09-20 22:16:55]

lgtm - afaik this fixes both problems

[no sievers, 2013-09-20 22:18:38]

On 2013/09/20 22:14:17, zturner wrote:
> Check whether the web contents are hidden before showing the view, and remove
> the null check from RWHVA

lgtm if James is ok with it.

It might be possible to test all sorts of cases in content_browser_tests (maybe
content/browser/renderer_host/render_view_host_unittest.cc), but we can do it as
a follow-up. I think it's probably worth it considering how often this causes
nasty bugs. Nobody understands this code very well and there are often subtle
differences between the platforms, so unit tests would also serve as
documentation.

[piman, 2013-09-20 22:27:28]

LGTM, tests would be nice. For Aura we can likely unit test (see
content/browser/renderer_host/render_widget_host_view_aura_unittest.cc )
Otherwise, content_browsertests would be definitely nice.

[commit-bot: I haz the power, 2013-09-23 18:03:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zturner@chromium.org/24101003/2001

[commit-bot: I haz the power, 2013-09-23 21:09:35]

Change committed as 224790