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

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

(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

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).

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.

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?

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

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

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 on attachment 179909 [details] [diff] [review] fix fixes crashes when using absolute frames in columns (and also possibly printing)

Comment on attachment 179909 [details] [diff] [review] fix a=chofmann

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

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.

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

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

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

Linux current trunk too

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.

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.

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

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

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

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

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

Crashing earlier can be a good thing ;)

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

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?)

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.

checked in on trunk.

Comment on attachment 218763 [details] [diff] [review] better fix Seeking branch approvals

Comment on attachment 218763 [details] [diff] [review] better fix approved for 1.8.0 branch, a=dveditz for drivers

Fixed on branches.

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.