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]

This fixes the crash, but...

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.

(In reply to comment #3) Documentation like this is rather misleading then... http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1432&root=/cvsroot&mark=9476-9479#9454 "the following property holds" ... http://wiki.mozilla.org/Gecko:Continuation_Model#Relationship_of_continuations_to_frame_tree_structure

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

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)

You should get roc to review this; he understands our continuation model invariants better than I do.

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.

Is the attached patch good or bad?

I'm not sure.

Mats, can you address comment #9?

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.

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.

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.