1546856 - Invalidation artifacts when scrolling spotify playlists

Status: VERIFIED FIXED

(Core :: Web Painting, defect, P2)

Description

[Markus Stange [:mstange]]

Attachment: screenshot

Steps to reproduce:

1. Run a browser with WebRender OFF.
2. Go to https://open.spotify.com/user/spotify/playlist/37i9dQZF1DWZHgXl6gjGtA?si=Z-xYyFkYTz6MV-bna3PuZQ
3. Move your mouse over one of the tracks in the list.
4. Scroll down so that your mouse is over the next track.

Expected results:
The hover effect should update properly.

Actual results:
Sometimes, some rectangles in the old hovered row keep the hovered appearance.

Comment 1

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=2d8ce84e0107c99974201c1b67864786b22f3ff8&tochange=94ecc40729d0302ee3953dbab0d280d4a6b174c4

Regressed by: Bug 1504065

:hiro,
Your bunch of patch seems to cause the regression. Can you please look into this?

Comment 2

[Hiroyuki Ikezoe (:hiro)]

I can't reproduce it on my Linux box at all..

Comment 3

[Alice0775 White]

FYI, I can reproduce this on Windows10.
Build ID 20190424215525
User Agent Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:68.0) Gecko/20100101 Firefox/68.0

Comment 4

[Hiroyuki Ikezoe (:hiro)]

The 'background-color animation on the compositor' is disabled by default on beta and releases.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: A reduced test case

I can't still reproduce the issue on Linux, but can reproduce on Windows 10.

It seems the issue is somehow caused by interactions with a position:fixed element.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Another reduced test case

The issue also happens with a position:sticky element.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

CCing kats and Botond since they might have some insight on this, this is related to APZC.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

I can intermittently repro on windows 10 with WR off. But it doesn't seem related to APZ at all, as far as I can tell. Looks like an invalidation problem, as it says in the bug title.

Comment 9

[Hiroyuki Ikezoe (:hiro)]

A place I could find where we need to do something for background-color animations is here in ContainerState::FinishPaintedLayerData. From the function

    // Store the background color                                               
    if (userData->mForcedBackgroundColor != backgroundColor) {                  
      // Invalidate the entire target PaintedLayer since we're changing         
      // the background color                                                   
      (snip)                                                              
      data->mLayer->InvalidateWholeLayer();                                     
    }       

When we are going to reuse PaintedLayer(s) we call InvalidateWholeLayer() if the corresponding background color has changed. But in the case where the background color runs on the compositor, we can't tell the difference with the same way. We need to somehow detect the background color change and call InvalidateWholeLayer().

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1546856 - Invalidate PaintedLayers in case where we reuse the layers and if it's in PaintedLayerDataNode which has async background color animations.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

I think I finally found the real reason why we can't tell the background color changes by async animations.

That is that when a color layer is created for an async background color animation, we add the background rect to mVisibleAboveBackgroundRegion, but unfortunately, when we call PaintedLayerDataNode::FindOpaqueBackgroundColor() for a PaintedLayer which is for a nsDisplayText on the nsDisplayBackgroundColor animation, we return transparent color unfortunately.

NI to Markus to tell whether that makes sense or not.

Comment 12

[Markus Stange [:mstange]]

The behavior you describe makes sense to me. FindOpaqueBackgroundColor() does not want to adopt opaque background colors from active layers. It only wants to adopt background colors from PaintedLayers, or from the "container background" (mContainerUniformBackgroundColor). Any active layers are considered "foreign" things that the PaintedLayerDataTree/Node doesn't really know about - the only information about them that it knows is where they interleave with the PaintedLayerData elements in the PaintedLayerData stack (these are the "visible above" regions).
However, you say "we return transparent color unfortunately"; do you think finding a transparent background color is a problem? I think the transparent case is the "safe" case. If FindOpaqueBackgroundColor() always returned transparency, everything would look correct, we'd just have more transparent layers. It's the non-transparent case that we need to be careful about getting right.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

"unfortunately" I meant is that we can't tell the background color changes with the FindOpaqueBackgroundColor() now.

(In reply to Markus Stange [:mstange] from comment #12)

Any active layers are considered "foreign" things that the PaintedLayerDataTree/Node doesn't really know about

This "foreign" word makes me a bit understand what PaintedLayerDataTree/Node is supposed do.

I don't yet have any ideas how we can tell the background color changes of PaintedLayers and how we can invalidate it.

Comment 14

[Hiroyuki Ikezoe (:hiro)]

I figured out a way to solve this issue. That is that when we create a ColorLayer for async background color animations we store the info in PaintedDisplayItemLayerUserData which, I believe, persists during PaintedLayer lifetime, and tell the info when we reuse the PaintedLayer and if the layer was used to be on the ColorLayer.

I also noticed there is another possibility which cases flickering invalidation artifacts when we create aColorLayer. I am pretty sure we also invalidate reusing PaintedLayers which were used somewhere at that time.

The way I did to propagate the info to PaintedDisplayItemLayerUserData looks awkward because it's using a PaintedLayerDataNode as a mediator, but I can't think of any other good ways.

I should also note that I've tried to write automated tests for this issue, but I haven't been successful it to replicate it by JS, so I am going to fix this issue without automated tests.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Attachment: An attempt to replicate this issue by JS

FWIW, here is an attempt to replicate this issue by JS.

Comment 16

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1546856 - Invalidate PaintedLayers which were/will be upon async background color animations when we are going to reuse it. r?mstange

There are two cases where we need to invalidate reusing PaintedLayers.

1. PaintedLayers were on async background color animations' ColorLayers.
2. PaintedLayers will be on async background color animations' ColorLayers.

The case 1) fixes the invalidation artifacts which persist after the async
animations finished.

The case 2) fixes the flickering invalidation artifacts when an async
backgroud color animation starts.

Comment 17

[Markus Stange [:mstange]]

Attachment: A working JS-driven testcase

Comment 18

[Markus Stange [:mstange]]

I succeeded at making a JS-driven testcase. The following findings allowed me to make one:

• I turned off smooth scrolling in the preferences and found that I could reproduce the bug even with keyboard arrow scrolling, if I had the mouse cursor in the right place. So I tried to make a testcase that scrolled in 50px jumps.
• First, my testcase was making the scroll position adjustment at the same time as the hover class adjustment. But that didn't reproduce the bug. Then I thought about what might be different with :hover, and remembered that the synthesized mouseenter/mouseexit events during scrolling are sent asynchronously and throttled. So I tried making the hover class adjustment one frame after the scroll position adjustment, and it worked!

Interestingly, the testcase doesn't reproduce the bug when I move the mouse during the 500ms setTimeout window. I'm not sure why that is. Maybe it's forcing a display list build + framelayer build by flushing the current scroll position to a different place?
I'm also not sure where the 500ms come from. If I reduce the wait below 400ms, the bug no longer reproduces. Maybe layer activity? But there's no implicit layer activity, there's only the explicit layer activity from the CSS transition. So I'm not sure what's up with that.

I'll try to convert the testcase to a reftest now, and then investigate the bug with rr.

Comment 19

[Markus Stange [:mstange]]

Attachment: testcase

Comment 20

[Markus Stange [:mstange]]

Attachment: 1-Bug_1546856___Reftest__r_hiro.diff

Comment 21

[Markus Stange [:mstange]]

Attachment: reftest log

Here's the relevant excerpt from ./mach reftest layout/reftests/bugs/1546856-1.html --setpref layout.display-list.dump-content=true --setpref nglayout.debug.invalidation=true.

Here's the paint during which the background color gets added:

Starting ProcessPendingUpdates
---- PAINT START ----PresShell(0x10a6ca000), nsView(0x10a681200), nsIWidget(0x106795800)
Display item type Border(0x10a6e2e90) (in layer 0x1067dc000) belongs to an invalidated frame!
Invalidating layer 0x1067dc000: < (x=8, y=158, w=15, h=50); >
Display item type BackgroundColor(0x10a6e2e90) added to layer 0x1067dc000!
Invalidating layer 0x1067dc000: < (x=8, y=158, w=784, h=50); >
[...]
Painting --- after optimization:
SolidColor p=0x10a691b80 f=0x10a6e2020(Viewport(-1)) key=54 bounds(0,0,48000,60000) layerBounds(0,0,48000,60000) visible(0,0,48000,60000) building(0,0,48000,60000) componentAlpha(0,0,0,0) clip() asr() clipChain() uniform ref=0x10a6e2020 agr=0x10a6e2020 (opaque 0,0,48000,60000) (rgba 255,255,255,255) layer=0x1067db800
CompositorHitTestInfo p=0x10a691220 f=0x10a6e2198(HTMLScroll(html)(-1) class:reftest-wait) key=27 bounds(0,0,0,0) layerBounds(0,0,0,0) visible(0,0,48000,60000) building(0,0,48000,60000) componentAlpha(0,0,0,0) clip() asr() clipChain() ref=0x10a6e2020 agr=0x10a6e2020 hitTestInfo(0x1) hitTestArea(0,0,48000,60000)
CompositorHitTestInfo p=0x10a691100 f=0x10a6e20c0(Canvas(html)(-1) class:reftest-wait) key=283 bounds(0,0,0,0) layerBounds(0,6000,0,0) visible(0,0,48000,60000) building(0,0,48000,60000) componentAlpha(0,0,0,0) clip() asr(<0x10a6e2230>) clipChain(0x10a6911e0 <0,0,48000,60000> [root asr]) ref=0x10a6e2020 agr=0x10a6e20c0 hitTestInfo(0x1) hitTestArea(0,-6000,48000,240960)
CompositorHitTestInfo p=0x10a691020 f=0x10a6e20c0(Canvas(html)(-1) class:reftest-wait) key=27 bounds(0,0,0,0) layerBounds(0,6000,0,0) visible(0,0,48000,90000) building(0,0,48000,90000) componentAlpha(0,0,0,0) clip(0,0,48000,90000) asr(<0x10a6e2230>) clipChain(0x10a691300 <0,0,48000,90000> [0x10a6e2230], 0x10a6911e0 <0,0,48000,60000> [root asr]) ref=0x10a6e2020 agr=0x10a6e20c0 hitTestInfo(0x1) hitTestArea(0,-6000,48000,240960)
CanvasBackgroundColor p=0x10a691340 f=0x10a6e20c0(Canvas(html)(-1) class:reftest-wait) key=17 bounds(0,-6000,48000,240960) layerBounds(0,0,48000,240960) visible(0,0,48000,90000) building(0,0,48000,90000) componentAlpha(0,0,0,0) clip(0,0,48000,90000) asr(<0x10a6e2230>) clipChain(0x10a691300 <0,0,48000,90000> [0x10a6e2230], 0x10a6911e0 <0,0,48000,60000> [root asr]) uniform ref=0x10a6e2020 agr=0x10a6e20c0 (opaque 0,-6000,48000,240960) (rgba 255,255,255,255) layer=0x1067db000
FixedPosition p=0x10a691518 f=0x10a6e20c0(Canvas(html)(-1) class:reftest-wait) key=288 bounds(0,0,600,600) layerBounds(0,0,600,600) visible(0,-12000,48000,60000) building(0,-12000,48000,60000) componentAlpha(0,0,0,0) clip(0,0,48000,60000) asr() clipChain(0x10a6911e0 <0,0,48000,60000> [root asr]) ref=0x10a6e2020 agr=0x10a6e2020 (opaque 0,0,600,600) (containerASR ) (scrolltarget 3) layer=0x10679cc00
  CanvasBackgroundImage p=0x10a6913f8 f=0x10a6e20c0(Canvas(html)(-1) class:reftest-wait) key=18 bounds(0,0,600,600) layerBounds(0,0,600,600) visible(0,0,600,600) building(0,0,600,600) componentAlpha(0,0,0,0) clip() asr() clipChain() ref=0x10a6e2020 agr=0x10a6e2020 (opaque 0,0,600,600) layer=0x1067da800
BackgroundColor p=0x10a6917e8 f=0x10a6e2e90(Block(div)(6) id:main class:fill) key=6 bounds(480,3480,47040,3000) layerBounds(480,9480,47040,3000) visible(480,3480,47040,3000) building(0,0,48000,90000) componentAlpha(0,0,0,0) clip(0,0,48000,90000) asr(<0x10a6e2230>) clipChain(0x10a691300 <0,0,48000,90000> [0x10a6e2230], 0x10a6911e0 <0,0,48000,60000> [root asr]) uniform ref=0x10a6e2020 agr=0x10a6e20c0 (opaque 480,3480,47040,3000) (rgba 1,0,0,1) backgroundRect(480,3480,47040,3000) layer=0x1067dc000
Border p=0x10a6919c8 f=0x10a6e2e90(Block(div)(6) id:main class:fill) key=9 bounds(480,3480,900,3000) layerBounds(480,9480,900,3000) visible(480,3480,900,3000) building(0,0,48000,90000) componentAlpha(0,0,0,0) clip(0,0,48000,90000) asr(<0x10a6e2230>) clipChain(0x10a691300 <0,0,48000,90000> [0x10a6e2230], 0x10a6911e0 <0,0,48000,60000> [root asr]) ref=0x10a6e2020 agr=0x10a6e20c0 layer=0x1067dc000
BackgroundColor p=0x10a691a90 f=0x10a6e2dd0(Block(div)(4) class:box) key=6 bounds(480,480,36000,600) layerBounds(480,6480,36000,600) visible(480,480,36000,600) building(0,0,48000,90000) componentAlpha(0,0,0,0) clip(0,0,48000,90000) asr(<0x10a6e2230>) clipChain(0x10a691300 <0,0,48000,90000> [0x10a6e2230], 0x10a6911e0 <0,0,48000,60000> [root asr]) uniform ref=0x10a6e2020 agr=0x10a6e20c0 (opaque 480,480,36000,600) (rgba 0,0,1,1) backgroundRect(480,480,36000,600) layer=0x1067dc000
BackgroundColor p=0x10a6918d8 f=0x10a6e2f50(Block(div)(8) class:box) key=6 bounds(480,6480,600,600) layerBounds(480,12480,600,600) visible(480,6480,600,600) building(0,0,48000,90000) componentAlpha(0,0,0,0) clip(0,0,48000,90000) asr(<0x10a6e2230>) clipChain(0x10a691300 <0,0,48000,90000> [0x10a6e2230], 0x10a6911e0 <0,0,48000,60000> [root asr]) uniform ref=0x10a6e2020 agr=0x10a6e20c0 (opaque 480,6480,600,600) (rgba 0,0,1,1) backgroundRect(480,6480,600,600) layer=0x1067dc000

Here's the paint after the 100px scroll up:

Starting ProcessPendingUpdates
---- PAINT START ----PresShell(0x10a6ca000), nsView(0x10a681200), nsIWidget(0x106795800)
Display item type BackgroundColor(0x10a6e2c50) added to layer 0x1067dc000!
Invalidating layer 0x1067dc000: < (x=8, y=8, w=20, h=10); >
Display item type BackgroundColor(0x10a6e2d10) added to layer 0x1067dc000!
Invalidating layer 0x1067dc000: < (x=8, y=58, w=10, h=10); >
Painting --- before optimization (dirty 0,0,48000,60000):
[...]
Painting --- after optimization:
SolidColor p=0x10a692270 f=0x10a6e2020(Viewport(-1)) key=54 bounds(0,0,48000,60000) layerBounds(0,0,48000,60000) visible(0,0,48000,60000) building(0,0,48000,60000) componentAlpha(0,0,0,0) clip() asr() clipChain() uniform ref=0x10a6e2020 agr=0x10a6e2020 (opaque 0,0,48000,60000) (rgba 255,255,255,255) layer=0x1067db800
CompositorHitTestInfo p=0x10a691020 f=0x10a6e2198(HTMLScroll(html)(-1) class:reftest-wait) key=27 bounds(0,0,0,0) layerBounds(0,0,0,0) visible(0,0,48000,60000) building(0,0,48000,60000) componentAlpha(0,0,0,0) clip() asr() clipChain() ref=0x10a6e2020 agr=0x10a6e2020 hitTestInfo(0x1) hitTestArea(0,0,48000,60000)
CompositorHitTestInfo p=0x10a691100 f=0x10a6e20c0(Canvas(html)(-1) class:reftest-wait) key=283 bounds(0,0,0,0) layerBounds(0,0,0,0) visible(0,0,48000,60000) building(0,0,48000,60000) componentAlpha(0,0,0,0) clip() asr(<0x10a6e2230>) clipChain(0x10a6911e0 <0,0,48000,60000> [root asr]) ref=0x10a6e2020 agr=0x10a6e20c0 hitTestInfo(0x1) hitTestArea(0,0,48000,240960)
CompositorHitTestInfo p=0x10a691220 f=0x10a6e20c0(Canvas(html)(-1) class:reftest-wait) key=27 bounds(0,0,0,0) layerBounds(0,0,0,0) visible(0,0,48000,90000) building(0,0,48000,90000) componentAlpha(0,0,0,0) clip(0,0,48000,90000) asr(<0x10a6e2230>) clipChain(0x10a691300 <0,0,48000,90000> [0x10a6e2230], 0x10a6911e0 <0,0,48000,60000> [root asr]) ref=0x10a6e2020 agr=0x10a6e20c0 hitTestInfo(0x1) hitTestArea(0,0,48000,240960)
CanvasBackgroundColor p=0x10a691ef8 f=0x10a6e20c0(Canvas(html)(-1) class:reftest-wait) key=17 bounds(0,0,48000,240960) layerBounds(0,0,48000,240960) visible(0,0,48000,90000) building(0,0,48000,90000) componentAlpha(0,0,0,0) clip(0,0,48000,90000) asr(<0x10a6e2230>) clipChain(0x10a691300 <0,0,48000,90000> [0x10a6e2230], 0x10a6911e0 <0,0,48000,60000> [root asr]) uniform ref=0x10a6e2020 agr=0x10a6e20c0 (opaque 0,0,48000,240960) (rgba 255,255,255,255) layer=0x1067db000
FixedPosition p=0x10a691518 f=0x10a6e20c0(Canvas(html)(-1) class:reftest-wait) key=288 bounds(0,0,600,600) layerBounds(0,0,600,600) visible(0,0,48000,60000) building(0,0,48000,60000) componentAlpha(0,0,0,0) clip(0,0,48000,60000) asr() clipChain(0x10a6911e0 <0,0,48000,60000> [root asr]) ref=0x10a6e2020 agr=0x10a6e2020 (opaque 0,0,600,600) (containerASR ) (scrolltarget 3) layer=0x10679cc00
  CanvasBackgroundImage p=0x10a6913f8 f=0x10a6e20c0(Canvas(html)(-1) class:reftest-wait) key=18 bounds(0,0,600,600) layerBounds(0,0,600,600) visible(0,0,600,600) building(0,0,600,600) componentAlpha(0,0,0,0) clip() asr() clipChain() ref=0x10a6e2020 agr=0x10a6e2020 (opaque 0,0,600,600) layer=0x1067da800
BackgroundColor p=0x10a6918d8 f=0x10a6e2c50(Block(div)(0) class:box) key=6 bounds(480,480,1200,600) layerBounds(480,480,1200,600) visible(480,480,1200,600) building(0,0,48000,90000) componentAlpha(0,0,0,0) clip(0,0,48000,90000) asr(<0x10a6e2230>) clipChain(0x10a691300 <0,0,48000,90000> [0x10a6e2230], 0x10a6911e0 <0,0,48000,60000> [root asr]) uniform ref=0x10a6e2020 agr=0x10a6e20c0 (opaque 480,480,1200,600) (rgba 0,0,1,1) backgroundRect(480,480,1200,600) layer=0x1067dc000
BackgroundColor p=0x10a691a90 f=0x10a6e2d10(Block(div)(2) class:box) key=6 bounds(480,3480,600,600) layerBounds(480,3480,600,600) visible(480,3480,600,600) building(0,0,48000,90000) componentAlpha(0,0,0,0) clip(0,0,48000,90000) asr(<0x10a6e2230>) clipChain(0x10a691300 <0,0,48000,90000> [0x10a6e2230], 0x10a6911e0 <0,0,48000,60000> [root asr]) uniform ref=0x10a6e2020 agr=0x10a6e20c0 (opaque 480,3480,600,600) (rgba 0,0,1,1) backgroundRect(480,3480,600,600) layer=0x1067dc000
BackgroundColor p=0x10a6917e8 f=0x10a6e2dd0(Block(div)(4) class:box) key=6 bounds(480,6480,36000,600) layerBounds(480,6480,36000,600) visible(480,6480,36000,600) building(0,0,48000,90000) componentAlpha(0,0,0,0) clip(0,0,48000,90000) asr(<0x10a6e2230>) clipChain(0x10a691300 <0,0,48000,90000> [0x10a6e2230], 0x10a6911e0 <0,0,48000,60000> [root asr]) uniform ref=0x10a6e2020 agr=0x10a6e20c0 (opaque 480,6480,36000,600) (rgba 0,0,1,1) backgroundRect(480,6480,36000,600) layer=0x1067dc000
BackgroundColor p=0x10a6916f8 f=0x10a6e2e90(Block(div)(6) id:main class:fill) key=6 bounds(480,9480,47040,3000) layerBounds(480,9480,47040,3000) visible(480,9480,47040,3000) building(0,0,48000,90000) componentAlpha(0,0,0,0) clip(0,0,48000,90000) asr(<0x10a6e2230>) clipChain(0x10a691300 <0,0,48000,90000> [0x10a6e2230], 0x10a6911e0 <0,0,48000,60000> [root asr]) uniform ref=0x10a6e2020 agr=0x10a6e20c0 (opaque 480,9480,47040,3000) (rgba 1,0,0,1) backgroundRect(480,9480,47040,3000) layer=0x1067dc000
Border p=0x10a6919c8 f=0x10a6e2e90(Block(div)(6) id:main class:fill) key=9 bounds(480,9480,900,3000) layerBounds(480,9480,900,3000) visible(480,9480,900,3000) building(0,0,48000,90000) componentAlpha(0,0,0,0) clip(0,0,48000,90000) asr(<0x10a6e2230>) clipChain(0x10a691300 <0,0,48000,90000> [0x10a6e2230], 0x10a6911e0 <0,0,48000,60000> [root asr]) ref=0x10a6e2020 agr=0x10a6e20c0 layer=0x1067dc000
BackgroundColor p=0x10a692348 f=0x10a6e2f50(Block(div)(8) class:box) key=6 bounds(480,12480,600,600) layerBounds(480,12480,600,600) visible(480,12480,600,600) building(0,0,48000,90000) componentAlpha(0,0,0,0) clip(0,0,48000,90000) asr(<0x10a6e2230>) clipChain(0x10a691300 <0,0,48000,90000> [0x10a6e2230], 0x10a6911e0 <0,0,48000,60000> [root asr]) uniform ref=0x10a6e2020 agr=0x10a6e20c0 (opaque 480,12480,600,600) (rgba 0,0,1,1) backgroundRect(480,12480,600,600) layer=0x1067dc000

The bounds.y coordinate for the BackgroundColor item of frame 0x10a6e2e90 has changed from 3480 to 9480 as a result of the scroll, but since the item and the layer both move by the same amount, nothing gets invalidated in the layer. That's correct so far.

Now we get to the paint where the background color starts being animated. The background color item moves from the PaintedLayer to its own ColorLayer. We should be invalidating the same area in the PaintedLayer that we invalidated when the background color item was initially added.

Starting ProcessPendingUpdates
---- PAINT START ----PresShell(0x10a6ca000), nsView(0x10a681200), nsIWidget(0x106795800)
Display item type BackgroundColor(0x10a6e2e90) changed layers 0x1067dc000 to 0x0!
Invalidating layer 0x1067dc000: < (x=8, y=258, w=784, h=50); >
Display item type Border(0x10a6e2e90) (in layer 0x1067dc000) belongs to an invalidated frame!
Invalidating layer 0x1067dc000: < (x=8, y=158, w=15, h=50); >
Painting --- before optimization (dirty 0,0,48000,60000):
[...]
Painting --- after optimization:
SolidColor p=0x10a692270 f=0x10a6e2020(Viewport(-1)) key=54 bounds(0,0,48000,60000) layerBounds(0,0,48000,60000) visible(0,0,48000,60000) building(0,0,48000,60000) componentAlpha(0,0,0,0) clip() asr() clipChain() uniform ref=0x10a6e2020 agr=0x10a6e2020 (opaque 0,0,48000,60000) (rgba 255,255,255,255) layer=0x1067db800
CompositorHitTestInfo p=0x10a691020 f=0x10a6e2198(HTMLScroll(html)(-1) class:) key=27 bounds(0,0,0,0) layerBounds(0,0,0,0) visible(0,0,48000,60000) building(0,0,48000,60000) componentAlpha(0,0,0,0) clip() asr() clipChain() ref=0x10a6e2020 agr=0x10a6e2020 hitTestInfo(0x1) hitTestArea(0,0,48000,60000)
CompositorHitTestInfo p=0x10a691100 f=0x10a6e20c0(Canvas(html)(-1) class:) key=283 bounds(0,0,0,0) layerBounds(0,0,0,0) visible(0,0,48000,60000) building(0,0,48000,60000) componentAlpha(0,0,0,0) clip() asr(<0x10a6e2230>) clipChain(0x10a6911e0 <0,0,48000,60000> [root asr]) ref=0x10a6e2020 agr=0x10a6e20c0 hitTestInfo(0x1) hitTestArea(0,0,48000,240960)
CompositorHitTestInfo p=0x10a691220 f=0x10a6e20c0(Canvas(html)(-1) class:) key=27 bounds(0,0,0,0) layerBounds(0,0,0,0) visible(0,0,48000,90000) building(0,0,48000,90000) componentAlpha(0,0,0,0) clip(0,0,48000,90000) asr(<0x10a6e2230>) clipChain(0x10a691300 <0,0,48000,90000> [0x10a6e2230], 0x10a6911e0 <0,0,48000,60000> [root asr]) ref=0x10a6e2020 agr=0x10a6e20c0 hitTestInfo(0x1) hitTestArea(0,0,48000,240960)
CanvasBackgroundColor p=0x10a691ef8 f=0x10a6e20c0(Canvas(html)(-1) class:) key=17 bounds(0,0,48000,240960) layerBounds(0,0,48000,240960) visible(0,0,48000,90000) building(0,0,48000,90000) componentAlpha(0,0,0,0) clip(0,0,48000,90000) asr(<0x10a6e2230>) clipChain(0x10a691300 <0,0,48000,90000> [0x10a6e2230], 0x10a6911e0 <0,0,48000,60000> [root asr]) uniform ref=0x10a6e2020 agr=0x10a6e20c0 (opaque 0,0,48000,240960) (rgba 255,255,255,255) layer=0x1067db000
FixedPosition p=0x10a691518 f=0x10a6e20c0(Canvas(html)(-1) class:) key=288 bounds(0,0,600,600) layerBounds(0,0,600,600) visible(0,0,48000,60000) building(0,0,48000,60000) componentAlpha(0,0,0,0) clip(0,0,48000,60000) asr() clipChain(0x10a6911e0 <0,0,48000,60000> [root asr]) ref=0x10a6e2020 agr=0x10a6e2020 (opaque 0,0,600,600) (containerASR ) (scrolltarget 3) layer=0x10679cc00
  CanvasBackgroundImage p=0x10a6913f8 f=0x10a6e20c0(Canvas(html)(-1) class:) key=18 bounds(0,0,600,600) layerBounds(0,0,600,600) visible(0,0,600,600) building(0,0,600,600) componentAlpha(0,0,0,0) clip() asr() clipChain() ref=0x10a6e2020 agr=0x10a6e2020 (opaque 0,0,600,600) layer=0x1067da800
BackgroundColor p=0x10a6916f8 f=0x10a6e2e90(Block(div)(6) id:main class:) key=6 bounds(480,9480,47040,3000) layerBounds(480,9480,47040,3000) visible(0,0,48000,90000) building(0,0,48000,90000) componentAlpha(0,0,0,0) clip(0,0,48000,90000) asr(<0x10a6e2230>) clipChain(0x10a691300 <0,0,48000,90000> [0x10a6e2230], 0x10a6911e0 <0,0,48000,60000> [root asr]) uniform ref=0x10a6e2020 agr=0x10a6e20c0 (rgba 1,0,0,1) backgroundRect(480,9480,47040,3000) layer=0x10738e800
Border p=0x10a6919c8 f=0x10a6e2e90(Block(div)(6) id:main class:) key=9 bounds(480,9480,900,3000) layerBounds(480,9480,900,3000) visible(480,9480,900,3000) building(0,0,48000,90000) componentAlpha(0,0,0,0) clip(0,0,48000,90000) asr(<0x10a6e2230>) clipChain(0x10a691300 <0,0,48000,90000> [0x10a6e2230], 0x10a6911e0 <0,0,48000,60000> [root asr]) ref=0x10a6e2020 agr=0x10a6e20c0 layer=0x1067dc000
BackgroundColor p=0x10a6918d8 f=0x10a6e2c50(Block(div)(0) class:box) key=6 bounds(480,480,1200,600) layerBounds(480,480,1200,600) visible(0,0,0,0) building(0,0,48000,90000) componentAlpha(0,0,0,0) clip(0,0,48000,90000) asr(<0x10a6e2230>) clipChain(0x10a691300 <0,0,48000,90000> [0x10a6e2230], 0x10a6911e0 <0,0,48000,60000> [root asr]) uniform ref=0x10a6e2020 agr=0x10a6e20c0 (opaque 480,480,1200,600) (rgba 0,0,1,1) backgroundRect(480,480,1200,600) layer=0x1067dc000
BackgroundColor p=0x10a691a90 f=0x10a6e2d10(Block(div)(2) class:box) key=6 bounds(480,3480,600,600) layerBounds(480,3480,600,600) visible(0,0,0,0) building(0,0,48000,90000) componentAlpha(0,0,0,0) clip(0,0,48000,90000) asr(<0x10a6e2230>) clipChain(0x10a691300 <0,0,48000,90000> [0x10a6e2230], 0x10a6911e0 <0,0,48000,60000> [root asr]) uniform ref=0x10a6e2020 agr=0x10a6e20c0 (opaque 480,3480,600,600) (rgba 0,0,1,1) backgroundRect(480,3480,600,600) layer=0x1067dc000
BackgroundColor p=0x10a6917e8 f=0x10a6e2dd0(Block(div)(4) class:box) key=6 bounds(480,6480,36000,600) layerBounds(480,6480,36000,600) visible(0,0,0,0) building(0,0,48000,90000) componentAlpha(0,0,0,0) clip(0,0,48000,90000) asr(<0x10a6e2230>) clipChain(0x10a691300 <0,0,48000,90000> [0x10a6e2230], 0x10a6911e0 <0,0,48000,60000> [root asr]) uniform ref=0x10a6e2020 agr=0x10a6e20c0 (opaque 480,6480,36000,600) (rgba 0,0,1,1) backgroundRect(480,6480,36000,600) layer=0x1067dc000
BackgroundColor p=0x10a692348 f=0x10a6e2f50(Block(div)(8) class:box) key=6 bounds(480,12480,600,600) layerBounds(480,12480,600,600) visible(0,0,0,0) building(0,0,48000,90000) componentAlpha(0,0,0,0) clip(0,0,48000,90000) asr(<0x10a6e2230>) clipChain(0x10a691300 <0,0,48000,90000> [0x10a6e2230], 0x10a6911e0 <0,0,48000,60000> [root asr]) uniform ref=0x10a6e2020 agr=0x10a6e20c0 (opaque 480,12480,600,600) (rgba 0,0,1,1) backgroundRect(480,12480,600,600) layer=0x1067dc000
Painting --- layer tree:
ClientLayerManager (0x103485130) --- in content order
  ClientContainerLayer (0x10679c000) [visible=< (x=0, y=0, w=800, h=1000); >] [opaqueContent] [presShellResolution=1]
    ClientTiledPaintedLayer (0x1067db800) [clip=(x=0, y=0, w=0, h=0)] [not visible]
      SingleTiledContentClient (0x107382290)
    ClientColorLayer (0x10679d800) [visible=< (x=10, y=0, w=790, h=10); (x=0, y=10, w=800, h=990); >] { hitregion=< (x=0, y=0, w=800, h=1000); >} [opaqueContent] [color=dev_rgba(255, 255, 255, 1.000000)] [bounds=(x=0, y=0, w=800, h=1000)]
    ClientTiledPaintedLayer (0x1067db000) [clip=(x=0, y=0, w=0, h=0)] [not visible] [metrics0={ [metrics={ [cb=(x=0.000000, y=0.000000, w=800.000000, h=1000.000000)] [sr=(x=0.000000, y=0.000000, w=800.000000, h=4016.000000)] [s=(0,0)] [dp=(x=0.000000, y=0.000000, w=800.000000, h=1500.000000)] [cdp=(x=0.000000, y=0.000000, w=0.000000, h=0.000000)] [scrollId=3] [rcd] [z=1] }] [color=dev_rgba(255, 255, 255, 1.000000)] [clip=(x=0, y=0, w=800, h=1000)] }]
      SingleTiledContentClient (0x10a6a5150)
    ClientColorLayer (0x10679c400) [visible=< (x=0, y=0, w=800, h=8); (x=0, y=8, w=8, h=10); (x=28, y=8, w=772, h=10); (x=0, y=18, w=800, h=40); (x=0, y=58, w=8, h=10); (x=18, y=58, w=782, h=10); (x=0, y=68, w=800, h=40); (x=0, y=108, w=8, h=10); (x=608, y=108, w=192, h=10); (x=0, y=118, w=800, h=90); (x=0, y=208, w=8, h=10); (x=18, y=208, w=782, h=10); (x=0, y=218, w=800, h=1282); >] { hitregion=< (x=0, y=0, w=800, h=4016); >} [opaqueContent] [metrics0={ [metrics={ [cb=(x=0.000000, y=0.000000, w=800.000000, h=1000.000000)] [sr=(x=0.000000, y=0.000000, w=800.000000, h=4016.000000)] [s=(0,0)] [dp=(x=0.000000, y=0.000000, w=800.000000, h=1500.000000)] [cdp=(x=0.000000, y=0.000000, w=0.000000, h=0.000000)] [scrollId=3] [rcd] [z=1] }] [color=dev_rgba(255, 255, 255, 1.000000)] [clip=(x=0, y=0, w=800, h=1000)] }] [color=dev_rgba(255, 255, 255, 1.000000)] [bounds=(x=0, y=0, w=800, h=1500)]
    ClientContainerLayer (0x10679cc00) [clip=(x=0, y=0, w=800, h=1000)] [visible=< (x=0, y=0, w=10, h=10); >] [opaqueContent] [presShellResolution=1]
      ClientTiledPaintedLayer (0x1067da800) [visible=< (x=0, y=0, w=10, h=10); >] [opaqueContent] [valid=< (x=0, y=0, w=10, h=10); >]
        SingleTiledContentClient (0x10a6a51a0)
    ClientColorLayer (0x10738e800) [clip=(x=0, y=0, w=800, h=1500)] [visible=< (x=8, y=158, w=784, h=50); >] [metrics0={ [metrics={ [cb=(x=0.000000, y=0.000000, w=800.000000, h=1000.000000)] [sr=(x=0.000000, y=0.000000, w=800.000000, h=4016.000000)] [s=(0,0)] [dp=(x=0.000000, y=0.000000, w=800.000000, h=1500.000000)] [cdp=(x=0.000000, y=0.000000, w=0.000000, h=0.000000)] [scrollId=3] [rcd] [z=1] }] [color=dev_rgba(255, 255, 255, 1.000000)] [clip=(x=0, y=0, w=800, h=1000)] }] [1 animations with id=363448722522113 ] [color=dev_rgba(255, 0, 0, 1.000000)] [bounds=(x=8, y=158, w=784, h=50)]
    ClientTiledPaintedLayer (0x1067dc000) [visible=< (x=8, y=8, w=600, h=210); >] [metrics0={ [metrics={ [cb=(x=0.000000, y=0.000000, w=800.000000, h=1000.000000)] [sr=(x=0.000000, y=0.000000, w=800.000000, h=4016.000000)] [s=(0,0)] [dp=(x=0.000000, y=0.000000, w=800.000000, h=1500.000000)] [cdp=(x=0.000000, y=0.000000, w=0.000000, h=0.000000)] [scrollId=3] [rcd] [z=1] }] [color=dev_rgba(255, 255, 255, 1.000000)] [clip=(x=0, y=0, w=800, h=1000)] }] [valid=< (x=8, y=8, w=600, h=210); >]
      SingleTiledContentClient (0x10a6a5330)
---- PAINT END ----
Ending ProcessPendingUpdates

I think here's the bug!

Invalidating layer 0x1067dc000: < (x=8, y=258, w=784, h=50); >

It should be invalidating at (x=8, y=158, w=784, h=50) - the same rect that got invalidated when the item got added.
So something's causing us to use a bad offset here. Maybe the call to mLayerBuilder->GetLastPaintOffset(t) returns an outdated offset?

I haven't debugged this any further because rr is currently broken on the machine that I wanted to use for this.

Comment 22

[Markus Stange [:mstange]]

Attachment: another testcase, no background color animation

I used printf debugging and think I've found the cause. This is a long-standing issue and I'm surprised we haven't run into it sooner. It has nothing to do with background color animations.

The bug is that mLastPaintOffset on a PaintedLayer's user data is only initialized for the current paint once we start recycling that PaintedLayer. Before we start recycling the PaintedLayer, the value of mLastPaintOffset is the offset from 2 paints ago, not from the previous paint. If frame layer building encounters a display item that has been removed from a painted layer before that painted layer is recycled for the current paint, then we use a wrong offset as we invalidate for that removal.
The only way to encounter such a display item is when it still exists but has moved to a different layer, and if its order in the display list is before any other display items that would be assigned to the painted layer. If it no longer exists, it will only be invalidated once the PaintedLayer is finished and we detect that the item is now unused. And if its order is after the first item for that layer, then the layer will have been recycled by the time we get to the item.
One way to get into the bad situation is to put the item below the rest of the items in the layer, and then move it to a different layer by forcing it into an active container layer, for example with will-change: opacity.

Comment 23

[Markus Stange [:mstange]]

Attachment: don't break this testcase as part of the fix

I've been thinking about a few different fixes but haven't landed on anything good yet. Here's a testcase that makes sure that we still use the right mLastPaintOffset for removed items, which are invalidated in FrameLayerBuilder::WillEndTransaction() which runs long after ContainerState::FinishPaintedLayerData. There should be no red sticking around in this testcase.

Comment 24

[Markus Stange [:mstange]]

Matt, see comment 22 for a description of the long-standing DLBI bug. If you have any preferences for how to fix this, I'd love to hear them.

Comment 25

[Hiroyuki Ikezoe (:hiro)]

Thank you, Markus!

Dropping bug 1504065 from the regression bug, but I keep this bug as a blocker to ship async background color animations (bug 1535532) because this issue is pretty noticeable with async background color animations.

Comment 26

[Matt Woodrow (:mattwoodrow)]

(In reply to Markus Stange [:mstange] from comment #24)

Matt, see comment 22 for a description of the long-standing DLBI bug. If you have any preferences for how to fix this, I'd love to hear them.

Can we tag/track the Painted Layer user data so that we know whether it's been recycled yet? And then use that to decide if we should use mLastPaintOffset, or look it up from the Layer?

Comment 27

[Markus Stange [:mstange]]

I wasn't sure where to put such a tag. If we put it on the layer userData, then we don't know whether the tag is from the current paint or from the previous paint, unless we introduce some kind of paint generation counter. And if we put the tag inside FLB, for example by adding the layers to a hash set, then we have a hashtable lookup on every call to GetLastPaintOffset.
But then I found FLB::AddPaintedLayerItemsEntry() which already tracks all PaintedLayers that we encounter in a paint and does something with them in the FLB destructor. I'm just going to store the last paint offset there.

Comment 28

[Markus Stange [:mstange]]

Attachment: Bug 1546856 - Store mLastPaintOffset at the end of the previous paint rather than at an arbitrary point during the current paint. r=miko

The old code caused GetLastPaintOffset to return an offset from two paints ago if it was called before RecyclePaintedLayer, which can happen when the first item in a painted layer moves to a different layer.

Comment 29

[Markus Stange [:mstange]]

Attachment: Bug 1546856 - Make mLastPaintOffset a Maybe<>. r=miko

Depends on D70241

Comment 30

[Markus Stange [:mstange]]

Attachment: Bug 1546856 - Group related functions and make GetLastPaintOffset static. r=miko

Depends on D70242

Comment 31

[Markus Stange [:mstange]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f7bc9d0911c6420ad7844166e8325276b754650

Comment 32

[Markus Stange [:mstange]]

Attachment: Bug 1546856 - Use GetPaintedDisplayItemLayerUserData in more places and use auto*. r=miko

Depends on D70243

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/93359226f358
Store mLastPaintOffset at the end of the previous paint rather than at an arbitrary point during the current paint. r=miko
https://hg.mozilla.org/integration/autoland/rev/712676bf41f9
Make mLastPaintOffset a Maybe<>. r=miko
https://hg.mozilla.org/integration/autoland/rev/66abbf831780
Group related functions and make GetLastPaintOffset static. r=miko
https://hg.mozilla.org/integration/autoland/rev/b20797b80b4d
Use GetPaintedDisplayItemLayerUserData in more places and use auto*. r=miko

Comment 34

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/93359226f358
https://hg.mozilla.org/mozilla-central/rev/712676bf41f9
https://hg.mozilla.org/mozilla-central/rev/66abbf831780
https://hg.mozilla.org/mozilla-central/rev/b20797b80b4d

Comment 35

[Hiroyuki Ikezoe (:hiro)]

\o/
Thank you so much, Markus!

Comment 36

[Alexandru Trif, Desktop Test Engineering [:atrif]]

Reproduced the issue using Firefox 68.0a1 (20190424215525) on Windows 10x64 and test cases from comment 5, comment 6 and comment 22 by slowly autoscrolling and moving the mouse on the first two test cases.
The issue is verified fixed with Firefox 77.0b3 (20200507233245) on Windows 10x64.