Bug 1209603 patch 9 - Cache inherited style structs on the style context when we found already-cached data in the rule tree. r=heycam

This means we obey the invariant that if we've requested an inherited
struct on a context, that struct will be cached on the style context.  I
believe bug 527977 intended to do make us obey this invariant, but it
missed the case where nsRuleNode::GetStyle* found cached data already on
the rule node, and the case where nsRuleNode::WalkRuleTree found a
usable struct higher in the rule tree.

Without this change, patch 10 will not function correctly for inherited
structs when we encounter this case, and will cause assertions in
dom/base/test/test_bug560780.html due to triggering style change hints
on text nodes that inherited a color struct from a parent on whose rule
node the struct was stored.  (It may also have caused some of the other
test failures.)

This should be a clear performance improvement, since the path that's
being slowed down by the added work in this patch will, with the patch,
now only execute once because of that work.

--HG--
extra : commitid : 5hueOZihqNx

layout/style/nsRuleNode.cpp

@@ -2348,11 +2348,12 @@ nsRuleNode::WalkRuleTree(const nsStyleStructID aSID,
     // branch that they never need to examine their rules for this particular struct type
     // ever again.
     PropagateDependentBit(aSID, ruleNode, startStruct);
-    if (isReset) {
-      // Record that we have asked for this struct on this context, but
-      // it is not cached on the context.
-      aContext->AddStyleBit(nsCachedStyleData::GetBitForSID(aSID));
-    }
+    // With this same bit set, we do two different things:
+    // For reset structs, record that we have asked for this struct on
+    // this context, but it is not cached on the context.
+    // For inherited structs, mark the struct (which will be set on
+    // the context by our caller) as not being owned by the context.
+    aContext->AddStyleBit(nsCachedStyleData::GetBitForSID(aSID));
     return startStruct;
   }
   if ((!startStruct && !isReset &&
@@ -2724,8 +2725,8 @@ nsRuleNode::SetDefaultOnRoot(const nsStyleStructID aSID, nsStyleContext* aContex
     /* Tell the style context that it doesn't own the data */                 \
     aContext->AddStyleBit(NS_STYLE_INHERIT_BIT(type_));                       \
   }                                                                           \
-  /* Always cache inherited data on the style context */                      \
-  aContext->SetStyle##type_(data_);                                           \
+  /* For inherited structs, our caller will cache the data on the */          \
+  /* style context */                                                         \
                                                                               \
   return data_;
 
@@ -9413,11 +9414,12 @@ nsRuleNode::GetStyleData(nsStyleStructID aSID,
   if (!(HasAnimationData() && ParentHasPseudoElementData(aContext))) {
     data = mStyleData.GetStyleData(aSID, aContext);
     if (MOZ_LIKELY(data != nullptr)) {
+      // With this same bit set, we do two different things:
       // For reset structs, mark the struct as having been retrieved for
       // this context.
-      if (nsCachedStyleData::IsReset(aSID)) {
-        aContext->AddStyleBit(nsCachedStyleData::GetBitForSID(aSID));
-      }
+      // For inherited structs, mark the struct (which will be set on
+      // the context by our caller) as not being owned by the context.
+      aContext->AddStyleBit(nsCachedStyleData::GetBitForSID(aSID));
       return data; // We have a fully specified struct. Just return it.
     }
   }

layout/style/nsRuleNode.h

@@ -883,7 +883,7 @@ public:
   #define STYLE_STRUCT_INHERITED(name_, checkdata_cb_)                        \
   template<bool aComputeData>                                                 \
   const nsStyle##name_*                                                       \
-  GetStyle##name_(nsStyleContext* aContext)                                   \
+  GetStyle##name_(nsStyleContext* aContext, uint64_t& aContextStyleBits)      \
   {                                                                           \
     NS_ASSERTION(IsUsedDirectly(),                                            \
                  "if we ever call this on rule nodes that aren't used "       \
@@ -898,8 +898,15 @@ public:
     /* see comment on cacheability in AnimValuesStyleRule::MapRuleInfoInto */ \
     if (!(HasAnimationData() && ParentHasPseudoElementData(aContext))) {      \
       data = mStyleData.GetStyle##name_();                                    \
-      if (MOZ_LIKELY(data != nullptr))                                        \
+      if (data != nullptr) {                                                  \
+        /* For inherited structs, mark the struct (which will be set on */    \
+        /* the context by our caller) as not being owned by the context. */   \
+        /* Normally this would be aContext->AddStyleBit(), but aContext is */ \
+        /* an incomplete type here, so we work around that with a param. */   \
+        aContextStyleBits |= NS_STYLE_INHERIT_BIT(name_);                     \
+        /* Our caller will cache the data on the style context. */            \
         return data;                                                          \
+      }                                                                       \
     }                                                                         \
                                                                               \
     if (!aComputeData)                                                        \

layout/style/nsStyleContext.cpp

@@ -417,7 +417,14 @@ const void* nsStyleContext::StyleData(nsStyleStructID aSID)
   const void* cachedData = GetCachedStyleData(aSID);
   if (cachedData)
     return cachedData; // We have computed data stored on this node in the context tree.
-  return mRuleNode->GetStyleData(aSID, this, true); // Our rule node will take care of it for us.
+  // Our rule node will take care of it for us.
+  const void* newData = mRuleNode->GetStyleData(aSID, this, true);
+  if (!nsCachedStyleData::IsReset(aSID)) {
+    // always cache inherited data on the style context; the rule
+    // node set the bit in mBits for us if needed.
+    mCachedInheritedData.mStyleStructs[aSID] = const_cast<void*>(newData);
+  }
+  return newData;
 }
 
 // This is an evil evil function, since it forces you to alloc your own separate copy of

layout/style/nsStyleContext.h

@@ -535,7 +535,13 @@ private:
         return cachedData;                                              \
       /* Have the rulenode deal */                                      \
       AUTO_CHECK_DEPENDENCY(eStyleStruct_##name_);                      \
-      return mRuleNode->GetStyle##name_<aComputeData>(this);            \
+      const nsStyle##name_ * newData =                                  \
+        mRuleNode->GetStyle##name_<aComputeData>(this, mBits);          \
+      /* always cache inherited data on the style context; the rule */  \
+      /* node set the bit in mBits for us if needed. */                 \
+      mCachedInheritedData.mStyleStructs[eStyleStruct_##name_] =        \
+        const_cast<nsStyle##name_ *>(newData);                          \
+      return newData;                                                   \
     }
   #define STYLE_STRUCT_RESET(name_, checkdata_cb_)                      \
     template<bool aComputeData>                                         \