Closed Bug 1546856 Opened 5 years ago Closed 4 years ago

Invalidation artifacts when scrolling spotify playlists

Categories

(Core :: Web Painting, defect, P2)

Product:
Core
Component:
Web Painting
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla77
Tracking Flags:
Tracking Status
firefox-esr60 --- unaffected
firefox66 --- disabled
firefox67 --- disabled
firefox68 --- disabled
firefox77 --- verified

People

(Reporter: mstange, Assigned: mstange)

References

Details

(Whiteboard: [layout:backlog:77])

Attachments

(14 files, 2 obsolete files)

screenshot
1.26 MB, image/png
Details
A reduced test case
1.93 KB, text/html
Details
Another reduced test case
1.91 KB, text/html
Details
An attempt to replicate this issue by JS
1.09 KB, text/html
Details
A working JS-driven testcase
2.61 KB, text/html
Details
testcase
1.18 KB, text/html
Details
1-Bug_1546856___Reftest__r_hiro.diff
3.90 KB, patch
Details | Diff | Splinter Review
reftest log
199.76 KB, text/plain
Details
another testcase, no background color animation
1.09 KB, text/html
Details
don't break this testcase as part of the fix
884 bytes, text/html
Details
Bug 1546856 - Store mLastPaintOffset at the end of the previous paint rather than at an arbitrary point during the current paint. r=miko
47 bytes, text/x-phabricator-request
Details | Review
Bug 1546856 - Make mLastPaintOffset a Maybe<>. r=miko
47 bytes, text/x-phabricator-request
Details | Review
Bug 1546856 - Group related functions and make GetLastPaintOffset static. r=miko
47 bytes, text/x-phabricator-request
Details | Review
Bug 1546856 - Use GetPaintedDisplayItemLayerUserData in more places and use auto*. r=miko
47 bytes, text/x-phabricator-request
Details | Review
Attached image 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.

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?

Has Regression Range: --- → yes
Has STR: --- → yes
status-firefox66: --- → wontfix
status-firefox67: --- → wontfix
status-firefox-esr60: --- → unaffected
Flags: needinfo?(hikezoe)
Keywords: regressionwindow-wanted → regression
Regressed by: 1504065

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

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

Priority: -- → P2

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

status-firefox66: wontfix → disabled
status-firefox67: wontfix → disabled
status-firefox68: affected → disabled
Attached file 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.

The issue also happens with a position:sticky element.

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

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.

Blocks: 1535532

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().

Flags: needinfo?(hikezoe.birchill)

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.

Flags: needinfo?(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.

Flags: needinfo?(mstange)

"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.

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.

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

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.

Assignee: nobody → hikezoe.birchill
Status: NEW → ASSIGNED
Attachment #9117716 - Attachment is obsolete: true
Attachment #9137327 - Attachment description: A working JS → A working JS-driven testcase

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.

Attached file testcase —
Attached file 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.

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.

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.

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.

Flags: needinfo?(matt.woodrow)
Attachment #9133480 - Attachment is obsolete: true

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.

Has Regression Range: yes → ---
Has STR: yes → ---
Keywords: regression
No longer regressed by: 1504065

(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?

Flags: needinfo?(matt.woodrow)

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.

Assignee: hikezoe.birchill → mstange

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.

Depends on D70241

Pushed by mstange@themasta.com:
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

\o/
Thank you so much, Markus!

Whiteboard: [layout:backlog:77]
Flags: qe-verify+

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.

Status: RESOLVED → VERIFIED
status-firefox77: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: