MergeDisplayLists container asr tracking doesn't handle empty lists

RESOLVED FIXED in Firefox 58

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

Tracking

Trunk
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed, firefox59 fixed)

Details

Attachments

(1 attachment)

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.
Attachment #8927550 - Flags: review?(mikokm)
Attachment #8927550 - Flags: review?(mstange)
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.
Attachment #8927550 - Flags: review?(mikokm) → review+
(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.
Attachment #8927550 - Flags: review?(mstange)
Pushed by mwoodrow@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/2ec68ec7ff01
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
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
Attachment #8927550 - Flags: approval-mozilla-beta?
Comment on attachment 8927550 [details] [diff] [review]
skip-update-asr-empty-list

Take this to support the shield study in 58. Beta58+.
Attachment #8927550 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(matt.woodrow)
You need to log in before you can comment on or make changes to this bug.