Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

VERIFIED FIXED

Status

()

Core
Layout
--
critical
VERIFIED FIXED
13 years ago
6 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: roc)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
x86
All
crash, fixed1.8.1, testcase, verified1.8.0.5
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1 +
blocking1.8.0.5 +

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(3 attachments)

(Reporter)

Description

13 years ago
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
(Reporter)

Comment 1

13 years ago
Created attachment 179103 [details]
Testcase

Comment 2

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

Comment 3

13 years ago
(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

13 years ago
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).
Summary: Evil testcase causes crash [@ DoDeletingFrameSubtree] (-moz-column-count:2;position:absolute) → Evil testcase causes crash [@ DoDeletingFrameSubtree] (-moz-column-count:2;position:absolute) [columns]
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.
Assignee: nobody → roc
Status: NEW → ASSIGNED
Attachment #179909 - Flags: superreview?(dbaron)
Attachment #179909 - Flags: review?(dbaron)
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.

Comment 8

12 years ago
> 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.
Attachment #179909 - Flags: superreview?(dbaron)
Attachment #179909 - Flags: superreview+
Attachment #179909 - Flags: review?(dbaron)
Attachment #179909 - Flags: review+
Comment on attachment 179909 [details] [diff] [review]
fix

fixes crashes when using absolute frames in columns (and also possibly
printing)
Attachment #179909 - Flags: approval1.8b2?

Comment 11

12 years ago
Comment on attachment 179909 [details] [diff] [review]
fix

a=chofmann
Attachment #179909 - Flags: approval1.8b2? → approval1.8b2+
(Reporter)

Comment 12

12 years ago
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.
(Reporter)

Comment 15

12 years ago
Yes, works for me also with a 2005-08-19 trunk build.
Marking WFM.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → WORKSFORME
(Reporter)

Comment 16

12 years ago
In 2005-10-20 trunk build, I crash again and now directly when clicking on the
button.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---

Comment 17

12 years ago
Linux current trunk too
OS: Windows 2000 → All
Blocks: 321106
Blocks: 331446
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.
Attachment #218763 - Flags: superreview?(dbaron)
Attachment #218763 - Flags: review?(dbaron)
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.
Depends on: 335140

Comment 20

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

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

Comment 21

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

Updated

11 years ago
Flags: blocking1.9a1?
Flags: blocking1.8.1?
Flags: blocking1.8.0.5?
Just ignore the nsFrame.cpp change. The nsCSSFrameConstructor.cpp change should apply.

Comment 23

11 years ago
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.

Comment 25

11 years ago
Crashing earlier can be a good thing ;)
Flags: blocking1.8.0.5? → blocking1.8.0.5+
David, I need review here if this is to make 1.8.0.5. Thanks

Updated

11 years ago
Flags: blocking1.8.1? → blocking1.8.1+
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?)
Attachment #218763 - Flags: superreview?(dbaron)
Attachment #218763 - Flags: superreview+
Attachment #218763 - Flags: review?(dbaron)
Attachment #218763 - Flags: review+
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.
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago11 years ago
Resolution: --- → FIXED
Comment on attachment 218763 [details] [diff] [review]
better fix

Seeking branch approvals
Attachment #218763 - Flags: approval1.8.1?
Attachment #218763 - Flags: approval1.8.0.6?
Attachment #218763 - Flags: approval1.8.0.6? → approval1.8.0.5?
Comment on attachment 218763 [details] [diff] [review]
better fix

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #218763 - Flags: approval1.8.0.5? → approval1.8.0.5+

Updated

11 years ago
Attachment #218763 - Flags: approval1.8.1? → approval1.8.1+
Fixed on branches.
Keywords: fixed1.8.0.5, fixed1.8.1

Comment 33

11 years ago
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.
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.0.5 → verified1.8.0.5
(Reporter)

Updated

11 years ago
Flags: blocking1.9a1?
Crash Signature: [@ DoDeletingFrameSubtree] [@ nsCSSFrameConstructor::ReinsertContent]
You need to log in before you can comment on or make changes to this bug.