Is this a recent regression? What should we expect in the dev vs canary traces?

I mean, compare the two versions, it seems recent?

I don't know what you should expect. It doesn't seem slower in either trace. Docs takes about 18ms a frame, which shouldn't checkerboard if we prepainted and you're not scrolling very fast.

I guess I'm asking what do you see on dev vs canary?

Dev doesn't checkerboard when you scroll on the trackpad, Canary does. The traces don't look different to me, but someone who works on the compositor should try to reproduce and debug this. :)

Deleted: checkerboards.png 354 KB

	checkerboards.png 354 KB View Download

Any recent clipping changes?

The trace doesn't appear to be taken with "frame viewer" mode enabled, so I can't see the actual tiles/etc.

Are you running both dev and canary with gpu forced enabled? I see the no-checkerboard trace has a SoftwareImageDecodeController slice, which makes me believe that it's running software.

The checkerboard case, there are no such slices. Also in the checkerboard case, it seems that all of the tasks are running on the same thread (which is probably a sign that it's using GPU). 

Good catch, I forced it into GPU mode and I see the same behavior (no checker boards).

Deleted: trace_no-checkerboard-actually-gpu.json.gz 1.9 MB

trace_no-checkerboard-actually-gpu.json.gz 1.9 MB Download

Just to summarize - In Canary this is seen with/without GPU raster - in Dev this is not seen with/without GPU raster. So sounds like a very recent regression, somewhat unrelated to raster type.

Can someone with a mac bisect?

I can repro locally - will bisect.

This bug was introduced with change (confirmed with revert):

crrev.com/1843813002 - 913f48327c7c50fa79df5af74d68fc7af7d3e2f4

It looks like we changed some event processing logic for scrolling - might be messing up Google doc's logic?

Assigning to dtapuska, who authored that change.

This change will actually allow things to run on the compositor that should have been allowed before. Could that have caused the checkerboarding? Since now likely the compositor is scrolling the document.

Hmm... ok, that might help explain things - it seems like there's no prepaint at all - let me investigate this a bit.

So, narrowed this down a bit further. It looks like, with the current change, part of our system thinks we're main thread scrolling, and the other part thinks we're compositor scrolling, which leads to this weird behavior. From what I see, while scrolling this page:

LayerTreeHostImpl::ScrollBegin - ends up deciding that we are compositor scrolling - not scrolling on main thread (scroll_on_main_thread is false at function exit)

InputHandlerManager::HandleInputEvent thinks we are main thread scrolling (we hit CASE INPUT_EVENT_ACK_STATE_NOT_CONSUMED).

It seems like the discrepancy here leads to us compositor scrolling a layer that is only populated for main thread scrolling (has no prepaint).

It seems like LayerTreeHostImpl::ScrollBegin needs to be updated to use the tree-based scrolling logic rather than layer based.

Dave - I'm happy to look into a fix, but wanted to check if you had any ideas on how this should be working?

I can try to debug it Monday; looking at it I don't see anything obvious.

ericrk@ I wasn't able to reproduce the checkboard in canary 51.0.2699.0 on a mac workstation; I'm not sure if something changed or my reproduction steps were different (non-retina). I've looked at the code and I don't fully understand where the scenario was happening in comment #16. I'm hoping you can have a look.

Thanks for taking a look! Double checked this locally - it seems that you do need a retina display to reproduce.

I'd like to understand the intent of the change a bit better: It seemed like we moved from tracking the existence of a wheel handler on individual layers to the layer tree object. I'm probably misunderstanding, but it seems like  we shouldn't be enabling composited scrolling in more cases than before? If any layer in the tree had a scroll handler, wouldn't the tree then have the handler, and wouldn't we still do main-thread-scrolling? Under what cases would we have a tree with a scroll handler, but still do composited scrolling?

Thanks!

For mouse wheel scrolling we now generate gesture scroll begin, update and end.

The mouse wheel event goes via the main thread if it has an event handler.

See:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/blink/input_handler_proxy.cc&sq=package:chromium&l=444&rcl=1459768633

The main thread will return to the browser process if it was handled or not. If it was not handled the gestures will then be generated; and those will goto the compositor and should be handled there.

The change was adjusting finding the appropriate layer; it is not necessary to hit test whether the item has a mouse wheel event handler now. Because those don't really change anything anymore.

Looked at this again. I think the change you made had additional consequences - that check is how LayerTreeHostImpl decides whether we should be compositor or main-thread scrolling - by removing that check LTHI is now deciding that we are compositor scrolling, even though InputHandlerProxy thinks we are main thread scrolling. This appears to be causing the incorrect behavior.

To give more detailed info - for the scroll in question:

On the Blink side of things:
- InputHandlerProxy::HandleMouseWheel returns DID_NOT_HANDLE
- This propogates up to InputHandlerManager::HandleInputEvent, which converts DID_NOT_HANDLE to INPUT_EVENT_ACK_STATE_NOT_CONSUMED.
- ACK_STATE_NOT_CONSUMED causes us to call RendererScheduler with RendererScheduler::InputEventState::EVENT_FORWARDED_TO_MAIN_THREAD
- This appears to be the main-thread-scrolling path

On the CC/LayerTreeHostImpl side of things:
- LayerTreeHostImpl::ScrollBegin calls FindLayerThatIsHitByPoint, which calls FindClosestMatchingLayer, this no longer checks for wheel handlers with the latest change, and returns false / nullptr.
- As no layer is hit by point, ScrollBegin decides that we are not main thread scrolling and proceeds with compositor scroll.

These two sides seem at odds with each other, and it seems like the CC/LayerTreeHostImpl side should be picking up that we are main thread scrolling from somewhere.

Please note the branch point is Friday, should we revert this change so we can solve this in M52?

Probably makes sense to revert for now.

FYI - it appears that docs modifies its content to almost exactly fit the viewport, adjusting the content as you scroll. This pretty much requires main-thread-scrolling to keep things looking correct, as there's no ability to prepaint - so it seems like this scenario should use main thread scrolling.

I don't see that when looking at docs. Why do you think the content is exactly fitting the viewport? I see all the previous pages of the document on either side of the viewport.

Good call - was looking at the wrong DOM elements... thanks!

Reverting the change will cause the regression the issue was out to fix originally.. See  issue 597913 .

Mouse wheel events don't scroll on main thread when gestures are enabled; so even though it indicates RendererScheduler::InputEventState::EVENT_FORWARDED_TO_MAIN_THREAD that is the MouseWheel event only and it will come back to the compositor to scroll with GestureScrollBegin/Update/End.

Ok, so this can happen with or without scroll handlers - we just started exposing this in docs, as we used to do main-thread scrolling, which fixes the issue.

I have a very small repro case which should shed some light on the issue:

<div style="height:700px; overflow-y:scroll; background-color:red;">
  <div style="overflow:hidden; width: 700px; height:2000px;">
    <div style="background-color:blue; width: 700px; height:2000px; position:relative;"></div>
  </div>
</div>

This code is a simplified version of what happens in docs. Basically, we have an outer div, which does overflow:scroll. Immediately inside this we have a tall div which does overflow:hidden. Further inside we have a tall div with position:relative.

This appears to cause Blink to generate a layer that is clipped to the outer div, even though the overflow:hidden div should not be clipping (it is sized to match the tall div).

Will investigate more tomorrow.

This was broken with touch scrolling before dtapuska@'s change.

That change split up wheel events (which are sent from the main thread) and gesture events (which scroll on the compositor thread). Touch input already worked that way, where touch events were send to the main thread, and the gesture events were handled on the compositor thread.

I've attached a trace from M50 with touch on HDPI of reproing on https://output.jsbin.com/gefaduw/quiet, from comment 28.

Deleted: trace_docs-trace.json.gz 3.7 MB

trace_docs-trace.json.gz 3.7 MB Download

Yup, I agree. This change just exposed an existing cc/blink bug in some new cases (we always had the bug for touch scroll). Will investigate further.

Here's a simple repro html.

Deleted: test_scroll.html 11.6 KB

test_scroll.html 11.6 KB View Download

Here's a trace of the repro that shows the layers generated.

Deleted: trace_repro.json.gz 769 KB

trace_repro.json.gz 769 KB Download

This seems like a bug in PaintLayerCompositor/CompositedLayerMapping.  Either this scenario shouldn't be able to scroll on the compositor thread or it should set up to have the correct 

Looking at this earlier with ericrk, it looks like the overflow:hidden div ends up turning into an ancestor clip layer, but its size is 700x700 (the intersection of its clip with its parent clip) instead of 700x2000ish (its real size / the size of the scrolling content).

I think the compositor is doing the right thing here in terms of scrolling.  However, because Blink has intersected these clips, it requires an update from Blink to correct for scrolling, which is why I think it looks wrong for any frame in which there is a compositor scroll but no Blink update.

 Issue 600889  is possibly related.

Yep, we're certainly computing the geometry of the ancestor clipping layer incorrectly.

We set up the arguments to PaintLayerClipper::backgroundClipRect here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp&l=817

In this case we have a composited layer tree that roughly looks like this (note: greatly simplified)
 * composited ancestor
   * scroll clip
     * overflow hidden clip
   * ancestor clipping layer
     * thing that's scrolled, but since the scroller isn't an overflow scroller doesn't force a stacking context.

Normally content with a "scroll parent" doesn't need an ancestor clip layer. The CompositedLayerMapping decides that since its background clip rect is infinite if we ignore the overflow scroll clip, we can just set up the scroll parent pointer and we're happy.

In this case, the overflow hidden layer (as can be seen in the tree) is not an ancestor of the scrolled content, so we need an ACL but the clipper produces the wrong value.

I think the simplest solution is to fix the clipping container we use. In this case, the clipping container should be the scroll parent if we have one. If we do this the clipper can correctly ignore the overflow clip.

I haven't yet tested for unexpected side effects, but the following patch corrects the sizing of the ACL.
https://codereview.chromium.org/1863143002

Looks like this change fixes the issue in my simplified repro case, but not the issue in the real google docs case. There are probably multiple things going on here. Attaching a trace of the google docs case with frameviewer.

Deleted: trace_scroll_docs (1).json.gz 15.5 MB

trace_scroll_docs (1).json.gz 15.5 MB Download

Looks like OS-All based on what I know about Ganesh but please correct me if I'm wrong.  Friendly reminder - all release blockers need an OS label, it's how we track 'em.

Just to give an update:

There are at least two problems here - the first issue was described / solved by Vollick@ in the post above. This first issue had to do with the layers / clips produced by Blink and rendered by CC.

The second problem (Which seems similar to the first) appears to be an issue with the actual recordings generated by Blink - not with the layers it generates. In this second case, the actual layer content is clipped in the recording - each frame Blink updates the recording with a new clip value. In this case, the layer in question is the correct size, we just don't have any content to render into it (as the recording is clipped/blank for most of it).

Here are the generated displaylist that contains the incorrect clip. Also included is an updated trace of the repro scenario.

Deleted: trace_repro_clipped_paint.json.gz 7.2 MB

trace_repro_clipped_paint.json.gz 7.2 MB Download

Deleted: repro_clipped_paint_displayitemlist.json 84.7 KB

repro_clipped_paint_displayitemlist.json 84.7 KB View Download

wangxianzhu@ has offered to take a look at the apparent blink paint issues this week. Thanks!

ericrk@ what's the change you mentioned in #39 and #41 that fixes the issue?

Never mind. I saw it in #36.

I think I found the problem: when the problem occurs, the scrolling contents are not painted on the Scrolling Contents GraphicsLayer (for which PaintLayerPainter will ignore container's overflow clipping), but painted on the foreground GraphicsLayer which is a child of the Scrolling Contents layer.

In PaintLayerPainter, we ignore container's overflow clipping for "Scrolling Contents" layer which is marked GraphicsLayerPaintOverflowContents, but don't ignore overflow clipping for the foreground layer. As the foreground layer also contains overflow contents, we should also mark it GraphicsLayerPaintOverflowContents.

Will create a test case and upload a patch soon.

https://codereview.chromium.org/1895443003/ will fix the Google Docs issue. This is not related to ancestor clipping layer.

ericrk@'s test case in #31 is another issue, related to ancestor clipping layer. Separated into  bug 603997 .

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

commit 9b8e5d585a1a1f61b7afee8353e346f7764cecd2
Author: wangxianzhu <wangxianzhu@chromium.org>
Date: Mon Apr 18 20:26:51 2016

Mark foregroundLayer under scrollingContentsLayer GraphicsLayerPaintOverflowContents

Missing the flag made contents of foregroundLayer under
scrollingContentsLayer incorrectly applied overflow clip, causing
checkboarding during composited scrolling.

BUG= 599682 

Review URL: https://codereview.chromium.org/1895443003

Cr-Commit-Position: refs/heads/master@{#388000}

[modify] https://crrev.com/9b8e5d585a1a1f61b7afee8353e346f7764cecd2/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
[modify] https://crrev.com/9b8e5d585a1a1f61b7afee8353e346f7764cecd2/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp
[modify] https://crrev.com/9b8e5d585a1a1f61b7afee8353e346f7764cecd2/third_party/WebKit/Source/platform/graphics/GraphicsLayer.h

Your change meets the bar and is auto-approved for M51 (branch: 2704)

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

commit 0f9cc7df179a7eb63d1ab8bed167af8b9d346c39
Author: Xianzhu Wang <wangxianzhu@chromium.org>
Date: Tue Apr 19 19:44:15 2016

Mark foregroundLayer under scrollingContentsLayer GraphicsLayerPaintOverflowContents

Missing the flag made contents of foregroundLayer under
scrollingContentsLayer incorrectly applied overflow clip, causing
checkboarding during composited scrolling.

BUG= 599682 

Review URL: https://codereview.chromium.org/1895443003

Cr-Commit-Position: refs/heads/master@{#388000}

Review URL: https://codereview.chromium.org/1902023003 .

Cr-Commit-Position: refs/branch-heads/2704@{#128}
Cr-Branched-From: 6e53600def8f60d8c632fadc70d7c1939ccea347-refs/heads/master@{#386251}

[modify] https://crrev.com/0f9cc7df179a7eb63d1ab8bed167af8b9d346c39/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMapping.cpp
[modify] https://crrev.com/0f9cc7df179a7eb63d1ab8bed167af8b9d346c39/third_party/WebKit/Source/core/layout/compositing/CompositedLayerMappingTest.cpp
[modify] https://crrev.com/0f9cc7df179a7eb63d1ab8bed167af8b9d346c39/third_party/WebKit/Source/platform/graphics/GraphicsLayer.h

 Issue 599340  has been merged into this issue.