Closed Bug 1356601 Opened 7 years ago Closed 7 years ago

Crash with ::first-line, CSS variables and CSS animations

Categories

(Core :: CSS Parsing and Computation, defect)

42 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr45 --- unaffected
firefox52 --- wontfix
firefox-esr52 54+ fixed
firefox53 - wontfix
firefox54 + fixed
firefox55 + fixed

People

(Reporter: Oriol, Assigned: heycam)

References

Details

(5 keywords)

Crash Data

Attachments

(2 files)

Attached file testcase.htm
Open the attached testcase.
Firefox crashes.

Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d3228c82badd&tochange=33dc8a83cfc0

Possibly the same as bug 1324313.
Keywords: crash, testcase
From that range, maybe bug 1181011? Not sure if this needs to be s-s or not. Can you please take a look, Cam?
Flags: needinfo?(cam)
I didn't make the bug public because any page can trigger this crash and it affects released versions.
But if s-s is only for exploitable vulnerabilities, it doesn't seem to be the case. I couldn't find any document explaining when a bug should be private and when not.
Assignee: nobody → cam
Status: NEW → ASSIGNED
Flags: needinfo?(cam)
With a debug build, we bump into the assertion in nsRuleNode::ComputeVariablesData.  The stack trace at that point is:

#0  0x00007f71f982ac33 in nsRuleNode::ComputeVariablesData (
    this=0x7f71c68a1220, aStartStruct=0x0, aRuleData=0x7ffed79767c0, 
    aContext=0x7f71c689a8f0, aHighestNode=0x7f71c68a1220, 
    aRuleDetail=nsRuleNode::eRulePartialMixed, aConditions=...)
    at /z/moz/a/layout/style/nsRuleNode.cpp:10195
#1  0x00007f71f980ee71 in nsRuleNode::WalkRuleTree (this=0x7f71c68a1220, 
    aSID=eStyleStruct_Variables, aContext=0x7f71c689a8f0)
    at /z/moz/a/layout/style/nsRuleNode.cpp:2663
#2  0x00007f71f97f4086 in nsRuleNode::GetStyleVariables<true> (
    this=0x7f71c68a1220, aContext=0x7f71c689a8f0, 
    aContextStyleBits=@0x7f71c689a980: 1924178903040)
    at /z/moz/a/obj/layout/style/nsStyleStructList.h:79
#3  0x00007f71f97ec228 in nsStyleContext::DoGetStyleVariables<true> (
    this=0x7f71c689a8f0) at /z/moz/a/obj/layout/style/nsStyleStructList.h:79
#4  0x00007f71f97e7de0 in nsStyleContext::StyleVariables (this=0x7f71c689a8f0)
    at /z/moz/a/obj/layout/style/nsStyleStructList.h:79
#5  0x00007f71f980e420 in nsRuleNode::ResolveVariableReferences (
    aSID=eStyleStruct_Display, aRuleData=0x7ffed7976e30, 
    aContext=0x7f71c689a8f0) at /z/moz/a/layout/style/nsRuleNode.cpp:2420
#6  0x00007f71f980e999 in nsRuleNode::WalkRuleTree (this=0x7f71c68a1220, 
    aSID=eStyleStruct_Display, aContext=0x7f71c689a8f0)
    at /z/moz/a/layout/style/nsRuleNode.cpp:2547
#7  0x00007f71f6f482a8 in nsRuleNode::GetStyleDisplay<true> (
    this=0x7f71c68a1220, aContext=0x7f71c689a8f0)
    at /z/moz/a/obj/dist/include/nsStyleStructList.h:98
#8  0x00007f71f6f4376d in nsStyleContext::DoGetStyleDisplay<true> (
    this=0x7f71c689a8f0) at /z/moz/a/obj/dist/include/nsStyleStructList.h:98
#9  0x00007f71f6f3eda0 in nsStyleContext::StyleDisplay (this=0x7f71c689a8f0)
    at /z/moz/a/obj/dist/include/nsStyleStructList.h:98
#10 0x00007f71f9830f8f in nsStyleContext::SetStyleBits (this=0x7f71c689a8f0)
    at /z/moz/a/layout/style/nsStyleContext.cpp:729
#11 0x00007f71f982d048 in nsStyleContext::FinishConstruction (
    this=0x7f71c689a8f0, aSkipParentDisplayBasedStyleFixup=false)
    at /z/moz/a/layout/style/nsStyleContext.cpp:171
#12 0x00007f71f982cec1 in nsStyleContext::nsStyleContext (this=0x7f71c689a8f0, 
    aParent=0x7f71c6899880, aPseudoTag=0x0, 
    aPseudoType=mozilla::CSSPseudoElementType::NotPseudo, aRuleNode=..., 
    aSkipParentDisplayBasedStyleFixup=false)
    at /z/moz/a/layout/style/nsStyleContext.cpp:129
#13 0x00007f71f983275f in NS_NewStyleContext (aParentContext=0x7f71c6899880, 
    aPseudoTag=0x0, aPseudoType=mozilla::CSSPseudoElementType::NotPseudo, 
    aRuleNode=0x7f71c68a1220, aSkipParentDisplayBasedStyleFixup=false)
    at /z/moz/a/layout/style/nsStyleContext.cpp:1383
#14 0x00007f71f9838811 in nsStyleSet::GetContext (this=0x7f71db991c60, 
    aParentContext=0x7f71c6899880, aRuleNode=0x7f71c68a1220, 
    aVisitedRuleNode=0x0, aPseudoTag=0x0, 
    aPseudoType=mozilla::CSSPseudoElementType::NotPseudo, 
    aElementForAnimation=0x0, aFlags=0)
    at /z/moz/a/layout/style/nsStyleSet.cpp:939
#15 0x00007f71f983ad7d in nsStyleSet::ResolveStyleByAddingRules (
    this=0x7f71db991c60, aBaseContext=0x7f71c6899a48, aRules=...)
    at /z/moz/a/layout/style/nsStyleSet.cpp:1464
#16 0x00007f71f7283f95 in mozilla::dom::CreateStyleContextForAnimationValue (
    aProperty=eCSSProperty_left, aValue=..., aBaseStyleContext=0x7f71c6899a48)
    at /z/moz/a/dom/animation/KeyframeEffectReadOnly.cpp:1687
#17 0x00007f71f72841f5 in mozilla::dom::KeyframeEffectReadOnly::CalculateCumulativeChangeHint (this=0x7f71dae60670, aStyleContext=0x7f71c6899a48)
    at /z/moz/a/dom/animation/KeyframeEffectReadOnly.cpp:1717
#18 0x00007f71f7293b9f in mozilla::dom::KeyframeEffectReadOnly::DoUpdateProperties<nsStyleContext*>(nsStyleContext*&&) (this=0x7f71dae60670, 
    aStyle=<unknown type in /z/moz/a/obj/dist/bin/libxul.so, CU 0x732ea84, DIE 0x7479867>) at /z/moz/a/dom/animation/KeyframeEffectReadOnly.cpp:370
#19 0x00007f71f727fddf in mozilla::dom::KeyframeEffectReadOnly::UpdateProperties (this=0x7f71dae60670, aStyleContext=0x7f71c6899a48)
    at /z/moz/a/dom/animation/KeyframeEffectReadOnly.cpp:305
#20 0x00007f71f72937bd in mozilla::dom::KeyframeEffectReadOnly::DoSetKeyframes<nsStyleContext*>(nsTArray<mozilla::Keyframe>&&, nsStyleContext*&&) (
    this=0x7f71dae60670, 
    aKeyframes=<unknown type in /z/moz/a/obj/dist/bin/libxul.so, CU 0x732ea84, DIE 0x7479695>, 
    aStyle=<unknown type in /z/moz/a/obj/dist/bin/libxul.so, CU 0x732ea84, DIE 0x747969a>) at /z/moz/a/dom/animation/KeyframeEffectReadOnly.cpp:235
#21 0x00007f71f727fa09 in mozilla::dom::KeyframeEffectReadOnly::SetKeyframes(nsTArray<mozilla::Keyframe>&&, nsStyleContext*) (this=0x7f71dae60670, 
    aKeyframes=<unknown type in /z/moz/a/obj/dist/bin/libxul.so, CU 0x732ea84, DIE 0x746cd22>, aStyleContext=0x7f71c6899a48)
    at /z/moz/a/dom/animation/KeyframeEffectReadOnly.cpp:195
#22 0x00007f71f974a899 in GeckoCSSAnimationBuilder::SetKeyframes(mozilla::dom::KeyframeEffectReadOnly&, nsTArray<mozilla::Keyframe>&&) (this=0x7ffed79779b0, 
    aEffect=..., 
    aKeyframes=<unknown type in /z/moz/a/obj/dist/bin/libxul.so, CU 0x13a0e563, DIE 0x13b5755d>) at /z/moz/a/layout/style/nsAnimationManager.cpp:457
#23 0x00007f71f97431c4 in BuildAnimation<GeckoCSSAnimationBuilder> (
    aPresContext=0x7f71cbc42800, aTarget=..., aSrc=..., aBuilder=..., 
    aCollection=0x0) at /z/moz/a/layout/style/nsAnimationManager.cpp:596
#24 0x00007f71f9742638 in BuildAnimations<GeckoCSSAnimationBuilder> (
    aPresContext=0x7f71cbc42800, aTarget=..., aStyleAnimations=..., 
    aStyleAnimationNameCount=1, aBuilder=..., aCollection=0x0)
    at /z/moz/a/layout/style/nsAnimationManager.cpp:1018
#25 0x00007f71f974fe24 in nsAnimationManager::DoUpdateAnimations<GeckoCSSAnimationBuilder> (this=0x7f71d1efffb0, aTarget=..., aStyleDisplay=..., aBuilder=...)
    at /z/moz/a/layout/style/nsAnimationManager.cpp:1106
#26 0x00007f71f973ecef in nsAnimationManager::UpdateAnimations (
    this=0x7f71d1efffb0, aStyleContext=0x7f71c6899a48, aElement=0x7f71cf0e1e90)
    at /z/moz/a/layout/style/nsAnimationManager.cpp:1048
#27 0x00007f71f9838a74 in nsStyleSet::GetContext (this=0x7f71db991c60, 
    aParentContext=0x7f71c6899880, aRuleNode=0x7f71c6899238, 
    aVisitedRuleNode=0x0, aPseudoTag=0x0, 
    aPseudoType=mozilla::CSSPseudoElementType::NotPseudo, 
    aElementForAnimation=0x7f71cf0e1e90, aFlags=4)
    at /z/moz/a/layout/style/nsStyleSet.cpp:971
#28 0x00007f71f983e4a5 in nsStyleSet::ReparentStyleContext (
    this=0x7f71db991c60, aStyleContext=0x7f71c6899298, 
    aNewParentContext=0x7f71c6899880, aElement=0x7f71cf0e1e90)
    at /z/moz/a/layout/style/nsStyleSet.cpp:2454
#29 0x00007f71f98c6ff3 in mozilla::GeckoRestyleManager::ReparentStyleContext (
    this=0x7f71cfecd6e0, aFrame=0x7f71c6899728)
    at /z/moz/a/layout/base/GeckoRestyleManager.cpp:963
#30 0x00007f71f996f5d2 in mozilla::RestyleManager::ReparentStyleContext (
    this=0x7f71cfecd6e0, aFrame=0x7f71c6899728)
    at /z/moz/a/obj/dist/include/mozilla/RestyleManagerInlines.h:79
#31 0x00007f71f992b647 in ReparentFrame (aRestyleManager=0x7f71cfecd6e0, 
    aNewParentFrame=0x7f71c6899928, aFrame=0x7f71c6899728)
    at /z/moz/a/layout/base/nsCSSFrameConstructor.cpp:471
#32 0x00007f71f992b6d8 in ReparentFrames (aFrameConstructor=0x7f71d1d87ae0, 
    aNewParentFrame=0x7f71c6899928, aFrameList=...)
    at /z/moz/a/layout/base/nsCSSFrameConstructor.cpp:481
#33 0x00007f71f9948eb1 in nsCSSFrameConstructor::WrapFramesInFirstLineFrame (
    this=0x7f71d1d87ae0, aState=..., aBlockContent=0x7f71c8d54f70, 
    aBlockFrame=0x7f71c6899018, aLineFrame=0x7f71c6899928, aFrameItems=...)
    at /z/moz/a/layout/base/nsCSSFrameConstructor.cpp:11329
...

So we are during frame construction, shuffling frames around to handle the ::first-line.  In frame #29, we are computing a new style context for the nsInlineFrame which has just been moved under the nsFirstLineFrame.  Because it is under the nsFirstLineFrame, it now inherits the --bar from it.  Down in frame #16, we call ResolveStyleByAddingRules with the |from| keyframe rule added for the 'left' property, as part of CalculateCumulativeChangeHint.  In frame #10 we call StyleDisplay() on the new style context (to check if we're display:none).  That ends up calling StyleVariables(), as part of ResolveVariableReferences, and because of http://searchfox.org/mozilla-central/rev/4bd7a206dea5382c97a8a0c30beef668cc449f5b/layout/style/nsRuleNode.cpp#2576-2578 we decide we can't actually just use the inherited Variables struct (which is what would normally happen, if there weren't an AnimValuesStyleRule to call aRuleData->mConditions.SetUncacheable()) and so we compute a new one.  The assertion in ComputeVariablesData thinks we'll only ever get called if we had some variable declarations specified on the rules we matched, so it looks like the assertion condition is not thinking about this case where we inherit all variable declarations but still need to compute a new struct.

If we skip that assertion, we get to another assertion, and the null pointer dereference crash in release builds, in CSSVariableResolver::Resolve, where it expects aSpecified to be non-null.

Rather than trying to make ComputeVariablesData and the CSSVariableResolver handle the case where there are no extra variable declarations to add, I think we should try harder to re-use the inherited Variables struct.  Variables structs are never cached in the rule tree anyway, because they effectively always rely on inherited data, so we can skip the bit of WalkRuleTree that forces detail = eRulePartialMixed if we're computing a Variables struct.
This might be bug 1324313 and bug 1345579.
(In reply to Oriol from comment #2)
> I didn't make the bug public because any page can trigger this crash and it
> affects released versions.
> But if s-s is only for exploitable vulnerabilities, it doesn't seem to be
> the case. I couldn't find any document explaining when a bug should be
> private and when not.

I can't see anything on https://wiki.mozilla.org/Security_Severity_Ratings that talks about crashes due to null pointer dereferences, but we don't tend to mark those bugs as s-s.  (And bug 1324313 and bug 1345579, which I'm reasonably sure are the same as this bug, are open.)  So I think we can open this up.  (Though I don't have access to do that.)
Attached patch patchSplinter Review
Attachment #8859040 - Flags: review?(dbaron)
Comment on attachment 8859040 [details] [diff] [review]
patch

>+    // We don't need to do this for Variables structs since we know those are
>+    // never cached in the rule tree, and it avoids wasteful computaiton of a

"computation"

r=dbaron with that
Attachment #8859040 - Flags: review?(dbaron) → review+
[Tracking Requested - why for this release]: Easily triggerable content crash.
[Tracking Requested - why for this release]:
Group: core-security
Flags: in-testsuite+
See Also: → 1324313
Tracking 54/55+ per Comment 8.
Looks like a very low volume crash even on release. Too late to fix this in 53.
https://hg.mozilla.org/mozilla-central/rev/08a6b726c361
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment on attachment 8859040 [details] [diff] [review]
patch

Approval Request Comment
[Feature/Bug causing the regression]: bug 1181011
[User impact if declined]: content crash with certain uses of CSS Variables, CSS Animations, and ::first-line / ::first-letter, in combination
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: no, it just merged to m-c
[Needs manual test from QE? If yes, steps to reproduce]: verification just involves loading the test case from this bug and seeing if we crash
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: pretty localized change
[String changes made/needed]: none
Attachment #8859040 - Flags: approval-mozilla-beta?
Bug 1345579 was introduced by bug 1166500 and fixed by bug 1339332. Is it really the same?

And bug 1324313 was originally reported for fennec only. It's strange that the 99% of the reports with this signature are in fennec, I suspect there is something else in there.
The test case in bug 1345579 is pretty similar to the on in this bug.  I suspect it was just that we were running the invisible animation after bug 1166500 and stopped running it again after bug 1339332, avoiding the crash.  For bug 1324313, my guess is that there are mobile-specific styles that tripped this.  Let's see what happens with the crash volumes; we can re-open one or both if my guesses are wrong.
Comment on attachment 8859040 [details] [diff] [review]
patch

Fix a crash. Beta54+. Should be in 54 beta 1.
Attachment #8859040 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Cameron McCormack (:heycam) from comment #16)
> [Is this code covered by automated tests?]: yes
> [Has the fix been verified in Nightly?]: no, it just merged to m-c
> [Needs manual test from QE? If yes, steps to reproduce]: verification just
> involves loading the test case from this bug and seeing if we crash

Since the fix pushed for this bug comes with an automated test, I think manual testing would be redundant.

Cameron, if you think Release QA should in fact be looking at this, please flip the qe-verify flag or ni? me directly.
Flags: qe-verify-
Agreed, no need for manual testing.  Thanks.
Comment on attachment 8859040 [details] [diff] [review]
patch

See comment 16.
Attachment #8859040 - Flags: approval-mozilla-esr52?
Comment on attachment 8859040 [details] [diff] [review]
patch

I don't see any instances of this crash on esr channel in the past 3 months. I don't feel the need to port this to ESR52.2. Please let me know if I misread the crash data.
Attachment #8859040 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52-
Flags: needinfo?(rkothari)
Comment on attachment 8859040 [details] [diff] [review]
patch

esr52.2+ based on comment 25
Flags: needinfo?(rkothari)
Attachment #8859040 - Flags: approval-mozilla-esr52- → approval-mozilla-esr52+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: