Bug 1474793 - Part 4: Allow C++ URLValue objects to be lazily created from Rust SpecifiedUrls. r=emilio

This avoids having to support storing refcounted URLValue objects in shared memory,
which would be tricky.

Depends on D17183

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

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

servo/components/style/gecko/conversions.rs

@@ -123,14 +123,11 @@ impl nsStyleImage {
         match image {
             GenericImage::Gradient(boxed_gradient) => self.set_gradient(*boxed_gradient),
             GenericImage::Url(ref url) => unsafe {
-                bindings::Gecko_SetLayerImageImageValue(self, (url.0).0.url_value.get());
+                bindings::Gecko_SetLayerImageImageValue(self, url.url_value_ptr())
             },
             GenericImage::Rect(ref image_rect) => {
                 unsafe {
-                    bindings::Gecko_SetLayerImageImageValue(
-                        self,
-                        (image_rect.url.0).0.url_value.get(),
-                    );
+                    bindings::Gecko_SetLayerImageImageValue(self, image_rect.url.url_value_ptr());
                     bindings::Gecko_InitializeImageCropRect(self);
 
                     // Set CropRect

servo/components/style/gecko/url.rs

@@ -17,7 +17,9 @@ use cssparser::Parser;
 use malloc_size_of::{MallocSizeOf, MallocSizeOfOps};
 use nsstring::nsCString;
 use servo_arc::Arc;
+use std::collections::HashMap;
 use std::fmt::{self, Write};
+use std::sync::RwLock;
 use style_traits::{CssWriter, ParseError, ToCss};
 
 /// A CSS url() value for gecko.
@@ -81,6 +83,20 @@ impl CssUrlData {
     }
 }
 
+#[cfg(debug_assertions)]
+impl Drop for CssUrlData {
+    fn drop(&mut self) {
+        assert!(
+            !URL_VALUE_TABLE
+                .read()
+                .unwrap()
+                .contains_key(&CssUrlDataKey(self as *mut _ as *const _)),
+            "All CssUrlData objects used as keys in URL_VALUE_TABLE should be \
+             from shared memory style sheets, and so should never be dropped",
+        );
+    }
+}
+
 impl Parse for CssUrl {
     fn parse<'i, 't>(
         context: &ParserContext,
@@ -104,6 +120,24 @@ impl MallocSizeOf for CssUrl {
     }
 }
 
+/// A key type for URL_VALUE_TABLE.
+#[derive(Hash, PartialEq, Eq)]
+struct CssUrlDataKey(*const CssUrlData);
+
+unsafe impl Sync for CssUrlDataKey {}
+unsafe impl Send for CssUrlDataKey {}
+
+/// The source of a Gecko URLValue object for a SpecifiedUrl.
+#[derive(Clone, Debug)]
+pub enum URLValueSource {
+    /// A strong reference to a Gecko URLValue object.
+    URLValue(RefPtr<URLValue>),
+    /// A CORSMode value used to lazily construct a Gecko URLValue object.
+    ///
+    /// The lazily created object will be stored in URL_VALUE_TABLE.
+    CORSMode(CORSMode),
+}
+
 /// A specified non-image `url()` value.
 #[derive(Clone, Debug, SpecifiedValueInfo, ToCss)]
 pub struct SpecifiedUrl {
@@ -111,8 +145,20 @@ pub struct SpecifiedUrl {
     pub url: CssUrl,
     /// Gecko's URLValue so that we can reuse it while rematching a
     /// property with this specified value.
+    ///
+    /// Box this to avoid SpecifiedUrl getting bigger than two words,
+    /// and increasing the size of PropertyDeclaration.
     #[css(skip)]
-    pub url_value: RefPtr<URLValue>,
+    url_value: Box<URLValueSource>,
+}
+
+fn make_url_value(url: &CssUrl, cors_mode: CORSMode) -> RefPtr<URLValue> {
+    unsafe {
+        let ptr = bindings::Gecko_URLValue_Create(url.0.clone().into_strong(), cors_mode);
+        // We do not expect Gecko_URLValue_Create returns null.
+        debug_assert!(!ptr.is_null());
+        RefPtr::from_addrefed(ptr)
+    }
 }
 
 impl SpecifiedUrl {
@@ -122,12 +168,7 @@ impl SpecifiedUrl {
     }
 
     fn from_css_url_with_cors(url: CssUrl, cors: CORSMode) -> Self {
-        let url_value = unsafe {
-            let ptr = bindings::Gecko_URLValue_Create(url.0.clone().into_strong(), cors);
-            // We do not expect Gecko_URLValue_Create returns null.
-            debug_assert!(!ptr.is_null());
-            RefPtr::from_addrefed(ptr)
-        };
+        let url_value = Box::new(URLValueSource::URLValue(make_url_value(&url, cors)));
         Self { url, url_value }
     }
 
@@ -140,6 +181,45 @@ impl SpecifiedUrl {
         use crate::gecko_bindings::structs::root::mozilla::CORSMode_CORS_ANONYMOUS;
         Self::from_css_url_with_cors(url, CORSMode_CORS_ANONYMOUS)
     }
+
+    fn with_url_value<F, T>(&self, f: F) -> T
+    where
+        F: FnOnce(&RefPtr<URLValue>) -> T,
+    {
+        match *self.url_value {
+            URLValueSource::URLValue(ref r) => f(r),
+            URLValueSource::CORSMode(cors_mode) => {
+                {
+                    let guard = URL_VALUE_TABLE.read().unwrap();
+                    if let Some(r) = guard.get(&(CssUrlDataKey(&*self.url.0 as *const _))) {
+                        return f(r);
+                    }
+                }
+                let mut guard = URL_VALUE_TABLE.write().unwrap();
+                let r = guard
+                    .entry(CssUrlDataKey(&*self.url.0 as *const _))
+                    .or_insert_with(|| make_url_value(&self.url, cors_mode));
+                f(r)
+            },
+        }
+    }
+
+    /// Clone a new, strong reference to the Gecko URLValue.
+    pub fn clone_url_value(&self) -> RefPtr<URLValue> {
+        self.with_url_value(RefPtr::clone)
+    }
+
+    /// Get a raw pointer to the URLValue held by this SpecifiedUrl, for FFI.
+    pub fn url_value_ptr(&self) -> *mut URLValue {
+        self.with_url_value(RefPtr::get)
+    }
+}
+
+/// Clears URL_VALUE_TABLE.  Entries in this table, which are for specified URL
+/// values that come from shared memory style sheets, would otherwise persist
+/// until the end of the process and be reported as leaks.
+pub fn shutdown() {
+    URL_VALUE_TABLE.write().unwrap().clear();
 }
 
 impl Parse for SpecifiedUrl {
@@ -165,7 +245,7 @@ impl MallocSizeOf for SpecifiedUrl {
         // Although this is a RefPtr, this is the primary reference because
         // SpecifiedUrl is responsible for creating the url_value. So we
         // measure unconditionally here.
-        n += unsafe { bindings::Gecko_URLValue_SizeOfIncludingThis(self.url_value.get()) };
+        n += unsafe { bindings::Gecko_URLValue_SizeOfIncludingThis(self.url_value_ptr()) };
         n
     }
 }
@@ -258,7 +338,8 @@ impl ToCss for ComputedUrl {
     where
         W: Write,
     {
-        serialize_computed_url(&self.0.url_value, dest, bindings::Gecko_GetComputedURLSpec)
+        self.0
+            .with_url_value(|r| serialize_computed_url(r, dest, bindings::Gecko_GetComputedURLSpec))
     }
 }
 
@@ -267,12 +348,20 @@ impl ComputedUrl {
     pub unsafe fn from_url_value(url_value: RefPtr<URLValue>) -> Self {
         let css_url = &*url_value.mCssUrl.mRawPtr;
         let url = CssUrl(CssUrlData::as_arc(&css_url).clone_arc());
-        ComputedUrl(SpecifiedUrl { url, url_value })
+        ComputedUrl(SpecifiedUrl {
+            url,
+            url_value: Box::new(URLValueSource::URLValue(url_value)),
+        })
+    }
+
+    /// Clone a new, strong reference to the Gecko URLValue.
+    pub fn clone_url_value(&self) -> RefPtr<URLValue> {
+        self.0.clone_url_value()
     }
 
     /// Get a raw pointer to the URLValue held by this ComputedUrl, for FFI.
     pub fn url_value_ptr(&self) -> *mut URLValue {
-        self.0.url_value.get()
+        self.0.url_value_ptr()
     }
 }
 
@@ -285,11 +374,9 @@ impl ToCss for ComputedImageUrl {
     where
         W: Write,
     {
-        serialize_computed_url(
-            &(self.0).0.url_value,
-            dest,
-            bindings::Gecko_GetComputedImageURLSpec,
-        )
+        (self.0).0.with_url_value(|r| {
+            serialize_computed_url(r, dest, bindings::Gecko_GetComputedImageURLSpec)
+        })
     }
 }
 
@@ -299,11 +386,27 @@ impl ComputedImageUrl {
         let url_value = image_request.mImageValue.to_safe();
         let css_url = &*url_value.mCssUrl.mRawPtr;
         let url = CssUrl(CssUrlData::as_arc(&css_url).clone_arc());
-        ComputedImageUrl(SpecifiedImageUrl(SpecifiedUrl { url, url_value }))
+        ComputedImageUrl(SpecifiedImageUrl(SpecifiedUrl {
+            url,
+            url_value: Box::new(URLValueSource::URLValue(url_value)),
+        }))
+    }
+
+    /// Clone a new, strong reference to the Gecko URLValue.
+    pub fn clone_url_value(&self) -> RefPtr<URLValue> {
+        (self.0).0.clone_url_value()
     }
 
     /// Get a raw pointer to the URLValue held by this ComputedImageUrl, for FFI.
     pub fn url_value_ptr(&self) -> *mut URLValue {
-        (self.0).0.url_value.get()
+        (self.0).0.url_value_ptr()
     }
 }
+
+lazy_static! {
+    /// A table mapping CssUrlData objects to their lazily created Gecko
+    /// URLValue objects.
+    static ref URL_VALUE_TABLE: RwLock<HashMap<CssUrlDataKey, RefPtr<URLValue>>> = {
+        Default::default()
+    };
+}

servo/components/style/properties/gecko.mako.rs

@@ -826,7 +826,7 @@ def set_gecko_property(ffi_name, expr):
     pub fn set_${ident}(&mut self, v: longhands::${ident}::computed_value::T) {
         match v {
             UrlOrNone::Url(ref url) => {
-                self.gecko.${gecko_ffi_name}.set_move(url.0.url_value.clone())
+                self.gecko.${gecko_ffi_name}.set_move(url.clone_url_value())
             }
             UrlOrNone::None => {
                 unsafe {
@@ -3783,7 +3783,7 @@ fn static_assert() {
                 },
                 Url(ref url) => {
                     unsafe {
-                        bindings::Gecko_nsStyleFilter_SetURLValue(gecko_filter, url.0.url_value.get());
+                        bindings::Gecko_nsStyleFilter_SetURLValue(gecko_filter, url.url_value_ptr());
                     }
                 },
             }
@@ -4164,7 +4164,7 @@ fn set_style_svg_path(
             % if ident == "clip_path":
             ShapeSource::ImageOrUrl(ref url) => {
                 unsafe {
-                    bindings::Gecko_StyleShapeSource_SetURLValue(${ident}, url.0.url_value.get())
+                    bindings::Gecko_StyleShapeSource_SetURLValue(${ident}, url.url_value_ptr())
                 }
             }
             % elif ident == "shape_outside":

servo/ports/geckolib/glue.rs

@@ -33,7 +33,7 @@ use style::gecko::data::{GeckoStyleSheet, PerDocumentStyleData, PerDocumentStyle
 use style::gecko::restyle_damage::GeckoRestyleDamage;
 use style::gecko::selector_parser::{NonTSPseudoClass, PseudoElement};
 use style::gecko::traversal::RecalcStyleOnly;
-use style::gecko::url::CssUrlData;
+use style::gecko::url::{self, CssUrlData};
 use style::gecko::wrapper::{GeckoElement, GeckoNode};
 use style::gecko_bindings::bindings;
 use style::gecko_bindings::bindings::nsACString;
@@ -192,6 +192,7 @@ pub extern "C" fn Servo_InitializeCooperativeThread() {
 pub unsafe extern "C" fn Servo_Shutdown() {
     DUMMY_URL_DATA = ptr::null_mut();
     Stylist::shutdown();
+    url::shutdown();
 }
 
 #[inline(always)]
@@ -2744,7 +2745,7 @@ pub unsafe extern "C" fn Servo_FontFaceRule_GetSources(
             for source in sources.iter() {
                 match *source {
                     Source::Url(ref url) => {
-                        set_next(FontFaceSourceListComponent::Url(url.url.url_value.get()));
+                        set_next(FontFaceSourceListComponent::Url(url.url.url_value_ptr()));
                         for hint in url.format_hints.iter() {
                             set_next(FontFaceSourceListComponent::FormatHint {
                                 length: hint.len(),