1678108 - mozilla::layers::DisplayItemCache doesn't know about nsDisplayItem mutations

Status: RESOLVED FIXED

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

Description

[Jim Blandy :jimb]

The cache used to avoid re-transmitting display list items to WebRender doesn't know that Gecko display list items are occasionally modified by processes like opacity flattening. As a result, it will direct WebRender to reuse cached items that no longer correspond to the Gecko display list, resulting in incorrect display.

This bug is concealed by bug 1672846, which generates new clip chain ids unnecessarily. Differing clip chain ids cause cache misses, which happens to cause the stale cache entries to never be used, at least in the cases covered by our reftests. Fixing that bug causes layout/reftests/display-list/1420480-1.html to fail, due to this bug.

Details:

If an nsPaintedDisplayItem is assigned an index in WebRenderLayerManager::mDisplayItemCache and transmitted to WebRender, but then retained for use in a subsequent paint and mutated by some process like opacity flattening, mozilla::wr::DisplayListBuilder::ReuseItem will still try to send ReuseItems display list items to WebRender, and the outdated version of the primitive gets drawn.

In the reftest mentioned above, a square div with a solid blue background and 95% opacity gets drawn as an nsDisplayOpacity node with an nsDisplayBackgroundColor child. Because the nsDisplayOpacity node's children do not overlap (there is only one), opacity flattening is applied to it, the nsDisplayBackgroundColor node's alpha is changed from 1 to 0.95, and the nsDisplayOpacity node is ignored. WebRender receives a single 95% opaque Rectangle display item, which is assigned an entry in the DisplayItemCache.

However, the nsDisplayOpacity node does, in fact, have overlapping children; it's just that their display CSS property was set to none. After a delay to allow the painting described above to complete, the reftest changes this child's display to block, causing a new paint. Since opacity flattening is no longer a valid optimization, Gecko correctly restores the nsDisplayBackgroundColor node's opacity to 1, and renders the nsDisplayOpacity in the most general way: as a stacking context containing both children, with an opacity filter applied.

But because the nsDisplayBackgroundColor node is reused in this new paint, and still assigned an entry in the DisplayItemCache, it is transmitted to WebRender as a ReuseItems item, not a fresh Rectangle item, so WebRender draws a 95% opaque blue square, and then applies a 95% opacity filter to it, resulting in a 90.25% opaque square on the screen.

Comment 1

[Miko Mynttinen]

This bug might also affect nsDisplayText items.

Comment 2

[Jim Blandy :jimb]

Attachment: Bug 1678108: Invalidate DisplayItemCache entries when nsDisplayItem::RestoreState makes a change. r?miko

Partial fix.

If nsDisplayItem::RestoreState changes the state of an nsDisplayItem, that
invalidates any prior RetainedItems items sent to WebRender for it, and its
DisplayItemCache entry is invalid. Clear the cache index in the
nsDisplayItem.

RetainedDisplayListBuilder::PreProcessDisplayList doesn't have convenient access
to the DisplayItemCache, so don't clear the cache entry in the DisplayItemCache.
The cache itself will eventually realize the entry is unused and clear it.

Comment 3

[Jim Blandy :jimb]

Try push looks good.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b9e7fd3542fb
Invalidate DisplayItemCache entries when nsDisplayItem::RestoreState makes a change. r=miko

Comment 5

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/b9e7fd3542fb

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:jimm, maybe it's time to close this bug?