Crash [@ nsHTMLReflowState::GetNearestContainingBlock] with display: -moz-box, generated content, positioning and fieldset

VERIFIED FIXED

Status

()

defect
P3
critical
VERIFIED FIXED
12 years ago
6 years ago

People

(Reporter: martijn.martijn, Assigned: roc)

Tracking

({crash, regression, testcase})

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [dbaron-1.9:RsCo][RC2+], crash signature)

Attachments

(7 attachments, 3 obsolete attachments)

(Reporter)

Description

12 years ago
Posted file testcase
See testcase, which crashes current trunk build on load.

This seems to have regressed between 2007-10-01 and 2007-10-02:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-10-01+04&maxdate=2007-10-02+09&cvsroot=%2Fcvsroot
I guess a regression from bug 154892 somehow.

Note that there might be a related bug, because it has a similar stack: bug 395609.

http://crash-stats.mozilla.com/report/index/09e0c911-7136-11dc-a23c-001a4bd43ed6
0  	nsHTMLReflowState::GetNearestContainingBlock(nsIFrame*, int&, int&)  	 mozilla/layout/generic/nsHTMLReflowState.cpp:704
1 	nsHTMLReflowState::InitAbsoluteConstraints(nsPresContext*, nsHTMLReflowState const*, int, int) 	mozilla/layout/generic/nsHTMLReflowState.cpp:1104
2 	nsHTMLReflowState::InitConstraints(nsPresContext*, int, int, nsMargin const*, nsMargin const*) 	mozilla/layout/generic/nsHTMLReflowState.cpp:1807
3 	nsHTMLReflowState::Init(nsPresContext*, int, int, nsMargin const*, nsMargin const*) 	mozilla/layout/generic/nsHTMLReflowState.cpp:315
4 	nsHTMLReflowState::nsHTMLReflowState(nsPresContext*, nsHTMLReflowState const&, nsIFrame*, nsSize const&, int, int, int) 	mozilla/layout/generic/nsHTMLReflowState.cpp:180
5 	nsAbsoluteContainingBlock::ReflowAbsoluteFrame(nsIFrame*, nsPresContext*, nsHTMLReflowState const&, int, int, int, nsIFrame*, unsigned int&, nsRect*) 	mozilla/layout/generic/nsAbsoluteContainingBlock.cpp:414
6 	nsAbsoluteContainingBlock::Reflow(nsContainerFrame*, nsPresContext*, nsHTMLReflowState const&, unsigned int&, int, int, int, int, int, nsRect*) 	mozilla/layout/generic/nsAbsoluteContainingBlock.cpp:168
7 	nsPositionedInlineFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) 	mozilla/layout/generic/nsInlineFrame.cpp:1137
8 	nsLineLayout::ReflowFrame(nsIFrame*, unsigned int&, nsHTMLReflowMetrics*, int&) 	mozilla/layout/generic/nsLineLayout.cpp:882
9 	nsBlockFrame::ReflowInlineFrame(nsBlockReflowState&, nsLineLayout&, nsLineList_iterator, nsIFrame*, LineReflowStatus*) 	mozilla/layout/generic/nsBlockFrame.cpp:3555
10 	nsBlockFrame::DoReflowInlineFrames(nsBlockReflowState&, nsLineLayout&, nsLineList_iterator, int*, LineReflowStatus*, int) 	mozilla/layout/generic/nsBlockFrame.cpp:3375
(Reporter)

Updated

12 years ago
Flags: blocking1.9?
Flags: blocking1.9? → blocking1.9+
Whiteboard: [dbaron-1.9:RsCo]
(Reporter)

Comment 1

12 years ago
Posted file testcase2
This testcase is probably about the same bug, it has a slightly different stacktrace.

http://crash-stats.mozilla.com/report/index/8d1b51e3-7dc5-11dc-89d7-001a4bd43ed6
0  	@0x0  	
1 	nsHTMLReflowState::CalculateHypotheticalBox(nsPresContext*, nsIFrame*, nsIFrame*, int, int, nsHTMLReflowState const*, nsHypotheticalBox&) 	mozilla/layout/generic/nsHTMLReflowState.cpp:1066
2 	nsHTMLReflowState::InitAbsoluteConstraints(nsPresContext*, nsHTMLReflowState const*, int, int) 	mozilla/layout/generic/nsHTMLReflowState.cpp:1116
3 	nsHTMLReflowState::InitConstraints(nsPresContext*, int, int, nsMargin const*, nsMargin const*) 	mozilla/layout/generic/nsHTMLReflowState.cpp:1807
4 	nsHTMLReflowState::Init(nsPresContext*, int, int, nsMargin const*, nsMargin const*) 	mozilla/layout/generic/nsHTMLReflowState.cpp:315
5 	nsHTMLReflowState::nsHTMLReflowState(nsPresContext*, nsHTMLReflowState const&, nsIFrame*, nsSize const&, int, int, int) 	mozilla/layout/generic/nsHTMLReflowState.cpp:180
6 	nsAbsoluteContainingBlock::ReflowAbsoluteFrame(nsIFrame*, nsPresContext*, nsHTMLReflowState const&, int, int, int, nsIFrame*, unsigned int&, nsRect*) 	mozilla/layout/generic/nsAbsoluteContainingBlock.cpp:414
7 	nsAbsoluteContainingBlock::Reflow(nsContainerFrame*, nsPresContext*, nsHTMLReflowState const&, unsigned int&, int, int, int, int, int, nsRect*) 	mozilla/layout/generic/nsAbsoluteContainingBlock.cpp:168
8 	ViewportFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) 	mozilla/layout/generic/nsViewportFrame.cpp:322
9 	PresShell::DoReflow(nsIFrame*) 	mozilla/layout/base/nsPresShell.cpp:6125
10 	PresShell::ProcessReflowCommands(int) 	mozilla/layout/base/nsPresShell.cpp:6230
Both testcases crash on Linux as well.
OS --> All.
OS: Windows XP → All
Priority: -- → P4
For the first testcase the problem is the DeleteNextInFlowChild() call in
nsLineLayout::ReflowFrame() - the next-in-flow we're deleting contains a
placeholder that escapes that subtree, leaving a dangling abs.pos. frame
that when it's reflowed passes a null placeholder to
GetNearestContainingBlock (which crashes):
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsHTMLReflowState.cpp&rev=1.284&root=/cvsroot&mark=705,1098-1099,1104#1086

(The second testcase does not crash for me on Linux.)
Posted patch wipSplinter Review
1. nsPositionedInlineFrame can have overflow so we need to let others know
   about that list (eg. DeletingFrameSubtree). This is important, but it's
   not the cause of the crash in this bug (it might be another bug though).
2. print nsPositionedInlineFrame as "PositionedInline" in frame dumps.
3. call RemoveMappingsForFrameSubtree() before destroying the next-in-flow,
   allowing DeletingFrameSubtree() to find and remove the abs.pos.
   (this fixes the crash in testcase1)
Posted patch some more?Splinter Review
I looked briefly at other callers of DeleteNextInFlowChild() during reflow --
we may need a RemoveMappingsForFrameSubtree() at these places too?

Updated

12 years ago
Duplicate of this bug: 409637
(Reporter)

Updated

12 years ago
Blocks: 410198
Reassigning to Mats since he's working on it
Assignee: nobody → mats.palmgren
Flags: wanted1.9.0.x+
Flags: blocking1.9-
Flags: tracking1.9+
(Reporter)

Updated

11 years ago
Blocks: 428113
(Reporter)

Comment 8

11 years ago
Posted file testcase3
(Reporter)

Updated

11 years ago
Blocks: 429881
(Reporter)

Updated

11 years ago
Blocks: 435047
we really shouldn't allow abs-pos children of a positioned inline frame to escape from the first-in-flow. That's what we do for blocks.
Hmm, I guess we already don't allow them to escape.

So now I'm wondering why we destroy the inline frame's next-in-flow when it's not empty.
Posted patch fix (obsolete) — Splinter Review
This fixes it.

The problem is that NS_FRAME_MERGE_INCOMPLETE was wiping out INLINE_BREAK status in the incoming reflow status(es). In particular, nsPositionedInlineFrame would never return INLINE_BREAK_BEFORE if it had an abs-pos child, because reflowing the child would wipe out that part of the status. The problem was worse because when INLINE_BREAK_BEFORE is returned, the result is not marked INCOMPLETE. So in these testcases, that led to nsPositionedInlineFrame returning COMPLETE when it certainly was not.

The fix is straightforward and quite safe IMHO. It also fixes motortrend.com.
Attachment #322213 - Flags: superreview?(dbaron)
Attachment #322213 - Flags: review?
Attachment #322213 - Flags: review? → review?(fantasai.bugs)
Comment on attachment 290643 [details] [diff] [review]
wip

I think we should take the nsInlineFrame part of Mats' patch --- it's obviously correct, even though we don't know any bugs that it fixes. Not for 1.9 though.
Attachment #290643 - Flags: superreview+
Attachment #290643 - Flags: review+
It seems like there are two changes here:
 * preserving the NS_INLINE_BREAK_MASK bits within primary
 * preserving NS_FRAME_TRUNCATED from either

You didn't mention the second in comment 11.

I wonder if there's a better name for the macro that makes it clear that the arguments are not symmetric.
(In reply to comment #13)
> It seems like there are two changes here:
>  * preserving the NS_INLINE_BREAK_MASK bits within primary
>  * preserving NS_FRAME_TRUNCATED from either
> 
> You didn't mention the second in comment 11.

Yeah sorry. I think we clearly should be propagating TRUNCATED status from at least the primary reflow. I'm less sure about the secondary but I think we should be propagating it there too.

> I wonder if there's a better name for the macro that makes it clear that the
> arguments are not symmetric.

NS_FRAME_MERGE_STATUS_INTO?
Thanks for picking this up roc.
I filed bug 435379 to investigate the "some more?" bits.
Assignee: mats.palmgren → roc

Comment 16

11 years ago
Why are we only preserving the NS_INLINE_BREAK_MASK bits in the primary? In the past these macros were |= assignments. We should probably be merging all the bits and just cutting out the lesser of the mutually-exclusive ones. (I.e. take out OVERFLOW_INCOMPETE if INCOMPLETE is set.)
It doesn't make sense to OR together line break statuses. NS_INLINE_BREAK_BEFORE | NS_LINE_BREAK_AFTER == NS_LINE_BREAK_AFTER, which definitely makes no sense, but it wouldn't make much more sense if it was NS_LINE_BREAK_BEFORE instead. ORing together different break types definitely doesn't make sense.

Fortunately, because overflow containers and out-of-flow elements always break at vertical boundaries and never at horizontal boundaries, we can't actually have multiple line-break statuses at the same time.
Whiteboard: [dbaron-1.9:RsCo] → [dbaron-1.9:RsCo][RC2?][RC2+]
Attachment #322213 - Flags: superreview?(dbaron) → superreview+

Comment 18

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

Ok. I'm going to ask that you change the name of the macro as dbaron suggested, and also to build the assignment into the macro. If we're going to give the arguments unequal treatment and say INTO, might as well add in the assignment part too.
Attachment #322213 - Flags: review?(fantasai.bugs) → review-
Posted patch fix v2 (obsolete) — Splinter Review
as requested
Attachment #322351 - Flags: superreview?(dbaron)
Attachment #322351 - Flags: review?(fantasai.bugs)
Status: NEW → ASSIGNED
Hardware: PC → All

Comment 20

11 years ago
Comment on attachment 322351 [details] [diff] [review]
fix v2

Typo "carried overy". NS_INLINE_BREAK_MASK no longer seems to be necessary.

I would probably have written the body of the function as 2 steps: merging all relevant bits in, then taking out OVERFLOW_INCOMPLETE as necessary. Or maybe we should redefine OVERFLOW_INCOMPLETE as ignored when INCOMPLETE is set so the second step is unnecessary. But r=fantasai if dbaron thinks it's good.
Flags: blocking1.9- → blocking1.9+
Whiteboard: [dbaron-1.9:RsCo][RC2?][RC2+] → [dbaron-1.9:RsCo][RC2+]
Fantasai: can you mark one way or the other, please? :)

Updated

11 years ago
Attachment #322351 - Flags: review?(fantasai.bugs) → review-
(In reply to comment #20)
> I would probably have written the body of the function as 2 steps: merging all
> relevant bits in, then taking out OVERFLOW_INCOMPLETE as necessary. Or maybe we
> should redefine OVERFLOW_INCOMPLETE as ignored when INCOMPLETE is set so the
> second step is unnecessary. But r=fantasai if dbaron thinks it's good.

So is what you're proposing that the code look like:

nsReflowStatus result = *aPrimary | (aSecondary & (NS_FRAME_TRUNCATED | NS_FRAME_REFLOW_NEXTINFLOW | NS_FRAME_NOT_COMPLETE | NS_FRAME_OVERFLOW_INCOMPLETE);
if ((result & (NS_FRAME_NOT_COMPLETE | NS_FRAME_OVERFLOW_INCOMPLETE)) == (NS_FRAME_NOT_COMPLETE | NS_FRAME_OVERFLOW_INCOMPLETE)) {
  result &= ~NS_FRAME_OVERFLOW_INCOMPLETE;
}
*aPrimary = result;

or something like that?  (Is the idea that the function would always give the same results given the same input, or are you proposing a change in what it does (such as including more parts of aSecondary)?

If it's just the question of how the function that produces the same output is written, I don't think it matters, and I'm happy to sr either way.


But I'd note that the comment above the reflow statuses in nsIFrame.h should probably be changed to not mention the "MERGE macro" anymore.  (And I should probably file a followup bug on cleaning up a bunch of other issues there, such as putting the bits defined in order and not having so many macros for 0, which can easily lead to mistakes.)

Comment 23

11 years ago
aPrimary |= aSecondary & (NS_FRAME_TRUNCATED | NS_FRAME_REFLOW_NEXTINFLOW
                         | NS_FRAME_NOT_COMPLETE | NS_FRAME_OVERFLOW_INCOMPLETE);
if (*aPrimary & NS_FRAME_NOT_COMPLETE)
  *aPrimary &= ~NS_FRAME_OVERFLOW_INCOMPLETE;

would do. But regardless, the NS_INLINE_BREAK_MASK doesn't need to be added anymore, and there's a typo in one of the comments. :) As I said, r=fantasai if you don't mind about that and those two issues are fixed (and the MERGE comment updated, as you say).
Attachment #322351 - Flags: superreview?(dbaron) → superreview+
Posted patch fix v3 (obsolete) — Splinter Review
Attachment #322213 - Attachment is obsolete: true
Attachment #322351 - Attachment is obsolete: true
Attachment #322692 - Flags: superreview?(dbaron)
Attachment #322692 - Flags: review?(dbaron)
Comment on attachment 322692 [details] [diff] [review]
fix v3

r+sr=dbaron, except for this chunk in nsFrame.cpp:

>  * The Original Code is mozilla.org code.
>- *
>+ *NS_MergeReflowStatusInto
>  * The Initial Developer of the Original Code is
>  * Netscape Communications Corporation.
>  * Portions created by the Initial Developer are Copyright (C) 1998
>  * the Initial Developer. All Rights Reserved.
Attachment #322692 - Flags: superreview?(dbaron)
Attachment #322692 - Flags: superreview+
Attachment #322692 - Flags: review?(dbaron)
Attachment #322692 - Flags: review+

Comment 26

11 years ago
r=fantasai on that, too
Posted patch fix v4Splinter Review
Attachment #322692 - Attachment is obsolete: true
Attachment #322701 - Flags: superreview+
Attachment #322701 - Flags: review+
Attachment #322701 - Flags: approval1.9?
Comment on attachment 322701 [details] [diff] [review]
fix v4

a=shaver for immediate CVS trunk insertion.
Attachment #322701 - Flags: approval1.9? → approval1.9+
roc landed this, yay.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
verified fixed using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre)
Gecko/2008052816 Minefield/3.0pre and Mozilla/5.0 (Macintosh; U; Intel Mac OS X
10.5; en-US; rv:1.9pre) Gecko/2008052817 Firefox/3.0pre (Debug Builds with this
Patch included).

No Crash on the Motortrend.com site - no crash on testcases --> Verified fixed
Status: RESOLVED → VERIFIED

Updated

11 years ago
Flags: in-testsuite?
Flags: wanted1.9.0.x+
No longer blocks: 429881
Crash Signature: [@ nsHTMLReflowState::GetNearestContainingBlock]
You need to log in before you can comment on or make changes to this bug.