"ASSERTION: Null out-of-flow for placeholder?" and crash with -moz-column, float, {ib}

RESOLVED WORKSFORME

Status

()

P3
critical
RESOLVED WORKSFORME
11 years ago
10 years ago

People

(Reporter: jruderman, Assigned: mats)

Tracking

(Blocks: 2 bugs, {assertion, crash, testcase})

Trunk
assertion, crash, testcase
Points:
---
Dependency tree / graph
Bug Flags:
wanted-next +
blocking1.9 -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

11 years ago
Created attachment 289626 [details]
testcase (crashes Firefox when loaded)

Loading the testcase triggers three assertions and a crash.

###!!! ASSERTION: Null out-of-flow for placeholder?: 'outOfFlow', file /Users/jruderman/trunk/mozilla/layout/base/../generic/nsPlaceholderFrame.h, line 175

###!!! ASSERTION: null ptr: 'nsnull != aFrame', file /Users/jruderman/trunk/mozilla/layout/generic/nsFrameList.cpp, line 121

###!!! ASSERTION: Must have the out of flow in some child list: 'found', file /Users/jruderman/trunk/mozilla/layout/generic/nsBlockFrame.cpp, line 4277

Null deref crash at one of the following:
* [@ ReparentFrame] 
       calling [@ nsIFrame::GetParent]
* [@ nsLayoutUtils::GetFloatFromPlaceholder] 
       calling [@ nsIFrame::GetStyleDisplay]
Flags: blocking1.9?
(Assignee)

Comment 1

11 years ago
Created attachment 289775 [details] [diff] [review]
wip

This fixes the crash, but...
(Assignee)

Comment 2

11 years ago
Created attachment 289777 [details]
Frame dumps

We're removing the green Inline(span)@0x1ad6920 frame, which has a child
placeholder frame with continuations, however the span itself and its
parent (yellow Block(div)) does not have continuations.
Which means we're leaving a bunch of placeholder continuations dangling...

This frame tree looks weird to me. I thought it was a frame tree invariant
that continuations of in-flow descendants were descendants of the frame
itself or its continuation.  That is, there should be continuations for
the yellow DIV and green SPAN between the cyan DIV and red placeholder,
so that when the green SPAN is removed, its continuations are removed and
thereby also all (in-flow) descendant continuations.

If this is not the case (anymore?) then we need to make the
frame destruction code aware of this.
Float placeholder continuations have always been children of the next-in-flow block. I thought the frame destruction code was aware of that.
Blocks: 401734
(Assignee)

Comment 5

11 years ago
Created attachment 289864 [details] [diff] [review]
wip

(This isn't quite right, we shouldn't traverse out-of-flow continuations
 and the "!aRemovedFrame->GetNextContinuation()" optimization is probably
 not correct either. (eg. if the green Inline(span) had a continuation))
Attachment #289775 - Attachment is obsolete: true
(Assignee)

Comment 6

11 years ago
Created attachment 289865 [details] [diff] [review]
Patch rev. 1

This should do it, but only if the following assumption is correct:
All in-flow descendants (including placeholders) of an
*out-of-flow continuation* must be a continuation frame and the
start of that continuation chain must be a descendant of the first
frame in the out-of-flow continuation chain.
(so we can still avoid traversing continuations in DoDeletingFrameSubtree)
Assignee: nobody → mats.palmgren
Attachment #289864 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #289865 - Flags: review?(bzbarsky)
(Assignee)

Updated

11 years ago
Flags: in-testsuite?
OS: Mac OS X → All
Hardware: PC → All
(Assignee)

Comment 7

11 years ago
Created attachment 289870 [details] [diff] [review]
mochitest (based on testcase in this bug and bug 401734)
You should get roc to review this; he understands our continuation model invariants better than I do.
(Assignee)

Updated

11 years ago
Attachment #289865 - Flags: superreview?(roc)
Attachment #289865 - Flags: review?(roc)
Attachment #289865 - Flags: review?(bzbarsky)

Updated

11 years ago
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
I fixed the wiki docs. Sorry about that. We really need to get rid of float placeholder continuations :-(.

> All in-flow descendants (including placeholders) of an
> *out-of-flow continuation* must be a continuation frame

I'm not sure what you mean by this. It's certainly possible for a float continuation to have a child that is a first-in-flow. For example there might be a child element that just starts on the second page.
Flags: wanted-next+
Flags: blocking1.9-

Updated

11 years ago
Flags: tracking1.9+
Is the attached patch good or bad?
Testcase in comment #0 WFM on Mac trunk Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1a2pre) Gecko/20080818150509 Minefield/3.1a2pre with mallocscribble.
(Reporter)

Comment 14

10 years ago
Mats / roc, is this patch still relevant after the patch for bug 413048 (which disabled float breaking in columns)?
probably not. We'd like to reenable float breaking in columns eventually, but probably only after some major architectural changes that would obsolete this patch. By the way, it would be good if the WFM float/column bugs could have crashtests checked in so we don't lose them when we come to reenable float breaking in columns.
(Reporter)

Comment 16

10 years ago
I'm planning to check in crashtests as soon as I get a chance to check in :)  I'm going to skip the security-sensitive bugs for now, though.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → WORKSFORME
(Reporter)

Updated

10 years ago
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.