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).

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

(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.

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.

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.

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

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 :-).

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.

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 :-)

OK, that was a simple bug in CanvasFrame.

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 {}

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.

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

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.

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.

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.

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

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.

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.

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 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 on attachment 249205 [details] [diff] [review] crash fix r+sr=dbaron

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 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 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?

(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

(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.

(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.

checked in attachments 249205, 249207, and 249208.

Comment on attachment 249205 [details] [diff] [review] crash fix Crash fix, low risk.

Comment on attachment 249205 [details] [diff] [review] crash fix Approved for both branches. a=jay for drivers.

Fixed on branches.

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 on attachment 249209 [details] [diff] [review] floated first-letter frame ordering fix, part 1 ok, r+sr=dbaron.

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

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

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).

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

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.

Ok, I filed bug 369542 for that.