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

Status: VERIFIED FIXED

(Core :: Layout, defect, P3)

Description

[Martijn Wargers (dead)]

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

Comment 1

[Martijn Wargers (dead)]

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

Comment 2

[Daniel Holbert [:dholbert]]

Both testcases crash on Linux as well.
OS --> All.

Comment 3

[Mats Palmgren (inactive)]

Attachment: The life and mystery of a little placeholder

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

Comment 4

[Mats Palmgren (inactive)]

Attachment: wip

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)

Comment 5

[Mats Palmgren (inactive)]

Attachment: some more?

I looked briefly at other callers of DeleteNextInFlowChild() during reflow --
we may need a RemoveMappingsForFrameSubtree() at these places too?

Comment 7

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

Reassigning to Mats since he's working on it

Comment 8

[Martijn Wargers (dead)]

Attachment: testcase3

Comment 9

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

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.

Comment 10

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

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.

Comment 11

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

Attachment: fix

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.

Comment 12

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

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.

Comment 13

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

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.

Comment 14

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

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

Comment 15

[Mats Palmgren (inactive)]

Thanks for picking this up roc.
I filed bug 435379 to investigate the "some more?" bits.

Comment 16

[fantasai]

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

Comment 17

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

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.

Comment 18

[fantasai]

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.

Comment 19

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

Attachment: fix v2

as requested

Comment 20

[fantasai]

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.

Comment 21

[Mike Beltzner [:beltzner, not reading bugmail]]

Fantasai: can you mark one way or the other, please? :)

Comment 22

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

(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

[fantasai]

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

Comment 24

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

Attachment: fix v3

Comment 25

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

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.

Comment 26

[fantasai]

r=fantasai on that, too

Comment 27

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

Attachment: fix v4

Comment 28

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 322701 [details] [diff] [review]
fix v4

a=shaver for immediate CVS trunk insertion.

Comment 29

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

roc landed this, yay.

Comment 30

[Carsten Book [:Tomcat]]

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

Comment 31

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6973f8578e69

Comment 32

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/6973f8578e69