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

Status: RESOLVED FIXED

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

Description

[Markus Stange [:mstange]]

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.

Comment 1

[Markus Stange [:mstange]]

Attachment: testcase

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

Comment 2

[Markus Stange [:mstange]]

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?

Comment 3

[Matt Woodrow (:mattwoodrow)]

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

Comment 4

[Markus Stange [:mstange]]

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

Comment 5

[Matt Woodrow (:mattwoodrow)]

Attachment: 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

Comment 6

[Markus Stange [:mstange]]

Thanks! Can you add the testcase as a reftest?

Does this patch fix bug 1546856?

Comment 7

[Matt Woodrow (:mattwoodrow)]

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
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

Comment 9

[Noemi Erli[:noemi_erli]]

Backed out changeset 10a025aed3d4 (Bug 1414033) for failing in opacity-keep-intermediate-surface-too-long.html

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&selectedJob=244076935&revision=10a025aed3d426ac976d8dbd98cffc15a33cfb5f

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=244076935&repo=autoland&lineNumber=1531

Backout: https://hg.mozilla.org/integration/autoland/rev/b035b8bb0f648fce4690472b794899f2622fab70

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

Comment 11

[Raul Gurzau (:RaulG)]

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