844529 - Crash [@ nsBox::DoesNeedRecalc] with flex on <audio>

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

###!!! ASSERTION: A box layout method was called but InitBoxMetrics was never called: 'metrics', file layout/generic/nsFrame.cpp, line 8087

Crash [@ nsBox::DoesNeedRecalc]

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Jesse Ruderman]

bp-9ffa3134-f538-4083-9ce9-134df2130223

Comment 3

[Scoobidiver (away)]

On Windows: bp-ab85a331-23dc-4fa8-90d7-364242130224.

Comment 4

[Daniel Holbert [:dholbert]]

Seems related to bug 812822, but unfortunately not fixed by that bug's patch, since it crashes up-to-date nightlies.

Likely a similar underlying issue, though -- we might need to just take advantage of that bug's code in an additional spot.

Comment 5

[Daniel Holbert [:dholbert]]

Yeah, so this is basically the same as bug 812822, in that we're computing the style for helper-children of a parent with a fixed frame type. (nsVideoFrame in this case) that has "display:flex".  We want to have aSkipFlexItemStyleFixup set to true, but it's not, for this code-path.

When we get the relevant ApplyStyleFixups() call that does undesirable flex-item style fixup, we've got this backtrace:

#0  nsStyleContext::ApplyStyleFixups (this=0x7f9f858352d8, aSkipFlexItemStyleFixup=false) at layout/style/nsStyleContext.cpp:398
#1  0x00007f9fa855a7ba in nsStyleContext::nsStyleContext (this=0x7f9f858352d8, aParent=0x7f9f85797b48, aPseudoTag=0x0,aPseudoType=nsCSSPseudoElements::ePseudo_NotPseudoElement, aRuleNode=0x7f9f85835290, aSkipFlexItemStyleFixup=false) at layout/style/nsStyleContext.cpp:74
#2  0x00007f9fa855d9a6 in NS_NewStyleContext (aParentContext=0x7f9f85797b48, aPseudoTag=0x0,aPseudoType=nsCSSPseudoElements::ePseudo_NotPseudoElement, aRuleNode=0x7f9f85835290, aSkipFlexItemStyleFixup=false) at layout/style/nsStyleContext.cpp:726
#3  0x00007f9fa85621b5 in nsStyleSet::GetContext (this=0x7f9f8582a2e0, aParentContext=0x7f9f85797b48, aRuleNode=0x7f9f85835290, aVisitedRuleNode=0x0,aPseudoTag=0x0,aPseudoType=nsCSSPseudoElements::ePseudo_NotPseudoElement, aElementForAnimation=0x7f9f851853a0, aFlags=4) at layout/style/nsStyleSet.cpp:770
#4  0x00007f9fa8563d21 in nsStyleSet::ResolveStyleFor (this=0x7f9f8582a2e0, aElement=0x7f9f851853a0, aParentContext=0x7f9f85797b48, aTreeMatchContext=...) at layout/style/nsStyleSet.cpp:1184
#5  0x00007f9fa826b7c0 in nsCSSFrameConstructor::ResolveStyleContext (this=0x7f9f84b7bc00, aParentStyleContext=0x7f9f85797b48, aContent=0x7f9f851853a0, aState=0x7fff7f697480) at layout/base/nsCSSFrameConstructor.cpp:4615
#6  0x00007f9fa826b739 in nsCSSFrameConstructor::ResolveStyleContext (this=0x7f9f84b7bc00, aParentFrame=0x7f9f858351b0, aContent=0x7f9f851853a0, aState=0x7fff7f697480) at layout/base/nsCSSFrameConstructor.cpp:4600
#7  0x00007f9fa82796c7 in nsCSSFrameConstructor::ProcessChildren (this=0x7f9f84b7bc00, aState=..., aContent=0x7f9f876fb000, aStyleContext=0x7f9f85797b48, aFrame=0x7f9f858351b0, aCanHaveGeneratedContent=true, aFrameItems=..., aAllowBlockStyles=false, aPendingBinding=0x0,aPossiblyLeafFrame=0x7f9f858351b0) at layout/base/nsCSSFrameConstructor.cpp:10093
#8  0x00007f9fa8269c27 in nsCSSFrameConstructor::ConstructFrameFromItemInternal (this=0x7f9f84b7bc00, aItem=..., aState=..., aParentFrame=0x7f9f85797be8, aFrameItems=...) at layout/base/nsCSSFrameConstructor.cpp:3768

The frame we're passing to 'ProcessChildren' at level #7 there (0x7f9f858351b0) is the nsVideoFrame, and everything below that is us generate a nsStyleContext for one of its children.

Comment 6

[Daniel Holbert [:dholbert]]

(patch coming up)

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: fix v1, w/ test

This makes us use an AutoFlexItemStyleFixupSkipper in one other spot where we resolve style for anonymous content.

I included extra lines of context, to make it clear that we're in a "deal w/ anonymous content" chunk of code.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 718769 [details] [diff] [review]
fix v1, w/ test

Ah, right, I'd forgotten that this was the "normal" anonymous content path... r=me

Comment 9

[Daniel Holbert [:dholbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/63a6466d8c46

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/63a6466d8c46