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)
Tracking
()
RESOLVED
FIXED
mozilla55
People
(Reporter: Oriol, Assigned: heycam)
References
Details
(5 keywords)
Crash Data
Attachments
(2 files)
248 bytes,
text/html
|
Details | |
3.13 KB,
patch
|
dbaron
:
review+
gchang
:
approval-mozilla-beta+
jcristau
:
approval-mozilla-esr52+
|
Details | Diff | Splinter Review |
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•7 years ago
|
Comment 1•7 years ago
|
||
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•7 years 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•7 years ago
|
Assignee: nobody → cam
Status: NEW → ASSIGNED
Flags: needinfo?(cam)
Assignee | ||
Comment 3•7 years 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•7 years ago
|
||
This might be bug 1324313 and bug 1345579.
Assignee | ||
Comment 5•7 years 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•7 years ago
|
||
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•7 years 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•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/08a6b726c361a372b822627679b838ec3a7c888d
Comment 10•7 years ago
|
||
[Tracking Requested - why for this release]:
Comment 12•7 years ago
|
||
Looks like a very low volume crash even on release. Too late to fix this in 53.
Comment 13•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/08a6b726c361
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Assignee | ||
Comment 16•7 years 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•7 years 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•7 years 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 19•7 years ago
|
||
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•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/6ed18ad42fc2
Comment 21•7 years ago
|
||
(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•7 years ago
|
||
Agreed, no need for manual testing. Thanks.
Comment 23•7 years ago
|
||
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-
Updated•7 years ago
|
Flags: needinfo?(rkothari)
Comment 26•7 years ago
|
||
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+
Updated•7 years ago
|
Updated•7 years ago
|
Comment 27•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-esr52/rev/8191e403fedf
You need to log in
before you can comment on or make changes to this bug.
Description
•