263825 - Columns: Crash in [@ nsLineBox::IsEmpty ] with floating div inside column

Status: VERIFIED FIXED

(Core :: Layout: Floats, defect)

Description

[Martijn Wargers (dead)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a5) Gecko/20041010 Firefox/0.9.1+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a5) Gecko/20041010 Firefox/0.9.1+

See upcoming testcase.
This testcase crashes in recent builds which have columns support.

Talkback ID's: TB1238995K and TB1238856X

Reproducible: Always
Steps to Reproduce:
1.
2.
3.

Comment 1

[Martijn Wargers (dead)]

Attachment: Testcase (causes a crash)

Comment 2

[Martijn Wargers (dead)]

Forgot to say: it doesn't crash directly, it crashes when you load the testcase
and then resize your window.

Comment 3

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

nsLineBox::IsEmpty 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsLineBox.cpp,
line 285]
nsBlockFrame::Reflow 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsBlockFrame.cpp,
line 828]
nsContainerFrame::ReflowChild 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsContainerFrame.cpp,
line 993]
nsColumnSetFrame::ReflowChildren 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsColumnSetFrame.cpp,
line 464]
nsColumnSetFrame::Reflow 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsColumnSetFrame.cpp,
line 727]
nsBlockReflowContext::ReflowBlock 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsBlockReflowContext.cpp,
line 544]
nsBlockFrame::ReflowBlockFrame 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsBlockFrame.cpp,
line 3203]
nsBlockFrame::ReflowLine 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsBlockFrame.cpp,
line 2455]
nsBlockFrame::ReflowDirtyLines 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsBlockFrame.cpp,
line 2112]
nsBlockFrame::Reflow 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsBlockFrame.cpp,
line 828]
nsBlockReflowContext::ReflowBlock 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsBlockReflowContext.cpp,
line 544]
nsBlockFrame::ReflowBlockFrame 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsBlockFrame.cpp,
line 3203]
nsBlockFrame::ReflowLine 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsBlockFrame.cpp,
line 2455]
nsBlockFrame::ReflowDirtyLines 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsBlockFrame.cpp,
line 2112]
nsBlockFrame::Reflow 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsBlockFrame.cpp,
line 828]
nsContainerFrame::ReflowChild 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsContainerFrame.cpp,
line 993]
CanvasFrame::Reflow 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsHTMLFrame.cpp,
line 551]
nsFrame::BoxReflow 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsFrame.cpp,
line 5256]
nsFrame::DoLayout 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsFrame.cpp,
line 4996]
nsIFrame::Layout 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/xul/base/src/nsBox.cpp,
line 799]
nsIFrame::Layout 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/xul/base/src/nsBox.cpp,
line 799]
nsGfxScrollFrameInner::LayoutBox 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsGfxScrollFrame.cpp,
line 1668]
nsHTMLScrollFrame::DoLayout 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsGfxScrollFrame.cpp,
line 1081]
nsIFrame::Layout 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/xul/base/src/nsBox.cpp,
line 799]
nsHTMLScrollFrame::Reflow 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsGfxScrollFrame.cpp,
line 514]
nsContainerFrame::ReflowChild 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsContainerFrame.cpp,
line 993]
ViewportFrame::Reflow 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsViewportFrame.cpp,
line 249]
PresShell::ResizeReflow 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsPresShell.cpp,
line 2899]
PresShell::ResizeReflow 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsPresShell.cpp,
line 6043]
nsViewManager::DispatchEvent 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/view/src/nsViewManager.cpp,
line 1806]
HandleEvent 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/view/src/nsView.cpp, line
168]
nsWindow::DispatchEvent 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 1074]
nsWindow::OnResize 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 5176]
nsWindow::ProcessMessage 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 4306]
nsWindow::WindowProc 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 1355]
USER32.DLL + 0x1ef0 (0x77e11ef0)
USER32.DLL + 0x3869 (0x77e13869)
USER32.DLL + 0x66e4 (0x77e166e4)
ntdll.dll + 0x1ff57 (0x77f9ff57)
DocumentViewerImpl::SetBounds 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/content/base/src/nsDocumentViewer.cpp,
line 1446]
nsSubDocumentFrame::Reflow 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/html/document/src/nsFrameFrame.cpp,
line 412]
nsFrame::BoxReflow 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsFrame.cpp,
line 5256]
nsFrame::DoLayout 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsFrame.cpp,
line 4996]
nsIFrame::Layout 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/xul/base/src/nsBox.cpp,
line 799]
nsBoxFrame::DoLayout 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/xul/base/src/nsBoxFrame.cpp,
line 1100]
nsIFrame::Layout 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/xul/base/src/nsBox.cpp,
line 799]
nsBoxFrame::DoLayout 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/xul/base/src/nsBoxFrame.cpp,
line 1100]
nsBoxFrame::DoLayout 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/xul/base/src/nsBoxFrame.cpp,
line 1100]
nsBoxFrame::DoLayout 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/xul/base/src/nsBoxFrame.cpp,
line 1100]
nsBoxFrame::DoLayout 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/xul/base/src/nsBoxFrame.cpp,
line 1100]
nsBoxFrame::DoLayout 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/xul/base/src/nsBoxFrame.cpp,
line 1100]
nsBoxFrame::DoLayout 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/xul/base/src/nsBoxFrame.cpp,
line 1100]
nsRootBoxFrame::Reflow 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/xul/base/src/nsRootBoxFrame.cpp,
line 240]
nsContainerFrame::ReflowChild 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsContainerFrame.cpp,
line 993]
ViewportFrame::Reflow 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsViewportFrame.cpp,
line 249]
PresShell::ResizeReflow 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsPresShell.cpp,
line 2899]
PresShell::ResizeReflow 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsPresShell.cpp,
line 6043]
nsViewManager::DispatchEvent 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/view/src/nsViewManager.cpp,
line 1806]
HandleEvent 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/view/src/nsView.cpp, line
168]
nsWindow::DispatchEvent 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 1074]
nsWindow::OnResize 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 5176]
nsWindow::ProcessMessage 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 4306]
nsWindow::WindowProc 
[c:/builds/tinderbox/firefox/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 1355]
USER32.DLL + 0x1ef0 (0x77e11ef0)

Comment 4

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

What's happening here is that I never tested floats with continuations in
columns. I did take out the code to destroy all the continuations of a float
before reflowing it, because that simply won't be acceptable for incremental reflow.

One problem we encounter in this testcase is that after we're in a state where
the float is split across two columns, we reflow again and after reflowing the
first chunk of the float we die in nsBlockFrame::SplitPlaceholders because
CreateNextInFlow didn't create a next in flow, because one already exists.
That's easy to fix, SplitPlaceholders should just bail when a next-in-flow
already exists.

But then we get to a deeper problem. Layout starts by reflowing the columns once
to find the max height, which is 102 pixels, then reflowing again at 51 pixels
to see if the content fits. The reflow works correctly, breaking the float into
two pieces, but it doesn't fit. Maybe it should fit --- I'm not sure --- but
that's not important right now. Next we reflow at 102px - 1 twip to see if the
content fits. We reflow the first column with its float, no problem. But since
there's plenty of room on the first column's line, it does PullFrame to try to
pull some content over from the first line in the second column. That pulls the
placeholder for the second chunk of the float from the second column into the
first column! So now we're trying to lay out the second chunk of the float next
to the first in the same column. Bummer. Crazy stuff happens and we crash pretty
soon.

We need to disallow multiple placeholders for the same float in the same block.
I think this will do it:
-- don't allow PullFrame to pull continuation placeholders
Actually that should be enough. In fact, it fixes this bug.

It does leave an assertion that I need to figure out:
###!!! ASSERTION: Registering a placeholder for a frame that already has a
placeholder!: '!entry->placeholderFrame', file nsFrameManager.cpp, line 539

Also, I can now see that the column balancing algorithm doesn't work well for
this situation where a child element can be broken at any vertical position. It
does binary search for a while and then when the window of feasible heights gets
"small enough", we start doing a linear search from the largest known feasible
height down to the smallest, each iteration trying a height constraint that's
1twip smaller than the desired height in the last iteration. For columns of
normal text lines, this means forcing one whole line of text to move elsewhere
in each iteration. In this case it means that once the feasible height window is
small, we decrease the height constraint by 1twip each time until we reach the
minimum feasible height. That is slow :-). I'll need to think about this...

Comment 5

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

OK, just fixing PullFrameFrom isn't enough. ReflowDirtyLines can also pull whole
lines from the next block, which could contain a float continuation. It's also
possible for the first-in-flow placeholder to be pushed to the block containing
its next in flow. And it's possible for the first-in-flow placeholder to be
pulled to an earlier block, which would leave its continuation placeholder in
the wrong place. I could just add code to check for these.

I wonder if it would simplify the handling of continuations for out-of-flows by
doing the following:
1) Get rid of BuildFloatList so that the block's float-list is maintained at all
times. Along with the nsCSSFrameConstructor work bz is doing, plus the float
reparenting I've already done, I think this is within reach now.
2) Don't have placeholders for continuations of out-of-flow frames (David, I
think you suggested this before). This would depend on 1) since BuildFloatList
depends on finding placeholders for the floats it puts into the child list.
3) Then we still have to implement checks similar to the above to make sure that
an out-of-flow frame's next in flows don't collide in the same parent and don't
skip a parent.
I'm not sure if this would lead to something simpler or not.

I'm keeping in mind that we eventually want to do continuations for absolute
frames too.

Comment 6

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

I think having some floats/absolutes not have placeholders would make some of
the code more complex, so I'm dubious this would be an overall win.

An alternative would be to put continuation placeholders in a special child
list, so we didn't have to skip around them when pulling and pushing. But that's
not appealing either for similar reasons, they would need special handling.

And I think the hard case is going to be when a first-in-flow overflow frame
moves into, or away from, the block containing its next in flow, and none of my
suggestions can make that go away.

Comment 7

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

I filed bug 264839 to deal with the problem I mentioned in the last paragraph of
comment #4, with a patch. That fix was not very hard. I need to think more about
the handling of continuations. I will probably write an essay in the wiki about
our continuation model, that should clarify my thinking.

Comment 8

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

The testcase https://bugzilla.mozilla.org/attachment.cgi?id=161746 works for me
now using build 2005-02-02-04 on Windows XP Seamonkey trunk.

Comment 9

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

I lied; it pays to read the whole bug before pressing Commit.  Comment 2 es muy
importante.  Lo siento.

Comment 10

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

The easy way to fix this is probably to restore the code that destroys all float
continuations when a float is reflowed. This will be slow but it will be simple
and correct. I should probably do that first and rearchitect float continuation
later.

Comment 11

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

I thought about this some more. The "fix" described in comment 10 is actually a
bit tricky because the child frames of the continuations have to be rescued and
dealt with.

I wrote more in http://wiki.mozilla.org/wiki/Gecko:Continuation_Model about how
a correct fix *should* work.

I think it makes sense to eliminate overflow placeholders. In
nsBlockFrame::DoReflowInilneFrames we can detect if this is the first line of a
block continuation, collect the floats with overflows from the block's
prev-in-flow, and reflow them as if we'd encountered a placeholder for them. We
should probably do this first. It may or may not fix this bug but it will make a
complete fix to the continuation model (as described in the wiki page) easier.

Comment 12

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

Attachment: fix

Here's the fix. It's rather large because it's a fundamental fix to a lot of
problems. It fixes many bugs.

Here's an overview of the patch:
-- Changed indenting of debug output to use a class so we don't get mistmatched
begin/end indenting causing misindented debug output.
-- Changed the 'overflow placeholder' list to be a frame list in the block
reflow state instead of a dynamically allocated frame property. This makes it
easier to work with when the block reflow state is available and also clarifies
that it only exists during block reflow.
-- Added a helper class to simplify access to the overflow out-of-flows list.
-- Reflow now puts overflow placeholders in a line at the *end* of the overflow
lines. This is necessary to avoid violating flow-order in the overflow line
list.
-- Made CollectFloats accumulate its results in an nsFrameList and remove
floats from whatever child list they were previously in.
-- Made ReflowDirtyLines try to pull lines from the next in flow in more
situations. In particular if float geometry changes then available space can
change to allow content to fit in the current page/column when it didn't
before.
-- When pulling frames from next-in-flow we have to identify continuation
placeholders and move them immediately to our overflow placeholder list (trying
to reflow them normally would be disastrous since we will already have their
prev-in-flows in the current block).
-- When pulling inline frames from blocks other than the current block's
overflow lines one-by-one into the current line, we need to set the current
last frame's next-sibling pointer. This fixes a huge gaping bug in the current
code.
-- UndoSplitPlaceholders needs to destroy the placeholders in all cases
(currently misses when all the placeholders are being undone). This fixes a big
leak in the current code.
-- Need to undo changes to the current block's overflow placeholder list when
we retry a reflow to get the clearance right.
-- Blocks that are impacted by floats or have positive clearance shouldn't be
treated as "top of page" and therefore forced to fit. They can profitably be
moved to the next page.
-- If a child block asks us to reflow its next in flow then we need to make
sure
we find it; it might not be on the first line of our next in flow, because of
continuation placeholders at the front of our next in flow.
-- If we try to place a float and no part of it fits anywhere, then we need to
detect that and push the float to the next block. I'm doing this by making the
line break before the float's placeholder. When the placeholder is first on the
line, we'll push the line to the next block.
-- PushLines needs to be able to handle the case where we're doing the second
pass of a shrink-wrap reflow and we need to add to existing overflow lines.
-- DrainOverflowLines has been retooled to collect all continuation
placeholders from three sources (prev-in-flow's overflow, our current lines,
our overflow lines) and figure out their disposition. Continuation placeholders
whose prev-in-flow is in the previous block get to stay in our block and are
placed at the front of the normal lines. The other continuation placeholders
already have a prev-in-flow in the current block so they get put on the
overflow placeholders list. Except that we detect when continuation
placeholders have been pushed up into this block out of a complete container;
we try to undo that if possible --- it may no longer be necessary.
-- RemoveFloat needs to be a lot more aggressive about removing the float from
wherever it may be.
-- In general I try to keep the mFloats list correct on the fly. For blocks
that aren't currently being reflowed it's essential for correctness. For blocks
that are currently being reflowed it's not necessary right now because they'll
do BuildFloatList, but it's still a good idea because we want to remove
BuildFloatList eventually.
-- DoRemoveFrame needs to handle continuation placeholders correctly; currently
it assumes when two frames have the same block parent then they're consecutive
siblings, and continuation placeholders can violate that.
-- DoRemoveFrame can optionally not delete the removed frames. This is used
when we need to extract all the placeholders for an element at once from all
block continuations and put them in a block's overflow placeholders list.
-- I've removed the code that forces a float continuation's x-offset and width
to match its prev-in-flow's. I can't think of a good reason to do this and I
can think of some good reasons not to.
-- There are some changes to enforce the rule that top margins only apply to
the first in flow and bottom margins only to the last in flow.
-- ComputeCollapsedTopMargin needs to search overflow lines and next-in-flows
to find lines which might collapse with the current top margin; those lines
might be pulled in and reflowed in the current block container.
-- I've reorganized PlaceBlock. It doesn't do anything differently AFAIK but
it's easier to read the logic. In particular the 'fits?' decision is made in
just one place with a single conditional expression.
-- nsColumnSetFrame needs to treat itself as part of the background for event
handling purposes, otherwise you can't click on floats in columns.
-- nsColumnSetFrame needs to work a little harder to guarantee that there's
always at least one column.
-- nsFrameList::AppendFrame needs to clear the next-sibling pointer of the
appendedFrame.
-- nsFrameList::RemoveFrame should not change anything if the frame is not
found in the frame list.
-- I've added nsFrameList::SortByContentOrder because it's useful to fix things
up after we've been shuffling frames around or collecting frames from different
sources.
-- I've added more assertions and debug code to help detect and track down
frame tree structure violations especially.
-- nsSpaceManager::GetBandData needs to adjust to size of the returned space to
take into account the y-offset of the band that was requested.

I'll attach a testcase with nested, breaking floats that this patch makes
work*. Because column reflow requires many reflows at different heights, this
testcase really thrashes the code (especially if you change the font size and
window size interactively). This patch also fixes the testcases here and in bug
282714 and probably many others.

* We do not compute the minimum balanced height in many cases. What's happening
is that we place the last line of a float in the current column without taking
into account the bottom margin that must be added, so after adding the bottom
margin we no longer fit in the available height. This makes column balancing
think it's found a minimum height for the columns. Fixing this issue is another
ambitious mini-project for the future. For now I'll be happy if we don't crash
and build a reasonably valid layout.

I'm not exactly sure how to dispose of this patch at this point in the
development cycle. It's uncomfortably large and complex. On the other hand,
clearly its effects are mostly limited to paginated contexts, and mostly where
floats are involved. I'm requesting review mainly just to collect feedback.

Comment 13

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

Attachment: slightly cleaned up version of the patch

This just deletes HasNextInFlowWithChildren which isn't actually used.

Comment 14

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

Attachment: testcase

nasty testcase

Comment 15

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

It appears to fix bug 248075 too.

Comment 16

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

Hmm. Thanks to Martijn, this fix is starting to look a bit more important/useful.

Comment 17

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

This fixes the current testcase in bug 262403 (the infamouse "IGN crasher"). It
does not however fix print preview of IGN itself. Still...

Comment 18

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

Attachment: updated to trunk

Updated to trunk. I ran the visual regression tests with this patch and it
looks good. Of course they don't use pagination but at least we're not horking
galley layout in a major way. I'm advocating that we land this.

Comment 19

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

(In reply to comment #12)
> -- nsFrameList::AppendFrame needs to clear the next-sibling pointer of the
> appendedFrame.

Shouldn't it assert that the next-sibling pointer is null, lest the caller leak
frames?

Comment 20

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

Comment on attachment 178156 [details] [diff] [review]
updated to trunk

>+static int PR_CALLBACK CompareByContentOrder(const void* aF1, const void* aF2,
...
>+  return 0;

Shouldn't this assert that the frames are equal before returning zero?	Or
perhaps check that earlier if it might actually happen?

Comment 21

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

(In reply to comment #12)
> -- DoRemoveFrame needs to handle continuation placeholders correctly; currently
> it assumes when two frames have the same block parent then they're consecutive
> siblings, and continuation placeholders can violate that.

Could you explain how we store continuation placeholders in a little more
detail?  I'm having trouble understanding what this means.

Comment 22

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

(In reply to comment #19)
> (In reply to comment #12)
> > -- nsFrameList::AppendFrame needs to clear the next-sibling pointer of the
> > appendedFrame.
> 
> Shouldn't it assert that the next-sibling pointer is null, lest the caller leak
> frames?

Okay, I'll do it that way.

(In reply to comment #20)
> (From update of attachment 178156 [details] [diff] [review] [edit])
> >+static int PR_CALLBACK CompareByContentOrder(const void* aF1, const void* aF2,
> ...
> >+  return 0;
> 
> Shouldn't this assert that the frames are equal before returning zero?
> Or perhaps check that earlier if it might actually happen?

Yeah I should check that early, otherwise I'll return -1 when I should return 0.
It is possible for two different frames to get here... E.g. a scrollframe and
its child. That means this function isn't transitive in that case. So I'll
assert we never reach there.

> Could you explain how we store continuation placeholders in a little more
> detail?  I'm having trouble understanding what this means.

Continuation placeholders can be stored:
-- in the first line(s) of mLines (max 1 placeholder per element)
-- in the overflow lines (anywhere in the line list, any number of placeholders
per element, must be in flow order though)
-- in the overflow placeholders list (any number of placheolders per element,
must be in flow order)

In particular you can have situations like
mLines:
  <line: placeholder 2 for float F>
  <line>
  <line>
  <line>

overflow lines:
  <line: placeholder 3 for float F>
  <line>
  <line: placeholder 4 for float F>

That's why DoRemoveFrame has to be flexible about deleting placeholders.

Comment 23

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

Comment on attachment 178156 [details] [diff] [review]
updated to trunk

>Index: layout/generic/nsFrame.cpp
>+          placeholder->SetOutOfFlowFrame(nsnull);
>+          shell->FrameManager()->UnregisterPlaceholderFrame(placeholder);

Don't actually do things inside |#ifdef DEBUG|, just make assertions about it.


With that and the comments above, r+sr=dbaron.	(You can probably guess I
didn't look too closely.)

Comment 24

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

checked in.

I added some debug code to nsPlaceholderFrame.cpp and made changes to
nsBlockFrame::UndoSplitPlaceholders, nsBlockReflowState::AddFloat and
nsBlockReflowState::PlaceBelowCurrentLineFloats to avoid some issues with
cleaning up placeholders and floats and removing them from the placeholder map.
You might want to look at those.

Comment 25

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

Could you either ifdef all of DumpLine and ifdef its callers or make it a MACRO?

Also, I just |#ifdef DEBUG_roc|d a printf that you checked in (quite noisy!).

Comment 26

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

(In reply to comment #25)
> Could you either ifdef all of DumpLine and ifdef its callers or make it a MACRO?

Yeah, I could. Is this because you want it clearly marked as DEBUG code?

> Also, I just |#ifdef DEBUG_roc|d a printf that you checked in (quite noisy!).

Oops. I'll take it out.

Comment 27

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

I tried both https://bugzilla.mozilla.org/attachment.cgi?id=161746 and
https://bugzilla.mozilla.org/attachment.cgi?id=178094, and no longer crash using
build 2005-03-23-05, Windows XP Seamonkey trunk.

Verified FIXED

Comment 28

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

This appears to have fixed bug 265867, bug 265899, bug 265973, bug 265999, and
bug 285727

This has probably fixed bug 266360 but I am waiting on a confirmation since the
crash condition has morphed some since initial creation.

Comment 29

[Christian :Biesinger (don't email me, ping me on IRC)]

fwiw this introduced a warning on windows:
c:/builds/tinderbox/MozillaTrunk/WINNT_5.0_Clobber/mozilla/layout/generic/nsBlockFrame.h(628)
: warning C4099: 'nsAutoOOFFrameList' : type name first seen using 'struct' now
seen using 'class'