Fix resize ACK bug

Fix resize ACK bug

It's possible for a view to start up and resize itself to
a nonzero size without ever receiving (and without ever requesting)
a resize ACK:

1) Resize from 0,0 to W,H while physical_backing_size == 0
  -> no paint, no ack because physical backing is empty

2) Resize from W,H to W,H (viewport doesn't change), but with
physical backing changing to nonzero
  -> this now triggers a compositor resize and a paint,
     but the current logic does not think an ACK is necessary

However, receiving at least one resize ACK for every viewport change
is essential. (For example to update
RenderWidgetHostImpl::current_size_.)

BUG=338011
Committed: https://crrev.com/ae6cfa2c5e0cebb5c7f6c7bdf62f8117138f93e1
Cr-Commit-Position: refs/heads/master@{#318296}

[no sievers, 2015-02-21 00:42:57]

sievers@chromium.org changed reviewers:
+ aelias@chromium.org, jaekyun@chromium.org

[no sievers, 2015-02-21 00:44:38]

This is an issue jaekyun is running into with
https://codereview.chromium.org/888793002/ and content_browser_tests.

I applied that patch but reverted the changes to ContentViewRenderView.java,
which seemed to have worked around this problem.

I guess this could use some sort of test. Maybe the 'needs ack' logic should
also be pulled out and shared between browser and renderer? It's getting fairly
complicated.

[Jaekyun Seok (inactive), 2015-02-23 05:35:54]

On 2015/02/21 00:44:38, sievers wrote:
> This is an issue jaekyun is running into with
> https://codereview.chromium.org/888793002/ and content_browser_tests.
> 
> I applied that patch but reverted the changes to ContentViewRenderView.java,
> which seemed to have worked around this problem.
> 
> I guess this could use some sort of test. Maybe the 'needs ack' logic should
> also be pulled out and shared between browser and renderer? It's getting
fairly
> complicated.

lgtm. I confirmed that this CL resolved the resizing problem.

[aelias_OOO_until_Jul13, 2015-02-23 20:47:35]

For reference, the last patches to touch this were
https://chromiumcodereview.appspot.com/14779010 and
https://chromiumcodereview.appspot.com/18413004.  Do we know if something else
changed since then that made them no longer correct.

I'm not really a fan of how this patch makes the logic more complicated than
before.  I wonder if we've finally reached the point that legacy software path
is sufficiently deleted that we can simplify this flow?

[no sievers, 2015-02-23 22:01:53]

I'll do a little more digging and see if this is a regression somehow.

But I think the issue description (i.e. the bug) stands, just have to figure out
what the right way to fix this is.

[no sievers, 2015-02-24 01:54:08]

Unfortunately after reviewing the previous discussions, I think this patch is
actually the logical consequence of how to fix this issue.
Unless there is a way to rethink how we handle physical_backing_size more
generally.

But in goes into the direction of what piman@ said here:
https://chromiumcodereview.appspot.com/18413004/#msg3
"I think the correct fix is for
RenderWidget::Resize so send an ack for #2, which means we may need more
tracking on the renderer side, for the case where we failed to send an ack when
physical_backing_size_ is 0."

That patch already identified the problematic sequence, but it only addressed
mismatches between the needs_resize_ack logic between RWHost and RW.
But the problem remained that we never get an ack ever in this sequence (and was
hardcoded in the unittest), which is bad.

It sounded like we don't want an ack until we have a real backing (and the
compositor actually draws).
In that case we do however need an ack once we resize into a state that changes
this, and we do have a valid backing.

[no sievers, 2015-02-24 01:54:26]

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

[no sievers, 2015-02-24 01:54:40]

On 2015/02/24 01:54:08, sievers wrote:
> Unfortunately after reviewing the previous discussions, I think this patch is
> actually the logical consequence of how to fix this issue.
> Unless there is a way to rethink how we handle physical_backing_size more
> generally.
> 
> But in goes into the direction of what piman@ said here:
> https://chromiumcodereview.appspot.com/18413004/#msg3
> "I think the correct fix is for
> RenderWidget::Resize so send an ack for #2, which means we may need more
> tracking on the renderer side, for the case where we failed to send an ack
when
> physical_backing_size_ is 0."
> 
> That patch already identified the problematic sequence, but it only addressed
> mismatches between the needs_resize_ack logic between RWHost and RW.
> But the problem remained that we never get an ack ever in this sequence (and
was
> hardcoded in the unittest), which is bad.
> 
> It sounded like we don't want an ack until we have a real backing (and the
> compositor actually draws).
> In that case we do however need an ack once we resize into a state that
changes
> this, and we do have a valid backing.

+piman for comment

[piman, 2015-02-24 02:50:35]

On 2015/02/24 01:54:40, sievers wrote:
> On 2015/02/24 01:54:08, sievers wrote:
> > Unfortunately after reviewing the previous discussions, I think this patch
is
> > actually the logical consequence of how to fix this issue.
> > Unless there is a way to rethink how we handle physical_backing_size more
> > generally.
> > 
> > But in goes into the direction of what piman@ said here:
> > https://chromiumcodereview.appspot.com/18413004/#msg3
> > "I think the correct fix is for
> > RenderWidget::Resize so send an ack for #2, which means we may need more
> > tracking on the renderer side, for the case where we failed to send an ack
> when
> > physical_backing_size_ is 0."
> > 
> > That patch already identified the problematic sequence, but it only
addressed
> > mismatches between the needs_resize_ack logic between RWHost and RW.
> > But the problem remained that we never get an ack ever in this sequence (and
> was
> > hardcoded in the unittest), which is bad.
> > 
> > It sounded like we don't want an ack until we have a real backing (and the
> > compositor actually draws).
> > In that case we do however need an ack once we resize into a state that
> changes
> > this, and we do have a valid backing.
> 
> +piman for comment

Couple of things:
1- should we send an Ack in case #1? It may be a little simpler than tracking
more complex state.
2- would it simplify things if we sent a "needs ack" bit in the ViewMsg_Resize?
I think it would reduce the amount of logic duplication. The browser doesn't
have to second-guess the renderer compositor's behavior. The renderer would just
have to track whether an ack is due, and possibly send one immediately if it
knows it won't draw.

WDYT?

[no sievers, 2015-02-25 00:55:58]

On 2015/02/24 02:50:35, piman (Very slow to review) wrote:
> On 2015/02/24 01:54:40, sievers wrote:
> > On 2015/02/24 01:54:08, sievers wrote:
> > > Unfortunately after reviewing the previous discussions, I think this patch
> is
> > > actually the logical consequence of how to fix this issue.
> > > Unless there is a way to rethink how we handle physical_backing_size more
> > > generally.
> > > 
> > > But in goes into the direction of what piman@ said here:
> > > https://chromiumcodereview.appspot.com/18413004/#msg3
> > > "I think the correct fix is for
> > > RenderWidget::Resize so send an ack for #2, which means we may need more
> > > tracking on the renderer side, for the case where we failed to send an ack
> > when
> > > physical_backing_size_ is 0."
> > > 
> > > That patch already identified the problematic sequence, but it only
> addressed
> > > mismatches between the needs_resize_ack logic between RWHost and RW.
> > > But the problem remained that we never get an ack ever in this sequence
(and
> > was
> > > hardcoded in the unittest), which is bad.
> > > 
> > > It sounded like we don't want an ack until we have a real backing (and the
> > > compositor actually draws).
> > > In that case we do however need an ack once we resize into a state that
> > changes
> > > this, and we do have a valid backing.
> > 
> > +piman for comment
> 
> Couple of things:
> 1- should we send an Ack in case #1? It may be a little simpler than tracking
> more complex state.

We can, but not from didCompleteSwapBuffers() since there will be no composite
for an empty physical backing.

> 2- would it simplify things if we sent a "needs ack" bit in the
ViewMsg_Resize?
> I think it would reduce the amount of logic duplication. The browser doesn't
> have to second-guess the renderer compositor's behavior. The renderer would
just
> have to track whether an ack is due, and possibly send one immediately if it
> knows it won't draw.
> 

I like that, done over here: https://codereview.chromium.org/953233002/

[no sievers, 2015-02-26 00:23:06]

Ok rebased over my other patch.

See updated comment and added test. I thought of another potential issue.

https://codereview.chromium.org/948603002/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/948603002/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_impl.cc:642: if
(params->new_size.IsEmpty())
This is purely to make one of the tests happy which verifies that setting the
bounds to the same empty rect repeatedly does not send redundant updates.

I went back and forth on this :)
So we could also set |current_size_| to |new_size| here always if
|!needs_resize_ack|. But then we would end up skipping the ack for the case I
was originally trying to address, i.e. when the view bounds are already nonzero
and physical backing size becomes nonzero. Since that implies a paint it's maybe
better to throttle then.

[piman, 2015-02-26 00:53:00]

On 2015/02/26 00:23:06, sievers wrote:
> Ok rebased over my other patch.
> 
> See updated comment and added test. I thought of another potential issue.
> 
>
https://codereview.chromium.org/948603002/diff/40001/content/browser/renderer...
> File content/browser/renderer_host/render_widget_host_impl.cc (right):
> 
>
https://codereview.chromium.org/948603002/diff/40001/content/browser/renderer...
> content/browser/renderer_host/render_widget_host_impl.cc:642: if
> (params->new_size.IsEmpty())
> This is purely to make one of the tests happy which verifies that setting the
> bounds to the same empty rect repeatedly does not send redundant updates.
> 
> I went back and forth on this :)
> So we could also set |current_size_| to |new_size| here always if
> |!needs_resize_ack|. But then we would end up skipping the ack for the case I
> was originally trying to address, i.e. when the view bounds are already
nonzero
> and physical backing size becomes nonzero. Since that implies a paint it's
maybe
> better to throttle then.

I feel that comparing against current_size_ is inherently racy... At the very
least a lot less intuitive, and back to second-guessing the renderer. While a
resize is in progress, redundant updates are only cancelled because of
throttling, but if throttling doesn't exactly match the renderer behavior, we
may send an update that needs_resize_ack, but the ack will never come because
it's redundant on the renderer side (unless we add code to force an ack in that
case).

I think I agree that we want the ack to come on the second resize, and I can be
convinced that we don't want it on the first. But can we make that logic purely
on the host state (old/new params) rather than the renderer state
(current_size_)? E.g. based on !resize_params->physical_backing_size.IsEmpty()
&& (size_changed || old_resize_params_->physical_backing_size.IsEmpty())?
Maybe I'm missing something, but if not purely on the old/new params, maybe by
keeping an extra bool state, or something?

[no sievers, 2015-02-26 01:07:44]

On 2015/02/26 00:53:00, piman (Very slow to review) wrote:
> On 2015/02/26 00:23:06, sievers wrote:
> > Ok rebased over my other patch.
> > 
> > See updated comment and added test. I thought of another potential issue.
> > 
> >
>
https://codereview.chromium.org/948603002/diff/40001/content/browser/renderer...
> > File content/browser/renderer_host/render_widget_host_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/948603002/diff/40001/content/browser/renderer...
> > content/browser/renderer_host/render_widget_host_impl.cc:642: if
> > (params->new_size.IsEmpty())
> > This is purely to make one of the tests happy which verifies that setting
the
> > bounds to the same empty rect repeatedly does not send redundant updates.
> > 
> > I went back and forth on this :)
> > So we could also set |current_size_| to |new_size| here always if
> > |!needs_resize_ack|. But then we would end up skipping the ack for the case
I
> > was originally trying to address, i.e. when the view bounds are already
> nonzero
> > and physical backing size becomes nonzero. Since that implies a paint it's
> maybe
> > better to throttle then.
> 
> I feel that comparing against current_size_ is inherently racy... At the very
> least a lot less intuitive, and back to second-guessing the renderer. While a
> resize is in progress, redundant updates are only cancelled because of
> throttling, but if throttling doesn't exactly match the renderer behavior, we
> may send an update that needs_resize_ack, but the ack will never come because
> it's redundant on the renderer side (unless we add code to force an ack in
that
> case).
> 

Ok I believe you :)


> I think I agree that we want the ack to come on the second resize, and I can
be
> convinced that we don't want it on the first. But can we make that logic
purely
> on the host state (old/new params) rather than the renderer state
> (current_size_)? E.g. based on !resize_params->physical_backing_size.IsEmpty()
> && (size_changed || old_resize_params_->physical_backing_size.IsEmpty())?
> Maybe I'm missing something, but if not purely on the old/new params, maybe by
> keeping an extra bool state, or something?

Yes, the original case is easily fixed, probably like in patch set 1.

But what about the other hypothetical case? For example, assume there is an
auto-resize happening in the renderer and it sends an UpdateRect.
Then later the browser wants to resize again and it happens to be the old size
(that matches old_params) and it incorrectly thinks it's redundant. Does that
mean OnUpdateRect() should clobber old_params_.new_size with the size from the
renderer - at least if the browser is not waiting for an ack, or if this
UpdateRect happens to imply an ack (it could be the size we requested, or it
could have been clobbered in the renderer)?

[piman, 2015-02-26 01:25:20]

On Wed, Feb 25, 2015 at 5:07 PM, <sievers@chromium.org> wrote:

> On 2015/02/26 00:53:00, piman (Very slow to review) wrote:
>
>> On 2015/02/26 00:23:06, sievers wrote:
>> > Ok rebased over my other patch.
>> >
>> > See updated comment and added test. I thought of another potential
>> issue.
>> >
>> >
>>
>
> https://codereview.chromium.org/948603002/diff/40001/
> content/browser/renderer_host/render_widget_host_impl.cc
>
>> > File content/browser/renderer_host/render_widget_host_impl.cc (right):
>> >
>> >
>>
>
> https://codereview.chromium.org/948603002/diff/40001/
> content/browser/renderer_host/render_widget_host_impl.cc#newcode642
>
>> > content/browser/renderer_host/render_widget_host_impl.cc:642: if
>> > (params->new_size.IsEmpty())
>> > This is purely to make one of the tests happy which verifies that
>> setting
>>
> the
>
>> > bounds to the same empty rect repeatedly does not send redundant
>> updates.
>> >
>> > I went back and forth on this :)
>> > So we could also set |current_size_| to |new_size| here always if
>> > |!needs_resize_ack|. But then we would end up skipping the ack for the
>> case
>>
> I
>
>> > was originally trying to address, i.e. when the view bounds are already
>> nonzero
>> > and physical backing size becomes nonzero. Since that implies a paint
>> it's
>> maybe
>> > better to throttle then.
>>
>
>  I feel that comparing against current_size_ is inherently racy... At the
>> very
>> least a lot less intuitive, and back to second-guessing the renderer.
>> While a
>> resize is in progress, redundant updates are only cancelled because of
>> throttling, but if throttling doesn't exactly match the renderer
>> behavior, we
>> may send an update that needs_resize_ack, but the ack will never come
>> because
>> it's redundant on the renderer side (unless we add code to force an ack in
>>
> that
>
>> case).
>>
>
>
> Ok I believe you :)
>
>
>  I think I agree that we want the ack to come on the second resize, and I
>> can
>>
> be
>
>> convinced that we don't want it on the first. But can we make that logic
>>
> purely
>
>> on the host state (old/new params) rather than the renderer state
>> (current_size_)? E.g. based on !resize_params->physical_
>> backing_size.IsEmpty()
>> && (size_changed || old_resize_params_->physical_backing_size.IsEmpty())?
>> Maybe I'm missing something, but if not purely on the old/new params,
>> maybe by
>> keeping an extra bool state, or something?
>>
>
> Yes, the original case is easily fixed, probably like in patch set 1.
>
> But what about the other hypothetical case? For example, assume there is an
> auto-resize happening in the renderer and it sends an UpdateRect.
> Then later the browser wants to resize again and it happens to be the old
> size
> (that matches old_params) and it incorrectly thinks it's redundant. Does
> that
> mean OnUpdateRect() should clobber old_params_.new_size with the size from
> the
> renderer - at least if the browser is not waiting for an ack, or if this
> UpdateRect happens to imply an ack (it could be the size we requested, or
> it
> could have been clobbered in the renderer)?
>

If the renderer enables auto-resize, I'm pretty sure we stop sending
resizes from the browser side (see RenderWidgetHostImpl::WasResized).
Now, maybe we should reset old_params_.new_size in SetAutoResize(false,
...). RenderViewHostImpl::DisableAutoResize will call RWHV::SetSize, which
will call WasResized, so we may currently be sending a redundant resize.


>
>
> https://codereview.chromium.org/948603002/
>

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

[no sievers, 2015-02-26 02:56:53]

On 2015/02/26 01:25:20, piman (Very slow to review) wrote:
> On Wed, Feb 25, 2015 at 5:07 PM, <mailto:sievers@chromium.org> wrote:
> 
> > On 2015/02/26 00:53:00, piman (Very slow to review) wrote:
> >
> >> On 2015/02/26 00:23:06, sievers wrote:
> >> > Ok rebased over my other patch.
> >> >
> >> > See updated comment and added test. I thought of another potential
> >> issue.
> >> >
> >> >
> >>
> >
> > https://codereview.chromium.org/948603002/diff/40001/
> > content/browser/renderer_host/render_widget_host_impl.cc
> >
> >> > File content/browser/renderer_host/render_widget_host_impl.cc (right):
> >> >
> >> >
> >>
> >
> > https://codereview.chromium.org/948603002/diff/40001/
> > content/browser/renderer_host/render_widget_host_impl.cc#newcode642
> >
> >> > content/browser/renderer_host/render_widget_host_impl.cc:642: if
> >> > (params->new_size.IsEmpty())
> >> > This is purely to make one of the tests happy which verifies that
> >> setting
> >>
> > the
> >
> >> > bounds to the same empty rect repeatedly does not send redundant
> >> updates.
> >> >
> >> > I went back and forth on this :)
> >> > So we could also set |current_size_| to |new_size| here always if
> >> > |!needs_resize_ack|. But then we would end up skipping the ack for the
> >> case
> >>
> > I
> >
> >> > was originally trying to address, i.e. when the view bounds are already
> >> nonzero
> >> > and physical backing size becomes nonzero. Since that implies a paint
> >> it's
> >> maybe
> >> > better to throttle then.
> >>
> >
> >  I feel that comparing against current_size_ is inherently racy... At the
> >> very
> >> least a lot less intuitive, and back to second-guessing the renderer.
> >> While a
> >> resize is in progress, redundant updates are only cancelled because of
> >> throttling, but if throttling doesn't exactly match the renderer
> >> behavior, we
> >> may send an update that needs_resize_ack, but the ack will never come
> >> because
> >> it's redundant on the renderer side (unless we add code to force an ack in
> >>
> > that
> >
> >> case).
> >>
> >
> >
> > Ok I believe you :)
> >
> >
> >  I think I agree that we want the ack to come on the second resize, and I
> >> can
> >>
> > be
> >
> >> convinced that we don't want it on the first. But can we make that logic
> >>
> > purely
> >
> >> on the host state (old/new params) rather than the renderer state
> >> (current_size_)? E.g. based on !resize_params->physical_
> >> backing_size.IsEmpty()
> >> && (size_changed || old_resize_params_->physical_backing_size.IsEmpty())?
> >> Maybe I'm missing something, but if not purely on the old/new params,
> >> maybe by
> >> keeping an extra bool state, or something?
> >>
> >
> > Yes, the original case is easily fixed, probably like in patch set 1.
> >
> > But what about the other hypothetical case? For example, assume there is an
> > auto-resize happening in the renderer and it sends an UpdateRect.
> > Then later the browser wants to resize again and it happens to be the old
> > size
> > (that matches old_params) and it incorrectly thinks it's redundant. Does
> > that
> > mean OnUpdateRect() should clobber old_params_.new_size with the size from
> > the
> > renderer - at least if the browser is not waiting for an ack, or if this
> > UpdateRect happens to imply an ack (it could be the size we requested, or
> > it
> > could have been clobbered in the renderer)?
> >
> 
> If the renderer enables auto-resize, I'm pretty sure we stop sending
> resizes from the browser side (see RenderWidgetHostImpl::WasResized).
> Now, maybe we should reset old_params_.new_size in SetAutoResize(false,
> ...). RenderViewHostImpl::DisableAutoResize will call RWHV::SetSize, which
> will call WasResized, so we may currently be sending a redundant resize.
> 

Ok good. ptal, ps4.

[piman, 2015-02-26 19:28:49]

lgtm

[no sievers, 2015-02-26 19:31:23]

The CQ bit was checked by sievers@chromium.org

[no sievers, 2015-02-26 19:31:24]

The patchset sent to the CQ was uploaded after l-g-t-m from jaekyun@chromium.org
Link to the patchset: https://codereview.chromium.org/948603002/#ps60001 (title:
" ")

[commit-bot: I haz the power, 2015-02-26 19:32:23]

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

[commit-bot: I haz the power, 2015-02-26 20:22:41]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-26 20:23:49]

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