Closed
Bug 1233191
Opened 8 years ago
Closed 8 years ago
"Assertion failure: !prevChildWasAnonGridItem (two anon grid items in a row)" with fieldset
Categories
(Core :: Layout, defect)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: jruderman, Assigned: MatsPalmgren_bugz)
References
Details
(Keywords: assertion, testcase)
Attachments
(6 files)
141 bytes,
text/html
|
Details | |
13.23 KB,
text/plain
|
Details | |
1.07 KB,
text/plain
|
Details | |
5.92 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
3.95 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
1.45 KB,
patch
|
Details | Diff | Splinter Review |
Assertion failure: !prevChildWasAnonGridItem (two anon grid items in a row), at layout/generic/nsGridContainerFrame.cpp:3257
Reporter | ||
Comment 1•8 years ago
|
||
Assignee | ||
Comment 2•8 years ago
|
||
<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•8 years ago
|
Blocks: 1230207
Keywords: regression
Comment 5•8 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•8 years ago
|
||
Yes, this assert can be removed. Nothing bad happens when it triggers.
Flags: needinfo?(mats)
Assignee | ||
Comment 7•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
Assignee | ||
Comment 10•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bae2cda6c8b3
Comment 11•8 years ago
|
||
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 12•8 years ago
|
||
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 13•8 years ago
|
||
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•8 years ago
|
||
Thanks, I think I misread that as 125...
Comment 15•8 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•8 years ago
|
Flags: in-testsuite+
Comment 16•8 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
Closed: 8 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.
Description
•