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

RESOLVED FIXED in mozilla24

Status

()

defect
--
critical
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jruderman, Assigned: dholbert)

Tracking

(Blocks 1 bug, {assertion, testcase})

Trunk
mozilla24
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [assertion softened; bug 876749 tracks the underlying bug])

Attachments

(6 attachments)

Posted file testcase
No description provided.
Posted file stack trace
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).
OS: Mac OS X → All
Hardware: x86_64 → All
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.
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)
As you can see by comparing the attached frame trees, the canvas frame gets reinserted before the anonymous TableOuter frame, instead of after it.
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.
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.)
Attachment #754256 - Flags: review?(bzbarsky)
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.)
We should get a bug filed on the IsValidSibling breakage... I thought we had one, but I can't find it.  :(
Comment on attachment 754256 [details] [diff] [review]
patch 1 v1: Soften MOZ_ASSERT to NS_ASSERTION

r=me
Attachment #754256 - Flags: review?(bzbarsky) → review+
(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.
Whiteboard: [assertion softened; bug 876749 tracks the underlying bug]
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/c1f64bd309b6
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Blocks: 1059138
You need to log in before you can comment on or make changes to this bug.