268157 - Crash [@ nsHTMLReflowState::ComputePadding ]

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Robert Strong (they/them - no direct email)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a5) Gecko/20041106
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a5) Gecko/20041106

The soon to be attached simplified testcase causes a crash @
nsHTMLReflowState::ComputePadding - TB1770634H.

Reproducible: Always
Steps to Reproduce:
1. Open testcase
Actual Results:  
Crash

Expected Results:  
No crash

http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=2&type=iid&id=TB1770634H
Stack Signature nsHTMLReflowState::ComputePadding 0e003fe8
Source File, Line No.
c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsHTMLReflowState.cpp,
line 2327

Besides the latest trunk this also crashes Mozilla/5.0 (Windows; U; Windows NT
5.1; en-US; rv:1.7.5) Gecko/20041103 Firefox/1.0RC2

Comment 1

[Robert Strong (they/them - no direct email)]

Attachment: Testcase (causes crash)

Comment 2

[Robert Strong (they/them - no direct email)]

Attachment: Assertions before crash

Comment 3

[Robert Strong (they/them - no direct email)]

Adding keywords crash and testcase

Comment 4

[Boris Zbarsky [:bzbarsky]]

That first assert (about "unexpected flow") fires when |this| is a frame for the
<del> element (in fact, it's the second in-flow frame for the <del>)... it has
three kids that are text frames, and the last of these has a next-in-flow that's
not in the child list of the <del>'s inline frame.

I'll attach a testcase that's a little more minimal (<span> instead of <del>,
removes some content, etc)...  For some reason the first <object> and <div> pair
is in fact necessary.

At a guess, part of the issue here is that when the divs are processed because
we replace the object we end up with an {ib} split, so we blow away things in
WipeContainingBlock and somehow screw up when we reconstruct.  I'm not really
familiar enough with inline reflow to be able to say more than that.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Smaller testcase

Comment 6

[Boris Zbarsky [:bzbarsky]]

Notes:

1)  To get this to crash reliably, you want a debug build (so we 0xdddddddd out
    deleted stuff).
2)  We don't call WipeContainingBlock, just SplitToContainingBlock().  I can
    totally see SplitToContainingBlock() failing to adjust the in-flow stuff
    correctly...

The problem is that I don't know what the in-flow linking should look like in
this case.  Robert, David, is that documented somewhere?

Comment 7

[Robert Strong (they/them - no direct email)]

I don't crash or see any assetions when viewing the testcase using a debug build
from 20050324. Perhaps the checkin for bug 263825 has fixed this?

Comment 8

[Boris Zbarsky [:bzbarsky]]

I still crash quite nicely on the "smaller testcase" with a debug build from
"Fri Mar 25 21:34:13 CST 2005"

Comment 9

[Ben Fowler]

Attachment: Spackle

On the Mac, this crash appears to be due to attempt to use a null 
nsCOMPtr<nsIWidget> , in the function nsWindow::ResetInputState() at
http://lxr.mozilla.org/seamonkey/source/widget/src/mac/nsWindow.cpp#2471

This patch guards against this.

This file seems to have inconsistent brace and indenting - would a patch 
for this be accepted?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Ben, chances are the patch you posted isn't really a patch for this bug (which
is cross-platform) or is a patch for a consequence without addressing the real
issue...  You may want to put it in a separate bug (and request review from one
of the Mac widget owners).

Comment 11

[Ben Fowler]

I have opened Bug 291096 : I fear that I have wasted your time as the patch
actually does nothing anyway (the code path is exercised by the testcase though).

On my build using standard methods 2005-04-18, this crash is still in; but
using recent debug builds, I do not get a crash - just my warning. I suspect 
that either there is a partial fix in, or Mac is less susceptible or I have 
a patch in my tree which hides the crash.

Comment 12

[Mats Palmgren (inactive)]

I started debugging bug 305386 which I'm pretty sure is the same underlying bug.
The reason we crash is that someone pushes a frame that still has a next-in-flow
with a different parent and then we pick those up during initial reflow and
decide to set the parent pointer lazily:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsInlineFrame.cpp&rev=3.241&root=/cvsroot&mark=366,367#334

-- but the comments here seems to indicate this is a bad situation:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/generic/nsInlineFrame.cpp&rev=3.241&root=/cvsroot&mark=501,509-517#471

Is it ok to "steal" those next-in-flows in PushFrames()? or should this not
have happened in the first place, meaning something else is broken?

Comment 13

[Mats Palmgren (inactive)]

Attachment: fix?

I don't think this block makes sense. Removing it fixes the crashes and all the

assertions disappear. This block was a fix for bug 32192, but that crash does
not
come back after removing this block, so maybe it was just a wallpaper and that
the real bug that caused 32192 has since been fixed?

Comment 14

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

Why do you think that doesn't make sense?

I can't think of a case --- even in an IB situation --- where this block should
be asserting. Can you explain it in detail?

Comment 15

[Mats Palmgren (inactive)]

(In reply to comment #14)
> Why do you think that doesn't make sense?

Because the next-in-flow frame that we bluntly do SetParent() on here is still
in some other frame's child list (mFrames). That's not a healthy situation
as far as I know.

Comment 16

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

It should be in our child list, so this assertion:
-        NS_ASSERTION(mFrames.ContainsFrame(nextInFlow), "unexpected flow");
Should not be firing.

Can you explain the situation that leads to it firing?

Comment 17

[Mats Palmgren (inactive)]

Attachment: frame dump + stack

Here's a frame dump (at the start) in nsInlineFrame::PushFrames when the
problem occurs. The last of the frames we are pushing has a next-in-flow with
a different parent.

Comment 18

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

>     Inline(span)(1)@0x86b8668 next=0x86f71c4 {0,0,0,0} [state=00008403]
[content=0x86f24c0] [sc=0x86f682c]<

This is still in the intial reflow for this frame? We shouldn't be getting into
this state where we're doing an initial reflow on the frame while it already has
a next-in-flow (because of text wrapping, it appears). And I believe that at the
point of the dump, 0x86b8668 should have its next in flow set to 0x86f71c4.

Comment 19

[Mats Palmgren (inactive)]

Attachment: SplitToContainingBlock - "right case"

Comment 20

[Mats Palmgren (inactive)]

Attachment: SplitToContainingBlock - "left case"

Comment 21

[Mats Palmgren (inactive)]

It is SplitToContainingBlock that has reset the next/prev-in-flow
for the ancestors:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1114&root=/cvsroot&mark=13382-13386#13362

This is what leads to the "dangling" in-flow chains.
The "right" case is triggered by bug 305386, the "left" by this bug.
Note that for these traces I commented out the code block above so
when the crashes occurs the red prev/next-in-flow links have been
nulled out and that's the root cause for the crashes.

I tried various way to repair the situation, and it is possible
to make the crashes a harder to reproduce, but I failed to fix
it completely.

If someone can tell me what the prev/next-in-flow is supposed to
look like after a split that would be much appreciated.
The otherwise good documentation here:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1114&root=/cvsroot&mark=13180-13252#13180
does not mention them at all.

Comment 22

[Mats Palmgren (inactive)]

Attachment: wallpaper

This fixes all the crashes for me (and no assertions).
I think it is wrong to null out the in-flow chain like this though,
since it breaks text selection for example.

Comment 23

[Mats Palmgren (inactive)]

Comment on attachment 193524 [details] [diff] [review]
wallpaper

I think this worth taking as wallpaper until we know how
to deal with this situation.

Comment 24

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

I believe Boris' work on handling broken plugins and images will fix this for
real by eliminating SplitToContainingBlock (which is indescribably evil).

This patch is hideous --- it leaves flow chains without a primary frame at the
start --- but I suppose it's no more hideous than SplitToContaingBlock cutting
up the flow chain.

Comment 25

[Boris Zbarsky [:bzbarsky]]

I'm eliminating CantHandleReplacedElement, not SplitToContainingBlock,
unfortunately.

Comment 26

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

But CantRenderReplacedElement is the only caller of SplitToContainingBlock
(other than SplitToContainingBlock).

Comment 27

[Boris Zbarsky [:bzbarsky]]

Isn't that just a matter of needing more callers?  Right now we just flub
inserting a block as a child of an inline, don't we?  That is, I don't think we
create an IB split in that case.  I believe we have bugs on that...

Comment 28

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

I vote we kill SplitToContainingBlock and just reconstruct to the containing
block in the that case.

Comment 29

[Boris Zbarsky [:bzbarsky]]

OK.  Sounds reasonable to me.

Comment 30

[timeless]

*** Bug 300585 has been marked as a duplicate of this bug. ***

Comment 31

[Boris Zbarsky [:bzbarsky]]

Fixed by checkin for bug 11011 (which removed SplitToContainingBlock).

Comment 32

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED on trunk using both testcases here on build 1.5a of SeaMonkey Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051207 Mozilla/1.0

Comment 33

[Bob Clary [:bc] (inactive)]

layout/base/crashtests/268157-1.html
http://hg.mozilla.org/mozilla-central/rev/b0337b6287f3