Bug 1181011 - Don't use cached rule node structs for animations within pseudo-elements. r=dbaron, a=abillings
authorCameron McCormack <cam@mcc.id.au>
Mon, 27 Jul 2015 16:43:44 +1000
changeset 281708 13c68fd25c0cc39644ec83f7d39c667cf9125bc0
parent 281707 b4d36f3d36203bf5258ab796b5bca409633aa833
child 281709 c9613dd57ab0c1933603b2a88cf0aa17fef4bafd
push id4932
push userjlund@mozilla.com
push dateMon, 10 Aug 2015 18:23:06 +0000
treeherdermozilla-beta@6dd5a4f5f745 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdbaron, abillings
bugs1181011
milestone41.0a2
Bug 1181011 - Don't use cached rule node structs for animations within pseudo-elements. r=dbaron, a=abillings
layout/style/AnimationCommon.cpp
layout/style/nsRuleNode.cpp
layout/style/nsRuleNode.h
layout/style/nsStyleSet.cpp
layout/style/nsStyleStruct.h
--- a/layout/style/AnimationCommon.cpp
+++ b/layout/style/AnimationCommon.cpp
@@ -195,16 +195,17 @@ CommonAnimationManager::RulesMatching(El
 {
   MOZ_ASSERT(aData->mPresContext == mPresContext,
              "pres context mismatch");
   nsIStyleRule *rule =
     GetAnimationRule(aData->mElement,
                      nsCSSPseudoElements::ePseudo_NotPseudoElement);
   if (rule) {
     aData->mRuleWalker->Forward(rule);
+    aData->mRuleWalker->CurrentNode()->SetIsAnimationRule();
   }
 }
 
 /* virtual */ void
 CommonAnimationManager::RulesMatching(PseudoElementRuleProcessorData* aData)
 {
   MOZ_ASSERT(aData->mPresContext == mPresContext,
              "pres context mismatch");
@@ -214,16 +215,17 @@ CommonAnimationManager::RulesMatching(Ps
   }
 
   // FIXME: Do we really want to be the only thing keeping a
   // pseudo-element alive?  I *think* the non-animation restyle should
   // handle that, but should add a test.
   nsIStyleRule *rule = GetAnimationRule(aData->mElement, aData->mPseudoType);
   if (rule) {
     aData->mRuleWalker->Forward(rule);
+    aData->mRuleWalker->CurrentNode()->SetIsAnimationRule();
   }
 }
 
 /* virtual */ void
 CommonAnimationManager::RulesMatching(AnonBoxRuleProcessorData* aData)
 {
 }
 
@@ -449,16 +451,25 @@ NS_IMPL_ISUPPORTS(AnimValuesStyleRule, n
 /* virtual */ void
 AnimValuesStyleRule::MapRuleInfoInto(nsRuleData* aRuleData)
 {
   nsStyleContext *contextParent = aRuleData->mStyleContext->GetParent();
   if (contextParent && contextParent->HasPseudoElementData()) {
     // Don't apply transitions or animations to things inside of
     // pseudo-elements.
     // FIXME (Bug 522599): Add tests for this.
+
+    // Prevent structs from being cached on the rule node since we're inside
+    // a pseudo-element, as we could determine cacheability differently
+    // when walking the rule tree for a style context that is not inside
+    // a pseudo-element.  Note that nsRuleNode::GetStyle##name_ and GetStyleData
+    // will never look at cached structs when we're animating things inside
+    // a pseduo-element, so that we don't incorrectly return a struct that
+    // is only appropriate for non-pseudo-elements.
+    aRuleData->mConditions.SetUncacheable();
     return;
   }
 
   for (uint32_t i = 0, i_end = mPropertyValuePairs.Length(); i < i_end; ++i) {
     PropertyValuePair &cv = mPropertyValuePairs[i];
     if (aRuleData->mSIDs & nsCachedStyleData::GetBitForSID(
                              nsCSSProps::kSIDTable[cv.mProperty]))
     {
--- a/layout/style/nsRuleNode.cpp
+++ b/layout/style/nsRuleNode.cpp
@@ -1470,17 +1470,18 @@ nsRuleNode::nsRuleNode(nsPresContext* aC
                        nsIStyleRule* aRule, uint8_t aLevel,
                        bool aIsImportant)
   : mPresContext(aContext),
     mParent(aParent),
     mRule(aRule),
     mNextSibling(nullptr),
     mDependentBits((uint32_t(aLevel) << NS_RULE_NODE_LEVEL_SHIFT) |
                    (aIsImportant ? NS_RULE_NODE_IS_IMPORTANT : 0)),
-    mNoneBits(0),
+    mNoneBits(aParent ? aParent->mNoneBits & NS_RULE_NODE_HAS_ANIMATION_DATA :
+                        0),
     mRefCnt(0)
 {
   MOZ_ASSERT(aContext);
   MOZ_ASSERT(IsRoot() == !aRule,
              "non-root rule nodes must have a rule");
 
   mChildren.asVoid = nullptr;
   MOZ_COUNT_CTOR(nsRuleNode);
@@ -9316,19 +9317,24 @@ nsRuleNode::GetStyleData(nsStyleStructID
                          bool aComputeData)
 {
   NS_ASSERTION(IsUsedDirectly(),
                "if we ever call this on rule nodes that aren't used "
                "directly, we should adjust handling of mDependentBits "
                "in some way.");
 
   const void *data;
-  data = mStyleData.GetStyleData(aSID, aContext);
-  if (MOZ_LIKELY(data != nullptr))
-    return data; // We have a fully specified struct. Just return it.
+
+  // Never use cached data for animated style inside a pseudo-element;
+  // see comment on cacheability in AnimValuesStyleRule::MapRuleInfoInto.
+  if (!(HasAnimationData() && ParentHasPseudoElementData(aContext))) {
+    data = mStyleData.GetStyleData(aSID, aContext);
+    if (MOZ_LIKELY(data != nullptr))
+      return data; // We have a fully specified struct. Just return it.
+  }
 
   if (MOZ_UNLIKELY(!aComputeData))
     return nullptr;
 
   // Nothing is cached.  We'll have to delve further and examine our rules.
   data = WalkRuleTree(aSID, aContext);
 
   MOZ_ASSERT(data, "should have aborted on out-of-memory");
@@ -9774,8 +9780,15 @@ nsRuleNode::ComputeColor(const nsCSSValu
              "aValue shouldn't have eCSSUnit_Unset");
 
   RuleNodeCacheConditions conditions;
   bool ok = SetColor(aValue, NS_RGB(0, 0, 0), aPresContext, aStyleContext,
                      aResult, conditions);
   MOZ_ASSERT(ok || !(aPresContext && aStyleContext));
   return ok;
 }
+
+/* static */ bool
+nsRuleNode::ParentHasPseudoElementData(nsStyleContext* aContext)
+{
+  nsStyleContext* parent = aContext->GetParent();
+  return parent && parent->HasPseudoElementData();
+}
--- a/layout/style/nsRuleNode.h
+++ b/layout/style/nsRuleNode.h
@@ -838,16 +838,39 @@ public:
    * of some style context (as opposed to only being the ancestor of
    * some style context's mRuleNode)?
    */
   void SetUsedDirectly();
   bool IsUsedDirectly() const {
     return (mDependentBits & NS_RULE_NODE_USED_DIRECTLY) != 0;
   }
 
+  /**
+   * Is the mRule of this rule node an AnimValuesStyleRule?
+   */
+  void SetIsAnimationRule() {
+    MOZ_ASSERT(!HaveChildren() ||
+               (mDependentBits & NS_RULE_NODE_IS_ANIMATION_RULE),
+               "SetIsAnimationRule must only set the IS_ANIMATION_RULE bit "
+               "before the rule node has children");
+    mDependentBits |= NS_RULE_NODE_IS_ANIMATION_RULE;
+    mNoneBits |= NS_RULE_NODE_HAS_ANIMATION_DATA;
+  }
+  bool IsAnimationRule() const {
+    return (mDependentBits & NS_RULE_NODE_IS_ANIMATION_RULE) != 0;
+  }
+
+  /**
+   * Is the mRule of this rule node or any of its ancestors an
+   * AnimValuesStyleRule?
+   */
+  bool HasAnimationData() const {
+    return (mNoneBits & NS_RULE_NODE_HAS_ANIMATION_DATA) != 0;
+  }
+
   // NOTE:  Does not |AddRef|.  Null only for the root.
   nsIStyleRule* GetRule() const { return mRule; }
   // NOTE: Does not |AddRef|.  Never null.
   nsPresContext* PresContext() const { return mPresContext; }
 
   const void* GetStyleData(nsStyleStructID aSID,
                            nsStyleContext* aContext,
                            bool aComputeData);
@@ -861,19 +884,24 @@ public:
   GetStyle##name_(nsStyleContext* aContext)                                   \
   {                                                                           \
     NS_ASSERTION(IsUsedDirectly(),                                            \
                  "if we ever call this on rule nodes that aren't used "       \
                  "directly, we should adjust handling of mDependentBits "     \
                  "in some way.");                                             \
                                                                               \
     const nsStyle##name_ *data;                                               \
-    data = mStyleData.GetStyle##name_();                                      \
-    if (MOZ_LIKELY(data != nullptr))                                          \
-      return data;                                                            \
+                                                                              \
+    /* Never use cached data for animated style inside a pseudo-element; */   \
+    /* see comment on cacheability in AnimValuesStyleRule::MapRuleInfoInto */ \
+    if (!(HasAnimationData() && ParentHasPseudoElementData(aContext))) {      \
+      data = mStyleData.GetStyle##name_();                                    \
+      if (MOZ_LIKELY(data != nullptr))                                        \
+        return data;                                                          \
+    }                                                                         \
                                                                               \
     if (!aComputeData)                                                        \
       return nullptr;                                                         \
                                                                               \
     data = static_cast<const nsStyle##name_ *>                                \
              (WalkRuleTree(eStyleStruct_##name_, aContext));                  \
                                                                               \
     MOZ_ASSERT(data, "should have aborted on out-of-memory");                 \
@@ -886,19 +914,24 @@ public:
   GetStyle##name_(nsStyleContext* aContext)                                   \
   {                                                                           \
     NS_ASSERTION(IsUsedDirectly(),                                            \
                  "if we ever call this on rule nodes that aren't used "       \
                  "directly, we should adjust handling of mDependentBits "     \
                  "in some way.");                                             \
                                                                               \
     const nsStyle##name_ *data;                                               \
-    data = mStyleData.GetStyle##name_(aContext);                              \
-    if (MOZ_LIKELY(data != nullptr))                                          \
-      return data;                                                            \
+                                                                              \
+    /* Never use cached data for animated style inside a pseudo-element; */   \
+    /* see comment on cacheability in AnimValuesStyleRule::MapRuleInfoInto */ \
+    if (!(HasAnimationData() && ParentHasPseudoElementData(aContext))) {      \
+      data = mStyleData.GetStyle##name_(aContext);                            \
+      if (MOZ_LIKELY(data != nullptr))                                        \
+        return data;                                                          \
+    }                                                                         \
                                                                               \
     if (!aComputeData)                                                        \
       return nullptr;                                                         \
                                                                               \
     data = static_cast<const nsStyle##name_ *>                                \
              (WalkRuleTree(eStyleStruct_##name_, aContext));                  \
                                                                               \
     MOZ_ASSERT(data, "should have aborted on out-of-memory");                 \
@@ -1012,11 +1045,13 @@ public:
    *
    * @return false if we fail to extract a color; this will not happen if both
    *         aPresContext and aStyleContext are non-null
    */
   static bool ComputeColor(const nsCSSValue& aValue,
                            nsPresContext* aPresContext,
                            nsStyleContext* aStyleContext,
                            nscolor& aResult);
+
+  static bool ParentHasPseudoElementData(nsStyleContext* aContext);
 };
 
 #endif
--- a/layout/style/nsStyleSet.cpp
+++ b/layout/style/nsStyleSet.cpp
@@ -840,16 +840,17 @@ ReplaceAnimationRule(nsRuleNode *aOldRul
   }
 
   MOZ_ASSERT(!IsMoreSpecificThanAnimation(n) &&
              (n->IsRoot() || n->GetLevel() != nsStyleSet::eAnimationSheet),
              "wrong level");
 
   if (aNewAnimRule) {
     n = n->Transition(aNewAnimRule, nsStyleSet::eAnimationSheet, false);
+    n->SetIsAnimationRule();
   }
 
   for (uint32_t i = moreSpecificNodes.Length(); i-- != 0; ) {
     nsRuleNode *oldNode = moreSpecificNodes[i];
     n = n->Transition(oldNode->GetRule(), oldNode->GetLevel(),
                       oldNode->IsImportantRule());
   }
 
@@ -1434,16 +1435,17 @@ nsStyleSet::ResolveStyleByAddingRules(ns
                     aBaseContext->GetPseudoType(),
                     nullptr, flags);
 }
 
 struct RuleNodeInfo {
   nsIStyleRule* mRule;
   uint8_t mLevel;
   bool mIsImportant;
+  bool mIsAnimationRule;
 };
 
 struct CascadeLevel {
   uint8_t mLevel;
   bool mIsImportant;
   bool mCheckForImportantRules;
   nsRestyleHint mLevelReplacementHint;
 };
@@ -1503,16 +1505,17 @@ nsStyleSet::RuleNodeWithReplacement(Elem
 
   nsTArray<RuleNodeInfo> rules;
   for (nsRuleNode* ruleNode = aOldRuleNode; !ruleNode->IsRoot();
        ruleNode = ruleNode->GetParent()) {
     RuleNodeInfo* curRule = rules.AppendElement();
     curRule->mRule = ruleNode->GetRule();
     curRule->mLevel = ruleNode->GetLevel();
     curRule->mIsImportant = ruleNode->IsImportantRule();
+    curRule->mIsAnimationRule = ruleNode->IsAnimationRule();
   }
 
   nsRuleWalker ruleWalker(mRuleTree, mAuthorStyleDisabled);
   auto rulesIndex = rules.Length();
 
   // We need to transfer this information between the non-!important and
   // !important phases for the style attribute level.
   nsRuleNode* lastScopedRN = nullptr;
@@ -1533,28 +1536,30 @@ nsStyleSet::RuleNodeWithReplacement(Elem
         case nsStyleSet::eAnimationSheet: {
           if (aPseudoType == nsCSSPseudoElements::ePseudo_NotPseudoElement ||
               aPseudoType == nsCSSPseudoElements::ePseudo_before ||
               aPseudoType == nsCSSPseudoElements::ePseudo_after) {
             nsIStyleRule* rule = PresContext()->AnimationManager()->
               GetAnimationRule(aElement, aPseudoType);
             if (rule) {
               ruleWalker.ForwardOnPossiblyCSSRule(rule);
+              ruleWalker.CurrentNode()->SetIsAnimationRule();
             }
           }
           break;
         }
         case nsStyleSet::eTransitionSheet: {
           if (aPseudoType == nsCSSPseudoElements::ePseudo_NotPseudoElement ||
               aPseudoType == nsCSSPseudoElements::ePseudo_before ||
               aPseudoType == nsCSSPseudoElements::ePseudo_after) {
             nsIStyleRule* rule = PresContext()->TransitionManager()->
               GetAnimationRule(aElement, aPseudoType);
             if (rule) {
               ruleWalker.ForwardOnPossiblyCSSRule(rule);
+              ruleWalker.CurrentNode()->SetIsAnimationRule();
             }
           }
           break;
         }
         case nsStyleSet::eSVGAttrAnimationSheet: {
           SVGAttrAnimationRuleProcessor* ruleProcessor =
             static_cast<SVGAttrAnimationRuleProcessor*>(
               mRuleProcessors[eSVGAttrAnimationSheet].get());
@@ -1610,16 +1615,19 @@ nsStyleSet::RuleNodeWithReplacement(Elem
       if (ruleInfo.mLevel != level->mLevel ||
           ruleInfo.mIsImportant != level->mIsImportant) {
         ++rulesIndex;
         break;
       }
 
       if (!doReplace) {
         ruleWalker.ForwardOnPossiblyCSSRule(ruleInfo.mRule);
+        if (ruleInfo.mIsAnimationRule) {
+          ruleWalker.CurrentNode()->SetIsAnimationRule();
+        }
       }
     }
   }
 
   NS_ASSERTION(rulesIndex == 0,
                "rules are in incorrect cascading order, "
                "which means we replaced them incorrectly");
 
--- a/layout/style/nsStyleStruct.h
+++ b/layout/style/nsStyleStruct.h
@@ -62,22 +62,26 @@ class imgIContainer;
 // See nsStyleContext::IsInDisplayNoneSubtree
 #define NS_STYLE_IN_DISPLAY_NONE_SUBTREE   0x100000000
 // See nsStyleContext::FindChildWithRules
 #define NS_STYLE_INELIGIBLE_FOR_SHARING    0x200000000
 // See nsStyleContext::GetPseudoEnum
 #define NS_STYLE_CONTEXT_TYPE_SHIFT        34
 
 // Additional bits for nsRuleNode's mDependentBits:
+#define NS_RULE_NODE_IS_ANIMATION_RULE      0x01000000
 #define NS_RULE_NODE_GC_MARK                0x02000000
 #define NS_RULE_NODE_USED_DIRECTLY          0x04000000
 #define NS_RULE_NODE_IS_IMPORTANT           0x08000000
 #define NS_RULE_NODE_LEVEL_MASK             0xf0000000
 #define NS_RULE_NODE_LEVEL_SHIFT            28
 
+// Additional bits for nsRuleNode's mNoneBits:
+#define NS_RULE_NODE_HAS_ANIMATION_DATA     0x80000000
+
 // The lifetime of these objects is managed by the presshell's arena.
 
 struct nsStyleFont {
   nsStyleFont(const nsFont& aFont, nsPresContext *aPresContext);
   nsStyleFont(const nsStyleFont& aStyleFont);
   explicit nsStyleFont(nsPresContext *aPresContext);
 private:
   void Init(nsPresContext *aPresContext);