Closed Bug 837007 Opened 12 years ago Closed 12 years ago

"ASSERTION: Out of flow frame doesn't have the expected parent" and crash with -moz-column, float

Categories

(Core :: Layout, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla21
Tracking Status
firefox18 --- wontfix
firefox19 --- wontfix
firefox20 - affected
firefox21 - fixed
firefox-esr17 --- wontfix
b2g18 --- wontfix
b2g18-v1.0.0 --- wontfix

People

(Reporter: jruderman, Assigned: MatsPalmgren_bugz)

References

Details

(5 keywords, Whiteboard: [fuzzblocker][adv-main21+])

Attachments

(4 files, 1 obsolete file)

Attached file testcase
###!!! ASSERTION: Out of flow frame doesn't have the expected parent: 'outOfFlowFrame->GetParent() == this', file layout/generic/nsBlockFrame.cpp, line 6898 ###!!! ASSERTION: Broken frame linkage: 'prevSibling && prevSibling->GetNextSibling() == aFrame', file layout/generic/nsFrameList.cpp, line 95 ###!!! ASSERTION: Creating a circular frame list, this is very bad.: 'this != aNextSibling', file layout/generic/nsIFrame.h, line 1133 [@ nsFrameList::RemoveFrame]
Attached file stack
Regression window: 2012-09-07-03-05-54 -- 2012-09-08-03-05-56 http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=36427d4b2cf6&tochange=1d4fc0c60063 Prime suspect: bug 765409
Assignee: nobody → matspal
Keywords: regression
OS: Mac OS X → All
Hardware: x86_64 → All
I should have used the mozilla-central tree for the link: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=36427d4b2cf6&tochange=1d4fc0c60063 Still the same prime suspect though.
Attached file frame dumps + stacks
Step 1: Reflow the inline 0x7fffe1f70450 (lime) => pushing the (placeholder) child 0x7fffe1f70800 (blue) Step 2: Reflow the next-in-flow 0x7fffe1f71f58 it picks up the pushed frames, puts it in mFrames, assigns irs.mSetParentPointer = true. We reflow (and reparent) the first child (0x7fffe1f71ee0) but not the placeholder, which is pushed again. Step 3: The containing block 0x7fffe1fd3868 (magenta) decides to push the line (cyan) containing the inline with the placeholder to its next-in-flow block 0x7fffd07411b8 (yellow). PushLines() collects the floats for those lines and reparents them and insert them into the next-in-flow. Step 4: Reflow the inline 0x7fffe1f71f58 It detects the overflow list and decides to reparent the frames: http://hg.mozilla.org/mozilla-central/annotate/be76182b91a6/layout/generic/nsInlineFrame.cpp#l358 since the 'lineContainer' (yellow) has a prev-in-flow we also reparent any associated floats (ReparentFloatsForInlineChild) but it expects any float to still be on the prev-in-flow (but PushLines() already moved them) so we crash here: http://hg.mozilla.org/mozilla-central/annotate/be76182b91a6/layout/generic/nsBlockFrame.cpp#l6893
Attached patch fix (obsolete) — Splinter Review
Make ReflowFrames reparent all child frames when lazy reparenting, also those on the overflow list. I'm tempted to remove this whole lazy reparenting optimization altogether, but let's consider that in a follow-up bug. Green on Try: https://tbpl.mozilla.org/?tree=Try&rev=59f54a3a3c76 Also tested bug 765409.
Attachment #709741 - Flags: review?(roc)
Whiteboard: [fuzzblocker]
Doesn't this kind of break lazy reparenting? The issue is if we have something like <p><em><b>Hello</b> <b>Hello</b> <b>Hello</b> <b>Hello</b> ... <b>Hello</b> Say there are N <b> elements and each one ends up on its own line. Then without lazy reparenting, when we reflow the <em> frame at line K, we will have to reparent N - K <b> frames to the current frame. This makes reflowing this paragraph O(N^2) in the number of lines. That's really bad. With your patch here, it seems to me the first line will reflow OK, then on the second line we'll reparent *everything* to the second line. Or am I missing something?
(In reply to Mats Palmgren [:mats] from comment #3) > I should have used the mozilla-central tree for the link: > http://hg.mozilla.org/mozilla-central/ > pushloghtml?fromchange=36427d4b2cf6&tochange=1d4fc0c60063 > Still the same prime suspect though. To set expectations here, it's unlikely that we'll take a fix for this in our final beta. If there's any reason to have more urgency around this fix, please do provide context.
(also any context around why this should track at all would be great)
(In reply to Alex Keybl [:akeybl] from comment #8) > (also any context around why this should track at all would be great) The crashes I've seen we're accessing a null-pointer. If there's other variations it's very likely to be covered by frame poisoning so I guess this fix can ride the trains, possibly adding it to Aurora if the risk is low.
Attached patch fix, v2Splinter Review
Make CollectFloats ignore floats that isn't a child of 'this'. This patch looks more invasive than it really is - CollectFloats already does this for FRAME_IS_PUSHED_FLOATs, I'm just extending it to all floats and consolidating the two branches to share some code. https://tbpl.mozilla.org/?tree=Try&rev=8755ec711c0c
Attachment #709741 - Attachment is obsolete: true
Attachment #709741 - Flags: review?(roc)
Attachment #710744 - Flags: review?(roc)
Blocks: 838688
No need to track but will look at aurora approval nomination and take a fix if low risk.
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Whiteboard: [fuzzblocker] → [fuzzblocker][adv-main21+]
Group: core-security
Flags: in-testsuite? → in-testsuite+
Blocks: 5588
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: