288357 - Evil columns testcase causes crash (-moz-column-count:2;position:absolute) [@ DoDeletingFrameSubtree] [@ nsCSSFrameConstructor::ReinsertContent]

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Martijn Wargers (dead)]

The testcase that I'll attach causes a crash in the latest trunk nightly build,
when clicking on the button in the testcase.
This isn't likely to be a regression, since columns used to not work inside
absolutely positioned frames, if I remember correctly.

I get always the same backtrace. Talkback ID: TB4720575H

DoDeletingFrameSubtree 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp,
line 9496]
DoDeletingFrameSubtree 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp,
line 9519]
DeletingFrameSubtree 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp,
line 9565]
nsCSSFrameConstructor::ContentRemoved 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp,
line 9789]
nsCSSFrameConstructor::RecreateFramesForContent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp,
line 11686]
nsCSSFrameConstructor::RestyleElement 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp,
line 10281]
nsCSSFrameConstructor::ProcessOneRestyle 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp,
line 13640]
nsCSSFrameConstructor::ProcessPendingRestyles 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp,
line 13682]
nsCSSFrameConstructor::RestyleEvent::HandleEvent 
[c:/builds/tinderbox/Fx-Trunk/WINNT_5.0_Depend/mozilla/layout/base/nsCSSFrameConstructor.cpp,
line 13734]
0x778b0c24

Comment 1

[Martijn Wargers (dead)]

Attachment: Testcase

Comment 2

[Giles Robertson]

Confirm here.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050330
Firefox/1.0+

Comment 3

[Giles Robertson]

(In reply to comment #2)
> Confirm here.
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050330
> Firefox/1.0+

TB4720874Z

Comment 4

[Boris Zbarsky [:bzbarsky]]

roc, this looks like all you.  Some nice reflow assertions about continiation
frames being confused before we crash (final crash is because we have a
placeholder which points to null as its out of flow).

Comment 5

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

Attachment: fix

The problem is that we need to treat absolute frames like floats, and reparent
the out-of-flow when its placeholder gets dragged across a
previnflow/nextinflow boundary (and also put the frames in the right child
list).

This patch also includes some #ifdef DEBUG stuff that David asked me to put in
ages ago but I forgot.

Comment 6

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

So is this changing our behavior?  How are absolutely positioned elements
positioned when they're descendants of next-in-flows?  (In both pagination and
columns.)  In the columns case, what happens if the containing block is inside
the column?  outside the column?  Is this correct according to the spec, and how
will it change if/when bug 154892 lands?

Comment 7

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

(In reply to comment #6)
> So is this changing our behavior?

Yes, for testcases where a placeholder for an absolute frame ends up in a
next-in-flow). Currently, in contexts with dynamic reflow, these will almost
certainly crash. In contexts with static reflow the absolute frame will remain
positioned relative to the first page even though its placeholder moves.

> How are absolutely positioned elements
> positioned when they're descendants of next-in-flows?

Relative to the origin of the column (or page) that they belong to.

> In the columns case, what happens if the containing block is inside
> the column? outside the column?

Absolute frames are always children of their containing block's frame. This bug
only applies to absolute frames whose containing block is exactly a column (or
page). Otherwise nothing has changed.

> Is this correct according to the spec

No. As I understand it, absolute children of an element with columns should
always be positioned relative to the origin of the element's box (our
nsColumnSetFrame). I'm not sure what's supposed to happen for absolute elements
in paginated contexts, but I think positioning them relative to the first page's
origin is reasonable. But it's very difficult to implement the columns behaviour
without refactoring absolute positioning so any frame type can act as an
absolute container (something we do need to do!). The goal here is to fix the
crasher rather than make everything work per spec.

> and how will it change if/when bug 154892 lands?

Good question, and I hadn't thought enough about it. Here are my handwavy
ideas... First we refactor absolute positioning so any frame type can have
absolute children. Then, for frames that support continuations, we find a parent
for each absolute child by treating the parent in-flow frames as a single
vertical strip and choosing the in-flow frame where the absolute child's 'top'
coordinate falls. Then we reflow the absolute child with the appropriate height
constraint. If it's not complete, then the absolute child's continuation goes to
the parent's next-in-flow. If we run out of parent next-in-flows then we have to
create more ... or we could do what blocks do and try to jump up to some other
container that's still going ... this is hard.

That would mean that the absolute frame will not necessarily be in the same
frame as its placeholder so all this code would have to change. Given that,
perhaps we shouldn't land this after all, if this crash isn't worth temporarily
fixing. Your call.

Comment 8

[Giles Robertson]

> I'm not sure what's supposed to happen for absolute elements
> in paginated contexts, but I think positioning them relative to the first page's
> origin is reasonable.

(This is mildly ot for this bug)

If you position objects relative to the current page's origin, then it's much
easier to deal with several (physical) pages all with the same layout but
different data.

Comment 9

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

I think the best way to produce layouts like that is to create a series of
absolute containers (e.g., DIVs with 'position:relative') and use page-break
properties to put page breaks between them.

Comment 10

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

Comment on attachment 179909 [details] [diff] [review]
fix

fixes crashes when using absolute frames in columns (and also possibly
printing)

Comment 11

[chris hofmann]

Comment on attachment 179909 [details] [diff] [review]
fix

a=chofmann

Comment 12

[Martijn Wargers (dead)]

Did you forget about this, Robert? Your fix has r+/sr+ and approval1.8b2.

Comment 13

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

Yeah. I'm not sure if I should check it in. It's a bit risky, fixes a bug that
won't come up much at all, and isn't really the right way to do absolute
positioning in columns.

Comment 14

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

Martijn, do you still crash?  I just tested this with build 20050820 on Windows
XP; no crash.

Comment 15

[Martijn Wargers (dead)]

Yes, works for me also with a 2005-08-19 trunk build.
Marking WFM.

Comment 16

[Martijn Wargers (dead)]

In 2005-10-20 trunk build, I crash again and now directly when clicking on the
button.

Comment 17

[Felix Miata]

Linux current trunk too

Comment 18

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

Attachment: better fix

This patch is a better fix. It forces the abs-pos container to always be the first in flow, making block and inline containers consistent. The block code currently doesn't move abs-pos children across block continuations so they just stay there in the first-in-flow and everyone's happy. The patch doesn't crash on this testcase or the testcase in bug 331446.

Comment 19

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

This patch is branch-safe IMHO and should land on both branches.

Note that on trunk, because of frame display lists, abs-pos children whose placeholders aren't under the first-in-flow may not be displayed, as described in the comment. I will fix that on trunk with a separate patch which will have to refactor the way MarkOutOfFlowChild works, slightly.

Comment 20

[Jesse Ruderman]

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20060509 Minefield/3.0a1

[@ nsCSSFrameConstructor::ReinsertContent]

Comment 21

[Jesse Ruderman]

roc, can you post an updated patch?  I think your patch from bug 335140 conflicts with this one.

Comment 22

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

Just ignore the nsFrame.cpp change. The nsCSSFrameConstructor.cpp change should apply.

Comment 23

[Jesse Ruderman]

This patch makes the testcase in bug 337419 crash earlier (on load instead of on reload).

Comment 24

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

okay, but I still think this is the right thing.

Comment 25

[Jesse Ruderman]

Crashing earlier can be a good thing ;)

Comment 26

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

David, I need review here if this is to make 1.8.0.5. Thanks

Comment 27

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

Comment on attachment 218763 [details] [diff] [review]
better fix

r+sr=dbaron, although this doesn't seem like it would still work if we start moving absolutely positioned elements to anything other than the first page when printing.  (In fact, could it break some printing case that currently works?)

Comment 28

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

Currently, abs-pos elements are always relative to the first page, because we create all frames for the document initially on the first page, and when placeholders get moved to subsequent pages during reflow, the abs-pos frames are not moved. A DOM or style change could create an abs-pos frame relative to another page, but we don't allow those during printing. So this patch shouldn't change anything. Well, it actually will change one thing: the code I'm removing currently breaks positioning of abs-pos children of relative-pos, multi-frame inlines in paginated mode, and I'll be fixing that.

How we should ultimately handle abs-pos frames when printing is an open question (that really needs to be addressed by the CSS WG). I think that the only reasonable and compatible solution is to use the vertical offset to decide which page an abs-pos frame starts on, ignoring which page the placeholder happens to be on, so something like this patch is a starting point for that, anyway.

Comment 29

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

checked in on trunk.

Comment 30

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

Comment on attachment 218763 [details] [diff] [review]
better fix

Seeking branch approvals

Comment 31

[Daniel Veditz [:dveditz]]

Comment on attachment 218763 [details] [diff] [review]
better fix

approved for 1.8.0 branch, a=dveditz for drivers

Comment 32

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

Fixed on branches.

Comment 33

[Adam Guthrie]

Verified FIXED using Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.5) Gecko/20060622 Firefox/1.5.0.5.

No crash at all using a build with the patch in it.