Closed Bug 1233191 Opened 4 years ago Closed 4 years ago

"Assertion failure: !prevChildWasAnonGridItem (two anon grid items in a row)" with fieldset

Categories

(Core :: Layout, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox46 --- affected
firefox48 --- fixed

People

(Reporter: jruderman, Assigned: mats)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, testcase)

Attachments

(6 files)

Attached file testcase
Assertion failure: !prevChildWasAnonGridItem (two anon grid items in a row), at layout/generic/nsGridContainerFrame.cpp:3257
Attached file stack
Attached file frame tree
<fieldset style="display: grid;">y<legend></legend>x</fieldset>

I guess when the frame-constructor reparents the <legend> frame
so the two anon grid items became adjacent.

A similar assertion happens with display:flex BTW.
Assignee: nobody → mats
Blocks: 1230207
Keywords: regression
Blocks: 1248230
Mats, have you had a chance to take a look at this?
Flags: needinfo?(mats)
Jet, can we provide progress update here/
Flags: needinfo?(bugs)
In bug 1248230, Mats says it's OK for us to have two adjacent anonymous grid items in this case. This ASSERT should be relaxed or removed. I'll find out more when I see Mats.
Flags: needinfo?(bugs)
Keywords: regression
Yes, this assert can be removed.  Nothing bad happens when it triggers.
Flags: needinfo?(mats)
The VerifyGridFlexContainerChildren call here happens before the <legend>
child frame is removed from the list.

BTW, there is nothing in Grid layout that actually depends on these
invariants.  It will work just fine with any frame tree.
Attachment #8732264 - Flags: review?(dholbert)
I think these checks now have little value for flexbox, since it's
covered by the frame ctor checks in part 1.

This part is optional though, in case you want to keep them and fix
it in some other way.
Attachment #8732266 - Flags: review?(dholbert)
Attached patch crashtestsSplinter Review
Comment on attachment 8732264 [details] [diff] [review]
part 1, move the child frame checks into the frame ctor

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

Looks good. Thanks!
Attachment #8732264 - Flags: review?(dholbert) → review+
Comment on attachment 8732266 [details] [diff] [review]
part 2, remove the checks for flexbox too

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

r=me
Attachment #8732266 - Flags: review?(dholbert) → review+
Comment on attachment 8732267 [details] [diff] [review]
crashtests

Drive-by nit on the crashtest patch:

>+++ b/layout/generic/crashtests/crashtests.list
> load text-overflow-iframe.html
>+load 1233191.html
> asserts-if(Android,2-4) asserts-if(!Android,4) load 1225005.html # bug 682647 and bug 448083

Looks like you're inserting this out of order in the crashtests.list manifest here.

Your new line should go at the very end, after the currently-last-line for "load 1225005.html" (which is lower numerically than your new test).
Thanks, I think I misread that as 125...
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.