Bug 1356601 - Don't force computation of a Variables struct when animations are involved. r=dbaron, a=gchang
authorCameron McCormack <cam@mcc.id.au>
Tue, 18 Apr 2017 22:27:04 +1000
changeset 395950 6ed18ad42fc25d212c34b153cd8dcf025674827c
parent 395949 1a74650efe112263c6f65a90bb22ca66ac65d97e
child 395951 584df356a66b1f5d48c73625dd08589a11029845
push id1468
push userasasaki@mozilla.com
push dateMon, 05 Jun 2017 19:31:07 +0000
treeherdermozilla-release@0641fc6ee9d1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdbaron, gchang
bugs1356601
milestone54.0
Bug 1356601 - Don't force computation of a Variables struct when animations are involved. r=dbaron, a=gchang Without this change, we can end up asserting in ComputeVariablesData and crashing in CSSVariableResolver::Resolve due to not finding any variable declarations on the rules we matched, when we have content like in the crashtest added here, i.e. variables inheriting into a pseudo like ::first-line and animations on the element inside the ::first-line. We could solve this alternatively by removing the assertion and making CSSVariableResolver::Resolve handle a null aDeclarations more gracefully, but since we can save the effort of recomputing the Variables struct in this case, we may as well. MozReview-Commit-ID: 6l06ZF3WGsy
layout/style/crashtests/1356601-1.html
layout/style/crashtests/crashtests.list
layout/style/nsRuleNode.cpp
new file mode 100644
--- /dev/null
+++ b/layout/style/crashtests/1356601-1.html
@@ -0,0 +1,18 @@
+<!doctype html>
+<html>
+<style>
+div::first-line {
+  --bar: left;
+}
+span {
+  animation: var(--bar) 5s infinite alternate;
+}
+@keyframes left {
+  from {left: 0;}
+  to {left: 30px;}
+}
+</style>
+<div>
+  <span>Crash</span>
+</div>
+</html>
--- a/layout/style/crashtests/crashtests.list
+++ b/layout/style/crashtests/crashtests.list
@@ -165,8 +165,9 @@ load 1315889-1.html
 load 1315894-1.html
 load 1319072-1.html
 HTTP load 1320423-1.html
 load 1321357-1.html
 load 1328535-1.html
 load 1331272.html
 HTTP load 1333001-1.html
 pref(dom.animations-api.core.enabled,true) load 1340344.html
+load 1356601-1.html
--- a/layout/style/nsRuleNode.cpp
+++ b/layout/style/nsRuleNode.cpp
@@ -2548,19 +2548,33 @@ nsRuleNode::WalkRuleTree(const nsStyleSt
                                 detail != eRuleFullMixed &&
                                 detail != eRuleFullInherited),
                "can't have start struct and be fully specified");
 
   bool isReset = nsCachedStyleData::IsReset(aSID);
   if (!highestNode)
     highestNode = rootNode;
 
-  if (!ruleData.mConditions.CacheableWithoutDependencies())
-    detail = eRulePartialMixed; // Treat as though some data is specified to avoid
-                                // the optimizations and force data computation.
+  MOZ_ASSERT(!(aSID == eStyleStruct_Variables && startStruct),
+             "if we start caching Variables structs in the rule tree, then "
+             "not forcing detail to eRulePartialMixed just below is no "
+             "longer valid");
+
+  if (!ruleData.mConditions.CacheableWithoutDependencies() &&
+      aSID != eStyleStruct_Variables) {
+    // Treat as though some data is specified to avoid the optimizations and
+    // force data computation.
+    //
+    // 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 computation of a
+    // new Variables struct when we have no additional variable declarations,
+    // which otherwise could happen when there is an AnimValuesStyleRule
+    // (which calls SetUncacheable for style contexts with pseudo data).
+    detail = eRulePartialMixed;
+  }
 
   if (detail == eRuleNone && startStruct) {
     // We specified absolutely no rule information, but a parent rule in the tree
     // specified all the rule information.  We set a bit along the branch from our
     // node in the tree to the node that specified the data that tells nodes on that
     // branch that they never need to examine their rules for this particular struct type
     // ever again.
     PropagateDependentBit(aSID, ruleNode, startStruct);