Closed Bug 1414033 Opened 7 years ago Closed 6 years ago

ContainerLayer intermediate surfaces are not invalidated if a ContainerLayer is reparented to a child of its old parent

Categories

(Core :: Graphics: Layers, defect, P3)

Product:
Core
Component:
Graphics: Layers
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla68
Tracking Flags:
Tracking Status
firefox58 --- wontfix
firefox68 --- fixed

People

(Reporter: mstange, Assigned: mattwoodrow)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(2 files)

testcase
(deleted), text/html
Details
Bug 1414033 - Recursively invalidate any cached ContainerLayer surfaces for new attached Layers, since they might have been moved and we don't track invalid areas within them. r?mstange
(deleted), text/x-phabricator-request
Details
This is the bug about the underlying compositor bug behind bug 1400259. I'm trying to create a testcase that would reproduce this bug in web content.
Attached file testcase (deleted) —
Clicking the red square should turn it green, and shift it one pixel to the right. In Firefox with accelerated compositing, it stays red. A tab switch fixes it. (This was way too easy to reproduce. It's a miracle we're not running into this bug more often in the wild. The broken code has been around for a long time.)
LayerTreeInvalidation.cpp doesn't recurse into child layers that have been added or removed. So SetChildrenChanged is never called for layers that are being reparented. I'm still of the opinion that adding side-effects to ComputeChange was a mistake, and ContainerLayers that want to retain intermediate surfaces should have done their own property cloning and change computing instead of being handled as part of the overall layer tree change computation. Matt, do you have a preferred way of fixing this bug? Do you want to take this bug?
Flags: needinfo?(matt.woodrow)
(In reply to Markus Stange [:mstange] from comment #2) > I'm still of the opinion that adding side-effects to ComputeChange was a > mistake, and ContainerLayers that want to retain intermediate surfaces > should have done their own property cloning and change computing instead of > being handled as part of the overall layer tree change computation. You're right, but I'm not sure it's worth rewriting this now. > > Matt, do you have a preferred way of fixing this bug? Do you want to take > this bug? How about just adding code that runs when we attach a Layer to a new location that just goes through the descendants and makes sure we invalidate any retained state? Alternatively, switch ContainerLayer to doing its own ComputeChange, but I don't have time to help out with doing this at the moment.
Flags: needinfo?(matt.woodrow)
Priority: -- → P3
Whiteboard: [gfx-noted]

Matt, could you find somebody to take a look at this? The testcase still reproduces the bug, and the workaround from bug 1400259 is apparently still causing trouble (e.g. bug 1543508).

Flags: needinfo?(matt.woodrow)
Assignee: nobody → matt.woodrow
Flags: needinfo?(matt.woodrow)

Thanks! Can you add the testcase as a reftest?

Does this patch fix bug 1546856?

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

Thanks! Can you add the testcase as a reftest?

Will do!

Does this patch fix bug 1546856?

Partially? I see two bugs there, the hover effect not moving when we scroll, as well as some smaller rects showing the hover color (seemingly in random places).

It fixes the former, not the latter.

Pushed by mwoodrow@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/10a025aed3d4 Recursively invalidate any cached ContainerLayer surfaces for new attached Layers, since they might have been moved and we don't track invalid areas within them. r=mstange
Pushed by mwoodrow@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2de2d4ea2587 Recursively invalidate any cached ContainerLayer surfaces for new attached Layers, since they might have been moved and we don't track invalid areas within them. r=mstange
Flags: needinfo?(matt.woodrow)

https://hg.mozilla.org/mozilla-central/rev/2de2d4ea2587

Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox68: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: