363813 - Crash [@ nsHTMLReflowState::nsHTMLReflowState][@ nsRuleNode::Mark] with unminimised testcase, using generated content and lots of floats

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Martijn Wargers (dead)]

See upcoming testcases.
I don't have a minimised testcase, only a partly minimised testcase. Minimising is difficult, this time.

The unminimised testcase has this talkback ID: TB27310357H
nsRuleNode::Mark  [mozilla\layout\style\nsrulenode.cpp, line 4381]

The partly minimised testcase has this talkback ID: TB27311489H
0x02b46958
0x02b3c3ad
nsHTMLReflowState::nsHTMLReflowState  [mozilla\layout\generic\nshtmlreflowstate.cpp, line 181]
nsLineLayout::ReflowFrame  [mozilla\layout\generic\nslinelayout.cpp, line 735]

The 1.8.0.x and the 1.8.1 builds are also crashing, so I'm marking this security sensitive (also the stacktrace has garbage at the top).

Comment 1

[Martijn Wargers (dead)]

Attachment: Unminimised testcase

Comment 2

[Martijn Wargers (dead)]

Attachment: Partly minimised testcase

Comment 3

[Mats Palmgren (inactive)]

Does the patch in bug 359371 help?
(the stack and assertions looks a bit like it could)

Comment 4

[Martijn Wargers (dead)]

(In reply to comment #3)
> Does the patch in bug 359371 help?
> (the stack and assertions looks a bit like it could)

You mean the Simple fix, right (the other one is already checked in according to roc)?
I've tried that patch, it doesn't seem to fix the crash for the partly minimised testcase.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: simplified testcase

This testcase is much simplified but still triggers

WARNING: nsBlockFrame::CheckFloats: Explicit float list is out of sync with float cache: file ../../../mozilla/layout/generic/nsBlockFrame.cpp, line 6300

which I think is related to the main testcase. I'll try to debug this later.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The problem is that the float for "b" (first letter of "before text") should come before the span float in the body's float-list, but it comes afterward for some reason, presumably because of the horrific details of frame construction.

Comment 7

[Daniel Veditz [:dveditz]]

Hopefully roc can help with this one or find a better home for it

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Well, some of the problems here are due to floats for floating first-letter being put in the wrong place. I've fixed all those. Now we crash for some other reason. Still working on it :-).

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: testcase #2

for the record, this is another testcase that triggers out-of-order floats via ReconstructLetterFrame. The previous testcase triggers out-of-order floats in a different way, via CreateLetterFrame.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: another simplified testcase

This testcase gives me
###!!! ASSERTION: frame not dirty: 'aFrame->GetStateBits() & (NS_FRAME_IS_DIRTY | NS_FRAME_HAS_DIRTY_CHILDREN)', file ../../../mozilla/layout/base/nsPresShell.cpp, line 3414
so I'll fix that next :-)

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, that was a simple bug in CanvasFrame.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: next simplified testcase

This testcase produces
frame: Block(span)(1) (0x18cd720) style: 0x189c3a0 :-moz-anonymous-block {}
Wrong parent style context:  style: 0x18cd8a0 {}
should be using:  style: 0x18cd408 {}

frame: Block(div)(1) (0x18cd960) style: 0x18cdb68 {}
Wrong parent style context:  style: 0x18cd8a0 {}
should be using:  style: 0x18cd408 {}

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That one is an interesting one. We restyle the first-line frames, but the parent style context for the IB-split anonymous block is an inline frame in the first-line, and we forget to restyle the anonymous block, so things are inconsistent. I've "fixed" it by going ahead and restyling the anonymous block too in that case, which is actually incorrect because the anonymous block should not be affected by the first-line style. A real fix would suppress the warning for this case because we really *don't* want the anonymous block style context parent to be the style context of the leading IB-split inline. In fact the parent style context may not be associated with any frame.

My "fix" is safest, though, because it matches existing expectations most closely. We still crash on the full testcase though.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: yet another minimized testcase

This testcase gives me
###!!! ASSERTION: Computed overflow area must contain frame bounds: 'aNewSize.width == 0 || aNewSize.height == 0 || aOverflowArea->Contains(nsRect(nsPoint(0, 0), aNewSize))', file ../../../mozilla/layout/generic/nsFrame.cpp, line 5164

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That bug appears to be something to do with bidi reordering, and I can reproduce a crash after removing all 'direction:rtl', so I'll skip that issue for now.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: crash fix

OK, the problem is to do with nsInlineFrame's lazy update of frame parent pointers. A frame is pulled from inline frame A's overflow list to inline frame B; then it moves to B's overflow list; then we reflow B again and B pulls it back to the main child list. We need to set the parent pointer on the frame, but we don't, because the first frame on B's overflow list *does* have the right parent. There is no reason to believe that just because the first frame on the list has the right parent, all the frames do.

Removing this "optimization" shouldn't hurt performance in any interesting cases, I think, because having an inline pull back its own overflow should be uncommon.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix for attachment 249152

This extends style context reparenting across IB-split special siblings, since the anonymous block's style context is parented by the style context for the IB split's leading inline frames.

As noted in comments, this introduces a bug in IB splits where the leading inline frames are in a first-line (or possibly first-letter). I'm not really sure how to fix that but I think it's more important to make reparenting work consistently for now.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix CanvasFrame dirty warning (attachment 249148)

I think CanvasFrame needs to mark itself dirty before calling FrameNeedsReflow.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: floated first-letter frame ordering fix, part 1

When creating frames for a block with floated first-letter where the other frames in the block also contain floats, we end up putting the first-letter float last in the float-list because it gets added to nsFrameConstructorState::mFloatedItems when we do WrapFirstLetterFrame after processing children. This patch makes us find the right place to insert the first-letter float: it's just before any other floats for this block.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: floated first-letter frame ordering fix, part 2

nsFrameConstructorState::ProcessFrameInsertions figures out where out-of-flow frames should be added into their containers' child lists by the postions of the associated content nodes in the content tree. This doesn't work when some of the content is anonymous. This patch replaces that approach with an approach based on comparing the positions of the placeholder frames in the frame tree. This comparison is normally going to give the same results, except for some cases involving continuations that don't matter*; we definitely want to match placeholder ordering in any case.

* E.g. given a float F that spans two pages followed in the document by another float G that spans the same two pages, F's second frame would be ordered after G's first frame in the frame tree even though G's content is after F's. I don't think this can result in ordering differences for frames in the same child list, though.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I believe that only attachment #249205 [details] [diff] [review] is required to fix the crashes, so that's the only one that we'd want to take on branch.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 249208 [details] [diff] [review]
fix CanvasFrame dirty warning (attachment 249148 [details])

I'd actually use NS_FRAME_HAS_DIRTY_CHILDREN for this -- I explicitly noted that that's valid when a child has been removed and no current children are actually dirty.  With that, r+sr=dbaron.

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 249205 [details] [diff] [review]
crash fix

r+sr=dbaron

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 249207 [details] [diff] [review]
fix for attachment 249152 [details]

You should comment that this may call ReParentStyleContext multiple times, but that's not a problem thanks to the newContext != oldContext check.  I'd also change "Recreate its style context just in case." to "Reparent its style context just in case one of our ancestors (split or not) hasn't done so already)."

Do those make sense?  I haven't looked closely at this code for a while, so please yell if they don't.

Anyway, with those changes, r+sr=dbaron.

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 249209 [details] [diff] [review]
floated first-letter frame ordering fix, part 1

Is this really the right thing to do?  It seems like it could improve vertical positioning at the cost of worsening horizontal positioning.  Does it?

I think the long term solution to make floating ::first-letter work correctly is what I just filed as bug 364423.

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 249210 [details] [diff] [review]
floated first-letter frame ordering fix, part 2

This vaguely makes sense (although I haven't looked at the frame comparison functions yet).

But I don't really understand the comment about ProcessFrameInsertions calles needing to be ordered a certain way.
I don't see how anything else could mess with the nsFrameConstructorState |this|.  Is there a problem where the other method of object construction, if it happens later, doesn't maintain the proper ordering, but if it happens first, then ProcessFrameInsertions's calls to CompareTreePosition fix things?

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #24)
> (From update of attachment 249207 [details] [diff] [review] [edit])
> You should comment that this may call
> ReParentStyleContext multiple times, but that's not a
> problem thanks to the newContext != oldContext check. 
> I'd also change "Recreate its style context just in
> case." to "Reparent its style context just in case one
> of our ancestors (split or not) hasn't done so
> already)."

OK

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #25)
> (From update of attachment 249209 [details] [diff] [review] [edit])
> Is this really the right thing to do?  It seems like it
> could improve vertical positioning at the cost of
> worsening horizontal positioning.  Does it?

Not sure what you mean. It shouldn't actually affect layout since floats are laid out as we encounter their placeholders. It shouldn't affect rendering because floats are added to the display list as we encounter their placeholders.

> I think the long term solution to make floating
> ::first-letter work correctly is what I just filed as
> bug 364423.

Sounds good to me.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #26)
> But I don't really understand the comment about
> ProcessFrameInsertions calls needing to be ordered a
> certain way.

Sorry, I should have been more verbose. The issue is that the frame comparison functions only work properly when the placeholders have been inserted into the frame tree. So for example if we have a new float containing the placeholder for a new abs-pos frame, and we process the abs-pos insertion first, then we won't be able to find the right place to insert in in the abs-pos list.

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in attachments 249205, 249207, and 249208.

Comment 31

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 249205 [details] [diff] [review]
crash fix

Crash fix, low risk.

Comment 32

[Jay Patel [:jay]]

Comment on attachment 249205 [details] [diff] [review]
crash fix

Approved for both branches. a=jay for drivers.

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Fixed on branches.

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 249210 [details] [diff] [review]
floated first-letter frame ordering fix, part 2

r+sr=dbaron if you fix the comment per comment 29.  (I didn't look too closely at the comparison function; I assume it's pretty similar to the existing content comparison function.)

Comment 35

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 249209 [details] [diff] [review]
floated first-letter frame ordering fix, part 1

ok, r+sr=dbaron.

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked in attachment 249209 [details] [diff] [review].

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Checked in attachment 249210 [details] [diff] [review] with new comment.

Comment 38

[Boris Zbarsky [:bzbarsky]]

The comments for the new stuff in nsLayoutUtils say aContent when they mean aFrame...  (e.g in the docs for aCommonAncestor and the docs for the return value).

Comment 39

[Martijn Wargers (dead)]

Verified fixed, using current trunk build.
However, I still crash on the "Partly minimised testcase" with the latest branch builds. Should I file a new bug for that?
Talkback ID: TB28534877X
0x00000922
nsHTMLReflowState::ComputePadding  [mozilla/layout/generic/nsHTMLReflowState.cpp, line 2444]
nsHTMLReflowState::InitConstraints  [mozilla/layout/generic/nsHTMLReflowState.cpp, line 1759]
nsHTMLReflowState::Init  [mozilla/layout/generic/nsHTMLReflowState.cpp, line 342]
nsHTMLReflowState::nsHTMLReflowState  [mozilla/layout/generic/nsHTMLReflowState.cpp, line 217]
nsLineLayout::ReflowFrame  [mozilla/layout/generic/nsLineLayout.cpp, line 913]
nsInlineFrame::ReflowInlineFrame  [mozilla/layout/generic/nsInlineFrame.cpp, line 689]
nsInlineFrame::ReflowFrames  [mozilla/layout/generic/nsInlineFrame.cpp, line 519]
nsFirstLineFrame::Reflow  [mozilla/layout/generic/nsInlineFrame.cpp, line 1049]
nsLineLayout::ReflowFrame  [mozilla/layout/generic/nsLineLayout.cpp, line 996]
nsBlockFrame::ReflowInlineFrame  [mozilla/layout/generic/nsBlockFrame.cpp, line 4245]
nsBlockFrame::DoReflowInlineFrames  [mozilla/layout/generic/nsBlockFrame.cpp, line 3898]
nsBlockFrame::ReflowInlineFrames  [mozilla/layout/generic/nsBlockFrame.cpp, line 3779]
nsBlockFrame::ReflowLine  [mozilla/layout/generic/nsBlockFrame.cpp, line 2772]
nsBlockFrame::ReflowDirtyLines  [mozilla/layout/generic/nsBlockFrame.cpp, line 2302]
nsBlockFrame::Reflow  [mozilla/layout/generic/nsBlockFrame.cpp, line 905]
nsContainerFrame::ReflowChild  [mozilla/layout/generic/nsContainerFrame.cpp, line 905]
nsHTMLScrollFrame::ReflowScrolledFrame  [mozilla/layout/generic/nsGfxScrollFrame.cpp, line 523]
nsHTMLScrollFrame::ReflowContents  [mozilla/layout/generic/nsGfxScrollFrame.cpp, line 571]
nsHTMLScrollFrame::Reflow  [mozilla/layout/generic/nsGfxScrollFrame.cpp, line 769]
nsBlockReflowContext::ReflowBlock  [mozilla/layout/generic/nsBlockReflowContext.cpp, line 606]
nsBlockFrame::ReflowFloat  [mozilla/layout/generic/nsBlockFrame.cpp, line 6030]
nsBlockReflowState::FlowAndPlaceFloat  [mozilla/layout/generic/nsBlockReflowState.cpp, line 863]
nsBlockReflowState::PlaceBelowCurrentLineFloats  [mozilla/layout/generic/nsBlockReflowState.cpp, line 1132]
nsBlockFrame::PlaceLine  [mozilla/layout/generic/nsBlockFrame.cpp, line 4609]
nsBlockFrame::DoReflowInlineFrames  [mozilla/layout/generic/nsBlockFrame.cpp, line 4010]
nsBlockFrame::ReflowInlineFrames  [mozilla/layout/generic/nsBlockFrame.cpp, line 3779]
nsBlockFrame::ReflowLine  [mozilla/layout/generic/nsBlockFrame.cpp, line 2772]
nsBlockFrame::ReflowDirtyLines  [mozilla/layout/generic/nsBlockFrame.cpp, line 2302]
nsBlockFrame::Reflow  [mozilla/layout/generic/nsBlockFrame.cpp, line 905]
nsBlockReflowContext::ReflowBlock  [mozilla/layout/generic/nsBlockReflowContext.cpp, line 606]
nsBlockFrame::ReflowBlockFrame  [mozilla/layout/generic/nsBlockFrame.cpp, line 3492]
nsBlockFrame::ReflowLine  [mozilla/layout/generic/nsBlockFrame.cpp, line 2651]
nsBlockFrame::ReflowDirtyLines  [mozilla/layout/generic/nsBlockFrame.cpp, line 2302]
nsBlockFrame::Reflow  [mozilla/layout/generic/nsBlockFrame.cpp, line 905]
nsContainerFrame::ReflowChild  [mozilla/layout/generic/nsContainerFrame.cpp, line 905]
CanvasFrame::Reflow  [mozilla/layout/generic/nsHTMLFrame.cpp, line 536]
nsContainerFrame::ReflowChild  [mozilla/layout/generic/nsContainerFrame.cpp, line 905]
nsHTMLScrollFrame::ReflowScrolledFrame  [mozilla/layout/generic/nsGfxScrollFrame.cpp, line 523]
nsHTMLScrollFrame::ReflowContents  [mozilla/layout/generic/nsGfxScrollFrame.cpp, line 571]
nsHTMLScrollFrame::Reflow  [mozilla/layout/generic/nsGfxScrollFrame.cpp, line 769]
nsContainerFrame::ReflowChild  [mozilla/layout/generic/nsContainerFrame.cpp, line 905]
ViewportFrame::Reflow  [mozilla/layout/generic/nsViewportFrame.cpp, line 240]
IncrementalReflow::Dispatch  [mozilla/layout/base/nsPresShell.cpp, line 914]
PresShell::ProcessReflowCommands  [mozilla/layout/base/nsPresShell.cpp, line 6928]
PresShell::WillPaint  [mozilla/layout/base/nsPresShell.cpp, line 6565]
0x778b0c24
0x00200064
0xe84d8d50
0x4badaf9a

Comment 40

[Jay Patel [:jay]]

v.fixed on 1.8 and 1.8.0 branches with 2/6 nightlies.  I do however see a crash with the "Partly minimised testcase", as Martijn did, so we should get a new bug logged on that.

Comment 41

[Martijn Wargers (dead)]

Ok, I filed bug 369542 for that.