Bug 1321176 - Handle unresolved url value gracefully. r?heycam,manishearth draft
authorXidorn Quan <me@upsuper.org>
Tue, 20 Dec 2016 18:02:44 +1100
changeset 451463 6413ca1f907c551e80abc92a4e95e639250dbc88
parent 451462 d2d404cc867e558e2a8ed1243459cc138cc2da99
child 540037 0485f7acb3c938cab7874d46ae54bad2d7c2db9f
push id39190
push userxquan@mozilla.com
push dateTue, 20 Dec 2016 11:58:56 +0000
reviewersheycam, manishearth
bugs1321176
milestone53.0a1
Bug 1321176 - Handle unresolved url value gracefully. r?heycam,manishearth This changes as_slice_components() to return the original url if the url is not resolved. It returns Ok() vs. Err() to distinguish between the two cases because we cannot assign an unresolvable url to -moz-bindings. MozReview-Commit-ID: FrWyDTIQfgH
layout/style/ServoBindings.cpp
servo/components/style/gecko/conversions.rs
servo/components/style/properties/gecko.mako.rs
servo/components/style/values/specified/url.rs
--- a/layout/style/ServoBindings.cpp
+++ b/layout/style/ServoBindings.cpp
@@ -648,18 +648,22 @@ NS_IMPL_HOLDER_FFI_REFCOUNTING(nsIURI, U
 
 void
 Gecko_SetMozBinding(nsStyleDisplay* aDisplay,
                     const uint8_t* aURLString, uint32_t aURLStringLength,
                     ThreadSafeURIHolder* aBaseURI,
                     ThreadSafeURIHolder* aReferrer,
                     ThreadSafePrincipalHolder* aPrincipal)
 {
+  if (!aURLString) {
+    aDisplay->mBinding = nullptr;
+    return;
+  }
+
   MOZ_ASSERT(aDisplay);
-  MOZ_ASSERT(aURLString);
   MOZ_ASSERT(aBaseURI);
   MOZ_ASSERT(aReferrer);
   MOZ_ASSERT(aPrincipal);
 
   nsString url;
   nsDependentCSubstring urlString(reinterpret_cast<const char*>(aURLString),
                                   aURLStringLength);
   AppendUTF8toUTF16(urlString, url);
--- a/servo/components/style/gecko/conversions.rs
+++ b/servo/components/style/gecko/conversions.rs
@@ -128,17 +128,19 @@ impl From<nsStyleCoord_CalcValue> for Le
 
 impl nsStyleImage {
     pub fn set(&mut self, image: Image, with_url: bool, cacheable: &mut bool) {
         match image {
             Image::Gradient(gradient) => {
                 self.set_gradient(gradient)
             },
             Image::Url(ref url) if with_url => {
-                let (ptr, len) = url.as_slice_components();
+                let (ptr, len) = match url.as_slice_components() {
+                    Ok(value) | Err(value) => value
+                };
                 let extra_data = url.extra_data();
                 unsafe {
                     Gecko_SetUrlImageValue(self,
                                            ptr,
                                            len as u32,
                                            extra_data.base.get(),
                                            extra_data.referrer.get(),
                                            extra_data.principal.get());
--- a/servo/components/style/properties/gecko.mako.rs
+++ b/servo/components/style/properties/gecko.mako.rs
@@ -1147,17 +1147,20 @@ fn static_assert() {
 
     #[allow(non_snake_case)]
     pub fn set__moz_binding(&mut self, v: longhands::_moz_binding::computed_value::T) {
         use values::Either;
         match v {
             Either::Second(_none) => debug_assert!(self.gecko.mBinding.mRawPtr.is_null()),
             Either::First(ref url) => {
                 let extra_data = url.extra_data();
-                let (ptr, len) = url.as_slice_components();
+                let (ptr, len) = match url.as_slice_components() {
+                    Ok(value) => value,
+                    Err(_) => (ptr::null(), 0),
+                };
                 unsafe {
                     Gecko_SetMozBinding(&mut self.gecko,
                                         ptr,
                                         len as u32,
                                         extra_data.base.get(),
                                         extra_data.referrer.get(),
                                         extra_data.principal.get());
                 }
@@ -1732,17 +1735,19 @@ fn static_assert() {
         use values::Either;
         match image {
             Either::Second(_none) => {
                 unsafe {
                     Gecko_SetListStyleImageNone(&mut self.gecko);
                 }
             }
             Either::First(ref url) => {
-                let (ptr, len) = url.as_slice_components();
+                let (ptr, len) = match url.as_slice_components() {
+                    Ok(value) | Err(value) => value
+                };
                 let extra_data = url.extra_data();
                 unsafe {
                     Gecko_SetListStyleImage(&mut self.gecko,
                                             ptr,
                                             len as u32,
                                             extra_data.base.get(),
                                             extra_data.referrer.get(),
                                             extra_data.principal.get());
@@ -2428,17 +2433,19 @@ clip-path
         } as u8;
 
         unsafe {
             Gecko_SetCursorArrayLength(&mut self.gecko, v.images.len());
         }
         for i in 0..v.images.len() {
             let image = &v.images[i];
             let extra_data = image.url.extra_data();
-            let (ptr, len) = image.url.as_slice_components();
+            let (ptr, len) = match image.url.as_slice_components() {
+                Ok(value) | Err(value) => value,
+            };
             unsafe {
                 Gecko_SetCursorImage(&mut self.gecko.mCursorImages[i],
                                      ptr, len as u32,
                                      extra_data.base.get(),
                                      extra_data.referrer.get(),
                                      extra_data.principal.get());
             }
             // We don't need to record this struct as uncacheable, like when setting
--- a/servo/components/style/values/specified/url.rs
+++ b/servo/components/style/values/specified/url.rs
@@ -8,17 +8,16 @@ use cssparser::{CssStringWriter, Parser}
 #[cfg(feature = "gecko")]
 use gecko_bindings::sugar::refptr::{GeckoArcPrincipal, GeckoArcURI};
 use parser::{Parse, ParserContext};
 #[cfg(feature = "gecko")]
 use parser::ParserContextExtraData;
 use servo_url::ServoUrl;
 use std::borrow::Cow;
 use std::fmt::{self, Write};
-use std::ptr;
 use std::sync::Arc;
 use style_traits::ToCss;
 use values::NoViewportPercentage;
 use values::computed::ComputedValueAsSpecified;
 
 #[derive(PartialEq, Clone, Debug)]
 #[cfg_attr(feature = "servo", derive(HeapSizeOf, Serialize, Deserialize, Eq))]
 pub struct UrlExtraData {
@@ -116,20 +115,25 @@ impl SpecifiedUrl {
     pub fn as_str(&self) -> &str {
         match self.resolved {
             Some(ref url) => url.as_str(),
             None => "",
         }
     }
 
     /// Little helper for Gecko's ffi.
-    pub fn as_slice_components(&self) -> (*const u8, usize) {
+    #[cfg(feature = "gecko")]
+    pub fn as_slice_components(&self) -> Result<(*const u8, usize), (*const u8, usize)> {
         match self.resolved {
-            Some(ref url) => (url.as_str().as_ptr(), url.as_str().len()),
-            None => (ptr::null(), 0),
+            Some(ref url) => Ok((url.as_str().as_ptr(), url.as_str().len())),
+            None => {
+                let url = self.original.as_ref()
+                    .expect("We should always have either the original or the resolved value");
+                Err((url.as_str().as_ptr(), url.as_str().len()))
+            }
         }
     }
 
     /// Creates an already specified url value from an already resolved URL
     /// for insertion in the cascade.
     pub fn for_cascade(url: ServoUrl, extra_data: UrlExtraData) -> Self {
         SpecifiedUrl {
             original: None,