So it appears that CompositingLayerAssigner::AssignLayersToBackingsInternal doesn't get called for the scroller after we apply overflow:visible, which means we never notice that it is no longer composited (we normally check this here https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/paint/compositing/compositing_layer_assigner.cc?l=316&rcl=39a3a9de6ce94ae4e5ef308bd056e3e7c0d6b956).

Digging into why this doesn't get called now.

Right, it doesn't even have a PaintLayer. So in https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/layout/layout_box_model_object.cc?l=274&rcl=c32a3d1ec4331f10e0efbd5d979a833f8e1a57ce we take the 'else' branch, destroy our layer, and never notice because we only take notice when the CompositedLayerMapping changes not when the PaintLayer does.

pdr/chrishtr - do you have opinions on an appropriate fix for this? I am currently thinking of adding yet another ScrollTimeline lookup and notification (akin to the one in compositing_layer_assigner.cc) when the PaintLayer is destroyed (maybe in PaintLayer::RemoveOnlyThisLayerAfterStyleChange()), but perhaps there is a cleaner way to handle this?

PaintLayer is a fairly high-level class (*cough, dumping ground) and does a lot more than scrolling. WDYT of refactoring the code to use PaintLayerScrollableArea::UpdateScrollableAreaSet which should get called when things gain or lose scrollability (see the calls to AddScrollableArea/RemoveScrollableArea near the bottom).

Spent a while looking at that suggestion. It doesn't work for the case in #1, because the Layer() is destroyed before UpdateScrollableAreaSet is ever called (as far as I can tell).

Once the style is switched to 'visible', we destroy the layer at:

https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/layout/layout_box_model_object.cc?l=274&rcl=c32a3d1ec4331f10e0efbd5d979a833f8e1a57ce

But the call which gets to the PLSA is down on line 323:

https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/layout/layout_box_model_object.cc?l=323&rcl=10ad99b832eb1f4770cb39f95557dbdcfa96fe69

I'm widening the scope of this bug, because we are seeing the DCHECK trigger in other cases (when it "shouldn't" according to my brain). Specifically it has been seen to trigger in animation-worklet-scroll-timeline.html and animation-worklet-scroll-timeline-overflow-hidden.html, only when running a DCHECK-on debug browser (e.g. timing is a factor it seems).

It seems we have at least one, possibly multiple issues with our expectations around when the scroller ids are set, when nodes are in the tree, etc.

 Issue 854053  has been merged into this issue.

From running multiple layout tests in default mode:

 virtual/threaded/fast/animationworklet/animation-worklet-scroll-timeline-becomes-nonscrollable.html:

STDERR: [1:7:0621/113557.709562:FATAL:scroll_timeline.cc(48)] Check failed: scroll_tree.FindNodeFromElementId(scroller_id). Failed to find (914) in the pending tree

virtual/threaded/fast/animationworklet/animation-worklet-scroll-timeline.html:

STDERR: [1:7:0621/113546.179594:FATAL:scroll_timeline.cc(48)] Check failed: scroll_tree.FindNodeFromElementId(scroller_id). Failed to find (738) in the active tree


The former is the case we already knew about; we miss updating when something becomes no longer scrollable, so we end up failing a lookup in the new pending tree (because we didnt clear our scroller id). But the former case is scary - why are we missing an active tree lookup! 

Investigating.

I believe I understand the active tree case. The issue is that tree activation and ScrollTimeline creation race depending on the raster cost of the tree. We end up initializing the impl ScrollTimeline with active_id == pending_id, but the node is actually only in the pending tree at this point so it later crashes on active tree animation.

Fixing this may be tricky however; we can't just not set active_id at impl ScrollTimeline creation because if the tree has *already* been promoted then the active_id will never be set :(.

Ah, it's not as racy as I thought. It's just bad logic in the code currently. (Thanks to flackr and majidvp for sorting my head out about that). Putting up a CL shortly for that one - and then we can return to the deleted-paint-layer case!

 Issue 854793  has been merged into this issue.

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

commit 23e3596c83c571912f7f29d1760fa4b0a151a1a8
Author: Stephen McGruer <smcgruer@chromium.org>
Date: Fri Jun 22 21:35:47 2018

Fix active/pending id issues in cc::ScrollTimeline

There were two subtle bugs in the original implementation:

i. cc::ScrollTimeline was incorrectly initialized with active_id_ ==
   pending_id_. This is incorrect as the current active tree may not
   yet have the relevant ScrollNode. Instead only pending_id_ should be
   set, and it will be promoted to active via the existing logic when
   the pending tree activates.

ii. cc::ScrollTimeline::PushPropertiesTo incorrectly updated the impl
    thread active_id_ from the main side. This is incorrect, main never
    has a valid active_id_ value (since it never gets updated by pending
    tree activations). This should only push the pending_id_ value,
    which may change e.g. if the scroller went display:none and back.

Bug:  853231 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux-blink-gen-property-trees;luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: Ib7bf4e82e779d11895c373f7c786727c1bcadb9d
Reviewed-on: https://chromium-review.googlesource.com/1110438
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Reviewed-by: Majid Valipour <majidvp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569790}
[modify] https://crrev.com/23e3596c83c571912f7f29d1760fa4b0a151a1a8/cc/animation/scroll_timeline.cc
[modify] https://crrev.com/23e3596c83c571912f7f29d1760fa4b0a151a1a8/cc/animation/scroll_timeline.h
[modify] https://crrev.com/23e3596c83c571912f7f29d1760fa4b0a151a1a8/cc/animation/scroll_timeline_unittest.cc
[modify] https://crrev.com/23e3596c83c571912f7f29d1760fa4b0a151a1a8/third_party/WebKit/LayoutTests/TestExpectations

There are still two known ways to cause the DCHECK in ScrollTimeline::CurrentTime to fire:

i. By making a previously scrollable scroller overflow:visible, and
ii. By enabling blink-gen-property-trees.

This issue will track (i). The second one is tracked in  issue 855718 

I discovered that further to #1 and #2, we actually have a larger problem. Even if we find some way to notice that the scroller has changed scrolling state (or lost its PaintLayer, as per #4 losing your PaintLayer means we don't get a scrolling state change notification), the element will still have an ElementId (because its LayoutObject is still valid).

This means that even if we force an update, we will be passed a valid ElementId, but it won't be in the ScrollTree (because it is no longer scrollable!).

Our options are (at least) the following:

i. Give up on having a DCHECK in the cc code, and accept that there are cases where we will have a scroller id but no node in the tree, OR

ii. Teach WorkletAnimation::UpdateOnCompositor to also check whether the scrollSource is currently scrollable, and pass a base::nullopt if it isn't. (This will also require finding a way to detect when PaintLayers disappear, e.g. via the idea in #2, maybe also needs #3?).

I am on vacation for a few weeks from tomorrow, however, so I will not be working on it until mid-July. Majid, feel free to tackle it (or find someone to tackle it) in the meantime!

Majid - catching up after my vacation, what are your thoughts on comment #14? I am leaning towards (i), given that its all temporary until we have main-thread Animation Worklet.

In the case where we have a scroll source with 'overflow:visible', then we effectively have
a ScrollTimeline with currentTime=NaN. That seems like a valid state to have either on compositor 
or main thread. In another word, it seems valid to have a compositor bound worklet animation 
with a non-scrollable scroll timeline. So (i) seems the right approach to me here. 


I am not sure if I follow how and why main thread animation worklet will make this a
temporary issue.

Ah, thats true, I was still focused on the case where the scroller isn't composited (but is a scroller). I'll send out a CL for approach (i).

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

commit 59ec1d863c20d9f5d543901bab5d35c4882f1b0a
Author: Stephen McGruer <smcgruer@chromium.org>
Date: Wed Jul 11 19:30:20 2018

Remove DCHECK for ScrollTree containment in ScrollTimeline::CurrentTime

The original assertion that if a ScrollTimeline had a non-null scroller
id (either pending or active) then said id would be in the ScrollTree
was actually false. There are cases where the ScrollTimeline can have a
scroller id, but the scrolling element would not be in the ScrollTree -
for example a composited overflow: visible element would meet this
criteria.

As such, this CL converts the DCHECK into an early-exit for this case.

Bug:  853231 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I0ca14a8e8f516357ee9814388514ba2f72fdfd47
Reviewed-on: https://chromium-review.googlesource.com/1133317
Commit-Queue: Stephen McGruer <smcgruer@chromium.org>
Reviewed-by: Majid Valipour <majidvp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574284}
[modify] https://crrev.com/59ec1d863c20d9f5d543901bab5d35c4882f1b0a/cc/animation/scroll_timeline.cc
[add] https://crrev.com/59ec1d863c20d9f5d543901bab5d35c4882f1b0a/third_party/WebKit/LayoutTests/virtual/threaded/fast/animationworklet/animation-worklet-scroll-timeline-non-scrollable-expected.html
[add] https://crrev.com/59ec1d863c20d9f5d543901bab5d35c4882f1b0a/third_party/WebKit/LayoutTests/virtual/threaded/fast/animationworklet/animation-worklet-scroll-timeline-non-scrollable.html