Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in Firefox 54

Status

()

Core
CSS Parsing and Computation
--
critical
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: Oriol, Assigned: heycam)

Tracking

(5 keywords)

42 Branch
mozilla55
assertion, crash, csectype-nullptr, regression, testcase
Points:
---
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox52 wontfix, firefox53- wontfix, firefox54+ fixed, firefox55+ fixed, firefox-esr45 unaffected, firefox-esr5254+ fixed)

Details

(crash signature)

Attachments

(2 attachments)

(Reporter)

Description

3 months ago
Created attachment 8858344 [details]
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.
(Reporter)

Updated

3 months ago
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)
(Reporter)

Comment 2

3 months ago
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)

Updated

3 months ago
Assignee: nobody → cam
Status: NEW → ASSIGNED
Flags: needinfo?(cam)
(Assignee)

Comment 3

3 months ago
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.
(Assignee)

Comment 4

3 months ago
This might be bug 1324313 and bug 1345579.
(Assignee)

Comment 5

3 months ago
(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.)
Keywords: assertion, csectype-nullptr
(Assignee)

Comment 6

3 months ago
Created attachment 8859040 [details] [diff] [review]
patch
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+
(Assignee)

Comment 8

3 months ago
[Tracking Requested - why for this release]: Easily triggerable content crash.
status-firefox52: --- → affected
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox55: --- → affected
status-firefox-esr45: --- → unaffected
status-firefox-esr52: --- → affected
tracking-firefox52: --- → ?
tracking-firefox53: --- → ?
tracking-firefox54: --- → ?
tracking-firefox55: --- → ?
Keywords: regression
(Assignee)

Comment 9

3 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/08a6b726c361a372b822627679b838ec3a7c888d
[Tracking Requested - why for this release]:
Group: core-security
status-firefox52: affected → wontfix
tracking-firefox52: ? → ---
Flags: in-testsuite+
See Also: → bug 1324313
Tracking 54/55+ per Comment 8.
tracking-firefox54: ? → +
tracking-firefox55: ? → +
Looks like a very low volume crash even on release. Too late to fix this in 53.
status-firefox53: affected → wontfix
status-firefox54: affected → fix-optional
tracking-firefox53: ? → -

Comment 13

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/08a6b726c361
Status: ASSIGNED → RESOLVED
Last Resolved: 3 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Updated

3 months ago
Duplicate of this bug: 1324313
(Assignee)

Updated

3 months ago
Duplicate of this bug: 1345579
(Assignee)

Comment 16

3 months ago
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?
(Reporter)

Comment 17

3 months ago
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.
(Assignee)

Comment 18

3 months ago
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+

Comment 20

3 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/6ed18ad42fc2
status-firefox54: fix-optional → fixed
(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-
(Assignee)

Comment 22

3 months ago
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 24

2 months ago
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-

Updated

2 months ago
status-firefox-esr52: affected → wontfix
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+
status-firefox-esr52: wontfix → affected
tracking-firefox-esr52: --- → ?
tracking-firefox-esr52: ? → 54+

Comment 27

2 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-esr52/rev/8191e403fedf
status-firefox-esr52: affected → fixed
You need to log in before you can comment on or make changes to this bug.