Bug 1616691 - Properly reject numbers as part of <length-percentage>. r=heycam

We never fast-reject numbers (because they could be part of a product). Without
this refactoring we'd accept stuff like calc(10) and crash during the evaluation
for obvious reasons.

Differential Revision: https://phabricator.services.mozilla.com/D63401

--HG--
extra : moz-landing-system : lando

servo/components/style/values/computed/length_percentage.rs

@@ -242,7 +242,7 @@ impl LengthPercentage {
         mut node: CalcNode,
         clamping_mode: AllowedNumericType,
     ) -> Self {
-        node.simplify_and_sort_children();
+        node.simplify_and_sort();
 
         match node {
             CalcNode::Leaf(l) => {

servo/components/style/values/generics/calc.rs

@@ -257,11 +257,45 @@ impl<L: CalcNodeLeaf> CalcNode<L> {
         }
     }
 
-    /// Simplifies and sorts the calculation. This is only needed if it's going
+    /// Visits all the nodes in this calculation tree recursively, starting by
-    /// to be preserved after parsing (so, for `<length-percentage>`). Otherwise
+    /// the leaves and bubbling all the way up.
-    /// we can just evaluate it and we'll come up with a simplified value
+    ///
-    /// anyways.
+    /// This is useful for simplification, but can also be used for validation
-    pub fn simplify_and_sort_children(&mut self) {
+    /// and such.
+    pub fn visit_depth_first(&mut self, mut f: impl FnMut(&mut Self)) {
+        self.visit_depth_first_internal(&mut f);
+    }
+
+    fn visit_depth_first_internal(&mut self, f: &mut impl FnMut(&mut Self)) {
+        match *self {
+            Self::Clamp {
+                ref mut min,
+                ref mut center,
+                ref mut max,
+            } => {
+                min.visit_depth_first_internal(f);
+                center.visit_depth_first_internal(f);
+                max.visit_depth_first_internal(f);
+            },
+            Self::Sum(ref mut children) | Self::MinMax(ref mut children, _) => {
+                for child in &mut **children {
+                    child.visit_depth_first_internal(f);
+                }
+            },
+            Self::Leaf(..) => {},
+        }
+        f(self);
+    }
+
+    /// Simplifies and sorts the calculation of a given node. All the nodes
+    /// below it should be simplified already, this only takes care of
+    /// simplifying directly nested nodes. So, probably should always be used in
+    /// combination with `visit_depth_first()`.
+    ///
+    /// This is only needed if it's going to be preserved after parsing (so, for
+    /// `<length-percentage>`). Otherwise we can just evaluate it using
+    /// `resolve()`, and we'll come up with a simplified value anyways.
+    pub fn simplify_and_sort_direct_children(&mut self) {
         macro_rules! replace_self_with {
             ($slot:expr) => {{
                 let dummy = Self::MinMax(Default::default(), MinMaxOp::Max);
@@ -275,10 +309,6 @@ impl<L: CalcNodeLeaf> CalcNode<L> {
                 ref mut center,
                 ref mut max,
             } => {
-                min.simplify_and_sort_children();
-                center.simplify_and_sort_children();
-                max.simplify_and_sort_children();
-
                 // NOTE: clamp() is max(min, min(center, max))
                 let min_cmp_center = match min.partial_cmp(&center) {
                     Some(o) => o,
@@ -323,10 +353,6 @@ impl<L: CalcNodeLeaf> CalcNode<L> {
                 return replace_self_with!(&mut **center);
             },
             Self::MinMax(ref mut children, op) => {
-                for child in &mut **children {
-                    child.simplify_and_sort_children();
-                }
-
                 let winning_order = match op {
                     MinMaxOp::Min => cmp::Ordering::Less,
                     MinMaxOp::Max => cmp::Ordering::Greater,
@@ -355,9 +381,8 @@ impl<L: CalcNodeLeaf> CalcNode<L> {
             Self::Sum(ref mut children_slot) => {
                 let mut sums_to_merge = SmallVec::<[_; 3]>::new();
                 let mut extra_kids = 0;
-                for (i, child) in children_slot.iter_mut().enumerate() {
+                for (i, child) in children_slot.iter().enumerate() {
-                    child.simplify_and_sort_children();
+                    if let Self::Sum(ref children) = *child {
-                    if let Self::Sum(ref mut children) = *child {
                         extra_kids += children.len();
                         sums_to_merge.push(i);
                     }
@@ -411,6 +436,11 @@ impl<L: CalcNodeLeaf> CalcNode<L> {
         }
     }
 
+    /// Simplifies and sorts the kids in the whole calculation subtree.
+    pub fn simplify_and_sort(&mut self) {
+        self.visit_depth_first(|node| node.simplify_and_sort_direct_children())
+    }
+
     fn to_css_impl<W>(&self, dest: &mut CssWriter<W>, is_outermost: bool) -> fmt::Result
     where
         W: Write,

servo/components/style/values/specified/calc.rs

@@ -446,13 +446,26 @@ impl CalcNode {
         Ok(node)
     }
 
-    /// Tries to simplify this expression into a `<length>` or `<percentage`>
+    /// Tries to simplify this expression into a `<length>` or `<percentage>`
     /// value.
     fn into_length_or_percentage(
         mut self,
         clamping_mode: AllowedNumericType,
     ) -> Result<CalcLengthPercentage, ()> {
-        self.simplify_and_sort_children();
+        // Keep track of whether there's any invalid member of the calculation,
+        // so as to reject the calculation properly at parse-time.
+        let mut any_invalid = false;
+        self.visit_depth_first(|node| {
+            if let CalcNode::Leaf(ref l) = *node {
+                any_invalid |= !matches!(*l, Leaf::Percentage(..) | Leaf::Length(..));
+            }
+            node.simplify_and_sort_direct_children();
+        });
+
+        if any_invalid {
+            return Err(());
+        }
+
         Ok(CalcLengthPercentage {
             clamping_mode,
             node: self,