1416448 - MergeDisplayLists container asr tracking doesn't handle empty lists

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Matt Woodrow (:mattwoodrow)]

Attachment: skip-update-asr-empty-list

We compute the outer ASR for all the items in the list, but we don't differentiate between the initial nullptr value and the actual root asr (represented as nullptr).

This is causing a reftest failure since we compute the wrong ASR for an empty list, and propagate it up the tree.

Switching to use a Maybe<> value lets us distinguish between the two cases, and not modify the ASR when the list is empty.

I also tried just getting rid of empty items, but it's possible for an empty nsDisplaySVGEffects to still draw something.

Comment 1

[Miko Mynttinen]

Comment on attachment 8927550 [details] [diff] [review]
skip-update-asr-empty-list

Review of attachment 8927550 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/painting/RetainedDisplayListBuilder.cpp
@@ +414,3 @@
>            MergeDisplayLists(newItem->GetChildren(), oldItem->GetChildren(),
> +                            oldItem->GetChildren(), containerASRForChildren);
> +          if (containerASRForChildren) {

nit: UpdateASR could take Maybe<> as a parameter and have this check to avoid repeating this code.

Comment 2

[Matt Woodrow (:mattwoodrow)]

(In reply to Miko Mynttinen [:miko] from comment #1)
> Comment on attachment 8927550 [details] [diff] [review]
> skip-update-asr-empty-list
> 
> Review of attachment 8927550 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/painting/RetainedDisplayListBuilder.cpp
> @@ +414,3 @@
> >            MergeDisplayLists(newItem->GetChildren(), oldItem->GetChildren(),
> > +                            oldItem->GetChildren(), containerASRForChildren);
> > +          if (containerASRForChildren) {
> 
> nit: UpdateASR could take Maybe<> as a parameter and have this check to
> avoid repeating this code.

We don't actually allow Maybe<> as a parameter, but I'll make it a reference and it should work.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ec68ec7ff01
Don't update the ASR during merging for an empty container item, since we can't compute the ASR of the contents. r=miko

Comment 4

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 5

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8927550 [details] [diff] [review]
skip-update-asr-empty-list

Approval Request Comment
[Feature/Bug causing the regression]: bug 1352499. This is code that is preffed off, but we want to run a shield study enabling the pref.
[User impact if declined]: None, preffed off code.
[Is this code covered by automated tests?]: Yes, when the pref is enabled.
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Code is preffed off.
[String changes made/needed]: None

Comment 6

[Gerry Chang [:gchang]]

Comment on attachment 8927550 [details] [diff] [review]
skip-update-asr-empty-list

Take this to support the shield study in 58. Beta58+.

Comment 7

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/001b6191ff91

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed this out for conflicts when it got applied to beta. Please provide a patch which applies cleanly.

https://hg.mozilla.org/releases/mozilla-beta/rev/bab083518607c3e22a09a8befc6a1199748e31a4

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a41f982ed3a7