414 bytes, application/xhtml+xml
37.08 KB, text/html
8.99 KB, patch
|Details | Diff | Splinter Review|
5.45 KB, patch
|Details | Diff | Splinter Review|
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]
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.
(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
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
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)
OS: Mac OS X → All
Hardware: PC → All
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.
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.