Crash [@ nsHTMLReflowState::ComputePadding ]

VERIFIED FIXED in mozilla1.9alpha1

Status

()

--
critical
VERIFIED FIXED
14 years ago
7 years ago

People

(Reporter: rstrong, Assigned: bzbarsky)

Tracking

({crash, testcase})

Trunk
mozilla1.9alpha1
x86
All
crash, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(7 attachments, 2 obsolete attachments)

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
Created attachment 164923 [details]
Testcase (causes crash)
Created attachment 164924 [details]
Assertions before crash
Attachment #164923 - Attachment description: Testcase → Testcase (causes crash)
Adding keywords crash and testcase
Keywords: crash, testcase
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.
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?
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?
I still crash quite nicely on the "smaller testcase" with a debug build from
"Fri Mar 25 21:34:13 CST 2005"

Comment 9

14 years ago
Created attachment 181121 [details] [diff] [review]
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?
Attachment #181121 - Flags: review?
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

14 years ago
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.

Updated

13 years ago
Blocks: 305386

Updated

13 years ago
Attachment #181121 - Attachment is obsolete: true
Attachment #181121 - Flags: review?
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?
OS: Windows XP → All
Created attachment 193383 [details] [diff] [review]
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?
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?
(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.
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?
Created attachment 193399 [details]
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.
>     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.
Created attachment 193516 [details]
SplitToContainingBlock - "right case"
Attachment #193383 - Attachment is obsolete: true
Created attachment 193517 [details]
SplitToContainingBlock - "left case"
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.
Created attachment 193524 [details] [diff] [review]
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 on attachment 193524 [details] [diff] [review]
wallpaper

I think this worth taking as wallpaper until we know how
to deal with this situation.
Attachment #193524 - Flags: superreview?(roc)
Attachment #193524 - Flags: review?(roc)
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.
Attachment #193524 - Flags: superreview?(roc)
Attachment #193524 - Flags: superreview+
Attachment #193524 - Flags: review?(roc)
Attachment #193524 - Flags: review+
I'm eliminating CantHandleReplacedElement, not SplitToContainingBlock,
unfortunately.
But CantRenderReplacedElement is the only caller of SplitToContainingBlock
(other than SplitToContainingBlock).
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...
I vote we kill SplitToContainingBlock and just reconstruct to the containing
block in the that case.
OK.  Sounds reasonable to me.

Comment 30

13 years ago
*** Bug 300585 has been marked as a duplicate of this bug. ***
Blocks: 316026
Fixed by checkin for bug 11011 (which removed SplitToContainingBlock).
Assignee: nobody → bzbarsky
Depends on: 11011
Target Milestone: --- → mozilla1.9alpha
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED

Comment 33

9 years ago
layout/base/crashtests/268157-1.html
http://hg.mozilla.org/mozilla-central/rev/b0337b6287f3
Flags: in-testsuite+
Crash Signature: [@ nsHTMLReflowState::ComputePadding ]
You need to log in before you can comment on or make changes to this bug.