Closed Bug 837007 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Layout, defect, critical)

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: mats)

References

(Blocks 1 open bug)

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.
https://hg.mozilla.org/mozilla-central/rev/bf8667d7004d
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Whiteboard: [fuzzblocker] → [fuzzblocker][adv-main21+]
Landed a crashtest:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e553c9a0cc1d
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.