876074 - Assertion failure: "Frame list should've been sorted in reflow" with flexbox, table-caption, CSS content property

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Daniel Holbert [:dholbert]]

It looks like the style change makes us recreate frames for the canvas, and then we end up inserting the newly-created frame at the wrong position -- after the block, instead of after the anonymous table.

In particular, we get a call to nsContainerFrame::InsertFrames with...
  this == the nsFlexContainerFrame
  aPrevFrame == the nsBlockFrame
  aFrameList == { the newly-created nsHTMLCanvasFrame }

...where aPrevFrame *should* be the anonymous nsTableOuterFrame, if we're going to preserve the initial tree structure (and match the DOM order, as the failing assertion is mandating).

Comment 3

[Daniel Holbert [:dholbert]]

Specifically, nsCSSFrameConstructor::ContentRangeInserted calls nsCSSFrameConstructor::GetInsertionPrevSibling which calls nsCSSFrameConstructor::FindPreviousSibling, on the canvas's content node.

The loop in ::FindPreviousSibling walks backwards in the content tree, from that node, and it returns the first non-null frame returned by FindFrameForContentSibling().

Sadly, FindFrameForContentSibling() returns null when we call it for the table-caption div, so it gets skipped.  As a result, FindPreviousSibling ends up returning the block frame instead of the table outer frame, and that's why we end up reinserting the canvas after the block frame.

This actually isn't a flexbox-specific issue -- I've made a testcase where we hit the exact same problem in a block container, but we just don't bother asserting about it there.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: no-flexbox testcase (doesn't assert, but does reorder frametree incorrectly)

As noted above -- here's a testcase with a block container (i.e. no flexboxes) that reproduces this same incorrectly-ordered-frametree issue. (though we don't assert about it in this case)

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: frametree of no-flexbox testcase, before dynamic tweak

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: frametree of no-flexbox testcase, after dynamic tweak

Comment 7

[Daniel Holbert [:dholbert]]

As you can see by comparing the attached frame trees, the canvas frame gets reinserted before the anonymous TableOuter frame, instead of after it.

Comment 8

[Daniel Holbert [:dholbert]]

If I tweak the testcase to use "table-cell" instead of "table-caption", then FindPreviousSibling returns the nsTableCellFrame, and I get correct behavior.

But using table-caption, FindPreviousSibling returns null because its helper-method FindFrameForContentSibling() has a check for IsValidSibling, which returns false here:
> 5864     if ((siblingDisplay == NS_STYLE_DISPLAY_TABLE_CAPTION) !=
> 5865         (aDisplay == NS_STYLE_DISPLAY_TABLE_CAPTION)) {
> 5866       // One's a caption and the other is not.  Not valid siblings.
> 5867       return false;
> 5868     }
So, IsValidSibling() returns false, which makes FindFrameForContentSibling() return null (given the table-caption's div), which makes FindPreviousSibling() skip over the table-caption's div and return the thing before it.

A naive solution would be to make IsValidSibling return true in this case, but I'll bet that would have other implications.

BOTTOM LINE: Since this assertion is about a layout/stacking-order issue (not a security issue), and since there are unrelated-to-flexbox ways to trigger it, I think we should soften the assertion to NS_ASSERTION, so that debug builds (and fuzzers) can hit this and proceed instead of aborting.

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: patch 1 v1: Soften MOZ_ASSERT to NS_ASSERTION

It'd be great to fix the general issue, but for now, here's a patch to just soften the assert per end of my previous comment, so that this bug and other bugs of this sort will be less severe for fuzzers and for people running debug builds.

This adds the included test as a crashtest with "asserts(2)", for two instances of this assertion after the dynamic modification (from two separate BuildDisplayList calls).

(I might need to tweak that assertion count a bit -- I suspect the number of BuildDisplayList calls might vary per-platform or even per-run. Hopefully Try will help with determining that.)

Comment 10

[Daniel Holbert [:dholbert]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=0001471b5523

A few of the runs had only 1 assertion, and a few had 3 assertions.

I'll probably tweak this to be asserts(1-4), to be on the safe side.  (Kind of lax, but the point would be: (a) we know that the assertion fails some number of times (depending on paint count), and (b) it shouldn't abort (or crash), as it currently does.)

Comment 11

[Boris Zbarsky [:bzbarsky]]

We should get a bug filed on the IsValidSibling breakage... I thought we had one, but I can't find it.  :(

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 754256 [details] [diff] [review]
patch 1 v1: Soften MOZ_ASSERT to NS_ASSERTION

r=me

Comment 13

[Daniel Holbert [:dholbert]]

(In reply to Boris Zbarsky (:bz) from comment #11)
> We should get a bug filed on the IsValidSibling breakage... I thought we had
> one, but I can't find it.  :(

I filed bug 876749 on that, and I updated the crashtest.list annotation for this bug's assertion to point to that bug instead of this bug. (since fixing that bug will fix the assertions)

Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/c1f64bd309b6

Once that cset is merged to m-c, we can close this, and let bug 876749 track the remaining underlying issue.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c1f64bd309b6