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)
Core
Layout
Tracking
()
People
(Reporter: jruderman, Assigned: MatsPalmgren_bugz)
References
Details
(5 keywords, Whiteboard: [fuzzblocker][adv-main21+])
Attachments
(4 files, 1 obsolete file)
###!!! 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]
| Reporter | ||
Comment 1•12 years ago
|
||
| Assignee | ||
Comment 2•12 years ago
|
||
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 | ||
Comment 3•12 years ago
|
||
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.
| Assignee | ||
Updated•12 years ago
|
Blocks: 765409
status-b2g18:
--- → affected
status-b2g18-v1.0.0:
--- → affected
status-firefox18:
--- → affected
status-firefox19:
--- → affected
status-firefox20:
--- → affected
status-firefox21:
--- → affected
status-firefox-esr17:
--- → affected
tracking-b2g18:
--- → ?
tracking-firefox19:
--- → ?
tracking-firefox20:
--- → ?
tracking-firefox21:
--- → ?
Keywords: csec-nullptr,
sec-other
| Assignee | ||
Comment 4•12 years ago
|
||
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
| Assignee | ||
Comment 5•12 years ago
|
||
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)
| Reporter | ||
Updated•12 years ago
|
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?
Comment 7•12 years ago
|
||
(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•12 years ago
|
||
(also any context around why this should track at all would be great)
| Assignee | ||
Comment 9•12 years ago
|
||
(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.
| Assignee | ||
Comment 10•12 years ago
|
||
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)
Comment 11•12 years ago
|
||
No need to track but will look at aurora approval nomination and take a fix if low risk.
Attachment #710744 -
Flags: review?(roc) → review+
| Assignee | ||
Comment 12•12 years ago
|
||
Comment 13•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Updated•12 years ago
|
Whiteboard: [fuzzblocker] → [fuzzblocker][adv-main21+]
| Assignee | ||
Comment 14•10 years ago
|
||
Landed a crashtest:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e553c9a0cc1d
Group: core-security
Flags: in-testsuite? → in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•