Last Comment Bug 288357 - Evil columns testcase causes crash (-moz-column-count:2;position:absolute) [@ DoDeletingFrameSubtree] [@ nsCSSFrameConstructor::ReinsertContent]
: Evil columns testcase causes crash (-moz-column-count:2;position:absolute) [@...
Status: VERIFIED FIXED
: crash, fixed1.8.1, testcase, verified1.8.0.5
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: x86 All
: -- critical (vote)
: ---
Assigned To: Robert O'Callahan (:roc) (email my personal email if necessary)
:
: Jet Villegas (:jet)
Mentors:
Depends on: 335140
Blocks: 321106 331446
  Show dependency treegraph
 
Reported: 2005-03-30 14:20 PST by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2011-06-09 14:58 PDT (History)
10 users (show)
darin.moz: blocking1.8.1+
dveditz: blocking1.8.0.5+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (806 bytes, text/html)
2005-03-30 14:21 PST, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
fix (20.69 KB, patch)
2005-04-06 19:36 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
dbaron: review+
dbaron: superreview+
chofmann: approval1.8b2+
Details | Diff | Splinter Review
better fix (4.95 KB, patch)
2006-04-17 20:16 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
dbaron: review+
dbaron: superreview+
dveditz: approval1.8.0.5+
mconnor: approval1.8.1+
Details | Diff | Splinter Review

Description Martijn Wargers [:mwargers] (not working for Mozilla) 2005-03-30 14:20:36 PST
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 [:mwargers] (not working for Mozilla) 2005-03-30 14:21:32 PST
Created attachment 179103 [details]
Testcase
Comment 2 Giles Robertson 2005-03-30 14:38:47 PST
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 2005-03-30 15:02:38 PST
(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 [:bz] (still a bit busy) 2005-03-31 22:56:12 PST
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) 2005-04-06 19:36:57 PDT
Created attachment 179909 [details] [diff] [review]
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-8 2005-04-27 16:37:37 PDT
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) 2005-04-27 18:36:10 PDT
(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 2005-04-28 00:11:51 PDT
> 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) 2005-04-28 01:14:12 PDT
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) 2005-05-02 16:47:43 PDT
Comment on attachment 179909 [details] [diff] [review]
fix

fixes crashes when using absolute frames in columns (and also possibly
printing)
Comment 11 chris hofmann 2005-05-02 16:56:09 PDT
Comment on attachment 179909 [details] [diff] [review]
fix

a=chofmann
Comment 12 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-05-20 02:45:35 PDT
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) 2005-05-20 13:32:49 PDT
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] 2005-08-20 12:15:02 PDT
Martijn, do you still crash?  I just tested this with build 20050820 on Windows
XP; no crash.
Comment 15 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-08-21 12:39:11 PDT
Yes, works for me also with a 2005-08-19 trunk build.
Marking WFM.
Comment 16 Martijn Wargers [:mwargers] (not working for Mozilla) 2005-10-21 04:03:58 PDT
In 2005-10-20 trunk build, I crash again and now directly when clicking on the
button.
Comment 17 Felix Miata 2005-11-22 08:09:29 PST
Linux current trunk too
Comment 18 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-04-17 20:16:14 PDT
Created attachment 218763 [details] [diff] [review]
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) 2006-04-17 20:19:46 PDT
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 2006-05-10 05:50:47 PDT
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 2006-05-10 05:57:00 PDT
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) 2006-05-10 15:13:34 PDT
Just ignore the nsFrame.cpp change. The nsCSSFrameConstructor.cpp change should apply.
Comment 23 Jesse Ruderman 2006-05-11 12:58:19 PDT
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) 2006-05-11 13:06:57 PDT
okay, but I still think this is the right thing.
Comment 25 Jesse Ruderman 2006-05-11 13:16:11 PDT
Crashing earlier can be a good thing ;)
Comment 26 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-06-15 22:02:12 PDT
David, I need review here if this is to make 1.8.0.5. Thanks
Comment 27 David Baron :dbaron: ⌚️UTC-8 2006-06-19 21:12:37 PDT
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) 2006-06-20 13:52:02 PDT
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) 2006-06-20 14:10:46 PDT
checked in on trunk.
Comment 30 Robert O'Callahan (:roc) (email my personal email if necessary) 2006-06-20 14:12:00 PDT
Comment on attachment 218763 [details] [diff] [review]
better fix

Seeking branch approvals
Comment 31 Daniel Veditz [:dveditz] 2006-06-21 14:25:26 PDT
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) 2006-06-22 15:56:27 PDT
Fixed on branches.
Comment 33 Adam Guthrie 2006-06-22 17:21:50 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.