MergeDisplayLists container asr tracking doesn't handle empty lists

RESOLVED FIXED in Firefox 58

Status

()

Core
Layout: Web Painting
RESOLVED FIXED
14 days ago
2 days ago

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

Tracking

(Blocks: 1 bug)

Trunk
mozilla59
Points:
---

Firefox Tracking Flags

(firefox58 fixed, firefox59 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

14 days ago
Created attachment 8927550 [details] [diff] [review]
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.
Attachment #8927550 - Flags: review?(mikokm)
(Assignee)

Updated

13 days ago
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+
(Assignee)

Comment 2

12 days ago
(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.
(Assignee)

Updated

12 days ago
Attachment #8927550 - Flags: review?(mstange)

Comment 3

12 days ago
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
Last Resolved: 11 days ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
(Assignee)

Comment 5

5 days ago
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+

Comment 7

2 days ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/001b6191ff91
status-firefox58: affected → fixed
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
status-firefox58: fixed → affected
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(matt.woodrow)
https://hg.mozilla.org/releases/mozilla-beta/rev/a41f982ed3a7
status-firefox58: affected → fixed
You need to log in before you can comment on or make changes to this bug.