Crash [@ nsHTMLReflowState::ComputePadding ]

VERIFIED FIXED in mozilla1.9alpha1



14 years ago
7 years ago


(Reporter: rstrong, Assigned: bzbarsky)


({crash, testcase})

crash, testcase
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)


(crash signature)


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

Expected Results:  
No crash
Stack Signature nsHTMLReflowState::ComputePadding 0e003fe8
Source File, Line No.
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.

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

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]

On the Mac, this crash appears to be due to attempt to use a null 
nsCOMPtr<nsIWidget> , in the function nsWindow::ResetInputState() at

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.


13 years ago
Blocks: 305386


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:,367#334

-- but the comments here seems to indicate this is a bad situation:,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]

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

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:
does not mention them at all.
Created attachment 193524 [details] [diff] [review]

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]

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,
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
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

Comment 33

9 years ago
Flags: in-testsuite+
Crash Signature: [@ nsHTMLReflowState::ComputePadding ]
You need to log in before you can comment on or make changes to this bug.