New issue
Advanced search Search tips

Issue 742039 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug-Regression

Blocking:
issue 741956



Sign in to add a comment

telemetry_perf_unittests flaky on Android with FATAL:layer_tree_impl.cc(169): Check failed: property_trees()->needs_rebuild

Project Member Reported by kbr@chromium.org, Jul 13 2017

Issue description

This assertion failure is causing flakiness on the android_n5x_swarming_rel trybot, for example:
https://luci-milo.appspot.com/buildbot/tryserver.chromium.android/android_n5x_swarming_rel/219538

It was observed in Issue 741956 which was just filed, but needs to be split off and fixed separately from the larger issue of diagnosing general flakiness on that trybot. It's causing retries on the tryservers and delaying CLs.

Compositing / paint teams, could you please see whether this is caused by a recent change and if so link it to the associated bug? Thanks.

 

Comment 1 by ajuma@chromium.org, Jul 13 2017

Owner: pdr@chromium.org
Status: Assigned (was: Untriaged)
This DCHECK was very recently added in https://chromium-review.googlesource.com/c/560584/

At first glance, the DCHECK seems wrong to me; compositor thread property trees should never have needs_rebuild set as we only build property trees on the main thread.
Cc: sullivan@chromium.org ssid@chromium.org nedngu...@google.com
 Issue 740884  has been merged into this issue.

Comment 3 by pdr@chromium.org, Jul 13 2017

Status: Started (was: Assigned)
I will go ahead and revert the patch while I investigate this.

Comment 4 by kbr@chromium.org, Jul 13 2017

Thanks Philip.

Comment 5 by pdr@chromium.org, Jul 13 2017

Revert is in the queue: https://chromium-review.googlesource.com/c/570698
Project Member

Comment 6 by bugdroid1@chromium.org, Jul 13 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c2cd1a10878f6262cf0d1a4e3bd8ee8ff9ef9b30

commit c2cd1a10878f6262cf0d1a4e3bd8ee8ff9ef9b30
Author: Philip Rogers <pdr@chromium.org>
Date: Thu Jul 13 23:29:58 2017

Revert "Remove LayerImpl dependency from LayerTreeImpl::DidUpdateScrollOffset"

This reverts commit 5657b4033dc807c8e34a76ce44f3d1e1c3afe174.

Reason for revert: Telemetry test flakes, see:  crbug.com/742039 

Original change's description:
> Remove LayerImpl dependency from LayerTreeImpl::DidUpdateScrollOffset
> 
> This patch removes the LayerImpl references from DidUpdateScrollOffset.
> Instead of looking up scroll and transform property nodes with Layers,
> ElementId is used.
> 
> The invariant now used in LayerTreeImpl::DidUpdateScrollOffset (and
> Layer::UpdateScrollOffset) is that a scroll node should exist or the tree
> should be marked as needing a rebuild which will update the scroll node;
> there should be no other codepaths that update scroll offset. A TODO
> has been added to require that a scroll node exists in all cases.
> 
> Tests have been modified to ensure property trees exist before updating
> scroll offset. This should be true in all real cases but is not for
> synthetic testing scenarios.
> 
> A bug has been fixed in UpdateScrollChildPosition where the scroll layer
> was not actually scrollable.
> 
> Bug: 
> Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
> Change-Id: I75d881a0c05a6fd81ae028bdaead9481641d201f
> Reviewed-on: https://chromium-review.googlesource.com/560584
> Commit-Queue: Philip Rogers <pdr@chromium.org>
> Reviewed-by: enne <enne@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#485335}

TBR=pdr@chromium.org,enne@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  742039 
Change-Id: Idd87e6af6a88a3b033864310134b8a379316ab9e
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Reviewed-on: https://chromium-review.googlesource.com/570698
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#486529}
[modify] https://crrev.com/c2cd1a10878f6262cf0d1a4e3bd8ee8ff9ef9b30/cc/test/layer_test_common.h
[modify] https://crrev.com/c2cd1a10878f6262cf0d1a4e3bd8ee8ff9ef9b30/cc/trees/layer_tree_host_common_unittest.cc
[modify] https://crrev.com/c2cd1a10878f6262cf0d1a4e3bd8ee8ff9ef9b30/cc/trees/layer_tree_host_impl_unittest.cc
[modify] https://crrev.com/c2cd1a10878f6262cf0d1a4e3bd8ee8ff9ef9b30/cc/trees/layer_tree_impl.cc

Comment 7 by pdr@chromium.org, Jul 13 2017

Annie,
This test caught something useful because this codepath should never be hit (was planning on removing it entirely in a followup). I'm having trouble actually running the test though.

What command do I run to run "benchmarks.system_health_smoke_test.SystemHealthBenchmarkSmokeTest.system_health.memory_mobile.browse:news:cricbuzz" on my android device?
Philip: 
You can run:
tools/perf/run_tests --browser=android-chromium memory_mobile.browse:news:cricbuzz

To setup your device: go/telemetry-device-setup

Comment 9 by pdr@chromium.org, Jul 14 2017

Thanks Ned. I was able to get a Nexus 6 up and running pretty easily.

Unfortunately this does not reproduce on the Nexus 6. I am going to order a 5x.

Comment 10 by pdr@chromium.org, Jul 14 2017

Was able to get this to crash! Requires a release build for timing. Still is flaky. Stacktrace:
000a4787  logging::LogMessage::~LogMessage()+
00125ae7  cc::LayerTreeImpl::DidUpdateScrollOffset(cc::ElementId)+
00125ba1  cc::LayerTreeImpl::DidUpdateScrollOffset(cc::ElementId)+
001330d1  cc::ScrollTree::ScrollBy(cc::ScrollNode*, gfx::Vector2dF const&, cc::LayerTreeImpl*)+
00122ac9  cc::LayerTreeHostImpl::ScrollSingleNode(cc::ScrollNode*, gfx::Vector2dF const&, gfx::Point const&, bool, cc::ScrollTree*)+
000a0833  cc::Viewport::ScrollBy(gfx::Vector2dF const&, gfx::Point const&, bool, bool, bool)+
0011c3fd  cc::LayerTreeHostImpl::AnimateBrowserControls(base::TimeTicks)+
0011c1b3  cc::LayerTreeHostImpl::AnimateInternal(bool)+
00120153  cc::LayerTreeHostImpl::WillBeginImplFrame(cc::BeginFrameArgs const&)+
000f181f  cc::Scheduler::BeginImplFrame(cc::BeginFrameArgs const&, base::TimeTicks)+
000f1363  cc::Scheduler::BeginImplFrameWithDeadline(cc::BeginFrameArgs const&)+
Project Member

Comment 11 by bugdroid1@chromium.org, Jul 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/80d5a3ec9a3f7a2a94bfaa36aacc62979322125a

commit 80d5a3ec9a3f7a2a94bfaa36aacc62979322125a
Author: pdr <pdr@chromium.org>
Date: Mon Jul 17 19:04:07 2017

Remove LayerImpl dependency from LayerTreeImpl::DidUpdateScrollOffset

This patch removes the LayerImpl references from DidUpdateScrollOffset.
Instead of looking up scroll and transform property nodes with Layers,
ElementId is used.

Tests have been modified to ensure property trees exist before updating
scroll offset. This should be true in all real cases but is not for
synthetic testing scenarios.

A bug has been fixed in UpdateScrollChildPosition where the scroll layer
was not actually scrollable.

This is a reland of [1] which was rolled out because the active tree can
have a scroll offset for an element id that is not present in the pending
tree. The comment in DidUpdateScrollOffset has been updated to reflect
this. This is tested in PushPropertiesToMirrorsCurrentScrollOffset.
[1] https://chromium-review.googlesource.com/c/560584/

Bug:  743241 ,  742039 
Change-Id: Ia13380d749843064f30de76ccfeeb5e72502e6b2
Reviewed-on: https://chromium-review.googlesource.com/572426
Reviewed-by: enne <enne@chromium.org>
Commit-Queue: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487183}
[modify] https://crrev.com/80d5a3ec9a3f7a2a94bfaa36aacc62979322125a/cc/test/layer_test_common.h
[modify] https://crrev.com/80d5a3ec9a3f7a2a94bfaa36aacc62979322125a/cc/trees/layer_tree_host_common_unittest.cc
[modify] https://crrev.com/80d5a3ec9a3f7a2a94bfaa36aacc62979322125a/cc/trees/layer_tree_host_impl_unittest.cc
[modify] https://crrev.com/80d5a3ec9a3f7a2a94bfaa36aacc62979322125a/cc/trees/layer_tree_impl.cc

Comment 12 by pdr@chromium.org, Jul 17 2017

Status: Fixed (was: Started)
Project Member

Comment 13 by bugdroid1@chromium.org, Jul 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4131a1c1ef8bdde8e752353ad6e24771d3b866da

commit 4131a1c1ef8bdde8e752353ad6e24771d3b866da
Author: Philip Rogers <pdr@chromium.org>
Date: Fri Jul 21 19:36:50 2017

Revert "Remove LayerImpl dependency from LayerTreeImpl::DidUpdateScrollOffset"

This reverts commit 80d5a3ec9a3f7a2a94bfaa36aacc62979322125a.

Reason for revert: May be the cause of scroll offset crashes ( crbug.com/746718 )

Original change's description:
> Remove LayerImpl dependency from LayerTreeImpl::DidUpdateScrollOffset
> 
> This patch removes the LayerImpl references from DidUpdateScrollOffset.
> Instead of looking up scroll and transform property nodes with Layers,
> ElementId is used.
> 
> Tests have been modified to ensure property trees exist before updating
> scroll offset. This should be true in all real cases but is not for
> synthetic testing scenarios.
> 
> A bug has been fixed in UpdateScrollChildPosition where the scroll layer
> was not actually scrollable.
> 
> This is a reland of [1] which was rolled out because the active tree can
> have a scroll offset for an element id that is not present in the pending
> tree. The comment in DidUpdateScrollOffset has been updated to reflect
> this. This is tested in PushPropertiesToMirrorsCurrentScrollOffset.
> [1] https://chromium-review.googlesource.com/c/560584/
> 
> Bug:  743241 ,  742039 
> Change-Id: Ia13380d749843064f30de76ccfeeb5e72502e6b2
> Reviewed-on: https://chromium-review.googlesource.com/572426
> Reviewed-by: enne <enne@chromium.org>
> Commit-Queue: Philip Rogers <pdr@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#487183}

TBR=pdr@chromium.org,enne@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  743241 ,  746718 
Change-Id: I3a02c7a22c6db2d2e3eafcb83eace9e37db8c1d6
Reviewed-on: https://chromium-review.googlesource.com/581629
Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488737}
[modify] https://crrev.com/4131a1c1ef8bdde8e752353ad6e24771d3b866da/cc/test/layer_test_common.h
[modify] https://crrev.com/4131a1c1ef8bdde8e752353ad6e24771d3b866da/cc/trees/layer_tree_host_common_unittest.cc
[modify] https://crrev.com/4131a1c1ef8bdde8e752353ad6e24771d3b866da/cc/trees/layer_tree_host_impl_unittest.cc
[modify] https://crrev.com/4131a1c1ef8bdde8e752353ad6e24771d3b866da/cc/trees/layer_tree_impl.cc

Sign in to add a comment