Bug 1470145: Better debugging for stylesheets and URLs. r=xidorn

MozReview-Commit-ID: FIcz2K1ZYX0

layout/style/ServoBindings.cpp

@@ -2147,6 +2147,15 @@ Gecko_GetComputedURLSpec(const URLValueData* aURL, nsCString* aOut)
   aOut->AssignLiteral("about:invalid");
 }
 
+void
+Gecko_nsIURI_Debug(nsIURI* aURI, nsCString* aOut)
+{
+  // TODO(emilio): Do we have more useful stuff to put here, maybe?
+  if (aURI) {
+    *aOut = aURI->GetSpecOrDefault();
+  }
+}
+
 NS_IMPL_THREADSAFE_FFI_REFCOUNTING(css::URLValue, CSSURLValue);
 
 NS_IMPL_THREADSAFE_FFI_REFCOUNTING(URLExtraData, URLExtraData);

layout/style/ServoBindings.h

@@ -536,6 +536,8 @@ void Gecko_nsStyleSVG_CopyContextProperties(nsStyleSVG* dst, const nsStyleSVG* s
 mozilla::css::URLValue* Gecko_NewURLValue(ServoBundledURI uri);
 size_t Gecko_URLValue_SizeOfIncludingThis(mozilla::css::URLValue* url);
 void Gecko_GetComputedURLSpec(const mozilla::css::URLValueData* url, nsCString* spec);
+void Gecko_nsIURI_Debug(nsIURI*, nsCString* spec);
+
 NS_DECL_THREADSAFE_FFI_REFCOUNTING(mozilla::css::URLValue, CSSURLValue);
 NS_DECL_THREADSAFE_FFI_REFCOUNTING(RawGeckoURLExtraData, URLExtraData);
 

servo/components/style/error_reporting.rs

@@ -7,7 +7,6 @@
 #![deny(missing_docs)]
 
 use cssparser::{BasicParseErrorKind, ParseErrorKind, SourceLocation, Token};
-use log;
 use std::fmt;
 use style_traits::ParseError;
 use stylesheets::UrlExtraData;
@@ -229,8 +228,10 @@ pub trait ParseErrorReporter {
 /// This logging is silent by default, and can be enabled with a `RUST_LOG=style=info`
 /// environment variable.
 /// (See [`env_logger`](https://rust-lang-nursery.github.io/log/env_logger/).)
+#[cfg(feature = "servo")]
 pub struct RustLogReporter;
 
+#[cfg(feature = "servo")]
 impl ParseErrorReporter for RustLogReporter {
     fn report_error(
         &self,
@@ -238,6 +239,7 @@ impl ParseErrorReporter for RustLogReporter {
         location: SourceLocation,
         error: ContextualParseError,
     ) {
+        use log;
         if log_enabled!(log::Level::Info) {
             info!(
                 "Url:\t{}\n{}:{} {}",

servo/components/style/gecko/data.rs

@@ -17,14 +17,25 @@ use media_queries::{Device, MediaList};
 use properties::ComputedValues;
 use selector_parser::SnapshotMap;
 use servo_arc::Arc;
+use std::fmt;
 use shared_lock::{Locked, SharedRwLockReadGuard, StylesheetGuards};
 use stylesheets::{CssRule, Origin, StylesheetContents, StylesheetInDocument};
 use stylist::Stylist;
 
 /// Little wrapper to a Gecko style sheet.
-#[derive(Debug, Eq, PartialEq)]
+#[derive(Eq, PartialEq)]
 pub struct GeckoStyleSheet(*const DomStyleSheet);
 
+impl fmt::Debug for GeckoStyleSheet {
+    fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
+        let contents = self.contents();
+        formatter.debug_struct("GeckoStyleSheet")
+            .field("origin", &contents.origin)
+            .field("url_data", &*contents.url_data.read())
+            .finish()
+    }
+}
+
 impl ToMediaListKey for ::gecko::data::GeckoStyleSheet {
     fn to_media_list_key(&self) -> MediaListKey {
         use std::mem;

servo/components/style/gecko/url.rs

@@ -6,7 +6,7 @@
 
 use cssparser::Parser;
 use gecko_bindings::bindings;
-use gecko_bindings::structs::{ServoBundledURI, URLExtraData};
+use gecko_bindings::structs::ServoBundledURI;
 use gecko_bindings::structs::mozilla::css::URLValueData;
 use gecko_bindings::structs::root::{RustString, nsStyleImageRequest};
 use gecko_bindings::structs::root::mozilla::css::{ImageValue, URLValue};
@@ -17,6 +17,7 @@ use parser::{Parse, ParserContext};
 use servo_arc::{Arc, RawOffsetArc};
 use std::fmt::{self, Write};
 use std::mem;
+use stylesheets::UrlExtraData;
 use style_traits::{CssWriter, ParseError, ToCss};
 use values::computed::{Context, ToComputedValue};
 
@@ -32,7 +33,7 @@ pub struct CssUrl {
 
     /// The URL extra data.
     #[css(skip)]
-    pub extra_data: RefPtr<URLExtraData>,
+    pub extra_data: UrlExtraData,
 }
 
 impl CssUrl {
@@ -58,7 +59,7 @@ impl CssUrl {
             &url.mString as *const _ as *const RawOffsetArc<String>;
         CssUrl {
             serialization: Arc::from_raw_offset((*arc_type).clone()),
-            extra_data: url.mExtraData.to_safe(),
+            extra_data: UrlExtraData(url.mExtraData.to_safe()),
         }
     }
 
@@ -88,7 +89,7 @@ impl CssUrl {
         let arc_offset = Arc::into_raw_offset(self.serialization.clone());
         ServoBundledURI {
             mURLString: unsafe { mem::transmute::<_, RawOffsetArc<RustString>>(arc_offset) },
-            mExtraData: self.extra_data.get(),
+            mExtraData: self.extra_data.0.get(),
         }
     }
 }

servo/components/style/gecko_bindings/sugar/refptr.rs

@@ -78,15 +78,6 @@ impl<T: RefCounted> RefPtr<T> {
         ret
     }
 
-    /// Create a reference to RefPtr from a reference to pointer.
-    ///
-    /// The pointer must be valid and non null.
-    ///
-    /// This method doesn't touch refcount.
-    pub unsafe fn from_ptr_ref(ptr: &*mut T) -> &Self {
-        mem::transmute(ptr)
-    }
-
     /// Produces an FFI-compatible RefPtr that can be stored in style structs.
     ///
     /// structs::RefPtr does not have a destructor, so this may leak

servo/components/style/stylesheets/mod.rs

@@ -61,22 +61,52 @@ pub type UrlExtraData = ::servo_url::ServoUrl;
 
 /// Extra data that the backend may need to resolve url values.
 #[cfg(feature = "gecko")]
-pub type UrlExtraData =
-    ::gecko_bindings::sugar::refptr::RefPtr<::gecko_bindings::structs::URLExtraData>;
+#[derive(Clone, PartialEq)]
+pub struct UrlExtraData(
+    pub ::gecko_bindings::sugar::refptr::RefPtr<::gecko_bindings::structs::URLExtraData>
+);
 
 #[cfg(feature = "gecko")]
 impl UrlExtraData {
-    /// Returns a string for the url.
-    ///
-    /// Unimplemented currently.
-    pub fn as_str(&self) -> &str {
-        // TODO
-        "(stylo: not supported)"
+    /// True if this URL scheme is chrome.
+    #[inline]
+    pub fn is_chrome(&self) -> bool {
+        self.0.mIsChrome
     }
 
-    /// True if this URL scheme is chrome.
-    pub fn is_chrome(&self) -> bool {
-        self.mIsChrome
+    /// Create a reference to this `UrlExtraData` from a reference to pointer.
+    ///
+    /// The pointer must be valid and non null.
+    ///
+    /// This method doesn't touch refcount.
+    #[inline]
+    pub unsafe fn from_ptr_ref(ptr: &*mut ::gecko_bindings::structs::URLExtraData) -> &Self {
+        ::std::mem::transmute(ptr)
+    }
+}
+
+#[cfg(feature = "gecko")]
+impl fmt::Debug for UrlExtraData {
+    fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
+        use gecko_bindings::{structs, bindings};
+
+        struct DebugURI(*mut structs::nsIURI);
+        impl fmt::Debug for DebugURI {
+            fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
+                use nsstring::nsCString;
+                let mut spec = nsCString::new();
+                unsafe {
+                    bindings::Gecko_nsIURI_Debug(self.0, &mut spec);
+                }
+                spec.fmt(formatter)
+            }
+        }
+
+        formatter.debug_struct("URLExtraData")
+            .field("is_chrome", &self.is_chrome())
+            .field("base", &DebugURI(self.0.mBaseURI.raw::<structs::nsIURI>()))
+            .field("referrer", &DebugURI(self.0.mReferrer.raw::<structs::nsIURI>()))
+            .finish()
     }
 }
 

servo/components/style/stylesheets/stylesheet.rs

@@ -176,7 +176,7 @@ macro_rules! rule_filter {
 }
 
 /// A trait to represent a given stylesheet in a document.
-pub trait StylesheetInDocument {
+pub trait StylesheetInDocument : ::std::fmt::Debug {
     /// Get the stylesheet origin.
     fn origin(&self, guard: &SharedRwLockReadGuard) -> Origin;
 
@@ -263,7 +263,7 @@ impl StylesheetInDocument for Stylesheet {
 
 /// A simple wrapper over an `Arc<Stylesheet>`, with pointer comparison, and
 /// suitable for its use in a `StylesheetSet`.
-#[derive(Clone)]
+#[derive(Clone, Debug)]
 #[cfg_attr(feature = "servo", derive(MallocSizeOf))]
 pub struct DocumentStyleSheet(
     #[cfg_attr(feature = "servo", ignore_malloc_size_of = "Arc")] pub Arc<Stylesheet>,

servo/ports/geckolib/glue.rs

@@ -143,7 +143,7 @@ use style::style_adjuster::StyleAdjuster;
 use style::stylesheets::{CssRule, CssRules, CssRuleType, CssRulesHelpers, CounterStyleRule};
 use style::stylesheets::{DocumentRule, FontFaceRule, FontFeatureValuesRule, ImportRule};
 use style::stylesheets::{KeyframesRule, MediaRule, NamespaceRule, Origin, OriginSet, PageRule};
-use style::stylesheets::{StyleRule, StylesheetContents, SupportsRule};
+use style::stylesheets::{StyleRule, StylesheetContents, SupportsRule, UrlExtraData};
 use style::stylesheets::StylesheetLoader as StyleStylesheetLoader;
 use style::stylesheets::import_rule::ImportSheet;
 use style::stylesheets::keyframes_rule::{Keyframe, KeyframeSelector, KeyframesStepValue};
@@ -189,10 +189,10 @@ impl ClosureHelper for DeclarationBlockMutationClosure {
 
 // A dummy url data for where we don't pass url data in.
 // We need to get rid of this sooner than later.
-static mut DUMMY_URL_DATA: *mut URLExtraData = 0 as *mut URLExtraData;
+static mut DUMMY_URL_DATA: *mut URLExtraData = 0 as *mut _;
 
 #[no_mangle]
-pub extern "C" fn Servo_Initialize(dummy_url_data: *mut URLExtraData) {
+pub unsafe extern "C" fn Servo_Initialize(dummy_url_data: *mut URLExtraData) {
     use style::gecko_bindings::sugar::origin_flags;
 
     // Pretend that we're a Servo Layout thread, to make some assertions happy.
@@ -207,8 +207,7 @@ pub extern "C" fn Servo_Initialize(dummy_url_data: *mut URLExtraData) {
     specified::font::assert_variant_ligatures_matches();
     specified::box_::assert_touch_action_matches();
 
-    // Initialize the dummy url data
-    unsafe { DUMMY_URL_DATA = dummy_url_data; }
+    DUMMY_URL_DATA = dummy_url_data;
 }
 
 #[no_mangle]
@@ -218,15 +217,14 @@ pub extern "C" fn Servo_InitializeCooperativeThread() {
 }
 
 #[no_mangle]
-pub extern "C" fn Servo_Shutdown() {
-    // The dummy url will be released after shutdown, so clear the
-    // reference to avoid use-after-free.
-    unsafe { DUMMY_URL_DATA = ptr::null_mut(); }
+pub unsafe extern "C" fn Servo_Shutdown() {
+    DUMMY_URL_DATA = ptr::null_mut();
     Stylist::shutdown();
 }
 
-unsafe fn dummy_url_data() -> &'static RefPtr<URLExtraData> {
-    RefPtr::from_ptr_ref(&DUMMY_URL_DATA)
+#[inline(always)]
+unsafe fn dummy_url_data() -> &'static UrlExtraData {
+    UrlExtraData::from_ptr_ref(&DUMMY_URL_DATA)
 }
 
 #[allow(dead_code)]
@@ -1181,7 +1179,7 @@ pub extern "C" fn Servo_StyleSheet_FromUTF8Bytes(
     let input: &str = unsafe { (*bytes).as_str_unchecked() };
 
     let reporter = ErrorReporter::new(stylesheet, loader, extra_data);
-    let url_data = unsafe { RefPtr::from_ptr_ref(&extra_data) };
+    let url_data = unsafe { UrlExtraData::from_ptr_ref(&extra_data) };
     let loader = if loader.is_null() {
         None
     } else {
@@ -1208,7 +1206,7 @@ pub extern "C" fn Servo_StyleSheet_FromUTF8Bytes(
 }
 
 #[no_mangle]
-pub extern "C" fn Servo_StyleSheet_FromUTF8BytesAsync(
+pub unsafe extern "C" fn Servo_StyleSheet_FromUTF8BytesAsync(
     load_data: *mut SheetLoadDataHolder,
     extra_data: *mut URLExtraData,
     bytes: *const nsACString,
@@ -1216,15 +1214,15 @@ pub extern "C" fn Servo_StyleSheet_FromUTF8BytesAsync(
     line_number_offset: u32,
     quirks_mode: nsCompatibility,
 ) {
-    let (load_data, extra_data, bytes) = unsafe {
-        let mut b = nsCString::new();
-        b.assign(&*bytes);
-        (RefPtr::new(load_data), RefPtr::new(extra_data), b)
-    };
+    let load_data = RefPtr::new(load_data);
+    let extra_data = UrlExtraData(RefPtr::new(extra_data));
+
+    let mut sheet_bytes = nsCString::new();
+    sheet_bytes.assign(&*bytes);
     let async_parser = AsyncStylesheetParser::new(
         load_data,
         extra_data,
-        bytes,
+        sheet_bytes,
         mode_to_origin(mode),
         quirks_mode.into(),
         line_number_offset
@@ -2513,7 +2511,7 @@ pub unsafe extern "C" fn Servo_FontFaceRule_SetDescriptor(
     let value = value.as_ref().unwrap().as_str_unchecked();
     let mut input = ParserInput::new(&value);
     let mut parser = Parser::new(&mut input);
-    let url_data = RefPtr::from_ptr_ref(&data);
+    let url_data = UrlExtraData::from_ptr_ref(&data);
     let context = ParserContext::new(
         Origin::Author,
         url_data,
@@ -3224,7 +3222,7 @@ fn parse_property_into(
 ) -> Result<(), ()> {
     use style_traits::ParsingMode;
     let value = unsafe { value.as_ref().unwrap().as_str_unchecked() };
-    let url_data = unsafe { RefPtr::from_ptr_ref(&data) };
+    let url_data = unsafe { UrlExtraData::from_ptr_ref(&data) };
     let parsing_mode = ParsingMode::from_bits_truncate(parsing_mode);
 
     parse_one_declaration_into(
@@ -3284,7 +3282,7 @@ pub extern "C" fn Servo_ParseEasing(
     use style::properties::longhands::transition_timing_function;
 
     // FIXME Dummy URL data would work fine here.
-    let url_data = unsafe { RefPtr::from_ptr_ref(&data) };
+    let url_data = unsafe { UrlExtraData::from_ptr_ref(&data) };
     let context = ParserContext::new(
         Origin::Author,
         url_data,
@@ -3377,7 +3375,7 @@ pub extern "C" fn Servo_ParseStyleAttribute(
     let global_style_data = &*GLOBAL_STYLE_DATA;
     let value = unsafe { data.as_ref().unwrap().as_str_unchecked() };
     let reporter = ErrorReporter::new(ptr::null_mut(), loader, raw_extra_data);
-    let url_data = unsafe { RefPtr::from_ptr_ref(&raw_extra_data) };
+    let url_data = unsafe { UrlExtraData::from_ptr_ref(&raw_extra_data) };
     Arc::new(global_style_data.shared_lock.wrap(
         parse_style_attribute(
             value,
@@ -4216,7 +4214,7 @@ pub extern "C" fn Servo_DeclarationBlock_SetBackgroundImage(
     use style::values::generics::image::Image;
     use style::values::specified::url::SpecifiedImageUrl;
 
-    let url_data = unsafe { RefPtr::from_ptr_ref(&raw_extra_data) };
+    let url_data = unsafe { UrlExtraData::from_ptr_ref(&raw_extra_data) };
     let string = unsafe { (*value).to_string() };
     let context = ParserContext::new(
         Origin::Author,
@@ -5539,7 +5537,7 @@ pub extern "C" fn Servo_ParseFontShorthandForMatching(
     let string = unsafe { (*value).to_string() };
     let mut input = ParserInput::new(&string);
     let mut parser = Parser::new(&mut input);
-    let url_data = unsafe { RefPtr::from_ptr_ref(&data) };
+    let url_data = unsafe { UrlExtraData::from_ptr_ref(&data) };
     let context = ParserContext::new(
         Origin::Author,
         url_data,

servo/ports/geckolib/stylesheet_loader.rs

@@ -12,14 +12,13 @@ use style::gecko_bindings::bindings;
 use style::gecko_bindings::bindings::Gecko_LoadStyleSheet;
 use style::gecko_bindings::structs::{Loader, LoaderReusableStyleSheets};
 use style::gecko_bindings::structs::{StyleSheet as DomStyleSheet, SheetLoadData, SheetLoadDataHolder};
-use style::gecko_bindings::structs::URLExtraData;
 use style::gecko_bindings::sugar::ownership::FFIArcHelpers;
 use style::gecko_bindings::sugar::refptr::RefPtr;
 use style::media_queries::MediaList;
 use style::parser::ParserContext;
 use style::shared_lock::{Locked, SharedRwLock};
 use style::stylesheets::{ImportRule, Origin, StylesheetLoader as StyleStylesheetLoader};
-use style::stylesheets::StylesheetContents;
+use style::stylesheets::{StylesheetContents, UrlExtraData};
 use style::stylesheets::import_rule::ImportSheet;
 use style::values::CssUrl;
 
@@ -69,7 +68,7 @@ impl StyleStylesheetLoader for StylesheetLoader {
 
 pub struct AsyncStylesheetParser {
     load_data: RefPtr<SheetLoadDataHolder>,
-    extra_data: RefPtr<URLExtraData>,
+    extra_data: UrlExtraData,
     bytes: nsCString,
     origin: Origin,
     quirks_mode: QuirksMode,
@@ -79,7 +78,7 @@ pub struct AsyncStylesheetParser {
 impl AsyncStylesheetParser {
     pub fn new(
         load_data: RefPtr<SheetLoadDataHolder>,
-        extra_data: RefPtr<URLExtraData>,
+        extra_data: UrlExtraData,
         bytes: nsCString,
         origin: Origin,
         quirks_mode: QuirksMode,