765409 - 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

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Bob Clary [:bc] (inactive)]

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

Comment 1

[Loic]

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

[Scoobidiver (away)]

It might be a regression from bug 537624.

Comment 3

[Mats Palmgren (inactive)]

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

Comment 4

[Mats Palmgren (inactive)]

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

Comment 5

[Mats Palmgren (inactive)]

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.

Comment 6

[Mats Palmgren (inactive)]

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

Comment 7

[Jesse Ruderman]

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

Comment 9

[David Bolter [:davidb] (NeedInfo me for attention)]

Hi Mats, it looks like you aren't quite happy with your current fix?

Comment 10

[Mats Palmgren (inactive)]

Comment on attachment 633853 [details] [diff] [review]
fix

Right.

Comment 11

[David Bolter [:davidb] (NeedInfo me for attention)]

What should we do here?

Comment 12

[Alex Keybl [:akeybl]]

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

Comment 13

[Mats Palmgren (inactive)]

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

Comment 14

[Mats Palmgren (inactive)]

Attachment: fix v2, wdiff

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

Comment 15

[Mats Palmgren (inactive)]

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

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 654369 [details] [diff] [review]
fix v2

r=me

Comment 17

[Mats Palmgren (inactive)]

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

Comment 18

[Mats Palmgren (inactive)]

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?

Comment 19

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

Find our first non-empty next-in-flow and use that for all n frames?

Comment 20

[Mats Palmgren (inactive)]

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

Comment 21

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 22

[Mats Palmgren (inactive)]

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

Comment 23

[Ryan VanderMeulen [:RyanVM]]

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

Comment 24

[Robert Kaiser]

This is sec-critical and tracking-firefox17+, are we planning on landing this on beta?

Comment 25

[Mats Palmgren (inactive)]

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

Comment 28

[Lukas Blakk [:lsblakk] use ?needinfo]

Please set sec-approval? on this patch as it fits the criteria, being sec-critical and looking for uplift.

Comment 29

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f7d473f3a067

Comment 31

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed crash on build 2012-6-15 nightly
Verified fixed on build 2012-11-13, 17.0b6
Win7 and Mac OS 10.8

Comment 32

[u279076]

Thanks Matt, can you also verify in the latest 17esr nightly? Aurora 18.0a2 too if you have time.

Comment 33

[Matt Wobensmith [:mwobensmith][:matt:]]

Verified fixed on build 2012-11-14, 17.0 ESR
Verified fixed on build 2012-11-14, 18.0a2 Aurora

Comment 34

[u279076]

Thanks Matt.

Changing status-firefox16:affected to wontfix since we won't be issuing patches for that version.

Comment 35

[Mats Palmgren (inactive)]

Crash test:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c559cbeba13

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4c559cbeba13