Update source frame number even when commits abort

Update source frame number even when commits abort

If a commit aborts, then PictureLayer::update_source_frame_number_ will
be stale since only a finished commit would update the
LTH::source_frame_number_.  This could lead to an issue where a commit
aborts and then on the next frame a layer's size is updated *and* it is
set to not draw.  This causes it to skip being updated (meaning the pile
size is not updated) but the layer bounds have changed and trips an
assert in PushPropertiesTo that tries to make sure that the pile and
layer bounds are the same when a layer has been updated that frame.

The fix here is to update the source frame number even for aborted
commits.  This causes the PushPropertiesTo assert to properly be
ignored.

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

[enne (OOO), 2014-05-29 00:14:54]

I'm not sure how to test this properly.  I could test that the source frame
updates when a commit is aborted, but that's a little silly.  I could also repro
this scenario and make sure it doesn't crash, but I don't like those sorts of
tests.

[danakj, 2014-05-29 07:34:08]

Does this break every unit test that does switch(source frame number)?
On May 29, 2014 2:14 AM, <enne@chromium.org> wrote:

> Reviewers: danakj (OOO_back_june_6), vmpstr,
>
> Message:
> I'm not sure how to test this properly.  I could test that the source frame
> updates when a commit is aborted, but that's a little silly.  I could also
> repro
> this scenario and make sure it doesn't crash, but I don't like those sorts
> of
> tests.
>
> Description:
> Update source frame number even when commits abort
>
> If a commit aborts, then PictureLayer::update_source_frame_number_ will
> be stale since only a finished commit would update the
> LTH::source_frame_number_.  This could lead to an issue where a commit
> aborts and then on the next frame a layer's size is updated *and* it is
> set to not draw.  This causes it to skip being updated (meaning the pile
> size is not updated) but the layer bounds have changed and trips an
> assert in PushPropertiesTo that tries to make sure that the pile and
> layer bounds are the same when a layer has been updated that frame.
>
> The fix here is to update the source frame number even for aborted
> commits.  This causes the PushPropertiesTo assert to properly be
> ignored.
>
> BUG=375675
>
> Please review this at https://codereview.chromium.org/307653005/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files (+1, -2 lines):
>   M cc/trees/layer_tree_host.cc
>
>
> Index: cc/trees/layer_tree_host.cc
> diff --git a/cc/trees/layer_tree_host.cc b/cc/trees/layer_tree_host.cc
> index 82a49b5c35a5cf8638ac6eb380b7834cafc31910..
> 8bb609f625c8fbb0df21419725ae8a507ddd56e6 100644
> --- a/cc/trees/layer_tree_host.cc
> +++ b/cc/trees/layer_tree_host.cc
> @@ -392,8 +392,6 @@ void
LayerTreeHost::FinishCommitOnImplThread(LayerTreeHostImpl*
> host_impl) {
>    }
>
>    micro_benchmark_controller_.ScheduleImplBenchmarks(host_impl);
> -
> -  source_frame_number_++;
>  }
>
>  void LayerTreeHost::WillCommit() {
> @@ -714,6 +712,7 @@ bool LayerTreeHost::UpdateLayers(ResourceUpdateQueue*
> queue) {
>
>    DCHECK(!root_layer()->parent());
>
> +  source_frame_number_++;
>    bool result = UpdateLayers(root_layer(), queue);
>
>    micro_benchmark_controller_.DidUpdateLayers();
>
>
>

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

[danakj, 2014-05-29 07:36:07]

On May 29, 2014 2:14 AM, <enne@chromium.org> wrote:
>
> Reviewers: danakj (OOO_back_june_6), vmpstr,
>
> Message:
> I'm not sure how to test this properly.  I could test that the source
frame
> updates when a commit is aborted, but that's a little silly.

Why do you find this silly? That seems like exactly the sort of thing to
test to me..  Also verifying that a deferred commit causes it to increment
when defer is turned off.

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

[enne (OOO), 2014-05-29 19:29:57]

Moved the increment to DidCommit, which is called regardless of aborting.

https://codereview.chromium.org/307653005/diff/20001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/307653005/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:415: source_frame_number_++;
This is in line with test assumptions, that when DidCommit happens on the main
thread, that the source frame number has already been incremented.

https://codereview.chromium.org/307653005/diff/20001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_unittest_scroll.cc (right):

https://codereview.chromium.org/307653005/diff/20001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_unittest_scroll.cc:244: // The source frame number
still increases even with the abort.
Hey look, we already had tests, and I just had to fix them!

[enne (OOO), 2014-05-29 19:33:34]

On 2014/05/29 07:36:07, danakj (OOO_back_june_6) wrote:

> > I'm not sure how to test this properly.  I could test that the source
> frame
> > updates when a commit is aborted, but that's a little silly.
> 
> Why do you find this silly? That seems like exactly the sort of thing to
> test to me..  Also verifying that a deferred commit causes it to increment
> when defer is turned off.

Oh, silly because that would be testing that the fix is implemented correctly,
not that the fix fixes the bug.

[enne (OOO), 2014-06-03 00:35:51]

Ping?

[danakj, 2014-06-03 10:28:50]

LGTM

[danakj, 2014-06-03 10:28:52]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-06-03 10:29:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/enne@chromium.org/307653005/20001

[commit-bot: I haz the power, 2014-06-03 13:53:26]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-06-03 15:25:53]

Change committed as 274540