Auto merge of #17447 - emilio:root-disconnected-subtree, r=heycam,Manishearth

style: Be more strict when setting the root font size.

Before this commit, we assumed that if the element had no parent element, it was
the root of the document, which is plain false, since we can arrive there from,
let's say, getComputedStyle on a detached node.

Bug: 1374062
Reviewed-By: heycam
MozReview-Commit-ID: 65DxdzXgd0J

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

components/style/animation.rs

@@ -502,7 +502,6 @@ fn compute_style_for_animation_step(context: &SharedStyleContext,
             // as existing browsers don't appear to animate visited styles.
             let computed =
                 properties::apply_declarations(context.stylist.device(),
-                                               /* is_root = */ false,
                                                iter,
                                                previous_style,
                                                previous_style,

components/style/dom.rs

@@ -14,6 +14,7 @@ use atomic_refcell::{AtomicRef, AtomicRefCell, AtomicRefMut};
 use data::ElementData;
 use element_state::ElementState;
 use font_metrics::FontMetricsProvider;
+use media_queries::Device;
 use properties::{ComputedValues, PropertyDeclarationBlock};
 #[cfg(feature = "gecko")] use properties::animated_properties::AnimationValue;
 #[cfg(feature = "gecko")] use properties::animated_properties::TransitionProperty;
@@ -304,6 +305,13 @@ pub trait TElement : Eq + PartialEq + Debug + Hash + Sized + Copy + Clone +
     /// Get this element as a node.
     fn as_node(&self) -> Self::ConcreteNode;
 
+    /// A debug-only check that the device's owner doc matches the actual doc
+    /// we're the root of.
+    ///
+    /// Otherwise we may set document-level state incorrectly, like the root
+    /// font-size used for rem units.
+    fn owner_doc_matches_for_testing(&self, _: &Device) -> bool { true }
+
     /// Returns the depth of this element in the DOM.
     fn depth(&self) -> usize {
         let mut depth = 0;

components/style/gecko/data.rs

@@ -46,7 +46,7 @@ impl PerDocumentStyleData {
     pub fn new(pres_context: RawGeckoPresContextOwned) -> Self {
         let device = Device::new(pres_context);
         let quirks_mode = unsafe {
-            (*(*device.pres_context).mDocument.raw::<nsIDocument>()).mCompatMode
+            (*device.pres_context().mDocument.raw::<nsIDocument>()).mCompatMode
         };
 
         PerDocumentStyleData(AtomicRefCell::new(PerDocumentStyleDataImpl {

components/style/gecko/media_queries.rs

@@ -14,7 +14,7 @@ use gecko_bindings::bindings;
 use gecko_bindings::structs::{nsCSSKeyword, nsCSSProps_KTableEntry, nsCSSValue, nsCSSUnit, nsStringBuffer};
 use gecko_bindings::structs::{nsMediaExpression_Range, nsMediaFeature};
 use gecko_bindings::structs::{nsMediaFeature_ValueType, nsMediaFeature_RangeType, nsMediaFeature_RequirementFlags};
-use gecko_bindings::structs::RawGeckoPresContextOwned;
+use gecko_bindings::structs::{nsPresContext, RawGeckoPresContextOwned};
 use media_queries::MediaType;
 use parser::ParserContext;
 use properties::{ComputedValues, StyleBuilder};
@@ -36,7 +36,7 @@ pub struct Device {
     /// NB: The pres context lifetime is tied to the styleset, who owns the
     /// stylist, and thus the `Device`, so having a raw pres context pointer
     /// here is fine.
-    pub pres_context: RawGeckoPresContextOwned,
+    pres_context: RawGeckoPresContextOwned,
     default_values: Arc<ComputedValues>,
     viewport_override: Option<ViewportConstraints>,
     /// The font size of the root element
@@ -105,11 +105,16 @@ impl Device {
         self.root_font_size.store(size.0 as isize, Ordering::Relaxed)
     }
 
+    /// Gets the pres context associated with this document.
+    pub fn pres_context(&self) -> &nsPresContext {
+        unsafe { &*self.pres_context }
+    }
+
     /// Recreates the default computed values.
     pub fn reset_computed_values(&mut self) {
         // NB: A following stylesheet flush will populate this if appropriate.
         self.viewport_override = None;
-        self.default_values = ComputedValues::default_values(unsafe { &*self.pres_context });
+        self.default_values = ComputedValues::default_values(self.pres_context());
         self.used_root_font_size.store(false, Ordering::Relaxed);
     }
 
@@ -134,10 +139,10 @@ impl Device {
             // FIXME(emilio): Gecko allows emulating random media with
             // mIsEmulatingMedia / mMediaEmulated . Refactor both sides so that
             // is supported (probably just making MediaType an Atom).
-            if (*self.pres_context).mMedium == atom!("screen").as_ptr() {
+            if self.pres_context().mMedium == atom!("screen").as_ptr() {
                 MediaType::Screen
             } else {
-                debug_assert!((*self.pres_context).mMedium == atom!("print").as_ptr());
+                debug_assert!(self.pres_context().mMedium == atom!("print").as_ptr());
                 MediaType::Print
             }
         }
@@ -150,19 +155,19 @@ impl Device {
                         Au::from_f32_px(v.size.height))
         }).unwrap_or_else(|| unsafe {
             // TODO(emilio): Need to take into account scrollbars.
-            Size2D::new(Au((*self.pres_context).mVisibleArea.width),
+            let area = &self.pres_context().mVisibleArea;
-                        Au((*self.pres_context).mVisibleArea.height))
+            Size2D::new(Au(area.width), Au(area.height))
         })
     }
 
     /// Returns whether document colors are enabled.
     pub fn use_document_colors(&self) -> bool {
-        unsafe { (*self.pres_context).mUseDocumentColors() != 0 }
+        self.pres_context().mUseDocumentColors() != 0
     }
 
     /// Returns the default background color.
     pub fn default_background_color(&self) -> RGBA {
-        convert_nscolor_to_rgba(unsafe { (*self.pres_context).mBackgroundColor })
+        convert_nscolor_to_rgba(self.pres_context().mBackgroundColor)
     }
 }
 

components/style/gecko/values.rs

@@ -401,7 +401,7 @@ impl CounterStyleOrNone {
     pub fn to_gecko_value(self, gecko_value: &mut CounterStylePtr, device: &Device) {
         use gecko_bindings::bindings::Gecko_SetCounterStyleToName as set_name;
         use gecko_bindings::bindings::Gecko_SetCounterStyleToSymbols as set_symbols;
-        let pres_context = unsafe { &*device.pres_context };
+        let pres_context = device.pres_context();
         match self {
             CounterStyleOrNone::None => unsafe {
                 set_name(gecko_value, atom!("none").into_addrefed(), pres_context);

components/style/gecko/wrapper.rs

@@ -672,7 +672,7 @@ impl FontMetricsProvider for GeckoFontMetricsProvider {
              in_media_query: bool, device: &Device) -> FontMetricsQueryResult {
         use gecko_bindings::bindings::Gecko_GetFontMetrics;
         let gecko_metrics = unsafe {
-            Gecko_GetFontMetrics(&*device.pres_context,
+            Gecko_GetFontMetrics(device.pres_context(),
                                  wm.is_vertical() && !wm.is_sideways(),
                                  font.gecko(),
                                  font_size.0,
@@ -771,6 +771,11 @@ impl<'le> TElement for GeckoElement<'le> {
         unsafe { GeckoNode(&*(self.0 as *const _ as *const RawGeckoNode)) }
     }
 
+    fn owner_doc_matches_for_testing(&self, device: &Device) -> bool {
+        self.as_node().owner_doc() as *const structs::nsIDocument ==
+            device.pres_context().mDocument.raw::<structs::nsIDocument>()
+    }
+
     fn style_attribute(&self) -> Option<&Arc<Locked<PropertyDeclarationBlock>>> {
         if !self.may_have_style_attribute() {
             return None;

components/style/matching.rs

@@ -17,8 +17,8 @@ use invalidation::element::restyle_hints::{RESTYLE_CSS_ANIMATIONS, RESTYLE_CSS_T
 use invalidation::element::restyle_hints::{RESTYLE_SMIL, RESTYLE_STYLE_ATTRIBUTE};
 use invalidation::element::restyle_hints::RestyleHint;
 use log::LogLevel::Trace;
-use properties::{ALLOW_SET_ROOT_FONT_SIZE, PROHIBIT_DISPLAY_CONTENTS, SKIP_ROOT_AND_ITEM_BASED_DISPLAY_FIXUP};
 use properties::{AnimationRules, CascadeFlags, ComputedValues};
+use properties::{IS_ROOT_ELEMENT, PROHIBIT_DISPLAY_CONTENTS, SKIP_ROOT_AND_ITEM_BASED_DISPLAY_FIXUP};
 use properties::{VISITED_DEPENDENT_ONLY, cascade};
 use properties::longhands::display::computed_value as display;
 use rule_tree::{CascadeLevel, StrongRuleNode};
@@ -33,7 +33,7 @@ use stylist::RuleInclusion;
 ///
 /// Controls where we inherit styles from, and whether display:contents is
 /// prohibited.
-#[derive(PartialEq, Copy, Clone)]
+#[derive(PartialEq, Copy, Clone, Debug)]
 enum CascadeTarget {
     /// Inherit from the parent element, as normal CSS dictates, _or_ from the
     /// closest non-Native Anonymous element in case this is Native Anonymous
@@ -273,8 +273,9 @@ trait PrivateMatchMethods: TElement {
         }
         if self.is_native_anonymous() || cascade_target == CascadeTarget::EagerPseudo {
             cascade_flags.insert(PROHIBIT_DISPLAY_CONTENTS);
-        } else {
+        } else if self.is_root() {
-            cascade_flags.insert(ALLOW_SET_ROOT_FONT_SIZE);
+            debug_assert!(self.owner_doc_matches_for_testing(shared_context.stylist.device()));
+            cascade_flags.insert(IS_ROOT_ELEMENT);
         }
 
         // Grab the inherited values.

components/style/properties/gecko.mako.rs

@@ -1551,7 +1551,7 @@ fn static_assert() {
 
     pub fn fixup_none_generic(&mut self, device: &Device) {
         unsafe {
-            bindings::Gecko_nsStyleFont_FixupNoneGeneric(&mut self.gecko, &*device.pres_context)
+            bindings::Gecko_nsStyleFont_FixupNoneGeneric(&mut self.gecko, device.pres_context())
         }
     }
 
@@ -1621,7 +1621,7 @@ fn static_assert() {
     }
 
     pub fn fixup_font_min_size(&mut self, device: &Device) {
-        unsafe { bindings::Gecko_nsStyleFont_FixupMinFontSize(&mut self.gecko, &*device.pres_context) }
+        unsafe { bindings::Gecko_nsStyleFont_FixupMinFontSize(&mut self.gecko, device.pres_context()) }
     }
 
     pub fn apply_unconstrained_font_size(&mut self, v: Au) {

components/style/properties/longhand/color.mako.rs

@@ -108,7 +108,7 @@
             fn to_computed_value(&self, cx: &Context) -> Self::ComputedValue {
                 unsafe {
                     Gecko_GetLookAndFeelSystemColor(*self as i32,
-                                                    &*cx.device.pres_context)
+                                                    cx.device.pres_context())
                 }
             }
 

components/style/properties/longhand/font.mako.rs

@@ -2388,9 +2388,12 @@ ${helpers.single_keyword("-moz-math-variant",
 
                 let mut system: nsFont = unsafe { mem::uninitialized() };
                 unsafe {
-                    bindings::Gecko_nsFont_InitSystem(&mut system, id as i32,
+                    bindings::Gecko_nsFont_InitSystem(
+                        &mut system,
+                        id as i32,
                         cx.style.get_font().gecko(),
-                                            &*cx.device.pres_context)
+                        cx.device.pres_context()
+                    )
                 }
                 let family = system.fontlist.mFontlist.iter().map(|font| {
                     use properties::longhands::font_family::computed_value::*;

components/style/properties/properties.mako.rs

@@ -2472,20 +2472,24 @@ bitflags! {
         /// Whether to inherit all styles from the parent. If this flag is not
         /// present, non-inherited styles are reset to their initial values.
         const INHERIT_ALL = 0x01,
+
         /// Whether to skip any root element and flex/grid item display style
         /// fixup.
         const SKIP_ROOT_AND_ITEM_BASED_DISPLAY_FIXUP = 0x02,
+
         /// Whether to only cascade properties that are visited dependent.
         const VISITED_DEPENDENT_ONLY = 0x04,
-        /// Should we modify the device's root font size
+
-        /// when computing the root?
+        /// Whether the given element we're styling is the document element,
+        /// that is, matches :root.
         ///
-        /// Not set for native anonymous content since some NAC
+        /// Not set for native anonymous content since some NAC form their own
-        /// form their own root, but share the device.
+        /// root, but share the device.
         ///
-        /// ::backdrop and all NAC will resolve rem units against
+        /// This affects some style adjustments, like blockification, and means
-        /// the toplevel root element now.
+        /// that it may affect global state, like the Device's root font-size.
-        const ALLOW_SET_ROOT_FONT_SIZE = 0x08,
+        const IS_ROOT_ELEMENT = 0x08,
+
         /// Whether to convert display:contents into display:inline.  This
         /// is used by Gecko to prevent display:contents on generated
         /// content.
@@ -2520,15 +2524,13 @@ pub fn cascade(device: &Device,
                quirks_mode: QuirksMode)
                -> ComputedValues {
     debug_assert_eq!(parent_style.is_some(), layout_parent_style.is_some());
-    let (is_root_element, inherited_style, layout_parent_style) = match parent_style {
+    let (inherited_style, layout_parent_style) = match parent_style {
         Some(parent_style) => {
-            (false,
+            (parent_style,
-             parent_style,
              layout_parent_style.unwrap())
         },
         None => {
-            (true,
+            (device.default_computed_values(),
-             device.default_computed_values(),
              device.default_computed_values())
         }
     };
@@ -2558,7 +2560,6 @@ pub fn cascade(device: &Device,
         })
     };
     apply_declarations(device,
-                       is_root_element,
                        iter_declarations,
                        inherited_style,
                        layout_parent_style,
@@ -2574,7 +2575,6 @@ pub fn cascade(device: &Device,
 /// first.
 #[allow(unused_mut)] // conditionally compiled code for "position"
 pub fn apply_declarations<'a, F, I>(device: &Device,
-                                    is_root_element: bool,
                                     iter_declarations: F,
                                     inherited_style: &ComputedValues,
                                     layout_parent_style: &ComputedValues,
@@ -2629,7 +2629,7 @@ pub fn apply_declarations<'a, F, I>(device: &Device,
     };
 
     let mut context = computed::Context {
-        is_root_element: is_root_element,
+        is_root_element: flags.contains(IS_ROOT_ELEMENT),
         device: device,
         inherited_style: inherited_style,
         layout_parent_style: layout_parent_style,
@@ -2783,13 +2783,14 @@ pub fn apply_declarations<'a, F, I>(device: &Device,
                     // which Gecko just does regular cascading with. Do the same.
                     // This can only happen in the case where the language changed but the family did not
                     if generic != structs::kGenericFont_NONE {
-                        let pres_context = context.device.pres_context;
+                        let gecko_font = context.style.mutate_font().gecko_mut();
-                        let gecko_font = context.mutate_style().mutate_font().gecko_mut();
                         gecko_font.mGenericID = generic;
                         unsafe {
-                            bindings::Gecko_nsStyleFont_PrefillDefaultForGeneric(gecko_font,
+                            bindings::Gecko_nsStyleFont_PrefillDefaultForGeneric(
-                                                                                 &*pres_context,
+                                gecko_font,
-                                                                                 generic);
+                                context.device.pres_context(),
+                                generic
+                            );
                         }
                     }
                 }
@@ -2853,7 +2854,7 @@ pub fn apply_declarations<'a, F, I>(device: &Device,
             % endif
             }
 
-            if is_root_element && flags.contains(ALLOW_SET_ROOT_FONT_SIZE) {
+            if context.is_root_element {
                 let s = context.style.get_font().clone_font_size();
                 context.device.set_root_font_size(s);
             }
@@ -2863,7 +2864,7 @@ pub fn apply_declarations<'a, F, I>(device: &Device,
     let mut style = context.style;
 
     {
-        StyleAdjuster::new(&mut style, is_root_element)
+        StyleAdjuster::new(&mut style)
             .adjust(context.layout_parent_style, flags);
     }
 

components/style/style_adjuster.rs

@@ -7,7 +7,7 @@
 
 use app_units::Au;
 use properties::{self, CascadeFlags, ComputedValues};
-use properties::{SKIP_ROOT_AND_ITEM_BASED_DISPLAY_FIXUP, StyleBuilder};
+use properties::{IS_ROOT_ELEMENT, SKIP_ROOT_AND_ITEM_BASED_DISPLAY_FIXUP, StyleBuilder};
 use properties::longhands::display::computed_value::T as display;
 use properties::longhands::float::computed_value::T as float;
 use properties::longhands::overflow_x::computed_value::T as overflow;
@@ -17,15 +17,13 @@ use properties::longhands::position::computed_value::T as position;
 /// An unsized struct that implements all the adjustment methods.
 pub struct StyleAdjuster<'a, 'b: 'a> {
     style: &'a mut StyleBuilder<'b>,
-    is_root_element: bool,
 }
 
 impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> {
     /// Trivially constructs a new StyleAdjuster.
-    pub fn new(style: &'a mut StyleBuilder<'b>, is_root_element: bool) -> Self {
+    pub fn new(style: &'a mut StyleBuilder<'b>) -> Self {
         StyleAdjuster {
             style: style,
-            is_root_element: is_root_element,
         }
     }
 
@@ -66,7 +64,7 @@ impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> {
         }
 
         if !flags.contains(SKIP_ROOT_AND_ITEM_BASED_DISPLAY_FIXUP) {
-            blockify_if!(self.is_root_element);
+            blockify_if!(flags.contains(IS_ROOT_ELEMENT));
             blockify_if!(layout_parent_style.get_box().clone_display().is_item_container());
         }
 
@@ -81,7 +79,7 @@ impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> {
 
         let display = self.style.get_box().clone_display();
         let blockified_display =
-            display.equivalent_block_display(self.is_root_element);
+            display.equivalent_block_display(flags.contains(IS_ROOT_ELEMENT));
         if display != blockified_display {
             self.style.mutate_box().set_adjusted_display(blockified_display,
                                                          is_item_or_root);

components/style/stylesheets/document_rule.rs

@@ -141,7 +141,7 @@ impl UrlMatchingFunction {
             UrlMatchingFunction::RegExp(ref pat) => pat,
         });
         unsafe {
-            Gecko_DocumentRule_UseForPresentation(&*device.pres_context, &*pattern, func)
+            Gecko_DocumentRule_UseForPresentation(device.pres_context(), &*pattern, func)
         }
     }
 

components/style/values/specified/color.rs

@@ -254,7 +254,7 @@ impl ToComputedValue for Color {
             #[cfg(feature = "gecko")]
             Color::Special(special) => {
                 use self::gecko::SpecialColorKeyword as Keyword;
-                let pres_context = unsafe { &*_context.device.pres_context };
+                let pres_context = _context.device.pres_context();
                 convert_nscolor_to_computedcolor(match special {
                     Keyword::MozDefaultColor => pres_context.mDefaultColor,
                     Keyword::MozDefaultBackgroundColor => pres_context.mBackgroundColor,
@@ -268,7 +268,7 @@ impl ToComputedValue for Color {
                 use dom::TElement;
                 use gecko::wrapper::GeckoElement;
                 use gecko_bindings::bindings::Gecko_GetBody;
-                let pres_context = unsafe { &*_context.device.pres_context };
+                let pres_context = _context.device.pres_context();
                 let body = unsafe {
                     Gecko_GetBody(pres_context)
                 };

components/style/values/specified/length.rs

@@ -351,8 +351,7 @@ impl PhysicalLength {
         const MM_PER_INCH: f32 = 25.4;
 
         let physical_inch = unsafe {
-            let pres_context = &*context.device.pres_context;
+            bindings::Gecko_GetAppUnitsPerPhysicalInch(context.device.pres_context())
-            bindings::Gecko_GetAppUnitsPerPhysicalInch(&pres_context)
         };
 
         let inch = self.0 / MM_PER_INCH;

ports/geckolib/glue.rs

@@ -1546,7 +1546,7 @@ pub extern "C" fn Servo_ComputedValues_Inherit(
             StyleBuilder::for_inheritance(reference,
                                           &data.default_computed_values());
         if for_text {
-            StyleAdjuster::new(&mut style, /* is_root = */ false)
+            StyleAdjuster::new(&mut style)
                 .adjust_for_text();
         }
 
@@ -1608,8 +1608,8 @@ pub extern "C" fn Servo_StyleSet_Drop(data: RawServoStyleSetOwned) {
 pub extern "C" fn Servo_StyleSet_CompatModeChanged(raw_data: RawServoStyleSetBorrowed) {
     let mut data = PerDocumentStyleData::from_ffi(raw_data).borrow_mut();
     let quirks_mode = unsafe {
-        (*(*data.stylist.device().pres_context).mDocument
+        (*data.stylist.device().pres_context().mDocument.raw::<nsIDocument>())
-                                               .raw::<nsIDocument>()).mCompatMode
+            .mCompatMode
     };
 
     data.stylist.set_quirks_mode(quirks_mode.into());