style: Look at the snapshots when invalidating due to stylesheet changes.

Otherwise removal of stylesheets may get out of sync with other DOM changes, and
we may fail to invalidate the style of the affected elements.

Bug: 1432850
Reviewed-by: bz
MozReview-Commit-ID: DrMTgLzQcnk

8 files changed, 95 insertions, 37 deletions

diff --git a/components/layout_thread/lib.rs b/components/layout_thread/lib.rs
index fcd790fe6ec..66f328f4465 100644
--- a/components/layout_thread/lib.rs
+++ b/components/layout_thread/lib.rs
@@ -1194,8 +1194,6 @@ impl LayoutThread {
                 debug!("Doc sheets changed, flushing author sheets too");
                 self.stylist.force_stylesheet_origins_dirty(Origin::Author.into());
             }
-
-            self.stylist.flush(&guards, Some(element));
         }
 
         if viewport_size_changed {
@@ -1246,6 +1244,8 @@ impl LayoutThread {
             debug!("Noting restyle for {:?}: {:?}", el, style_data);
         }
 
+        self.stylist.flush(&guards, Some(element), Some(&map));
+
         // Create a layout context for use throughout the following passes.
         let mut layout_context =
             self.build_layout_context(guards.clone(), true, &map);
diff --git a/components/style/gecko/data.rs b/components/style/gecko/data.rs
index d3f5e303f75..b0e40415157 100644
--- a/components/style/gecko/data.rs
+++ b/components/style/gecko/data.rs
@@ -15,6 +15,7 @@ use invalidation::media_queries::{MediaListKey, ToMediaListKey};
 use malloc_size_of::MallocSizeOfOps;
 use media_queries::{Device, MediaList};
 use properties::ComputedValues;
+use selector_parser::SnapshotMap;
 use servo_arc::Arc;
 use shared_lock::{Locked, StylesheetGuards, SharedRwLockReadGuard};
 use std::sync::atomic::{AtomicUsize, Ordering};
@@ -202,6 +203,7 @@ impl PerDocumentStyleDataImpl {
         &mut self,
         guard: &SharedRwLockReadGuard,
         document_element: Option<E>,
+        snapshots: Option<&SnapshotMap>,
     ) -> bool
     where
         E: TElement,
@@ -209,6 +211,7 @@ impl PerDocumentStyleDataImpl {
         self.stylist.flush(
             &StylesheetGuards::same(guard),
             document_element,
+            snapshots,
         )
     }
 
diff --git a/components/style/gecko/snapshot.rs b/components/style/gecko/snapshot.rs
index 862f8ab545b..844c3636856 100644
--- a/components/style/gecko/snapshot.rs
+++ b/components/style/gecko/snapshot.rs
@@ -193,7 +193,8 @@ impl ElementSnapshot for GeckoElementSnapshot {
 
     #[inline]
     fn each_class<F>(&self, callback: F)
-        where F: FnMut(&Atom)
+    where
+        F: FnMut(&Atom)
     {
         if !self.has_any(Flags::MaybeClass) {
             return;
diff --git a/components/style/invalidation/element/element_wrapper.rs b/components/style/invalidation/element/element_wrapper.rs
index afd5bbcb2cb..22ca94a6e14 100644
--- a/components/style/invalidation/element/element_wrapper.rs
+++ b/components/style/invalidation/element/element_wrapper.rs
@@ -71,7 +71,8 @@ pub struct ElementWrapper<'a, E>
 }
 
 impl<'a, E> ElementWrapper<'a, E>
-    where E: TElement,
+where
+    E: TElement,
 {
     /// Trivially constructs an `ElementWrapper`.
     pub fn new(el: E, snapshot_map: &'a SnapshotMap) -> Self {
diff --git a/components/style/invalidation/stylesheets.rs b/components/style/invalidation/stylesheets.rs
index 01cff601560..53296878164 100644
--- a/components/style/invalidation/stylesheets.rs
+++ b/components/style/invalidation/stylesheets.rs
@@ -8,12 +8,14 @@
 #![deny(unsafe_code)]
 
 use Atom;
+use CaseSensitivityExt;
 use LocalName as SelectorLocalName;
 use dom::{TElement, TNode};
 use fnv::FnvHashSet;
+use invalidation::element::element_wrapper::{ElementSnapshot, ElementWrapper};
 use invalidation::element::restyle_hints::RestyleHint;
 use media_queries::Device;
-use selector_parser::SelectorImpl;
+use selector_parser::{SelectorImpl, Snapshot, SnapshotMap};
 use selectors::attr::CaseSensitivity;
 use selectors::parser::{Component, LocalName, Selector};
 use shared_lock::SharedRwLockReadGuard;
@@ -43,21 +45,40 @@ impl Invalidation {
         matches!(*self, Invalidation::ID(..) | Invalidation::Class(..))
     }
 
-    fn matches<E>(&self, element: E) -> bool
-        where E: TElement,
+    fn matches<E>(&self, element: E, snapshot: Option<&Snapshot>) -> bool
+    where
+        E: TElement,
     {
+        // FIXME This should look at the quirks mode of the document to
+        // determine case sensitivity.
+        //
+        // FIXME(emilio): Actually write a test and fix this.
+        let case_sensitivity = CaseSensitivity::CaseSensitive;
         match *self {
             Invalidation::Class(ref class) => {
-                // FIXME This should look at the quirks mode of the document to
-                // determine case sensitivity.
-                element.has_class(class, CaseSensitivity::CaseSensitive)
+                if element.has_class(class, case_sensitivity) {
+                    return true;
+                }
+
+                if let Some(snapshot) = snapshot {
+                    if snapshot.has_class(class, case_sensitivity) {
+                        return true;
+                    }
+                }
             }
             Invalidation::ID(ref id) => {
-                match element.get_id() {
-                    // FIXME This should look at the quirks mode of the document
-                    // to determine case sensitivity.
-                    Some(element_id) => element_id == *id,
-                    None => false,
+                if let Some(ref element_id) = element.get_id() {
+                    if case_sensitivity.eq_atom(element_id, id) {
+                        return true;
+                    }
+                }
+
+                if let Some(snapshot) = snapshot {
+                    if let Some(ref old_id) = snapshot.id_attr() {
+                        if case_sensitivity.eq_atom(old_id, id) {
+                            return true;
+                        }
+                    }
                 }
             }
             Invalidation::LocalName { ref name, ref lower_name } => {
@@ -65,9 +86,11 @@ impl Invalidation {
                 // of testing against both names, but it's probably not worth
                 // it.
                 let local_name = element.get_local_name();
-                *local_name == **name || *local_name == **lower_name
+                return *local_name == **name || *local_name == **lower_name
             }
         }
+
+        false
     }
 }
 
@@ -145,11 +168,17 @@ impl StylesheetInvalidationSet {
     /// `document_element` is provided.
     ///
     /// Returns true if any invalidations ocurred.
-    pub fn flush<E>(&mut self, document_element: Option<E>) -> bool
-        where E: TElement,
+    pub fn flush<E>(
+        &mut self,
+        document_element: Option<E>,
+        snapshots: Option<&SnapshotMap>,
+    ) -> bool
+    where
+        E: TElement,
     {
+        debug!("Stylist::flush({:?}, snapshots: {})", document_element, snapshots.is_some());
         let have_invalidations = match document_element {
-            Some(e) => self.process_invalidations(e),
+            Some(e) => self.process_invalidations(e, snapshots),
             None => false,
         };
         self.clear();
@@ -163,9 +192,17 @@ impl StylesheetInvalidationSet {
         self.fully_invalid = false;
     }
 
-    fn process_invalidations<E>(&self, element: E) -> bool
-        where E: TElement,
+    fn process_invalidations<E>(&self, element: E, snapshots: Option<&SnapshotMap>) -> bool
+    where
+        E: TElement,
     {
+        debug!(
+           "Stylist::process_invalidations({:?}, {:?}, {:?})",
+           element,
+           self.invalid_scopes,
+           self.invalid_elements,
+        );
+
         {
             let mut data = match element.mutate_data() {
                 Some(data) => data,
@@ -185,7 +222,7 @@ impl StylesheetInvalidationSet {
             return false;
         }
 
-        self.process_invalidations_in_subtree(element)
+        self.process_invalidations_in_subtree(element, snapshots)
     }
 
     /// Process style invalidations in a given subtree. This traverses the
@@ -194,9 +231,15 @@ impl StylesheetInvalidationSet {
     ///
     /// Returns whether it invalidated at least one element's style.
     #[allow(unsafe_code)]
-    fn process_invalidations_in_subtree<E>(&self, element: E) -> bool
-        where E: TElement,
+    fn process_invalidations_in_subtree<E>(
+        &self,
+        element: E,
+        snapshots: Option<&SnapshotMap>,
+    ) -> bool
+    where
+        E: TElement,
     {
+        debug!("process_invalidations_in_subtree({:?})", element);
         let mut data = match element.mutate_data() {
             Some(data) => data,
             None => return false,
@@ -212,8 +255,10 @@ impl StylesheetInvalidationSet {
             return false;
         }
 
+        let element_wrapper = snapshots.map(|s| ElementWrapper::new(element, s));
+        let snapshot = element_wrapper.as_ref().and_then(|e| e.snapshot());
         for invalidation in &self.invalid_scopes {
-            if invalidation.matches(element) {
+            if invalidation.matches(element, snapshot) {
                 debug!("process_invalidations_in_subtree: {:?} matched subtree {:?}",
                        element, invalidation);
                 data.hint.insert(RestyleHint::restyle_subtree());
@@ -225,7 +270,7 @@ impl StylesheetInvalidationSet {
 
         if !data.hint.contains(RestyleHint::RESTYLE_SELF) {
             for invalidation in &self.invalid_elements {
-                if invalidation.matches(element) {
+                if invalidation.matches(element, snapshot) {
                     debug!("process_invalidations_in_subtree: {:?} matched self {:?}",
                            element, invalidation);
                     data.hint.insert(RestyleHint::RESTYLE_SELF);
@@ -243,7 +288,8 @@ impl StylesheetInvalidationSet {
                 None => continue,
             };
 
-            any_children_invalid |= self.process_invalidations_in_subtree(child);
+            any_children_invalid |=
+                self.process_invalidations_in_subtree(child, snapshots);
         }
 
         if any_children_invalid {
diff --git a/components/style/stylesheet_set.rs b/components/style/stylesheet_set.rs
index c3cc0c7799d..b50c18cf14e 100644
--- a/components/style/stylesheet_set.rs
+++ b/components/style/stylesheet_set.rs
@@ -7,6 +7,7 @@
 use dom::TElement;
 use invalidation::stylesheets::StylesheetInvalidationSet;
 use media_queries::Device;
+use selector_parser::SnapshotMap;
 use shared_lock::SharedRwLockReadGuard;
 use std::slice;
 use stylesheets::{Origin, OriginSet, OriginSetIterator, PerOrigin, StylesheetInDocument};
@@ -502,6 +503,7 @@ where
     pub fn flush<'a, E>(
         &'a mut self,
         document_element: Option<E>,
+        snapshots: Option<&SnapshotMap>,
     ) -> StylesheetFlusher<'a, S>
     where
         E: TElement,
@@ -510,7 +512,9 @@ where
 
         debug!("StylesheetSet::flush");
 
-        let had_invalidations = self.invalidations.flush(document_element);
+        let had_invalidations =
+            self.invalidations.flush(document_element, snapshots);
+
         let origins_dirty =
             mem::replace(&mut self.origins_dirty, OriginSet::empty());
 
diff --git a/components/style/stylist.rs b/components/style/stylist.rs
index 6ed2b36ac89..4abe0aca207 100644
--- a/components/style/stylist.rs
+++ b/components/style/stylist.rs
@@ -25,7 +25,7 @@ use properties::{AnimationRules, PropertyDeclarationBlock};
 use rule_cache::{RuleCache, RuleCacheConditions};
 use rule_tree::{CascadeLevel, RuleTree, StrongRuleNode, StyleSource};
 use selector_map::{PrecomputedHashMap, SelectorMap, SelectorMapEntry};
-use selector_parser::{SelectorImpl, PerPseudoElementMap, PseudoElement};
+use selector_parser::{SelectorImpl, SnapshotMap, PerPseudoElementMap, PseudoElement};
 use selectors::NthIndexCache;
 use selectors::attr::{CaseSensitivity, NamespaceConstraint};
 use selectors::bloom::{BloomFilter, NonCountingBloomFilter};
@@ -504,6 +504,7 @@ impl Stylist {
         &mut self,
         guards: &StylesheetGuards,
         document_element: Option<E>,
+        snapshots: Option<&SnapshotMap>,
     ) -> bool
     where
         E: TElement,
@@ -548,7 +549,7 @@ impl Stylist {
             }
         }
 
-        let flusher = self.stylesheets.flush(document_element);
+        let flusher = self.stylesheets.flush(document_element, snapshots);
 
         let had_invalidations = flusher.had_invalidations();
 
diff --git a/ports/geckolib/glue.rs b/ports/geckolib/glue.rs
index ca344ac922d..701c594ef56 100644
--- a/ports/geckolib/glue.rs
+++ b/ports/geckolib/glue.rs
@@ -1219,21 +1219,23 @@ pub extern "C" fn Servo_StyleSet_RemoveStyleSheet(
 }
 
 #[no_mangle]
-pub extern "C" fn Servo_StyleSet_FlushStyleSheets(
+pub unsafe extern "C" fn Servo_StyleSet_FlushStyleSheets(
     raw_data: RawServoStyleSetBorrowed,
     doc_element: RawGeckoElementBorrowedOrNull,
+    snapshots: *const ServoElementSnapshotTable,
 ) {
     let global_style_data = &*GLOBAL_STYLE_DATA;
     let guard = global_style_data.shared_lock.read();
     let mut data = PerDocumentStyleData::from_ffi(raw_data).borrow_mut();
     let doc_element = doc_element.map(GeckoElement);
-    let have_invalidations = data.flush_stylesheets(&guard, doc_element);
+
+    let have_invalidations =
+        data.flush_stylesheets(&guard, doc_element, snapshots.as_ref());
+
     if have_invalidations && doc_element.is_some() {
-        // The invalidation machinery propagates the bits up, but we still
-        // need to tell the gecko restyle root machinery about it.
-        unsafe {
-            bindings::Gecko_NoteDirtySubtreeForInvalidation(doc_element.unwrap().0);
-        }
+        // The invalidation machinery propagates the bits up, but we still need
+        // to tell the Gecko restyle root machinery about it.
+        bindings::Gecko_NoteDirtySubtreeForInvalidation(doc_element.unwrap().0);
     }
 }