Flaky property tree scroll offset crash on android_n5x_swarming_rel |
||||
Issue descriptionOne of my CLs failed in telemetry_perf_unittests on android_n5x_swarming_rel and I found other instances of this crash in the last 200 builds: 07-19 22:07:41.844 8432 8494 F chromium: [FATAL:property_tree.cc(368)] Check failed: property_trees.scroll_tree.current_scroll_offset( scroll_node->element_id) == scroll_offset. Buildbot and swarming links: https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_swarming_rel/builds/224764 https://chromium-swarm.appspot.com/task?id=3775f69bf5acc010&refresh=10&show_raw=1 https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_swarming_rel/builds/224742 https://chromium-swarm.appspot.com/task?id=3775e71eb6485d10&refresh=10&show_raw=1 https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_swarming_rel/builds/224723 https://chromium-swarm.appspot.com/task?id=3775e4f7120dc410&refresh=10&show_raw=1 https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_swarming_rel/builds/224712 https://chromium-swarm.appspot.com/task?id=3775d2200d752d10&refresh=10&show_raw=1 https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_swarming_rel/builds/224665 https://chromium-swarm.appspot.com/task?id=3775aeedd733f910&refresh=10&show_raw=1
,
Jul 20 2017
I ran this 10x on my nexus 6 at ToT and was unable to get it to crash. There have still been recent crashes but it looks like the last 20 builds did not. I'm going to wait and see.
,
Jul 21 2017
This is still happening. I've kicked off 100 runs overnight to try to catch it.
,
Jul 21 2017
No crashes after running all night on a nexus 6 :(
,
Jul 21 2017
Ned, I'm sort of stuck on this because I cannot get this to reproduce locally. Can you send me a 5X to test this locally? I think it is plausible this is related to https://chromium.googlesource.com/chromium/src/+/80d5a3ec9a3f7a2a94bfaa36aacc62979322125a and I'm going to try to roll it out today and see if the crashes stop.
,
Jul 21 2017
Randy: since you are in SFO, can you lend Philip a 5X test device to reproduce the bug?
,
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
,
Jul 21 2017
This is only flakily failing once every ~20 builds so I'm going to wait and see if this revert has worked on Monday. @rnephew, do you have a Nexus 5X I could borrow Monday?
,
Jul 28 2017
I got this to crash locally one time: libbase.cr.so (_ZN4base5debug13BreakDebuggerEv+20) libbase.cr.so (_ZN7logging10LogMessageD1Ev+934) libcc.cr.so libcc.cr.so (_ZN2cc13TransformTree20UpdateLocalTransformEPNS_13TransformNodeE+460) libcc.cr.so (_ZN2cc13TransformTree16UpdateTransformsEi+100) libcc.cr.so (_ZN2cc19draw_property_utils36UpdatePropertyTreesAndRenderSurfacesEPNS_9LayerImplEPNS_13PropertyTreesEb+186) libcc.cr.so libcc.cr.so (_ZN2cc19LayerTreeHostCommon23CalculateDrawPropertiesEPNS0_23CalcDrawPropsImplInputsE+16) libcc.cr.so (_ZN2cc13LayerTreeImpl20UpdateDrawPropertiesEv+736) libcc.cr.so (_ZN2cc17LayerTreeHostImpl47UpdateSyncTreeAfterCommitOrImplSideInvalidationEv+200) libcc.cr.so (_ZN2cc17LayerTreeHostImpl14CommitCompleteEv+50) libcc.cr.so (_ZN2cc9ProxyImpl21ScheduledActionCommitEv+106) libcc.cr.so (_ZN2cc9Scheduler23ProcessScheduledActionsEv+496) libcc.cr.so (_ZN2cc9Scheduler19NotifyReadyToCommitEv+40) libcc.cr.so (_ZN2cc9ProxyImpl25NotifyReadyToCommitOnImplEPNS_15CompletionEventEPNS_13LayerTreeHostEN4base9TimeTicksEb+68) libcc.cr.so libbase.cr.so (_ZN4base5debug13TaskAnnotator7RunTaskEPKcPNS_11PendingTaskE+154) libblink_platform.cr.so (_ZN5blink9scheduler16TaskQueueManager24ProcessTaskFromWorkQueueEPNS0_8internal9WorkQueueEbNS0_7LazyNowEPN4base9TimeTicksE+524) libblink_platform.cr.so (_ZN5blink9scheduler16TaskQueueManager6DoWorkEb+266) libbase.cr.so (_ZN4base5debug13TaskAnnotator7RunTaskEPKcPNS_11PendingTaskE+154) libbase.cr.so (_ZN4base11MessageLoop7RunTaskEPNS_11PendingTaskE+324) libbase.cr.so (_ZN4base11MessageLoop21DeferOrRunPendingTaskENS_11PendingTaskE+134) libbase.cr.so (_ZN4base11MessageLoop6DoWorkEv+274) libbase.cr.so (_ZN4base18MessagePumpDefault3RunEPNS_11MessagePump8DelegateE+98) libbase.cr.so (_ZN4base7RunLoop3RunEv+44) libbase.cr.so (_ZN4base6Thread10ThreadMainEv+244) I suspect this may be fixed by https://chromium-review.googlesource.com/c/583565. I will be running this test 100x overnight to confirm.
,
Jul 28 2017
I saw no crashes with https://chromium-review.googlesource.com/c/583565 applied but that is not a guarantee since this is hard to get to repro. I'm going to wait a day to make sure https://chromium-review.googlesource.com/c/583565 sticks, then I'll re-land https://chromium-review.googlesource.com/c/572426 and manually watch the bots.
,
Aug 1 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/d09da8ef0ef6295d8a7e56147f22c0c70d4369ff commit d09da8ef0ef6295d8a7e56147f22c0c70d4369ff Author: pdr <pdr@chromium.org> Date: Tue Aug 01 17:54:52 2017 Remove LayerImpl dependency from 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 due to flakiness on telemetry_perf_unittests. This flakiness has been fixed by [2]. [1] itself is a reland of [3] 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.googlesource.com/chromium/src/+/80d5a3ec9a3f7a2a94bfaa36aacc62979322125a [2] https://chromium.googlesource.com/chromium/src/+/79fe848764520df822d23d9a234a7710a7ebfe83 [3] https://chromium.googlesource.com/chromium/src/+/5657b4033dc807c8e34a76ce44f3d1e1c3afe174 TBR=enne@chromium.org Bug: 746718 , 743241 Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel Change-Id: Iff2431887585b624264e5699ce0373f68f458929 Reviewed-on: https://chromium-review.googlesource.com/594707 Reviewed-by: Philip Rogers <pdr@chromium.org> Commit-Queue: Philip Rogers <pdr@chromium.org> Cr-Commit-Position: refs/heads/master@{#491048} [modify] https://crrev.com/d09da8ef0ef6295d8a7e56147f22c0c70d4369ff/cc/test/layer_test_common.h [modify] https://crrev.com/d09da8ef0ef6295d8a7e56147f22c0c70d4369ff/cc/trees/layer_tree_host_common_unittest.cc [modify] https://crrev.com/d09da8ef0ef6295d8a7e56147f22c0c70d4369ff/cc/trees/layer_tree_host_impl_unittest.cc [modify] https://crrev.com/d09da8ef0ef6295d8a7e56147f22c0c70d4369ff/cc/trees/layer_tree_impl.cc
,
Aug 1 2017
No flakes so far. I think this can be considered a success.
,
Aug 2 2017
Great work addressing this! |
||||
►
Sign in to add a comment |
||||
Comment 1 by pdr@chromium.org
, Jul 20 2017