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)

Reporter

Description

6 years ago
Posted file testcase
No description provided.
Reporter

Comment 1

6 years ago
Posted file stack trace
Assignee

Comment 2

6 years ago
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).
Assignee

Updated

6 years ago
OS: Mac OS X → All
Hardware: x86_64 → All
Assignee

Comment 3

6 years ago
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.
Assignee

Comment 4

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

Comment 7

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

Comment 8

6 years ago
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.
Assignee

Comment 9

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

Comment 10

6 years ago
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+
Assignee

Comment 13

6 years ago
(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

Updated

6 years ago
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/c1f64bd309b6
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Assignee

Updated

5 years ago
Blocks: 1059138
You need to log in before you can comment on or make changes to this bug.