1416991 - Rendering regression on rescam.org

Status: RESOLVED FIXED

(Core :: Web Painting, defect, P2)

Description

[Henri]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20171113220112

Steps to reproduce:

go to rescam.org


Actual results:

Graphics very glitchy, for example on faq page is impossible to see questions


Expected results:

normal rendering.

This worked until 59.0a1

mozregression result:
2017-11-14T09:18:59: DEBUG : Using url: https://hg.mozilla.org/integration/mozilla-inbound/json-pushes?changeset=d5e1ed773fefe2eb9d52402dd74b5fea19b8b4a4&full=1
2017-11-14T09:19:00: DEBUG : Found commit message:
Bug 1416055 - Enable retained display lists for Nightly builds. r=miko

2017-11-14T09:19:00: INFO : The bisection is done.

Comment 1

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Some debugging information:

Using my WIP retained-vs-non-retained checker, I got the following output: (extracts)
> non-retained: 0x12f252020 > 0x12e064c60 > 0x12e0bd020 > 0x12e0c1020 > 0x12e762660 > 0x12e762060
> > WrapList p=0x131963920 f=0x12f250250(HTMLButtonControl(button)(1) id:next class:next) key=68 - Cannot find corresponding item in:
> retained: 0x131a38ca0 > 0x12e0c2420 > 0x12e0c2020 > 0x12e0c1820 > 0x12e0be460 > 0x12e0be360
> 
> ***+ non-retained: (correct)
>   BackgroundColor p=0x12e7656a0 f=0x178bd69b8(HTMLScroll(body)(2)) key=4
>   WrapList p=0x12f252020 f=0x178bd6cd0(Block(div)(1) id:app) key=68
>     nsDisplayPerspective p=0x12e749460 f=0x178bd6cd0(Block(div)(1) id:app) key=322
>       nsDisplayTransform p=0x12e7657a0 f=0x178bd7840(Block(div)(9) id:sprited-video class:sprited-video) key=65
>         Opacity p=0x130dfe1a0 f=0x178bd7840(Block(div)(9) id:sprited-video class:sprited-video) key=36
>           LayerEventRegions p=0x130dfe2a0 f=0x178bd7840(Block(div)(9) id:sprited-video class:sprited-video) key=27
>     ...
> 5:  nsDisplayPerspective p=0x12e064c60 f=0x178bd6cd0(Block(div)(1) id:app) key=1090
>       nsDisplayTransform p=0x12e0bd020 f=0x12f245a30(Block(div)(27) id:layer3D class:layer3d) key=65
>         nsDisplayTransform p=0x12e0c1020 f=0x12f24fd28(Block(div)(5) class:caption-wrapper) key=65
>           nsDisplayTransform p=0x12e762660 f=0x12f250130(Block(div)(7) id:next-wrapper class:next-wrapper) key=65
>             Opacity p=0x12e762060 f=0x12f250130(Block(div)(7) id:next-wrapper class:next-wrapper) key=36
> 1:            WrapList p=0x131963920 f=0x12f250250(HTMLButtonControl(button)(1) id:next class:next) key=68
> 
> ***+ retained: (incorrect)
>   WrapList p=0x131a38ca0 f=0x178bd6cd0(Block(div)(1) id:app) key=68
> 4:  nsDisplayPerspective p=0x12da49d60 f=0x178bd6cd0(Block(div)(1) id:app) key=322
>       nsDisplayTransform p=0x12f282420 f=0x12f245a30(Block(div)(27) id:layer3D class:layer3d) key=65
>         nsDisplayTransform p=0x12f281c20 f=0x12f24fd28(Block(div)(5) class:caption-wrapper) key=65
>           nsDisplayTransform p=0x12f252520 f=0x12f250130(Block(div)(7) id:next-wrapper class:next-wrapper) key=65
>             Opacity p=0x12f252420 f=0x12f250130(Block(div)(7) id:next-wrapper class:next-wrapper) key=36
> 3:            WrapList p=0x12f251a20 f=0x12f250250(HTMLButtonControl(button)(1) id:next class:next) key=68
>     ...
>     nsDisplayPerspective p=0x12e0c2420 f=0x178bd6cd0(Block(div)(1) id:app) key=1090
>       nsDisplayTransform p=0x12e0c2020 f=0x12f245a30(Block(div)(27) id:layer3D class:layer3d) key=65
>         nsDisplayTransform p=0x12e0c1820 f=0x12f24fd28(Block(div)(5) class:caption-wrapper) key=65
>           nsDisplayTransform p=0x12e0be460 f=0x12f250130(Block(div)(7) id:next-wrapper class:next-wrapper) key=65
> 2:          Opacity p=0x12e0be360 f=0x12f250130(Block(div)(7) id:next-wrapper class:next-wrapper) key=36

The WrapList in the non-retained tree at '1' would be expected in the retained tree under '2' (by following the trail of display items with same frame and key).
However the Wraplist is now at '3', under a perspective '4' with the same frame but a different key from '5'!

As distinct keys are assigned during building, a partial build could explain that a perspective got assigned a different key, if other perspectives under the same wraplist didn't need rebuilding, or somehow were processed in a different order.

Some thoughts from Matt:
nsDisplayPerspective gets made unique by a fairly arbitrary extra bit (or two) in the key.
The ‘moved’ wraplist is still under the same transform.
So I think just the arbitrary index is different, thanks to partial building.
I think we either need to disable partial building for perspective (or possibly only if we detect the possibility of multiple perspectives?), or find a way to make nsDisplayPerspective::mIndex be unaffected by a partial build.

Comment 2

[Liz Henry (:lizzard) (relman/hg->git project)]

Does this seem likely to affect other sites? 
And, should we mark the bug as NEW at this point? 
Might it affect 58 as well if we are testing retained display lists on 58 beta?

Comment 3

[Matt Woodrow (:mattwoodrow)]

Yeah it probably will affect other sites, though it seems to only occur with fairly complex 3d content, so it's probably rare (and why we don't have other reports yet).

It will affect the 58 beta now with the shield study, and we might see more reports soon with the larger audience.

Gerald, are you still looking at this?

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #3)
> Yeah it probably will affect other sites, though it seems to only occur with
> fairly complex 3d content, so it's probably rare (and why we don't have
> other reports yet).

I think we possibly have the same issue with https://www.geloofhet.nl/nl/home 's  menu, see bug 1422199 comment 1.


> It will affect the 58 beta now with the shield study, and we might see more
> reports soon with the larger audience.
> 
> Gerald, are you still looking at this?

No I'm not, sorry if I gave that impression. (I only ran the RDL checker, to capture the display list issues in comment 1.)

Comment 5

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1416991 - Fix perspective indexing in partial DL builds -

Perspective item indices (used to produce unique per-frame keys) were generated
by incrementing a counter in the builder when building a perspective display
item. This caused problems with retained display lists, because an unmodified
perspective could be skipped during a partial build, causing other perspectives
to be incorrectly numbered and then incorrectly merged with the previous
retained display list.

To fix this, we need to always increment the counter if there is likely to be
a perspective, before that item may be skipped.

Review commit: https://reviewboard.mozilla.org/r/207574/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/207574/

Comment 6

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=424ebb68f211106302e17bbef1074e0cb05e6729

Comment 7

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8936850 [details]
Bug 1416991 - Fix perspective indexing in partial DL builds -

https://reviewboard.mozilla.org/r/207574/#review213456

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/132d000b6888
Fix perspective indexing in partial DL builds - r=mattwoodrow

Comment 9

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/132d000b6888

Comment 10

[Julien Cristau [:jcristau]]

AIUI this only shows up with retain display lists, which is off by default in 58.