Bug 1374062: Assert we never style a root element from another document. r?heycam draft
authorEmilio Cobos Álvarez <emilio@crisal.io>
Wed, 21 Jun 2017 13:01:44 +0200
changeset 598091 e5135dfd2b459b511cac638b9ae725fd03daf94e
parent 598090 0f5372c18d787b61cbe9dbcd07ed72adca080884
child 634411 7e4f1521e7eb11f57f886b15c6cea1658f72a1a8
push id65134
push userbmo:emilio+bugs@crisal.io
push dateWed, 21 Jun 2017 11:22:54 +0000
reviewersheycam
bugs1374062
milestone56.0a1
Bug 1374062: Assert we never style a root element from another document. r?heycam Right now when calling getComputedStyle with an element from another document, we return the style using the pres context of that document, not of the document of the window getComputedStyle was called on, so this holds. But it will stop holding if we ever change this, so assert it doesn't happen. MozReview-Commit-ID: 3g8yQWWdsen
servo/components/style/dom.rs
servo/components/style/gecko/data.rs
servo/components/style/gecko/media_queries.rs
servo/components/style/gecko/values.rs
servo/components/style/gecko/wrapper.rs
servo/components/style/matching.rs
servo/components/style/properties/gecko.mako.rs
servo/components/style/properties/longhand/color.mako.rs
servo/components/style/properties/longhand/font.mako.rs
servo/components/style/properties/properties.mako.rs
servo/components/style/stylesheets/document_rule.rs
servo/components/style/values/specified/color.rs
servo/components/style/values/specified/length.rs
servo/ports/geckolib/glue.rs
--- a/servo/components/style/dom.rs
+++ b/servo/components/style/dom.rs
@@ -9,16 +9,17 @@
 
 use {Atom, Namespace, LocalName};
 use applicable_declarations::ApplicableDeclarationBlock;
 use atomic_refcell::{AtomicRef, AtomicRefCell, AtomicRefMut};
 #[cfg(feature = "gecko")] use context::UpdateAnimationsTasks;
 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;
 use rule_tree::CascadeLevel;
 use selector_parser::{AttrValue, ElementExt, PreExistingComputedValues};
 use selector_parser::{PseudoClassStringArg, PseudoElement};
 use selectors::matching::{ElementSelectorFlags, VisitedHandlingMode};
 use shared_lock::Locked;
@@ -299,16 +300,23 @@ pub trait TElement : Eq + PartialEq + De
     ///
     /// XXXManishearth It would be better to make this a type parameter on
     /// ThreadLocalStyleContext and StyleContext
     type FontMetricsProvider: FontMetricsProvider;
 
     /// 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;
         let mut curr = *self;
         while let Some(parent) = curr.traversal_parent() {
             depth += 1;
             curr = parent;
         }
--- a/servo/components/style/gecko/data.rs
+++ b/servo/components/style/gecko/data.rs
@@ -41,17 +41,17 @@ pub struct PerDocumentStyleDataImpl {
 /// and unexpected races while trying to mutate it.
 pub struct PerDocumentStyleData(AtomicRefCell<PerDocumentStyleDataImpl>);
 
 impl PerDocumentStyleData {
     /// Create a dummy `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 {
             stylist: Stylist::new(device, quirks_mode.into()),
             stylesheets: StylesheetSet::new(),
             font_faces: vec![],
             counter_styles: FnvHashMap::default(),
         }))
--- a/servo/components/style/gecko/media_queries.rs
+++ b/servo/components/style/gecko/media_queries.rs
@@ -9,17 +9,17 @@ use context::QuirksMode;
 use cssparser::{CssStringWriter, Parser, RGBA, Token, BasicParseError};
 use euclid::Size2D;
 use font_metrics::get_metrics_provider_for_product;
 use gecko::values::convert_nscolor_to_rgba;
 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};
 use properties::longhands::font_size;
 use selectors::parser::SelectorParseError;
 use std::fmt::{self, Write};
 use std::sync::atomic::{AtomicBool, AtomicIsize, Ordering};
 use str::starts_with_ignore_ascii_case;
@@ -31,17 +31,17 @@ use values::{CSSFloat, specified};
 use values::computed::{self, ToComputedValue};
 
 /// The `Device` in Gecko wraps a pres context, has a default values computed,
 /// and contains all the viewport rule state.
 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
     /// This is set when computing the style of the root
     /// element, and used for rem units in other elements.
     ///
     /// When computing the style of the root element, there can't be any
     /// other style being computed at the same time, given we need the style of
@@ -100,21 +100,26 @@ impl Device {
         Au::new(self.root_font_size.load(Ordering::Relaxed) as i32)
     }
 
     /// Set the font size of the root element (for rem)
     pub fn set_root_font_size(&self, size: Au) {
         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);
     }
 
     /// Returns whether we ever looked up the root font size of the Device.
     pub fn used_root_font_size(&self) -> bool {
         self.used_root_font_size.load(Ordering::Relaxed)
     }
 
@@ -129,45 +134,45 @@ impl Device {
     }
 
     /// Returns the current media type of the device.
     pub fn media_type(&self) -> MediaType {
         unsafe {
             // 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
             }
         }
     }
 
     /// Returns the current viewport size in app units.
     pub fn au_viewport_size(&self) -> Size2D<Au> {
         self.viewport_override.as_ref().map(|v| {
             Size2D::new(Au::from_f32_px(v.size.width),
                         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),
-                        Au((*self.pres_context).mVisibleArea.height))
+            let area = &self.pres_context().mVisibleArea;
+            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)
     }
 }
 
 /// A expression for gecko contains a reference to the media feature, the value
 /// the media query contained, and the range to evaluate.
 #[derive(Debug, Clone)]
 pub struct Expression {
     feature: &'static nsMediaFeature,
--- a/servo/components/style/gecko/values.rs
+++ b/servo/components/style/gecko/values.rs
@@ -396,17 +396,17 @@ pub fn round_border_to_device_pixels(wid
     }
 }
 
 impl CounterStyleOrNone {
     /// Convert this counter style to a Gecko CounterStylePtr.
     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);
             },
             CounterStyleOrNone::Name(name) => unsafe {
                 set_name(gecko_value, name.0.into_addrefed(), pres_context);
             },
             CounterStyleOrNone::Symbols(symbols_type, symbols) => {
--- a/servo/components/style/gecko/wrapper.rs
+++ b/servo/components/style/gecko/wrapper.rs
@@ -667,17 +667,17 @@ impl FontMetricsProvider for GeckoFontMe
         cache.push((font_name.clone(), sizes));
         sizes.size_for_generic(font_family)
     }
 
     fn query(&self, font: &Font, font_size: Au, wm: WritingMode,
              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,
                                  // we don't use the user font set in a media query
                                  !in_media_query)
         };
         let metrics = FontMetrics {
             x_height: Au(gecko_metrics.mXSize),
@@ -766,16 +766,21 @@ impl<'le> TElement for GeckoElement<'le>
             };
         }
     }
 
     fn as_node(&self) -> Self::ConcreteNode {
         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;
         }
 
         let declarations = unsafe { Gecko_GetStyleAttrDeclarationBlock(self.0) };
         declarations.map_or(None, |s| s.as_arc_opt())
     }
--- a/servo/components/style/matching.rs
+++ b/servo/components/style/matching.rs
@@ -269,16 +269,17 @@ trait PrivateMatchMethods: TElement {
             cascade_flags.insert(SKIP_ROOT_AND_ITEM_BASED_DISPLAY_FIXUP)
         }
         if cascade_visited.visited_dependent_only() {
             cascade_flags.insert(VISITED_DEPENDENT_ONLY);
         }
         if self.is_native_anonymous() || cascade_target == CascadeTarget::EagerPseudo {
             cascade_flags.insert(PROHIBIT_DISPLAY_CONTENTS);
         } else if self.is_root() {
+            debug_assert!(self.owner_doc_matches_for_testing(shared_context.stylist.device()));
             cascade_flags.insert(IS_ROOT_ELEMENT);
         }
 
         // Grab the inherited values.
         let parent_el;
         let parent_data;
         let style_to_inherit_from = match cascade_target {
             CascadeTarget::Normal => {
--- a/servo/components/style/properties/gecko.mako.rs
+++ b/servo/components/style/properties/gecko.mako.rs
@@ -1546,17 +1546,17 @@ fn static_assert() {
         for (current, feature) in current_settings.iter_mut().zip(feature_settings.iter()) {
             current.mTag = feature.mTag;
             current.mValue = feature.mValue;
         }
     }
 
     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())
         }
     }
 
     pub fn set_font_family(&mut self, v: longhands::font_family::computed_value::T) {
         use properties::longhands::font_family::computed_value::FontFamily;
 
         let list = &mut self.gecko.mFont.fontlist;
         unsafe { Gecko_FontFamilyList_Clear(list); }
@@ -1616,17 +1616,17 @@ fn static_assert() {
         } else {
             self.gecko.mSize = v.0;
             self.fixup_font_min_size(device);
             Some(Au(parent.gecko.mScriptUnconstrainedSize))
         }
     }
 
     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) {
         self.gecko.mScriptUnconstrainedSize = v.0;
     }
 
     /// Calculates the constrained and unconstrained font sizes to be inherited
     /// from the parent.
--- a/servo/components/style/properties/longhand/color.mako.rs
+++ b/servo/components/style/properties/longhand/color.mako.rs
@@ -103,17 +103,17 @@
         }
 
         impl ToComputedValue for SystemColor {
             type ComputedValue = u32; // nscolor
             #[inline]
             fn to_computed_value(&self, cx: &Context) -> Self::ComputedValue {
                 unsafe {
                     Gecko_GetLookAndFeelSystemColor(*self as i32,
-                                                    &*cx.device.pres_context)
+                                                    cx.device.pres_context())
                 }
             }
 
             #[inline]
             fn from_computed_value(_: &Self::ComputedValue) -> Self {
                 unreachable!()
             }
         }
--- a/servo/components/style/properties/longhand/font.mako.rs
+++ b/servo/components/style/properties/longhand/font.mako.rs
@@ -2383,19 +2383,22 @@ https://drafts.csswg.org/css-fonts-4/#lo
                         SystemFont::${to_camel_case(font)} => {
                             LookAndFeel_FontID::eFont_${to_camel_case(font.replace("-moz-", ""))}
                         }
                     % endfor
                 };
 
                 let mut system: nsFont = unsafe { mem::uninitialized() };
                 unsafe {
-                    bindings::Gecko_nsFont_InitSystem(&mut system, id as i32,
-                                            cx.style.get_font().gecko(),
-                                            &*cx.device.pres_context)
+                    bindings::Gecko_nsFont_InitSystem(
+                        &mut system,
+                        id as i32,
+                        cx.style.get_font().gecko(),
+                        cx.device.pres_context()
+                    )
                 }
                 let family = system.fontlist.mFontlist.iter().map(|font| {
                     use properties::longhands::font_family::computed_value::*;
                     FontFamily::FamilyName(FamilyName {
                         name: (&*font.mName).into(),
                         quoted: true
                     })
                 }).collect::<Vec<_>>();
--- a/servo/components/style/properties/properties.mako.rs
+++ b/servo/components/style/properties/properties.mako.rs
@@ -2774,23 +2774,24 @@ pub fn apply_declarations<'a, F, I>(devi
                             }
                         }
                     }
 
                     // In case of just the language changing, the parent could have had no generic,
                     // 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.mutate_style().mutate_font().gecko_mut();
+                        let gecko_font = context.style.mutate_font().gecko_mut();
                         gecko_font.mGenericID = generic;
                         unsafe {
-                            bindings::Gecko_nsStyleFont_PrefillDefaultForGeneric(gecko_font,
-                                                                                 &*pres_context,
-                                                                                 generic);
+                            bindings::Gecko_nsStyleFont_PrefillDefaultForGeneric(
+                                gecko_font,
+                                context.device.pres_context(),
+                                generic
+                            );
                         }
                     }
                 }
             % endif
 
             // It is important that font_size is computed before
             // the late properties (for em units), but after font-family
             // (for the base-font-size dependence for default and keyword font-sizes)
--- a/servo/components/style/stylesheets/document_rule.rs
+++ b/servo/components/style/stylesheets/document_rule.rs
@@ -136,17 +136,17 @@ impl UrlMatchingFunction {
 
         let pattern = nsCString::from(match *self {
             UrlMatchingFunction::Url(ref url) => url.as_str(),
             UrlMatchingFunction::UrlPrefix(ref pat) |
             UrlMatchingFunction::Domain(ref pat) |
             UrlMatchingFunction::RegExp(ref pat) => pat,
         });
         unsafe {
-            Gecko_DocumentRule_UseForPresentation(&*device.pres_context, &*pattern, func)
+            Gecko_DocumentRule_UseForPresentation(device.pres_context(), &*pattern, func)
         }
     }
 
     #[cfg(not(feature = "gecko"))]
     /// Evaluate a URL matching function.
     pub fn evaluate(&self, _: &Device) -> bool {
         false
     }
--- a/servo/components/style/values/specified/color.rs
+++ b/servo/components/style/values/specified/color.rs
@@ -249,31 +249,31 @@ impl ToComputedValue for Color {
             Color::Numeric { ref parsed, .. } => ComputedColor::rgba(*parsed),
             Color::Complex(ref complex) => *complex,
             #[cfg(feature = "gecko")]
             Color::System(system) =>
                 convert_nscolor_to_computedcolor(system.to_computed_value(_context)),
             #[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,
                     Keyword::MozHyperlinktext => pres_context.mLinkColor,
                     Keyword::MozActiveHyperlinktext => pres_context.mActiveLinkColor,
                     Keyword::MozVisitedHyperlinktext => pres_context.mVisitedLinkColor,
                 })
             }
             #[cfg(feature = "gecko")]
             Color::InheritFromBodyQuirk => {
                 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)
                 };
                 if let Some(body) = body {
                     let wrap = GeckoElement(body);
                     let borrow = wrap.borrow_data();
                     ComputedColor::rgba(borrow.as_ref().unwrap()
                                               .styles().primary.values()
--- a/servo/components/style/values/specified/length.rs
+++ b/servo/components/style/values/specified/length.rs
@@ -346,18 +346,17 @@ impl PhysicalLength {
 
     /// Computes the given character width.
     pub fn to_computed_value(&self, context: &Context) -> Au {
         use gecko_bindings::bindings;
         // Same as Gecko
         const MM_PER_INCH: f32 = 25.4;
 
         let physical_inch = unsafe {
-            let pres_context = &*context.device.pres_context;
-            bindings::Gecko_GetAppUnitsPerPhysicalInch(&pres_context)
+            bindings::Gecko_GetAppUnitsPerPhysicalInch(context.device.pres_context())
         };
 
         let inch = self.0 / MM_PER_INCH;
 
         to_au_round(inch, physical_inch as f32)
     }
 }
 
--- a/servo/ports/geckolib/glue.rs
+++ b/servo/ports/geckolib/glue.rs
@@ -1592,18 +1592,18 @@ pub extern "C" fn Servo_StyleSet_Drop(da
 
 /// Updating the stylesheets and redoing selector matching is always happens
 /// before the document element is inserted. Therefore we don't need to call
 /// `force_dirty` here.
 #[no_mangle]
 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
-                                               .raw::<nsIDocument>()).mCompatMode
+        (*data.stylist.device().pres_context().mDocument.raw::<nsIDocument>())
+            .mCompatMode
     };
 
     data.stylist.set_quirks_mode(quirks_mode.into());
 }
 
 fn parse_property_into(declarations: &mut SourcePropertyDeclaration,
                        property_id: PropertyId,
                        value: *const nsACString,