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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: 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]

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Mats Palmgren (inactive)]

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

Comment 3

[Mats Palmgren (inactive)]

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.

Comment 4

[Mats Palmgren (inactive)]

Attachment: 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

Comment 5

[Mats Palmgren (inactive)]

Attachment: fix

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.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 7

[Alex Keybl [:akeybl]]

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

Comment 8

[Alex Keybl [:akeybl]]

(also any context around why this should track at all would be great)

Comment 9

[Mats Palmgren (inactive)]

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

Comment 10

[Mats Palmgren (inactive)]

Attachment: fix, v2

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

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

No need to track but will look at aurora approval nomination and take a fix if low risk.

Comment 12

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf8667d7004d

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bf8667d7004d

Comment 14

[Mats Palmgren (inactive)]

Landed a crashtest:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e553c9a0cc1d

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e553c9a0cc1d