Auto merge of #18248 - bholley:more_stack_limits, r=bholley

stylo: Check stack depth in invalidation machinery and re-enable limits

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

<!-- 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/18248)
<!-- Reviewable:end -->

components/style/context.rs

@@ -16,7 +16,7 @@ use euclid::Size2D;
 use fnv::FnvHashMap;
 use font_metrics::FontMetricsProvider;
 #[cfg(feature = "gecko")] use gecko_bindings::structs;
-use parallel::STYLE_THREAD_STACK_SIZE_KB;
+use parallel::{STACK_SAFETY_MARGIN_KB, STYLE_THREAD_STACK_SIZE_KB};
 #[cfg(feature = "servo")] use parking_lot::RwLock;
 use properties::ComputedValues;
 #[cfg(feature = "servo")] use properties::PropertyId;
@@ -628,24 +628,35 @@ impl StackLimitChecker {
     pub fn limit_exceeded(&self) -> bool {
         let curr_sp = StackLimitChecker::get_sp();
 
-        // Try to assert if we're called from a different thread than the
+        // Do some sanity-checking to ensure that our invariants hold, even in
-        // one that originally created this object.  This is a bit subtle
+        // the case where we've exceeded the soft limit.
-        // and relies on wraparound behaviour of unsigned integers.
-        //
-        // * If we're called from a thread whose stack has a higher address
-        //   than the one that created this object, then
-        //   |curr_sp - self.lower_limit| will (almost certainly) be larger
-        //   than the thread stack size, so the check will fail.
-        //
-        // * If we're called from a thread whose stack has a lower address
-        //   than the one that created this object, then
-        //   |curr_sp - self.lower_limit| will be negative, which will look
-        //   like a very large unsigned value, so the check will also fail.
         //
         // The correctness of depends on the assumption that no stack wraps
         // around the end of the address space.
-        //debug_assert!(curr_sp - self.lower_limit
+        if cfg!(debug_assertions) {
-        //              <= STYLE_THREAD_STACK_SIZE_KB * 1024);
+            // Compute the actual bottom of the stack by subtracting our safety
+            // margin from our soft limit. Note that this will be slightly below
+            // the actual bottom of the stack, because there are a few initial
+            // frames on the stack before we do the measurement that computes
+            // the limit.
+            let stack_bottom = self.lower_limit - STACK_SAFETY_MARGIN_KB * 1024;
+
+            // The bottom of the stack should be below the current sp. If it
+            // isn't, that means we've either waited too long to check the limit
+            // and burned through our safety margin (in which case we probably
+            // would have segfaulted by now), or we're using a limit computed for
+            // a different thread.
+            debug_assert!(stack_bottom < curr_sp);
+
+            // Compute the distance between the current sp and the bottom of
+            // the stack, and compare it against the current stack. It should be
+            // no further from us than the total stack size. We allow some slop
+            // to handle the fact that stack_bottom is a bit further than the
+            // bottom of the stack, as discussed above.
+            let distance_to_stack_bottom = curr_sp - stack_bottom;
+            let max_allowable_distance = (STYLE_THREAD_STACK_SIZE_KB + 10) * 1024;
+            debug_assert!(distance_to_stack_bottom <= max_allowable_distance);
+        }
 
         // The actual bounds check.
         curr_sp <= self.lower_limit
@@ -714,7 +725,7 @@ impl<E: TElement> ThreadLocalStyleContext<E> {
             current_element_info: None,
             font_metrics_provider: E::FontMetricsProvider::create_from(shared),
             stack_limit_checker: StackLimitChecker::new(
-                (STYLE_THREAD_STACK_SIZE_KB - 40) * 1024),
+                (STYLE_THREAD_STACK_SIZE_KB - STACK_SAFETY_MARGIN_KB) * 1024),
         }
     }
 
@@ -729,15 +740,8 @@ impl<E: TElement> ThreadLocalStyleContext<E> {
             statistics: TraversalStatistics::default(),
             current_element_info: None,
             font_metrics_provider: E::FontMetricsProvider::create_from(shared),
-            // Threads in the styling pool have small stacks, and we have to
-            // be careful not to run out of stack during recursion in
-            // parallel.rs.  Therefore set up a stack limit checker, in
-            // which we reserve 40KB of stack as a safety buffer.  Currently
-            // the stack size is 128KB, so this allows 88KB for recursive
-            // DOM traversal, which encompasses 53 levels of recursion before
-            // the limiter kicks in, on x86_64-Linux.  See Gecko bug 1376883.
             stack_limit_checker: StackLimitChecker::new(
-                (STYLE_THREAD_STACK_SIZE_KB - 40) * 1024),
+                (STYLE_THREAD_STACK_SIZE_KB - STACK_SAFETY_MARGIN_KB) * 1024),
         }
     }
 

components/style/data.rs

@@ -4,7 +4,7 @@
 
 //! Per-node data used in style calculation.
 
-use context::SharedStyleContext;
+use context::{SharedStyleContext, StackLimitChecker};
 use dom::TElement;
 use invalidation::element::restyle_hints::RestyleHint;
 use properties::ComputedValues;
@@ -327,8 +327,9 @@ impl ElementData {
     pub fn invalidate_style_if_needed<'a, E: TElement>(
         &mut self,
         element: E,
-        shared_context: &SharedStyleContext)
+        shared_context: &SharedStyleContext,
-    {
+        stack_limit_checker: Option<&StackLimitChecker>,
+    ) {
         // In animation-only restyle we shouldn't touch snapshot at all.
         if shared_context.traversal_flags.for_animation_only() {
             return;
@@ -349,6 +350,7 @@ impl ElementData {
                 element,
                 Some(self),
                 shared_context,
+                stack_limit_checker,
             );
             invalidator.invalidate();
             unsafe { element.set_handled_snapshot() }

components/style/gecko/global_style_data.rs

@@ -9,6 +9,7 @@ use gecko_bindings::bindings;
 use gecko_bindings::bindings::{Gecko_RegisterProfilerThread, Gecko_UnregisterProfilerThread};
 use gecko_bindings::bindings::Gecko_SetJemallocThreadLocalArena;
 use num_cpus;
+use parallel::STYLE_THREAD_STACK_SIZE_KB;
 use rayon;
 use shared_lock::SharedRwLock;
 use std::cmp;
@@ -92,9 +93,8 @@ lazy_static! {
                 .breadth_first()
                 .thread_name(thread_name)
                 .start_handler(thread_startup)
-                .exit_handler(thread_shutdown);
+                .exit_handler(thread_shutdown)
-                // Set thread stack size to 128KB.  See Gecko bug 1376883.
+                .stack_size(STYLE_THREAD_STACK_SIZE_KB * 1024);
-                //.stack_size(STYLE_THREAD_STACK_SIZE_KB * 1024);
             let pool = rayon::ThreadPool::new(configuration).ok();
             pool
         };

components/style/invalidation/element/invalidator.rs

@@ -6,7 +6,7 @@
 //! element styles need to be invalidated.
 
 use Atom;
-use context::SharedStyleContext;
+use context::{SharedStyleContext, StackLimitChecker};
 use data::ElementData;
 use dom::{TElement, TNode};
 use element_state::{ElementState, IN_VISITED_OR_UNVISITED_STATE};
@@ -40,6 +40,7 @@ pub struct TreeStyleInvalidator<'a, 'b: 'a, E>
     // descendants if an element has no data, though.
     data: Option<&'a mut ElementData>,
     shared_context: &'a SharedStyleContext<'b>,
+    stack_limit_checker: Option<&'a StackLimitChecker>,
 }
 
 type InvalidationVector = SmallVec<[Invalidation; 10]>;
@@ -130,11 +131,13 @@ impl<'a, 'b: 'a, E> TreeStyleInvalidator<'a, 'b, E>
         element: E,
         data: Option<&'a mut ElementData>,
         shared_context: &'a SharedStyleContext<'b>,
+        stack_limit_checker: Option<&'a StackLimitChecker>,
     ) -> Self {
         Self {
-            element: element,
+            element,
-            data: data,
+            data,
-            shared_context: shared_context,
+            shared_context,
+            stack_limit_checker,
         }
     }
 
@@ -276,7 +279,8 @@ impl<'a, 'b: 'a, E> TreeStyleInvalidator<'a, 'b, E>
             let mut sibling_invalidator = TreeStyleInvalidator::new(
                 sibling,
                 sibling_data,
-                self.shared_context
+                self.shared_context,
+                self.stack_limit_checker,
             );
 
             let mut invalidations_for_descendants = InvalidationVector::new();
@@ -338,7 +342,8 @@ impl<'a, 'b: 'a, E> TreeStyleInvalidator<'a, 'b, E>
         let mut child_invalidator = TreeStyleInvalidator::new(
             child,
             child_data,
-            self.shared_context
+            self.shared_context,
+            self.stack_limit_checker,
         );
 
         let mut invalidations_for_descendants = InvalidationVector::new();
@@ -448,12 +453,21 @@ impl<'a, 'b: 'a, E> TreeStyleInvalidator<'a, 'b, E>
         match self.data {
             None => return false,
             Some(ref data) => {
+                // FIXME(emilio): Only needs to check RESTYLE_DESCENDANTS,
+                // really.
                 if data.restyle.hint.contains_subtree() {
                     return false;
                 }
             }
         }
 
+        if let Some(checker) = self.stack_limit_checker {
+            if checker.limit_exceeded() {
+                self.data.as_mut().unwrap().restyle.hint.insert(RESTYLE_DESCENDANTS);
+                return true;
+            }
+        }
+
         let mut any_descendant = false;
 
         if let Some(anon_content) = self.element.xbl_binding_anonymous_content() {

components/style/parallel.rs

@@ -34,6 +34,22 @@ use traversal::{DomTraversal, PerLevelTraversalData};
 /// The minimum stack size for a thread in the styling pool, in kilobytes.
 pub const STYLE_THREAD_STACK_SIZE_KB: usize = 128;
 
+/// The stack margin. If we get this deep in the stack, we will skip recursive
+/// optimizations to ensure that there is sufficient room for non-recursive work.
+///
+/// When measured with 128KB stacks and 40KB margin, we could support 53
+/// levels of recursion before the limiter kicks in, on x86_64-Linux [1].
+///
+/// We currently use 45KB margins, because that seems to be the minimum amount
+/// of head room required by an unoptimized debug build, as measured on [2]. We
+/// could probably get away with a smaller margin on optimized release builds,
+/// but that would be a pain, and the extra padding reduces stability risk.
+///
+/// [1] See Gecko bug 1376883 for more discussion on the measurements.
+/// [2] layout/style/crashtests/1383981-3.html
+///
+pub const STACK_SAFETY_MARGIN_KB: usize = 45;
+
 /// The maximum number of child nodes that we will process as a single unit.
 ///
 /// Larger values will increase style sharing cache hits and general DOM

components/style/traversal.rs

@@ -139,7 +139,7 @@ pub trait DomTraversal<E: TElement> : Sync {
     fn pre_traverse(
         root: E,
         shared_context: &SharedStyleContext,
-        traversal_flags: TraversalFlags
+        traversal_flags: TraversalFlags,
     ) -> PreTraverseToken {
         // If this is an unstyled-only traversal, the caller has already verified
         // that there's something to traverse, and we don't need to do any
@@ -155,7 +155,7 @@ pub trait DomTraversal<E: TElement> : Sync {
         if let Some(ref mut data) = data {
             // Invalidate our style, and the one of our siblings and descendants
             // as needed.
-            data.invalidate_style_if_needed(root, shared_context);
+            data.invalidate_style_if_needed(root, shared_context, None);
 
             // Make sure we don't have any stale RECONSTRUCTED_ANCESTOR bits from
             // the last traversal (at a potentially-higher root). From the
@@ -828,7 +828,11 @@ where
             // as needed.
             //
             // NB: This will be a no-op if there's no snapshot.
-            child_data.invalidate_style_if_needed(child, &context.shared);
+            child_data.invalidate_style_if_needed(
+                child,
+                &context.shared,
+                Some(&context.thread_local.stack_limit_checker)
+            );
         }
 
         if D::element_needs_traversal(child, flags, child_data.map(|d| &*d), Some(data)) {

ports/geckolib/glue.rs

@@ -3701,6 +3701,6 @@ pub extern "C" fn Servo_ProcessInvalidations(set: RawServoStyleSetBorrowed,
     let mut data = data.as_mut().map(|d| &mut **d);
 
     if let Some(ref mut data) = data {
-        data.invalidate_style_if_needed(element, &shared_style_context);
+        data.invalidate_style_if_needed(element, &shared_style_context, None);
     }
 }