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

RESOLVED FIXED in Firefox 48

Status

()

Core
Layout
--
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Jesse Ruderman, Assigned: mats)

Tracking

(Blocks: 2 bugs, {assertion, testcase})

Trunk
mozilla48
assertion, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox46 affected, firefox48 fixed)

Details

Attachments

(6 attachments)

(Reporter)

Description

2 years ago
Created attachment 8699172 [details]
testcase

Assertion failure: !prevChildWasAnonGridItem (two anon grid items in a row), at layout/generic/nsGridContainerFrame.cpp:3257
(Reporter)

Comment 1

2 years ago
Created attachment 8699173 [details]
stack
(Assignee)

Comment 2

2 years ago
Created attachment 8699327 [details]
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
(Assignee)

Updated

2 years ago
Blocks: 1230207
Keywords: regression
(Assignee)

Updated

2 years ago
Blocks: 1248230
Mats, have you had a chance to take a look at this?
Flags: needinfo?(mats)

Comment 4

2 years ago
Jet, can we provide progress update here/
Flags: needinfo?(bugs)

Comment 5

2 years ago
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
(Assignee)

Comment 6

2 years ago
Yes, this assert can be removed.  Nothing bad happens when it triggers.
Flags: needinfo?(mats)
(Assignee)

Comment 7

2 years ago
Created attachment 8732264 [details] [diff] [review]
part 1, move the child frame checks into the frame ctor

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)
(Assignee)

Comment 8

2 years ago
Created attachment 8732266 [details] [diff] [review]
part 2, remove the checks for flexbox too

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)
(Assignee)

Comment 9

2 years ago
Created attachment 8732267 [details] [diff] [review]
crashtests
(Assignee)

Comment 10

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bae2cda6c8b3
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).
(Assignee)

Comment 14

2 years ago
Thanks, I think I misread that as 125...

Comment 15

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/61933d3dbe1c
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f91366b568a
https://hg.mozilla.org/integration/mozilla-inbound/rev/b80a07a27213
(Assignee)

Updated

2 years ago
Flags: in-testsuite+

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/61933d3dbe1c
https://hg.mozilla.org/mozilla-central/rev/6f91366b568a
https://hg.mozilla.org/mozilla-central/rev/b80a07a27213
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.