825810 - "Assertion failure: aFrame (why do we have an anonymous box without any children?)" with flexbox

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Assertion failure: aFrame (why do we have an anonymous box without any children?), at layout/generic/nsFlexContainerFrame.cpp:480

Comment 1

[Daniel Holbert [:dholbert]]

We're failing to walk from the (anonymous-box) nsTableFrame to its (non-anonymous-box) column frame descendant.

What happens is this: GetFirstPrincipalChild() on the table frame ends up returning null, because our non-anonymous descendant isn't stored in the principal child list -- it's in kColGroupList.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: fix v1

The idea of this fix is as follows:
When we're descending through anonymous box frames to find the non-anonymous frame buried in there somewhere, we'll now check anonymous table frames for a colgroup list -- and if they have one, then that must mean there's something non-anonymous there, so we descend into that instead of the principal list. We do the same for caption lists on table-outer frames, since they trigger this bug equally well.  (crashtests included for both of those cases.)

I *think* those are the only two situations where a non-anonymous frame can end up in a non-principal-child-list subtree with an anonymous-box root...  (bz, if you know of any others, let me know; or, we can add additional special cases for them here as we discover them.)

(This patch stacks on top of my patch for bug 873172, BTW, so you probably want to look at that bug first.)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 750801 [details] [diff] [review]
fix v1

I think you're right.  Ruby, if we ever do it, would add more cases, but that's for then to worry about.

Please fix the indent like so:

+    } else if (MOZ_UNLIKELY(aFrame->GetType() == nsGkAtoms::tableFrame) &&
+               !aFrame->GetChildList(kColGroupList).IsEmpty()) {

and r=me

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to Boris Zbarsky (:bz) from comment #3)
> Please fix the indent like so:

Oops; copypaste failure.  Thanks for catching -- fixed locally.

Try run (not including the indentation fix):
 https://tbpl.mozilla.org/?tree=Try&rev=21312c3ec51b

Comment 5

[Daniel Holbert [:dholbert]]

This caused some assertion failures on Try, from descending into a column-group list that's *only* has anonymous boxes (which sets off alarm bells, since we're looking for something non-anonymous). Turns out we create a (fully-anonymous) column group list even when there aren't any explicit column in the document.

So instead of taking "there's a list" to mean "there's something non-anonymous in the list", we instead need to make a recursive call into the list, and if that call return null, then we take that to mean that the list has only anonymous descendants (or it isn't there) and we can forget about it.

Try run, following that strategy: https://tbpl.mozilla.org/?tree=Try&rev=66f9e4e57c9d

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: part 1: Shift null-check out of GetFirstNonAnonBoxDescendant

Yay, Try server is green! Attaching the updated patch in a minute, but first here's a helper-patch.

In order to let ourselves make the recursive call mentioned in the previous comment, we need GetFirstNonAnonBoxDescendant to deal with null pointers gracefully. So, this "part 1" patch is just a logic-shifting patch, moving the sanity null-checks out of GetFirstNonAnonBoxDescendant() to its caller, IsOrderLEQWithDOMFallback.

This makes GetFirstNonAnonBoxDescendant itself now OK with handling null pointers, which will be useful in the next part. (coming up)

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: part 2: Make recursive call to GetFirstNonAnonBoxDescendant for caption & colgroup lists

Here's the updated "real" patch for this bug.

To recap: the old patch assumed that the presence of a caption/column child list meant that the list must have something non-anonymous in it [which turned out to be false].  This new patch is more sensitive -- it makes a recursive call to the list's first frame (if any) to dig for the first non-anonymous descendant in the list, and if that fails, then we fall back to digging through the principal child list.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 751459 [details] [diff] [review]
part 1: Shift null-check out of GetFirstNonAnonBoxDescendant

r=me

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 751460 [details] [diff] [review]
part 2:  Make recursive call to GetFirstNonAnonBoxDescendant for caption & colgroup lists

r=me, but I just realized something: if the anon table has both a non-anon col and a non-anon cell, we will now find the col.  I guess it doesn't matter too much, since they must be adjacent-ish in the DOM and we only use this for sorting?  Might be worth a comment, though.

Comment 10

[Daniel Holbert [:dholbert]]

Exactly, yeah -- shouldn't cause any problems, since this is just for sorting and if they're in the same anonymous table box, then they'll always sort identically since they'll be part of the same flex item.

(We have the same sort of thing if you have a jumble of non-anon table-rows and/or table-cells -- we'll just grab whichever one we hit first.)

I'll add a comment clarifying that before landing. Thanks for the review!

Comment 11

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/05a73d83e05f
https://hg.mozilla.org/integration/mozilla-inbound/rev/91a48bb34e1c

Comment 12

[Daniel Holbert [:dholbert]]

FWIW, the comment I added was:
+    // (Note: For anonymous tables that have a non-anon cell *and* a non-anon
+    // column, we'll always return the column. This is fine; we're really just
+    // looking for a handle to *anything* with a meaningful content node inside
+    // the table, for use in DOM comparisons to things outside of the table.)

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/05a73d83e05f
https://hg.mozilla.org/mozilla-central/rev/91a48bb34e1c