Auto merge of #16670 - emilio:sharing-is-sometimes-hard, r=bholley

style: Account for cousin sharing in the revalidation logic.

Fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1361013

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16670)
<!-- Reviewable:end -->

components/selectors/parser.rs

@@ -169,27 +169,6 @@ impl<Impl: SelectorImpl> SelectorInner<Impl> {
         }
     }
 
-    /// Creates a clone of this selector with everything to the left of
-    /// (and including) the rightmost ancestor combinator removed. So
-    /// the selector |span foo > bar + baz| will become |bar + baz|.
-    /// This is used for revalidation selectors in servo.
-    ///
-    /// The bloom filter hashes are copied, even though they correspond to
-    /// parts of the selector that have been stripped out, because they are
-    /// still useful for fast-rejecting the reduced selectors.
-    pub fn slice_to_first_ancestor_combinator(&self) -> Self {
-        let maybe_pos = self.complex.iter_raw()
-                            .position(|s| s.as_combinator()
-                                           .map_or(false, |c| c.is_ancestor()));
-        match maybe_pos {
-            None => self.clone(),
-            Some(index) => SelectorInner {
-                complex: self.complex.slice_to(index),
-                ancestor_hashes: self.ancestor_hashes.clone(),
-            },
-        }
-    }
-
     /// Creates a SelectorInner from a Vec of Components. Used in tests.
     pub fn from_vec(vec: Vec<Component<Impl>>) -> Self {
         let complex = ComplexSelector::from_vec(vec);

components/style/stylist.rs

@@ -27,7 +27,8 @@ use selectors::Element;
 use selectors::bloom::BloomFilter;
 use selectors::matching::{AFFECTED_BY_STYLE_ATTRIBUTE, AFFECTED_BY_PRESENTATIONAL_HINTS};
 use selectors::matching::{ElementSelectorFlags, StyleRelations, matches_selector};
-use selectors::parser::{Component, Selector, SelectorInner, SelectorMethods, LocalName as LocalNameSelector};
+use selectors::parser::{Combinator, Component, Selector, SelectorInner, SelectorIter};
+use selectors::parser::{SelectorMethods, LocalName as LocalNameSelector};
 use selectors::visitor::SelectorVisitor;
 use shared_lock::{Locked, SharedRwLockReadGuard, StylesheetGuards};
 use sink::Push;
@@ -375,10 +376,7 @@ impl Stylist {
     #[inline]
     fn note_for_revalidation(&mut self, selector: &Selector<SelectorImpl>) {
         if needs_revalidation(selector) {
-            // For revalidation, we can skip everything left of the first ancestor
+            self.selectors_for_cache_revalidation.push(selector.inner.clone());
-            // combinator.
-            let revalidation_sel = selector.inner.slice_to_first_ancestor_combinator();
-            self.selectors_for_cache_revalidation.push(revalidation_sel);
         }
     }
 
@@ -923,18 +921,47 @@ impl Drop for Stylist {
 /// Visitor determine whether a selector requires cache revalidation.
 ///
 /// Note that we just check simple selectors and eagerly return when the first
-/// need for revalidation is found, so we don't need to store state on the visitor.
+/// need for revalidation is found, so we don't need to store state on the
+/// visitor.
+///
+/// Also, note that it's important to check the whole selector, due to cousins
+/// sharing arbitrarily deep in the DOM, not just the rightmost part of it
+/// (unfortunately, though).
+///
+/// With cousin sharing, we not only need to care about selectors in stuff like
+/// foo:first-child, but also about selectors like p:first-child foo, since the
+/// two parents may have shared style, and in that case we can test cousins
+/// whose matching depends on the selector up in the chain.
+///
+/// TODO(emilio): We can optimize when matching only siblings to only match the
+/// rightmost selector until a descendant combinator is found, I guess, and in
+/// general when we're sharing at depth `n`, to the `n + 1` sequences of
+/// descendant combinators.
+///
+/// I don't think that in presence of the bloom filter it's worth it, though.
 struct RevalidationVisitor;
 
 impl SelectorVisitor for RevalidationVisitor {
     type Impl = SelectorImpl;
 
-    /// Check whether a rightmost sequence of simple selectors containing this
+
-    /// simple selector to be explicitly matched against both the style sharing
+    fn visit_complex_selector(&mut self,
-    /// cache entry and the candidate.
+                              _: SelectorIter<SelectorImpl>,
+                              combinator: Option<Combinator>) -> bool {
+        let is_sibling_combinator =
+            combinator.map_or(false, |c| c.is_sibling());
+
+        !is_sibling_combinator
+    }
+
+
+    /// Check whether sequence of simple selectors containing this simple
+    /// selector to be explicitly matched against both the style sharing cache
+    /// entry and the candidate.
     ///
-    /// We use this for selectors that can have different matching behavior between
+    /// We use this for selectors that can have different matching behavior
-    /// siblings that are otherwise identical as far as the cache is concerned.
+    /// between siblings that are otherwise identical as far as the cache is
+    /// concerned.
     fn visit_simple_selector(&mut self, s: &Component<SelectorImpl>) -> bool {
         match *s {
             Component::AttrExists(_) |
@@ -970,23 +997,7 @@ impl SelectorVisitor for RevalidationVisitor {
 /// Returns true if the given selector needs cache revalidation.
 pub fn needs_revalidation(selector: &Selector<SelectorImpl>) -> bool {
     let mut visitor = RevalidationVisitor;
-
+    !selector.visit(&mut visitor)
-    // We only need to consider the rightmost sequence of simple selectors, so
-    // we can stop at the first combinator. This is because:
-    // * If it's an ancestor combinator, we can ignore everything to the left
-    //   because matching won't differ between siblings.
-    // * If it's a sibling combinator, then we know we need revalidation.
-    let mut iter = selector.inner.complex.iter();
-    for ss in &mut iter {
-        if !ss.visit(&mut visitor) {
-            return true;
-        }
-    }
-
-    // If none of the simple selectors in the rightmost sequence required
-    // revalidation, we need revalidation if and only if the combinator is a
-    // sibling combinator.
-    iter.next_sequence().map_or(false, |c| c.is_sibling())
 }
 
 /// Map that contains the CSS rules for a specific PseudoElement

tests/unit/style/stylist.rs

@@ -109,13 +109,11 @@ fn test_revalidation_selectors() {
         "span + div",
         "span ~ div",
 
-        // Revalidation selectors that will get sliced.
+        // Selectors in the ancestor chain (needed for cousin sharing).
-        "td > h1[dir]",
+        "p:first-child span",
-        "td > span + h1[dir]",
-        "table td > span + div ~ h1[dir]",
     ]).into_iter()
       .filter(|s| needs_revalidation(&s))
-      .map(|s| s.inner.slice_to_first_ancestor_combinator().complex)
+      .map(|s| s.inner.complex)
       .collect::<Vec<_>>();
 
     let reference = parse_selectors(&[
@@ -147,10 +145,8 @@ fn test_revalidation_selectors() {
         "span + div",
         "span ~ div",
 
-        // Revalidation selectors that got sliced.
+        // Selectors in the ancestor chain (needed for cousin sharing).
-        "h1[dir]",
+        "p:first-child span",
-        "span + h1[dir]",
-        "span + div ~ h1[dir]",
     ]).into_iter()
       .map(|s| s.inner.complex)
       .collect::<Vec<_>>();

tests/wpt/mozilla/meta/MANIFEST.json

@@ -1211,6 +1211,18 @@
      {}
     ]
    ],
+   "css/bug-1361013-cousin-sharing.html": [
+    [
+     "/_mozilla/css/bug-1361013-cousin-sharing.html",
+     [
+      [
+       "/_mozilla/css/bug-1361013-cousin-sharing-ref.html",
+       "=="
+      ]
+     ],
+     {}
+    ]
+   ],
    "css/button_css_width.html": [
     [
      "/_mozilla/css/button_css_width.html",
@@ -7262,6 +7274,11 @@
      {}
     ]
    ],
+   "css/bug-1361013-cousin-sharing-ref.html": [
+    [
+     {}
+    ]
+   ],
    "css/button_css_width_ref.html": [
     [
      {}
@@ -20835,6 +20852,14 @@
    "2ebf9c8f963a2f3971a3c1b64b6b01825eacdedc",
    "support"
   ],
+  "css/bug-1361013-cousin-sharing-ref.html": [
+   "3451cb4533d650f6f624b89548ce7117bcc8bef3",
+   "support"
+  ],
+  "css/bug-1361013-cousin-sharing.html": [
+   "95c8c8d33cca3d6e969338a98c60ec9804256ef5",
+   "reftest"
+  ],
   "css/bug_1345483.html": [
    "41b55af9d5c95910b1af74c0fbffb24e20bbe869",
    "testharness"

tests/wpt/mozilla/tests/css/bug-1361013-cousin-sharing-ref.html

@@ -0,0 +1,8 @@
+<!doctype html>
+<style>
+div { color: green; }
+</style>
+<div>
+  <p><span></span></p>
+  <p><span>This should be green.</span></p>
+</div>

tests/wpt/mozilla/tests/css/bug-1361013-cousin-sharing.html

@@ -0,0 +1,13 @@
+<!doctype html>
+<meta charset="utf-8">
+<title>Bug 1361013: selectors for revalidation account for cousins sharing style</title>
+<link rel="author" name="Cameron McCormack" href="mailto:cam@mcc.id.au">
+<link rel="match" href="bug-1361013-cousin-sharing-ref.html">
+<style>
+div { color: green; }
+p:first-child > span { color: red; }
+</style>
+<div>
+  <p><span></span></p>
+  <p><span>This should be green.</span></p>
+</div>