Bug 1469076 - Fix the broken invariants of the rule node cache. r=heycam, a=RyanVM
authorEmilio Cobos Álvarez <emilio@crisal.io>
Sat, 16 Jun 2018 02:50:28 -0700
changeset 473780 f5ea361ceb75d5cdb9c44112ae6d37117b05e626
parent 473779 ea048f6200995a2ba9f35751c0e0e1503d72643d
child 473781 5afdaceeb96a3b80edeed5cff5ef917716f90b9d
push id1731
push userryanvm@gmail.com
push dateWed, 20 Jun 2018 14:21:08 +0000
treeherdermozilla-release@f5ea361ceb75 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersheycam, RyanVM
bugs1469076
milestone61.0
Bug 1469076 - Fix the broken invariants of the rule node cache. r=heycam, a=RyanVM We were spuriously reframing the <shadow> because it initially shared style with the <br>, which ended up being display: none, while the <shadow> should've been display: contents from the beginning. lookup_by_rules seems pretty prone to obscure bugs, and also it's pretty complex... Probably we should try to get rid of it, I'm unconvinced that it's worth it. Even with that, in a normal restyle the <details> wouldn't have ended up with a style. It of course never had it before the reframe because the <shadow> was display: none, but that doesn't mean it shouldn't have gotten one, since we detected we needed to go through kids in: https://searchfox.org/mozilla-central/rev/6eea08365e7386a2b81c044e7cc8a3daa51d8754/servo/components/style/matching.rs#500 That code did happen, but since it's an animation-only restyle, we don't look at unstyled stuff. That looks somewhat fishy, but I guess for now it's fine as long as display isn't animatable. MozReview-Commit-ID: B6NMSTNOKgK
layout/style/crashtests/1469076.html
layout/style/crashtests/crashtests.list
servo/components/style/sharing/mod.rs
servo/components/style/style_adjuster.rs
testing/web-platform/meta/MANIFEST.json
testing/web-platform/tests/css/css-display/display-contents-sharing-001-ref.html
testing/web-platform/tests/css/css-display/display-contents-sharing-001.html
new file mode 100644
--- /dev/null
+++ b/layout/style/crashtests/1469076.html
@@ -0,0 +1,12 @@
+<style>
+* { display: contents }
+</style>
+<script>
+function go() {
+  a.parentElement.animate({"ping": [0, 1]}, 0.281207776578);
+}
+</script>
+<body onload=go()>
+></br>
+<shadow>
+<details id="a">
--- a/layout/style/crashtests/crashtests.list
+++ b/layout/style/crashtests/crashtests.list
@@ -272,8 +272,9 @@ load 1439793.html
 load 1409183.html
 pref(dom.webcomponents.shadowdom.enabled,true) load 1445682.html
 load 1450691.html
 pref(dom.webcomponents.shadowdom.enabled,true) load 1453206.html
 load 1454140.html
 load 1455108.html
 load 1457288.html
 load 1457985.html
+load 1469076.html
--- a/servo/components/style/sharing/mod.rs
+++ b/servo/components/style/sharing/mod.rs
@@ -810,17 +810,21 @@ impl<E: TElement> StyleSharingCache<E> {
 
         debug!(
             "Sharing allowed between {:?} and {:?}",
             target.element, candidate.element
         );
         Some(candidate.element.borrow_data().unwrap().share_styles())
     }
 
-    /// Attempts to find an element in the cache with the given primary rule node and parent.
+    /// Attempts to find an element in the cache with the given primary rule
+    /// node and parent.
+    ///
+    /// FIXME(emilio): re-measure this optimization, and remove if it's not very
+    /// useful... It's probably not worth the complexity / obscure bugs.
     pub fn lookup_by_rules(
         &mut self,
         shared_context: &SharedStyleContext,
         inherited: &ComputedValues,
         rules: &StrongRuleNode,
         visited_rules: Option<&StrongRuleNode>,
         target: E,
     ) -> Option<PrimaryStyle> {
@@ -836,28 +840,37 @@ impl<E: TElement> StyleSharingCache<E> {
             let data = candidate.element.borrow_data().unwrap();
             let style = data.styles.primary();
             if style.rules.as_ref() != Some(&rules) {
                 return None;
             }
             if style.visited_rules() != visited_rules {
                 return None;
             }
-
+            // NOTE(emilio): We only need to check name / namespace because we
+            // do name-dependent style adjustments, like the display: contents
+            // to display: none adjustment.
+            if target.namespace() != candidate.element.namespace() {
+                return None;
+            }
+            if target.local_name() != candidate.element.local_name() {
+                return None;
+            }
             // Rule nodes and styles are computed independent of the element's
             // actual visitedness, but at the end of the cascade (in
             // `adjust_for_visited`) we do store the visitedness as a flag in
             // style.  (This is a subtle change from initial visited work that
             // landed when computed values were fused, see
             // https://bugzilla.mozilla.org/show_bug.cgi?id=1381635.)
             // So at the moment, we need to additionally compare visitedness,
             // since that is not accounted for by rule nodes alone.
             // FIXME(jryans): This seems like it breaks the constant time
             // requirements of visited, assuming we get a cache hit on only one
             // of unvisited vs. visited.
+            // TODO(emilio): We no longer have such a flag, remove this check.
             if target.is_visited_link() != candidate.element.is_visited_link() {
                 return None;
             }
 
             Some(data.share_primary_style())
         })
     }
 }
--- a/servo/components/style/style_adjuster.rs
+++ b/servo/components/style/style_adjuster.rs
@@ -14,16 +14,24 @@ use properties::longhands::float::comput
 use properties::longhands::overflow_x::computed_value::T as Overflow;
 use properties::longhands::position::computed_value::T as Position;
 
 /// A struct that implements all the adjustment methods.
 ///
 /// NOTE(emilio): If new adjustments are introduced that depend on reset
 /// properties of the parent, you may need tweaking the
 /// `ChildCascadeRequirement` code in `matching.rs`.
+///
+/// NOTE(emilio): Also, if new adjustments are introduced that break the
+/// following invariant:
+///
+///   Given same tag name, namespace, rules and parent style, two elements would
+///   end up with exactly the same style.
+///
+/// Then you need to adjust the lookup_by_rules conditions in the sharing cache.
 pub struct StyleAdjuster<'a, 'b: 'a> {
     style: &'a mut StyleBuilder<'b>,
 }
 
 #[cfg(feature = "gecko")]
 fn is_topmost_svg_svg_element<E>(e: E) -> bool
 where
     E: TElement,
--- a/testing/web-platform/meta/MANIFEST.json
+++ b/testing/web-platform/meta/MANIFEST.json
@@ -107390,16 +107390,28 @@
       [
        "/css/css-display/display-contents-pass-no-red-ref.html",
        "=="
       ]
      ],
      {}
     ]
    ],
+   "css/css-display/display-contents-sharing-001.html": [
+    [
+     "/css/css-display/display-contents-sharing-001.html",
+     [
+      [
+       "/css/css-display/display-contents-sharing-001-ref.html",
+       "=="
+      ]
+     ],
+     {}
+    ]
+   ],
    "css/css-display/display-contents-state-change-001.html": [
     [
      "/css/css-display/display-contents-state-change-001.html",
      [
       [
        "/css/css-display/display-contents-state-change-001-ref.html",
        "=="
       ]
@@ -240092,16 +240104,21 @@
      {}
     ]
    ],
    "css/css-display/display-contents-pass-ref.html": [
     [
      {}
     ]
    ],
+   "css/css-display/display-contents-sharing-001-ref.html": [
+    [
+     {}
+    ]
+   ],
    "css/css-display/display-contents-state-change-001-ref.html": [
     [
      {}
     ]
    ],
    "css/css-display/display-contents-suppression-dynamic-001-ref.html": [
     [
      {}
@@ -494565,16 +494582,24 @@
   "css/css-display/display-contents-pass-no-red-ref.html": [
    "8c4bf0927318a7612471fe09d91e80b37b1ab56c",
    "support"
   ],
   "css/css-display/display-contents-pass-ref.html": [
    "d6212621df87df9426ddb29b936703ace2813888",
    "support"
   ],
+  "css/css-display/display-contents-sharing-001-ref.html": [
+   "b258dc5bd923a67b912b01ab9a86e0149f4aef43",
+   "support"
+  ],
+  "css/css-display/display-contents-sharing-001.html": [
+   "0276d2d8870748dd0940716ee113d59ef349ade8",
+   "reftest"
+  ],
   "css/css-display/display-contents-state-change-001-ref.html": [
    "f7e25855cc7ef1896a9a52005d3c1379bf74746b",
    "support"
   ],
   "css/css-display/display-contents-state-change-001.html": [
    "0a689fbe90be794772c66d59b033d15336e6dfe3",
    "reftest"
   ],
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/css/css-display/display-contents-sharing-001-ref.html
@@ -0,0 +1,10 @@
+<!doctype html>
+<meta charset="utf-8">
+<title style="display: none">CSS Test Reference</title>
+<link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
+<style>
+  * { display: contents }
+</style>
+<whatever>
+  PASS
+</whatever>
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/css/css-display/display-contents-sharing-001.html
@@ -0,0 +1,14 @@
+<!doctype html>
+<meta charset="utf-8">
+<title style="display: none">CSS Test: display:contents style sharing.</title>
+<link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
+<link rel="match" href="display-contents-sharing-001-ref.html">
+<link rel="help" href="https://drafts.csswg.org/css-display/#unbox-html">
+<link rel="help" href="https://bugzil.la/1469076">
+<style>
+  * { display: contents }
+</style>
+<br>
+<whatever>
+  PASS
+</whatever>