Bug 1380198: Ensure sequential tasks run after the bloom filter is dropped. r?bholley draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Thu, 13 Jul 2017 21:35:22 +0200
changeset 608526 50d4d365ff59cd413314ceef0c83fffc15eda449
parent 608521 c278c4dcda9cde829ca47359270eea10aad4b8d3
child 608531 aef586b08bba1a0732787c2dc53e908d384e7a04
push id68307
push userbmo:emilio+bugs@crisal.io
push dateThu, 13 Jul 2017 19:52:38 +0000
reviewersbholley
bugs1380198
milestone56.0a1
Bug 1380198: Ensure sequential tasks run after the bloom filter is dropped. r?bholley MozReview-Commit-ID: 3LjiPP7THg7
servo/components/style/context.rs
--- a/servo/components/style/context.rs
+++ b/servo/components/style/context.rs
@@ -18,17 +18,17 @@ use font_metrics::FontMetricsProvider;
 #[cfg(feature = "servo")] use parking_lot::RwLock;
 use properties::ComputedValues;
 use rule_tree::StrongRuleNode;
 use selector_parser::{EAGER_PSEUDO_COUNT, SnapshotMap};
 use selectors::matching::ElementSelectorFlags;
 use shared_lock::StylesheetGuards;
 use sharing::{ValidationData, StyleSharingCandidateCache};
 use std::fmt;
-use std::ops::Add;
+use std::ops;
 #[cfg(feature = "servo")] use std::sync::Mutex;
 #[cfg(feature = "servo")] use std::sync::mpsc::Sender;
 use stylearc::Arc;
 use stylist::Stylist;
 use thread_state;
 use time;
 use timer::Timer;
 use traversal::{DomTraversal, TraversalFlags};
@@ -303,17 +303,17 @@ pub struct TraversalStatistics {
     pub traversal_time_ms: f64,
     /// Whether this was a parallel traversal.
     pub is_parallel: Option<bool>,
     /// Whether this is a "large" traversal.
     pub is_large: Option<bool>,
 }
 
 /// Implementation of Add to aggregate statistics across different threads.
-impl<'a> Add for &'a TraversalStatistics {
+impl<'a> ops::Add for &'a TraversalStatistics {
     type Output = TraversalStatistics;
     fn add(self, other: Self) -> TraversalStatistics {
         debug_assert!(self.traversal_time_ms == 0.0 && other.traversal_time_ms == 0.0,
                       "traversal_time_ms should be set at the end by the caller");
         debug_assert!(self.selectors == 0, "set at the end");
         debug_assert!(self.revalidation_selectors == 0, "set at the end");
         debug_assert!(self.dependency_selectors == 0, "set at the end");
         debug_assert!(self.declarations == 0, "set at the end");
@@ -502,34 +502,75 @@ impl<E: TElement> SelectorFlagsMap<E> {
     pub fn apply_flags(&mut self) {
         debug_assert!(thread_state::get() == thread_state::LAYOUT);
         for (el, flags) in self.map.drain() {
             unsafe { el.set_selector_flags(flags); }
         }
     }
 }
 
+/// A list of SequentialTasks that get executed on Drop.
+pub struct SequentialTaskList<E>(Vec<SequentialTask<E>>)
+where
+    E: TElement;
+
+impl<E> ops::Deref for SequentialTaskList<E>
+where
+    E: TElement,
+{
+    type Target = Vec<SequentialTask<E>>;
+
+    fn deref(&self) -> &Self::Target {
+        &self.0
+    }
+}
+
+impl<E> ops::DerefMut for SequentialTaskList<E>
+where
+    E: TElement,
+{
+    fn deref_mut(&mut self) -> &mut Self::Target {
+        &mut self.0
+    }
+}
+
+impl<E> Drop for SequentialTaskList<E>
+where
+    E: TElement,
+{
+    fn drop(&mut self) {
+        debug_assert!(thread_state::get() == thread_state::LAYOUT);
+        for task in self.0.drain(..) {
+            task.execute()
+        }
+    }
+}
+
 /// A thread-local style context.
 ///
 /// This context contains data that needs to be used during restyling, but is
 /// not required to be unique among worker threads, so we create one per worker
 /// thread in order to be able to mutate it without locking.
 pub struct ThreadLocalStyleContext<E: TElement> {
     /// A cache to share style among siblings.
     pub style_sharing_candidate_cache: StyleSharingCandidateCache<E>,
     /// The bloom filter used to fast-reject selector-matching.
     pub bloom_filter: StyleBloom<E>,
     /// A channel on which new animations that have been triggered by style
     /// recalculation can be sent.
     #[cfg(feature = "servo")]
     pub new_animations_sender: Sender<Animation>,
     /// A set of tasks to be run (on the parent thread) in sequential mode after
-    /// the rest of the styling is complete. This is useful for infrequently-needed
-    /// non-threadsafe operations.
-    pub tasks: Vec<SequentialTask<E>>,
+    /// the rest of the styling is complete. This is useful for
+    /// infrequently-needed non-threadsafe operations.
+    ///
+    /// It's important that goes after the style sharing cache and the bloom
+    /// filter, to ensure they're dropped before we execute the tasks, which
+    /// could create another ThreadLocalStyleContext for style computation.
+    pub tasks: SequentialTaskList<E>,
     /// ElementSelectorFlags that need to be applied after the traversal is
     /// complete. This map is used in cases where the matching algorithm needs
     /// to set flags on elements it doesn't have exclusive access to (i.e. other
     /// than the current element).
     pub selector_flags: SelectorFlagsMap<E>,
     /// Statistics about the traversal.
     pub statistics: TraversalStatistics,
     /// Information related to the current element, non-None during processing.
@@ -542,31 +583,31 @@ pub struct ThreadLocalStyleContext<E: TE
 impl<E: TElement> ThreadLocalStyleContext<E> {
     /// Creates a new `ThreadLocalStyleContext` from a shared one.
     #[cfg(feature = "servo")]
     pub fn new(shared: &SharedStyleContext) -> Self {
         ThreadLocalStyleContext {
             style_sharing_candidate_cache: StyleSharingCandidateCache::new(),
             bloom_filter: StyleBloom::new(),
             new_animations_sender: shared.local_context_creation_data.lock().unwrap().new_animations_sender.clone(),
-            tasks: Vec::new(),
+            tasks: SequentialTaskList(Vec::new()),
             selector_flags: SelectorFlagsMap::new(),
             statistics: TraversalStatistics::default(),
             current_element_info: None,
             font_metrics_provider: E::FontMetricsProvider::create_from(shared),
         }
     }
 
     #[cfg(feature = "gecko")]
     /// Creates a new `ThreadLocalStyleContext` from a shared one.
     pub fn new(shared: &SharedStyleContext) -> Self {
         ThreadLocalStyleContext {
             style_sharing_candidate_cache: StyleSharingCandidateCache::new(),
             bloom_filter: StyleBloom::new(),
-            tasks: Vec::new(),
+            tasks: SequentialTaskList(Vec::new()),
             selector_flags: SelectorFlagsMap::new(),
             statistics: TraversalStatistics::default(),
             current_element_info: None,
             font_metrics_provider: E::FontMetricsProvider::create_from(shared),
         }
     }
 
     /// Notes when the style system starts traversing an element.
@@ -599,21 +640,16 @@ impl<E: TElement> ThreadLocalStyleContex
 
 impl<E: TElement> Drop for ThreadLocalStyleContext<E> {
     fn drop(&mut self) {
         debug_assert!(self.current_element_info.is_none());
         debug_assert!(thread_state::get() == thread_state::LAYOUT);
 
         // Apply any slow selector flags that need to be set on parents.
         self.selector_flags.apply_flags();
-
-        // Execute any enqueued sequential tasks.
-        for task in self.tasks.drain(..) {
-            task.execute();
-        }
     }
 }
 
 /// A `StyleContext` is just a simple container for a immutable reference to a
 /// shared style context, and a mutable reference to a local one.
 pub struct StyleContext<'a, E: TElement + 'a> {
     /// The shared style context reference.
     pub shared: &'a SharedStyleContext<'a>,