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

VERIFIED FIXED

Status

()

Core
Layout
--
critical
VERIFIED FIXED
11 years ago
6 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: roc)

Tracking

({crash, verified1.8.0.10, verified1.8.1.2})

Trunk
x86
All
crash, verified1.8.0.10, verified1.8.1.2
Points:
---
Bug Flags:
blocking1.8.1.2 +
blocking1.8.0.10 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical], crash signature)

Attachments

(10 attachments)

347 bytes, text/html
Details
433 bytes, text/html
Details
276 bytes, text/html
Details
362 bytes, text/html
Details
510 bytes, text/html
Details
1.70 KB, patch
dbaron
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
3.18 KB, patch
dbaron
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
1.16 KB, patch
dbaron
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
17.27 KB, patch
dbaron
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
11.07 KB, patch
dbaron
: review+
dbaron
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

11 years ago
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).
(Reporter)

Comment 1

11 years ago
Created attachment 248631 [details]
Unminimised testcase
(Reporter)

Comment 2

11 years ago
Created attachment 248632 [details]
Partly minimised testcase

Updated

11 years ago
Whiteboard: [sg:critical]

Comment 3

11 years ago
Does the patch in bug 359371 help?
(the stack and assertions looks a bit like it could)
OS: Windows XP → All
(Reporter)

Comment 4

11 years ago
(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.
Flags: blocking1.8.1.2?
Flags: blocking1.8.0.10?
Created attachment 248960 [details]
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.
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
Assignee: nobody → roc
Flags: blocking1.8.1.2?
Flags: blocking1.8.1.2+
Flags: blocking1.8.0.10?
Flags: blocking1.8.0.10+
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 :-).
Created attachment 249146 [details]
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.
Created attachment 249148 [details]
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 :-)
OK, that was a simple bug in CanvasFrame.
Created attachment 249152 [details]
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 {}
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.
Created attachment 249172 [details]
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
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.
Created attachment 249205 [details] [diff] [review]
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.
Attachment #249205 - Flags: superreview?(dbaron)
Attachment #249205 - Flags: review?(dbaron)
Created attachment 249207 [details] [diff] [review]
fix for attachment 249152 [details]

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.
Attachment #249207 - Flags: superreview?(dbaron)
Attachment #249207 - Flags: review?(dbaron)
Created attachment 249208 [details] [diff] [review]
fix CanvasFrame dirty warning (attachment 249148 [details])

I think CanvasFrame needs to mark itself dirty before calling FrameNeedsReflow.
Attachment #249208 - Flags: superreview?(dbaron)
Attachment #249208 - Flags: review?(dbaron)
Created attachment 249209 [details] [diff] [review]
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.
Attachment #249209 - Flags: superreview?(dbaron)
Attachment #249209 - Flags: review?(dbaron)
Created attachment 249210 [details] [diff] [review]
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.
Attachment #249210 - Flags: superreview?(dbaron)
Attachment #249210 - Flags: review?(dbaron)
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.
Attachment #249208 - Flags: superreview?(dbaron)
Attachment #249208 - Flags: superreview+
Attachment #249208 - Flags: review?(dbaron)
Attachment #249208 - Flags: review+
Comment on attachment 249205 [details] [diff] [review]
crash fix

r+sr=dbaron
Attachment #249205 - Flags: superreview?(dbaron)
Attachment #249205 - Flags: superreview+
Attachment #249205 - Flags: review?(dbaron)
Attachment #249205 - Flags: review+
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.
Attachment #249207 - Flags: superreview?(dbaron)
Attachment #249207 - Flags: superreview+
Attachment #249207 - Flags: review?(dbaron)
Attachment #249207 - Flags: review+
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.
Attachment #249205 - Flags: approval1.8.1.2?
Attachment #249205 - Flags: approval1.8.0.10?

Comment 32

11 years ago
Comment on attachment 249205 [details] [diff] [review]
crash fix

Approved for both branches. a=jay for drivers.
Attachment #249205 - Flags: approval1.8.1.2?
Attachment #249205 - Flags: approval1.8.1.2+
Attachment #249205 - Flags: approval1.8.0.10?
Attachment #249205 - Flags: approval1.8.0.10+
Fixed on branches.
Keywords: fixed1.8.0.10, fixed1.8.1.2
(Reporter)

Updated

10 years ago
Blocks: 363448
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.)
Attachment #249210 - Flags: superreview?(dbaron)
Attachment #249210 - Flags: superreview+
Attachment #249210 - Flags: review?(dbaron)
Attachment #249210 - Flags: review+
Comment on attachment 249209 [details] [diff] [review]
floated first-letter frame ordering fix, part 1

ok, r+sr=dbaron.
Attachment #249209 - Flags: superreview?(dbaron)
Attachment #249209 - Flags: superreview+
Attachment #249209 - Flags: review?(dbaron)
Attachment #249209 - Flags: review+
Checked in attachment 249209 [details] [diff] [review].
Checked in attachment 249210 [details] [diff] [review] with new comment.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
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).
(Reporter)

Comment 39

10 years ago
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
Status: RESOLVED → VERIFIED

Comment 40

10 years ago
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.
Keywords: fixed1.8.0.10, fixed1.8.1.2 → verified1.8.0.10, verified1.8.1.2
(Reporter)

Comment 41

10 years ago
Ok, I filed bug 369542 for that.
Group: security
Crash Signature: [@ nsHTMLReflowState::nsHTMLReflowState] [@ nsRuleNode::Mark]
You need to log in before you can comment on or make changes to this bug.