If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Crash [@ nsLineLayout::ReflowFrame(nsIFrame *,unsigned int &,nsHTMLReflowMetrics *,bool &) ] | ASSERTION: unexpected flow: 'mFrames.ContainsFrame(nextInFlow)' | ASSERTION: StealFrame: can't find aChild: 'removed' | ASSERTION: StealFrame failure: 'NS_SUCCE

VERIFIED FIXED in Firefox 17

Status

()

Core
Layout
--
critical
VERIFIED FIXED
5 years ago
3 years ago

People

(Reporter: bc, Assigned: mats)

Tracking

(Blocks: 1 bug, 6 keywords)

14 Branch
mozilla18
assertion, crash, highrisk, regression, sec-critical, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox14- wontfix, firefox15- wontfix, firefox16- wontfix, firefox17+ verified, firefox18+ verified, firefox-esr10 unaffected, firefox-esr17 verified)

Details

(Whiteboard: [adv-track-main17+], crash signature, URL)

Attachments

(5 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 633722 [details]
crash report

1. http://www2.emersonprocess.com/en-US/brands/EIM-Actuators/Products/Electric/Pages/M2CP_Electric_Actuator.aspx

###!!! ASSERTION: unexpected flow: 'mFrames.ContainsFrame(nextInFlow)', file c:/work/mozilla/builds/nightly/mozilla/layout/generic/nsInlineFrame.cpp, line 500

###!!! ASSERTION: StealFrame: can't find aChild: 'removed', file c:/work/mozilla/builds/nightly/mozilla/layout/generic/nsContainerFrame.cpp, line 1210

###!!! ASSERTION: StealFrame failure: 'NS_SUCCEEDED(rv)', file c:/work/mozilla/builds/nightly/mozilla/layout/generic/nsContainerFrame.cpp, line 1339

Crash @ nsLineLayout::ReflowFrame(nsIFrame *,unsigned int &,nsHTMLReflowMetrics *,bool &)

This appears to be Beta/14, Aurora/15, and Nightly/16 though I am getting inconsistent results sometimes on different platforms.

Updated

5 years ago
Crash Signature: nsLineLayout::ReflowFrame(nsIFrame *,unsigned int &,nsHTMLReflowMetrics *,bool &) nsLineLayout::ReflowFrame) → [@ nsLineLayout::ReflowFrame(nsIFrame *,unsigned int &,nsHTMLReflowMetrics *,bool &) ] [@ nsLineLayout::ReflowFrame ]

Comment 1

5 years ago
Mozregression range:

m-c
good=2012-03-20
bad=2012-03-21
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b972b89518c3&tochange=4bdae514b9be

m-i
good=2012-03-19
bad=2012-03-20
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=6dd5b6014511&tochange=46c5015550af

Comment 2

5 years ago
It might be a regression from bug 537624.
Version: Trunk → 14 Branch
(Assignee)

Comment 3

5 years ago
Created attachment 633852 [details]
frame dump

I think the assumption here
http://hg.mozilla.org/mozilla-central/annotate/9602a9e99045/layout/generic/nsInlineFrame.cpp#l490
that all our children's NIFs must be in our child list and should
be reparented is wrong.
Assignee: nobody → matspal
(Assignee)

Comment 4

5 years ago
Created attachment 633853 [details] [diff] [review]
fix

Only reparent a child's next-in-flows that has our prevInFlow as parent (we stole those from its OverflowList).

https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=09c6f9338c13
(Assignee)

Comment 5

5 years ago
The code here still looks a bit fragile.  If we leave the loop that
reparent and reflow child frames early we leave a tail with the
wrong parent.  It looks like we have something in Reflow to fix that
(assuming they got pushed to our OverflowList):
http://hg.mozilla.org/mozilla-central/annotate/9602a9e99045/layout/generic/nsInlineFrame.cpp#l364
but it only does a simple SetParent, not the ReparentFloatsForInlineChild
and ReparentStyleContext that we do in the loop.

I think it would be more robust to continue reparenting all the child
frames, but not reflow any after 'done' is set true of course.
(Assignee)

Comment 6

5 years ago
It's more complicated than that since ReflowInlineFrame also does
reparenting, and may have pushed the tail to our own OverflowList:
http://hg.mozilla.org/mozilla-central/annotate/9602a9e99045/layout/generic/nsInlineFrame.cpp#l670
(Assignee)

Updated

5 years ago
Keywords: testcase-wanted

Comment 7

5 years ago
Created attachment 633872 [details]
testcase (crashes Firefox when loaded)

The reduction was fairly straightforward. The less obvious bits:

* A slow-loading image became a dynamic change to an inline-block element's size.

* <div style="display: inline"> became <span style="unicode-bidi: -moz-isolate">

Suddenly the fuzz testcase in bug 760957 doesn't seem so weird!

Updated

5 years ago
Keywords: testcase-wanted → testcase
Group: core-security
Blocks: 765711
status-firefox-esr10: --- → unaffected
Keywords: sec-critical
Duplicate of this bug: 765711
Hi Mats, it looks like you aren't quite happy with your current fix?
tracking-firefox14: --- → +
tracking-firefox15: --- → +
tracking-firefox16: --- → +
(Assignee)

Comment 10

5 years ago
Comment on attachment 633853 [details] [diff] [review]
fix

Right.
Attachment #633853 - Attachment is obsolete: true
What should we do here?
status-firefox14: affected → wontfix
status-firefox17: --- → affected
tracking-firefox14: + → -
tracking-firefox17: --- → +
(In reply to David Bolter [:davidb] from comment #11)
> What should we do here?

We're still targeting this for FF15 if a low risk fix is found. What are the next steps here?
(Assignee)

Comment 13

5 years ago
Created attachment 654369 [details] [diff] [review]
fix v2

Comments roughly per hunk in the order they appear:

Return earlier in case of IsFrameTreeTooDeep (to avoid leaving frames
on the principal list that needs reparenting)

If we're not lazily reparenting, then reparent any frames we still
from the prev-in-flow overflow list full - i.e. SetParent + 
ReparentFloatsForInlineChild + ReparentStyleContext (if :first-line).

Reparent the next-in-flows of a child we're lazily parenting, until
we find a sibling with our next-in-flow as the parent (this is the
correct condition to test for unlike the last patch).

When lazily reparenting, if we stop reflowing children for some
reason then continue reparenting the remaining children on the
principal list...

... thus no need to that in ReflowInlineFrame.

In PullOneFrame, we only need one frame so don't pull in all frames
from the nif's overflow list since then we would need to reparent
all of them. (thus leaving the nif's principal list empty when we
return - I don't see a problem with that).


Green on Try:
https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=c6defdd9ee36
Attachment #654369 - Flags: review?(bzbarsky)
(Assignee)

Comment 14

5 years ago
Created attachment 654370 [details] [diff] [review]
fix v2, wdiff

... well, the hunks I commented is the ones in this diff actually.
(Assignee)

Comment 15

5 years ago
(In reply to Alex Keybl [:akeybl] from comment #12)
> We're still targeting this for FF15 if a low risk fix is found. What are the
> next steps here?

I think this patch is too high risk for 15.
Keywords: highrisk
Comment on attachment 654369 [details] [diff] [review]
fix v2

r=me
Attachment #654369 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 17

5 years ago
Created attachment 658670 [details]
frame tree for failed assertion

Oops, I triggered this assertion while testing this again...

It seems our stop condition - that the child's nif's parent is
our nif, does not hold because it can be further away in the
nif chain.

In this case, we have an empty nif (lime 0x7fffddb49ae0) in
between us (blue 0x7fffd70793f0) and our child's nif (red).
Attachment #654369 - Attachment is obsolete: true
Attachment #654370 - Attachment is obsolete: true
(Assignee)

Comment 18

5 years ago
So to avoid doing O(n*m) frame traversals we could put all
the (m) nifs in a temporary hashtable, but that seems a bit
expensive for the gain of lazy parenting.

Perhaps we should just not do lazy parenting in case there
are more than one nif?

Suggestions?
Find our first non-empty next-in-flow and use that for all n frames?
status-firefox15: affected → wontfix
status-firefox18: --- → affected
tracking-firefox15: + → -
tracking-firefox16: + → -
tracking-firefox18: --- → +
(Assignee)

Comment 20

5 years ago
Created attachment 659230 [details] [diff] [review]
fix, v3

Thanks roc, yes that would work, but I think I've found better way.
I collected some statistics on the frame tree when
"child->GetNextInFlow()" is non-null:

Running reftests+crashtests results in 10 hits (~0.5%), in all of them
the nif equals GetNextSibling().

Loading the Alexa top 100 sites results in zero hits (even when
resizing the window, reloading etc).

For the crash URL, the hit rate is ~3%, in 75% of those the nif
is the next-sibling.  The remaining 25% are either in the inline's nif,
or in an empty nif's nif (and in these cases GetNextSibling() is null).

So, in 100% of the tests, the nif is either the next sibling (and
we should reparent it) or the next sibling is null (and we shouldn't).
I think it's rather unlikely that we will have any other cases.

====

This patch is the same as before but with the following inter-diff:
140a141
> +        nsIFrame* nextSibling = child->GetNextSibling();
142,145c143,149
< +        if (child && child->GetParent() == GetNextContinuation() &&
< +            GetNextContinuation()) {
< +          MOZ_ASSERT(!mFrames.ContainsFrame(child));
< +          child = nullptr;
---
> +        if (NS_UNLIKELY(child)) {
> +          while (child != nextSibling && nextSibling) {
> +            nextSibling = nextSibling->GetNextSibling();
> +          }
> +          if (!nextSibling) {
> +            child = nullptr;
> +          }

So instead of testing the child's parent I'm simply iterating the
siblings to see of it can be found (for the test data above we will
never enter the loop).  This is better because it's exactly what
we want to do - if it's in mFrames it should be reparented.

https://tbpl.mozilla.org/?tree=Try&rev=7ced33ae2dd1
Attachment #659230 - Flags: review?(bzbarsky)
Comment on attachment 659230 [details] [diff] [review]
fix, v3

r=me
Attachment #659230 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 22

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0c765e8be0f
Flags: in-testsuite?
Target Milestone: --- → mozilla18
https://hg.mozilla.org/mozilla-central/rev/e0c765e8be0f
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
status-firefox18: affected → fixed

Comment 24

5 years ago
This is sec-critical and tracking-firefox17+, are we planning on landing this on beta?
Crash Signature: [@ nsLineLayout::ReflowFrame(nsIFrame *,unsigned int &,nsHTMLReflowMetrics *,bool &) ] [@ nsLineLayout::ReflowFrame ] → [@ nsLineLayout::ReflowFrame(nsIFrame*, unsigned int&, nsHTMLReflowMetrics*, bool&) ] [@ nsLineLayout::ReflowFrame ]
(Assignee)

Comment 25

5 years ago
Comment on attachment 659230 [details] [diff] [review]
fix, v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: sec-critical crash
Testing completed (on m-c, etc.): on trunk/aurora since 2012-09-07
Risk to taking this patch (and alternatives if risky): medium risk since the code is somewhat fragile.  The final patch (v3) is much better than the earlier iterations though (which comment 15 refers to as high risk).
String or UUID changes made by this patch: none
Attachment #659230 - Flags: approval-mozilla-beta?

Updated

5 years ago
No longer blocks: 760952
Duplicate of this bug: 760952
(Assignee)

Updated

5 years ago
Duplicate of this bug: 802991
Please set sec-approval? on this patch as it fits the criteria, being sec-critical and looking for uplift.
Comment on attachment 659230 [details] [diff] [review]
fix, v3

Sorry, my bad - this doesn't need sec-approval given that it's already on trunk & Aurora now.
Attachment #659230 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/f7d473f3a067
status-firefox17: affected → fixed
status-firefox-esr17: --- → fixed
Whiteboard: [adv-track-main17+]
Keywords: verifyme
Confirmed crash on build 2012-6-15 nightly
Verified fixed on build 2012-11-13, 17.0b6
Win7 and Mac OS 10.8
status-firefox17: fixed → verified
Thanks Matt, can you also verify in the latest 17esr nightly? Aurora 18.0a2 too if you have time.
Verified fixed on build 2012-11-14, 17.0 ESR
Verified fixed on build 2012-11-14, 18.0a2 Aurora
status-firefox18: fixed → verified
status-firefox-esr17: fixed → verified
(Assignee)

Updated

5 years ago
Depends on: 837007
Thanks Matt.

Changing status-firefox16:affected to wontfix since we won't be issuing patches for that version.
Status: RESOLVED → VERIFIED
status-firefox16: affected → wontfix
Keywords: verifyme
Group: core-security
(Assignee)

Comment 35

4 years ago
Crash test:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c559cbeba13
Flags: in-testsuite? → in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/4c559cbeba13
(Assignee)

Updated

3 years ago
Blocks: 5588
You need to log in before you can comment on or make changes to this bug.