1420737 - z-index rendering is flaky

Status: RESOLVED FIXED

(Core :: Web Painting, defect, P2)

Description

[Axel Hecht [:Pike]]

On http://pasjans-online.pl/, the stacking of cards is done by z-index. But during the game play, the rendering get's out of order.

Switching to a different tab and back fixes the display again.

I've noticed it on Nightly for a few days now.

It's a bit tricky to reproduce, but it seems to happen when dragging a card over a stack with a couple of shown cards.

I'm on macos 10.12.6.

Comment 1

[Axel Hecht [:Pike]]

I've done some late-night game playing surrounded by mozregression, and the regression range I came up with is

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=45b910aad9dcf0e19064b42d8608bc169950a24a&tochange=9f3615964530ae32dc7ea95e5ae66d153534a3aa

Matt, both bugs in that push are by you, could you dive into this?

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1420737 - Fix merge algorithm to handle more complex z-index changes.

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

Comment 4

[Markus Stange [:mstange]]

Comment on attachment 8937620 [details]
Bug 1420737 - Fix merge algorithm to handle more complex z-index changes.

https://reviewboard.mozilla.org/r/208294/#review215838

::: layout/painting/RetainedDisplayListBuilder.cpp:310
(Diff revision 1)
> - *         Remove items from the bottom of the old list until we reach the matching item:
> + *         Remove items from the bottom of the old list until we reach an item that also exists in the new list:
>   *             Add valid items to the merged list, destroy invalid items.
>   *         Destroy the matching item from the old list.
>   *     Add the item from the new list into the merged list.

This is a bit confusing. I haven't read the code yet but I'm not sure how this description would translate into code:
"the matching item from the old list" - in example 2, in the first iteration of the outer loop, is this A or B? Same question for "the item from the new list".

::: layout/painting/RetainedDisplayListBuilder.cpp:314
(Diff revision 1)
>   *     If the item has a matching item in the old list:
> - *         Remove items from the bottom of the old list until we reach the matching item:
> + *         Remove items from the bottom of the old list until we reach an item that also exists in the new list:
>   *             Add valid items to the merged list, destroy invalid items.
>   *         Destroy the matching item from the old list.
>   *     Add the item from the new list into the merged list.
> - * Add all remaining valid items from the old list into the merged list.
> + *     If the top of the old list is a match for what we just added, remove and destroy it.

The top? Or the start / bottom? Which part of the code does this line correspond to?

::: layout/painting/RetainedDisplayListBuilder.cpp:336
(Diff revision 1)
>   * Merged List: A,B,D
>   *
> - * Example 2:
> + * Example 2 (layout/reftests/retained-dl-zindex-1.html):
>   *
>   * Old List: A, B
> - * New List, B, A
> + * Modified List, B, A

There should be a colon here

::: layout/painting/RetainedDisplayListBuilder.cpp:337
(Diff revision 1)
>   *
> - * Example 2:
> + * Example 2 (layout/reftests/retained-dl-zindex-1.html):
>   *
>   * Old List: A, B
> - * New List, B, A
> + * Modified List, B, A
>   * Invalidations: -

Doesn't B get invalidated in that reftest?

::: layout/painting/RetainedDisplayListBuilder.cpp:342
(Diff revision 1)
>   * We match the B items, but don't copy A since it's also present in the new list
>   * and then add the new B into the merged list. We then add A, and we're done.

"but don't copy A since it's also present in the new list" - this aspect isn't mentioned in the "basic algorithm" section, is it?

::: layout/painting/RetainedDisplayListBuilder.cpp:352
(Diff revision 1)
> + * Display tree:
> + *   A
> + *  / \
> + * B   C

Huh, a "display tree"? Oh, is this an "overlap graph" which happens to be a tree in this example?

Comment 5

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8937620 [details]
Bug 1420737 - Fix merge algorithm to handle more complex z-index changes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/208294/diff/1-2/

Comment 6

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8937620 [details]
Bug 1420737 - Fix merge algorithm to handle more complex z-index changes.

https://reviewboard.mozilla.org/r/208294/#review215838

> This is a bit confusing. I haven't read the code yet but I'm not sure how this description would translate into code:
> "the matching item from the old list" - in example 2, in the first iteration of the outer loop, is this A or B? Same question for "the item from the new list".

I rewrote this a bit to hopefully make it clearer.

> The top? Or the start / bottom? Which part of the code does this line correspond to?

Removed references to top/bottom, and just called it the 'start' so that it's hopefully simpler.

> "but don't copy A since it's also present in the new list" - this aspect isn't mentioned in the "basic algorithm" section, is it?

This is the inner loop executing 0 times, since the first thing we look at is an item with a match.

> Huh, a "display tree"? Oh, is this an "overlap graph" which happens to be a tree in this example?

I got rid of this, no need to try introduce a new concept at this point.

Comment 8

[Markus Stange [:mstange]]

Comment on attachment 8937620 [details]
Bug 1420737 - Fix merge algorithm to handle more complex z-index changes.

https://reviewboard.mozilla.org/r/208294/#review218684

Thanks! The comment looks great now. I've also confirmed that the updated code matches the new comment.
I have not thought about whether this algorithm is correct in all cases and whether it's the best we can do.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff731fad7630
Fix merge algorithm to handle more complex z-index changes. r=mstange

Comment 10

[Cosmin Sabou [:CosminS]]

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

Comment 12

[Axel Hecht [:Pike]]

I'm smoke-testing this a bit, and the newest nightly behaves much better.

I had one z-index glitch so far, and it self-repaired pretty quickly. So there might be some edge-cases still.

On that single incident, still on my card-game site. I had one card slipping under another during a drag, but I think it was properly stacked by the time the drag ended, or shortly after the drop. Hard to detail exactly what happened, as it was transient.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Backed out for causing bug 1431064.

https://hg.mozilla.org/mozilla-central/rev/6fe09f2de63da649e9a8bbff701b445df7d75094

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee3afc686ce5
Fix merge algorithm to handle more complex z-index changes. r=mstange

Comment 15

[Matt Woodrow (:mattwoodrow)]

Relanded with the typo fixed.

Comment 16

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/710b3c4c7a29
Backed out changeset ee3afc686ce5 for Leaks on multiple failures

Comment 17

[Ionuț Goldan [:igoldan]]

When this bug first landed (see comment 9), we noticed the regression bellow. It affected Windows 10 and Linux 64.
== Change summary for alert #11221 (as of Wed, 17 Jan 2018 20:27:05 GMT) ==

Regressions:

  8%  displaylist_mutate windows10-64 pgo e10s     4,740.41 -> 5,125.00

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=11221

Comment 18

[Ionuț Goldan [:igoldan]]

The backout (from comment 13) fixed the perf regressions:

== Change summary for alert #11218 (as of Tue, 16 Jan 2018 21:05:36 GMT) ==

Improvements:

  9%  displaylist_mutate linux64 opt e10s     4,993.15 -> 4,529.00
  9%  displaylist_mutate linux64 pgo e10s     4,468.82 -> 4,057.95
  9%  displaylist_mutate windows10-64 pgo e10s5,197.28 -> 4,752.42

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=11218

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5195f15b1b03
Fix merge algorithm to handle more complex z-index changes. r=mstange

Comment 20

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/5195f15b1b03

Comment 21

[Andrei Petcu]

A bug marked as duplicate of this one can be reproduced again. https://bugzilla.mozilla.org/show_bug.cgi?id=1425189

Did this bug come back or there might be a separate issue?

Comment 22

[Dirkjan Ochtman (:djc)]

Not sure if related, but I just found a very surprising bug where the content background scrolls over the Firefox chrome theme background; filed as bug 1431732. Since it seemed like it might be related to z-index stuff, I copied the component and am mentioning it here.