New issue
Advanced search Search tips

Issue 739738 link

Starred by 3 users

Issue metadata

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

Blocking:
issue 505516



Sign in to add a comment

Check failed: scrollbar_ids.vertical == Layer::INVALID_ID (24 vs. -1)Existing scrollbar should have been unregistered.

Project Member Reported by bokan@chromium.org, Jul 6 2017

Issue description

Chrome Version: ToT {#484301}
OS: Android

What steps will reproduce the problem?
(1) Add the flag --enable-blink-features=SetRootScroller, build Clank with DCHECKs on
(2) Visit https://bokand.github.io/rootscroller3.html

[6255:6359:0706/105506.972768:41685375626:FATAL:layer_tree_impl.cc(1676)] Check failed: scrollbar_ids.vertical == Layer::INVALID_ID (24 vs. -1)Existing scrollbar should have been unregistered.
#0 0x7f83e5b5668d base::debug::StackTrace::StackTrace()
#1 0x7f83e5b54ccc base::debug::StackTrace::StackTrace()
#2 0x7f83e5be4e4a logging::LogMessage::~LogMessage()
#3 0x7f83e577b692 cc::LayerTreeImpl::RegisterScrollbar()
#4 0x7f83e547394b cc::ScrollbarLayerImplBase::SetScrollElementId()
#5 0x7f83e54738a0 cc::ScrollbarLayerImplBase::PushPropertiesTo()
#6 0x7f83e54568e7 cc::PaintedOverlayScrollbarLayerImpl::PushPropertiesTo()
#7 0x7f83e580963b cc::PushLayerPropertiesInternal<>()
#8 0x7f83e5809316 cc::TreeSynchronizer::PushLayerProperties()
#9 0x7f83e57433b0 cc::LayerTreeHostImpl::ActivateSyncTree()
#10 0x7f83e57e9c74 cc::ProxyImpl::ScheduledActionActivateSyncTree()
#11 0x7f83e56314f7 cc::Scheduler::ProcessScheduledActions()
#12 0x7f83e5631ce8 cc::Scheduler::NotifyReadyToActivate()
#13 0x7f83e57e571a cc::ProxyImpl::NotifyReadyToActivate()
#14 0x7f83e573b14d cc::LayerTreeHostImpl::NotifyReadyToActivate()
#15 0x7f83e56bb521 cc::TileManager::CheckAndIssueSignals()
#16 0x7f83e550d6bd _ZN4base8internal13FunctorTraitsIMN2cc18LayerTreeFrameSinkEFvvEvE6InvokeIPS3_JEEEvS5_OT_DpOT0_
#17 0x7f83e550d604 _ZN4base8internal12InvokeHelperILb0EvE8MakeItSoIRKMN2cc18LayerTreeFrameSinkEFvvEJPS5_EEEvOT_DpOT0_
#18 0x7f83e56d6595 _ZN4base8internal7InvokerINS0_9BindStateIMN2cc11TileManagerEFvvEJNS0_17UnretainedWrapperIS4_EEEEEFvvEE7RunImplIRKS6_RKNSt3__15tupleIJS8_EEEJLm0EEEEvOT_OT0_NS_13IndexSequenceIJXspT1_EEEE
#19 0x7f83e56d64dc _ZN4base8internal7InvokerINS0_9BindStateIMN2cc11TileManagerEFvvEJNS0_17UnretainedWrapperIS4_EEEEEFvvEE3RunEPNS0_13BindStateBaseE
#20 0x7f83eb0402dd _ZNKR4base8CallbackIFvvELNS_8internal8CopyModeE1ELNS2_10RepeatModeE1EE3RunEv
#21 0x7f83eb072673 cc::UniqueNotifier::Notify()
#22 0x7f83eb04107f _ZN4base8internal13FunctorTraitsIMN2cc21DelayedUniqueNotifierEFvvEvE6InvokeINS_7WeakPtrIS3_EEJEEEvS5_OT_DpOT0_
#23 0x7f83eb07285a _ZN4base8internal12InvokeHelperILb1EvE8MakeItSoIMN2cc14UniqueNotifierEFvvENS_7WeakPtrIS5_EEJEEEvOT_OT0_DpOT1_
#24 0x7f83eb0727f0 _ZN4base8internal7InvokerINS0_9BindStateIMN2cc14UniqueNotifierEFvvEJNS_7WeakPtrIS4_EEEEEFvvEE7RunImplIS6_NSt3__15tupleIJS8_EEEJLm0EEEEvOT_OT0_NS_13IndexSequenceIJXspT1_EEEE
#25 0x7f83eb072749 _ZN4base8internal7InvokerINS0_9BindStateIMN2cc14UniqueNotifierEFvvEJNS_7WeakPtrIS4_EEEEEFvvEE7RunOnceEPNS0_13BindStateBaseE
#26 0x7f83e5b01fa1 _ZNO4base8CallbackIFvvELNS_8internal8CopyModeE0ELNS2_10RepeatModeE0EE3RunEv
#27 0x7f83e5b5b137 base::debug::TaskAnnotator::RunTask()
#28 0x7f83dbe80fd4 blink::scheduler::TaskQueueManager::ProcessTaskFromWorkQueue()
#29 0x7f83dbe7bf8a blink::scheduler::TaskQueueManager::DoWork()
#30 0x7f83dbe88e37 _ZN4base8internal13FunctorTraitsIMN5blink9scheduler16TaskQueueManagerEFvbEvE6InvokeIRKNS_7WeakPtrIS4_EEJRKbEEEvS6_OT_DpOT0_
#31 0x7f83dbe88d95 _ZN4base8internal12InvokeHelperILb1EvE8MakeItSoIRKMN5blink9scheduler16TaskQueueManagerEFvbERKNS_7WeakPtrIS6_EEJRKbEEEvOT_OT0_DpOT1_
#32 0x7f83dbe88d0d _ZN4base8internal7InvokerINS0_9BindStateIMN5blink9scheduler16TaskQueueManagerEFvbEJNS_7WeakPtrIS5_EEbEEEFvvEE7RunImplIRKS7_RKNSt3__15tupleIJS9_bEEEJLm0ELm1EEEEvOT_OT0_NS_13IndexSequenceIJXspT1_EEEE
#33 0x7f83dbe88c1c _ZN4base8internal7InvokerINS0_9BindStateIMN5blink9scheduler16TaskQueueManagerEFvbEJNS_7WeakPtrIS5_EEbEEEFvvEE3RunEPNS0_13BindStateBaseE
#34 0x7f83e5b01fa1 _ZNO4base8CallbackIFvvELNS_8internal8CopyModeE0ELNS2_10RepeatModeE0EE3RunEv
#35 0x7f83e5b5b137 base::debug::TaskAnnotator::RunTask()
#36 0x7f83e5c13bed base::MessageLoop::RunTask()
#37 0x7f83e5c13e77 base::MessageLoop::DeferOrRunPendingTask()
#38 0x7f83e5c14bfa base::MessageLoop::DoWork()
#39 0x7f83e5c1b458 base::MessagePumpDefault::Run()
#40 0x7f83e5c13514 base::MessageLoop::Run()
#41 0x7f83e5cc706d base::RunLoop::Run()
#42 0x7f83e5d92084 base::Thread::Run()
#43 0x7f83e5d92ca2 base::Thread::ThreadMain()
#44 0x7f83e5d73ed1 base::(anonymous namespace)::ThreadFunc()
#45 0x7f83eac6d184 start_thread
#46 0x7f83d81d8ffd clone

Note, I can repro this on linux using content_shell with the following flags:

./out/ChromeDebug/content_shell --single-process --enable-viewport --enable-features=OverlayScrollbar --enable-blink-features=SetRootScroller --enable-threaded-compositing --enable-prefer-compositing-to-lcd-text --enable-fixed-position-compositing ~/bokand.github.io/rootscroller3.html
 

Comment 1 by bokan@chromium.org, Jul 6 2017

Blocking: 505516
Project Member

Comment 2 by bugdroid1@chromium.org, Jul 12 2017

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

commit 8448024fe348b22b8c1e45f6b8b6437cc7853361
Author: David Bokan <bokan@chromium.org>
Date: Wed Jul 12 20:59:48 2017

Unregister cc scrollbar when registering new one

We previously DCHECKed that we could never register a new scrollbar
against a scroll layer that already has a scrollbar. i.e. We expected
that the old scrollbar would be unregistered prior to registering the
new one.

RootScroller can break this assumption on Android. On Android, the
global rootScroller (i.e. OuterViewport) is prevented from creating
its own scrollbars, instead inheriting the VisualViewport-created
scrollbars. In other words, the VisualViewport creates a pair of
scrollbars and they get re-assigned to whatever the current outer
viewport is.

This means that when we change the global rootScroller (for context see
core/page/scrolling/README.md), the previous rootScroller now creates
its own scrollbars and the VisualViewport scrollbars get registered to
the new rootScroller. This then creates a race between moving the
VisualViewport scrollbar registration and new scrollbar registration on
the old rootScroller.

Long-term, the outer viewport shouldn't own the scrollbars but for now
it doesn't hurt to just unregister the scrollbar if we notice the
scroller already has scrollbars registered.

Bug:  739738 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I99e8a95881e7dad19c0318a661da80f441b97b19
Reviewed-on: https://chromium-review.googlesource.com/562018
Reviewed-by: Weiliang Chen <weiliangc@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#486086}
[modify] https://crrev.com/8448024fe348b22b8c1e45f6b8b6437cc7853361/cc/trees/layer_tree_impl.cc

Comment 3 by bokan@chromium.org, Jul 12 2017

Status: Fixed (was: Started)
Project Member

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

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

commit 39a4e54d06758898966637d4c5e1a0ec701306ef
Author: David Bokan <bokan@chromium.org>
Date: Mon Jul 17 15:28:10 2017

Revert "Unregister cc scrollbar when registering new one"

This reverts commit 8448024fe348b22b8c1e45f6b8b6437cc7853361.

Reason for revert: This likely introduced a container-overflow: crbug.com/742520

Original change's description:
> Unregister cc scrollbar when registering new one
> 
> We previously DCHECKed that we could never register a new scrollbar
> against a scroll layer that already has a scrollbar. i.e. We expected
> that the old scrollbar would be unregistered prior to registering the
> new one.
> 
> RootScroller can break this assumption on Android. On Android, the
> global rootScroller (i.e. OuterViewport) is prevented from creating
> its own scrollbars, instead inheriting the VisualViewport-created
> scrollbars. In other words, the VisualViewport creates a pair of
> scrollbars and they get re-assigned to whatever the current outer
> viewport is.
> 
> This means that when we change the global rootScroller (for context see
> core/page/scrolling/README.md), the previous rootScroller now creates
> its own scrollbars and the VisualViewport scrollbars get registered to
> the new rootScroller. This then creates a race between moving the
> VisualViewport scrollbar registration and new scrollbar registration on
> the old rootScroller.
> 
> Long-term, the outer viewport shouldn't own the scrollbars but for now
> it doesn't hurt to just unregister the scrollbar if we notice the
> scroller already has scrollbars registered.
> 
> Bug:  739738 
> Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
> Change-Id: I99e8a95881e7dad19c0318a661da80f441b97b19
> Reviewed-on: https://chromium-review.googlesource.com/562018
> Reviewed-by: Weiliang Chen <weiliangc@chromium.org>
> Commit-Queue: David Bokan <bokan@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#486086}

TBR=bokan@chromium.org,weiliangc@chromium.org

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

Bug:  739738 , 742520, 742678
Change-Id: I1134e17dcb04a3a2cc50f07ecbc0fc79cc74f344
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
Reviewed-on: https://chromium-review.googlesource.com/574547
Commit-Queue: David Bokan <bokan@chromium.org>
Reviewed-by: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487082}
[modify] https://crrev.com/39a4e54d06758898966637d4c5e1a0ec701306ef/cc/trees/layer_tree_impl.cc

Comment 5 by bokan@chromium.org, Jul 19 2017

Status: Assigned (was: Fixed)
Project Member

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

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

commit ec9c793c15edaaf317b00c9bcf32c5472d698475
Author: David Bokan <bokan@chromium.org>
Date: Wed Jul 19 23:16:31 2017

Reland "Unregister cc scrollbar when registering new one"

This was previously landed in:
https://chromium-review.googlesource.com/c/562018/

but was reverted in:
https://chromium-review.googlesource.com/c/574547/

due to a a Clusterfuzz bug. Turns out that calling UnregisterScrollbar
can erase the references we're holding in scrollbar_ids and
scrollbar_layer_id so we have to refresh those.

Bug:  739738 
Change-Id: Ic80018212b1370be6a33a39add19da6848b59fb6
Reviewed-on: https://chromium-review.googlesource.com/578428
Reviewed-by: Weiliang Chen <weiliangc@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488030}
[modify] https://crrev.com/ec9c793c15edaaf317b00c9bcf32c5472d698475/cc/trees/layer_tree_impl.cc

Comment 7 by bokan@chromium.org, Jul 20 2017

Status: Fixed (was: Assigned)

Sign in to add a comment