[librsvg] svg: improve lookup error handling

[Federico Mena Quintero <federico src gnome org>]

commit e3c9d05a4828e9532015ad87732010a08aceda2c
Author: Paolo Borelli <pborelli gnome org>
Date:   Tue Jan 15 18:42:11 2019 +0100

    svg: improve lookup error handling
    
    Store full Result<>s in the hash tables, this way we can return
    errors when the resources are used.

 rsvg_internals/src/drawing_ctx.rs   |   8 +-
 rsvg_internals/src/error.rs         |   3 +
 rsvg_internals/src/filters/image.rs |  12 +--
 rsvg_internals/src/image.rs         | 144 ++++++++++++++++++------------------
 rsvg_internals/src/svg.rs           |  60 ++++++++-------
 5 files changed, 117 insertions(+), 110 deletions(-)
---
diff --git a/rsvg_internals/src/drawing_ctx.rs b/rsvg_internals/src/drawing_ctx.rs
index 1348fe04..45084ffc 100644
--- a/rsvg_internals/src/drawing_ctx.rs
+++ b/rsvg_internals/src/drawing_ctx.rs
@@ -263,7 +263,7 @@ impl DrawingCtx {
     // acquire it again.  If you acquire a node "#foo" and don't release it before
     // trying to acquire "foo" again, you will obtain a %NULL the second time.
     pub fn get_acquired_node(&mut self, fragment: &Fragment) -> Option<AcquiredNode> {
-        if let Some(node) = self.svg.lookup(fragment) {
+        if let Ok(node) = self.svg.lookup(fragment) {
             if !self.acquired_nodes_contains(&node) {
                 self.acquired_nodes.borrow_mut().push(node.clone());
                 let acq = AcquiredNode(self.acquired_nodes.clone(), node.clone());
@@ -714,8 +714,10 @@ impl DrawingCtx {
         }
     }
 
-    pub fn lookup_image(&self, href: &str) -> Option<SharedImageSurface> {
-        self.svg.lookup_image(href)
+    pub fn lookup_image(&self, href: &str) -> Result<SharedImageSurface, RenderingError> {
+        self.svg
+            .lookup_image(href)
+            .map_err(|_| RenderingError::InvalidHref)
     }
 
     pub fn draw_node_on_surface(
diff --git a/rsvg_internals/src/error.rs b/rsvg_internals/src/error.rs
index 853f35de..b763d1d2 100644
--- a/rsvg_internals/src/error.rs
+++ b/rsvg_internals/src/error.rs
@@ -103,6 +103,7 @@ pub enum RenderingError {
     CircularReference,
     InstancingLimit,
     InvalidId(DefsLookupErrorKind),
+    InvalidHref,
     SvgHasNoSize,
     OutOfMemory,
 }
@@ -241,6 +242,7 @@ impl error::Error for RenderingError {
             RenderingError::CircularReference => "circular reference",
             RenderingError::InstancingLimit => "instancing limit",
             RenderingError::InvalidId(_) => "invalid id",
+            RenderingError::InvalidHref => "invalid href",
             RenderingError::SvgHasNoSize => "svg has no size",
             RenderingError::OutOfMemory => "out of memory",
         }
@@ -254,6 +256,7 @@ impl fmt::Display for RenderingError {
             RenderingError::InvalidId(ref id) => write!(f, "invalid id: {:?}", id),
             RenderingError::CircularReference
             | RenderingError::InstancingLimit
+            | RenderingError::InvalidHref
             | RenderingError::SvgHasNoSize
             | RenderingError::OutOfMemory => write!(f, "{}", self.description()),
         }
diff --git a/rsvg_internals/src/filters/image.rs b/rsvg_internals/src/filters/image.rs
index 3dda5c7b..a79170da 100644
--- a/rsvg_internals/src/filters/image.rs
+++ b/rsvg_internals/src/filters/image.rs
@@ -117,18 +117,14 @@ impl Image {
         href: &Href,
     ) -> Result<ImageSurface, FilterError> {
         let surface = if let Href::PlainUrl(ref url) = *href {
-            draw_ctx.lookup_image(&url)
+            // FIXME: translate the error better here
+            draw_ctx
+                .lookup_image(&url)
+                .map_err(|_| FilterError::InvalidInput)?
         } else {
             unreachable!();
         };
 
-        // FIXME: translate the error better here
-        if surface.is_none() {
-            return Err(FilterError::InvalidInput);
-        }
-
-        let surface = surface.unwrap();
-
         let output_surface = ImageSurface::create(
             cairo::Format::ARgb32,
             ctx.source_graphic().width(),
diff --git a/rsvg_internals/src/image.rs b/rsvg_internals/src/image.rs
index 60fa18bd..ab551276 100644
--- a/rsvg_internals/src/image.rs
+++ b/rsvg_internals/src/image.rs
@@ -83,89 +83,85 @@ impl NodeTrait for NodeImage {
         clipping: bool,
     ) -> Result<(), RenderingError> {
         let surface = if let Some(Href::PlainUrl(ref url)) = *self.href.borrow() {
-            draw_ctx.lookup_image(&url)
+            draw_ctx.lookup_image(&url)?
         } else {
-            None
+            return Ok(());
         };
 
-        if let Some(ref surface) = surface {
-            let values = cascaded.get();
-            let params = draw_ctx.get_view_params();
+        let values = cascaded.get();
+        let params = draw_ctx.get_view_params();
 
-            let x = self.x.get().normalize(values, &params);
-            let y = self.y.get().normalize(values, &params);
-            let w = self.w.get().normalize(values, &params);
-            let h = self.h.get().normalize(values, &params);
+        let x = self.x.get().normalize(values, &params);
+        let y = self.y.get().normalize(values, &params);
+        let w = self.w.get().normalize(values, &params);
+        let h = self.h.get().normalize(values, &params);
 
-            if w.approx_eq_cairo(&0.0) || h.approx_eq_cairo(&0.0) {
-                return Ok(());
-            }
+        if w.approx_eq_cairo(&0.0) || h.approx_eq_cairo(&0.0) {
+            return Ok(());
+        }
 
-            draw_ctx.with_discrete_layer(node, values, clipping, &mut |dc| {
-                let aspect = self.aspect.get();
+        draw_ctx.with_discrete_layer(node, values, clipping, &mut |dc| {
+            let aspect = self.aspect.get();
 
-                if !values.is_overflow() && aspect.is_slice() {
-                    dc.clip(x, y, w, h);
-                }
+            if !values.is_overflow() && aspect.is_slice() {
+                dc.clip(x, y, w, h);
+            }
 
-                let width = surface.width();
-                let height = surface.height();
-                if clipping || width == 0 || height == 0 {
-                    return Ok(());
-                }
+            let width = surface.width();
+            let height = surface.height();
+            if clipping || width == 0 || height == 0 {
+                return Ok(());
+            }
 
-                // The bounding box for <image> is decided by the values of x, y, w, h and not by
-                // the final computed image bounds.
-                let bbox = BoundingBox::new(&dc.get_cairo_context().get_matrix()).with_rect(Some(
-                    cairo::Rectangle {
-                        x,
-                        y,
-                        width: w,
-                        height: h,
-                    },
-                ));
-
-                let width = f64::from(width);
-                let height = f64::from(height);
-
-                let (x, y, w, h) = aspect.compute(width, height, x, y, w, h);
-
-                let cr = dc.get_cairo_context();
-
-                cr.save();
-
-                dc.set_affine_on_cr(&cr);
-                cr.scale(w / width, h / height);
-                let x = x * width / w;
-                let y = y * height / h;
-
-                // We need to set extend appropriately, so can't use cr.set_source_surface().
-                //
-                // If extend is left at its default value (None), then bilinear scaling uses
-                // transparency outside of the image producing incorrect results.
-                // For example, in svg1.1/filters-blend-01-b.svgthere's a completely
-                // opaque 100×1 image of a gradient scaled to 100×98 which ends up
-                // transparent almost everywhere without this fix (which it shouldn't).
-                let ptn = surface.to_cairo_pattern();
-                let mut matrix = cairo::Matrix::identity();
-                matrix.translate(-x, -y);
-                ptn.set_matrix(matrix);
-                ptn.set_extend(cairo::Extend::Pad);
-                cr.set_source(&ptn);
-
-                // Clip is needed due to extend being set to pad.
-                cr.rectangle(x, y, width, height);
-                cr.clip();
-
-                cr.paint();
-
-                cr.restore();
-
-                dc.insert_bbox(&bbox);
-                Ok(())
-            })
-        } else {
+            // The bounding box for <image> is decided by the values of x, y, w, h and not by
+            // the final computed image bounds.
+            let bbox = BoundingBox::new(&dc.get_cairo_context().get_matrix()).with_rect(Some(
+                cairo::Rectangle {
+                    x,
+                    y,
+                    width: w,
+                    height: h,
+                },
+            ));
+
+            let width = f64::from(width);
+            let height = f64::from(height);
+
+            let (x, y, w, h) = aspect.compute(width, height, x, y, w, h);
+
+            let cr = dc.get_cairo_context();
+
+            cr.save();
+
+            dc.set_affine_on_cr(&cr);
+            cr.scale(w / width, h / height);
+            let x = x * width / w;
+            let y = y * height / h;
+
+            // We need to set extend appropriately, so can't use cr.set_source_surface().
+            //
+            // If extend is left at its default value (None), then bilinear scaling uses
+            // transparency outside of the image producing incorrect results.
+            // For example, in svg1.1/filters-blend-01-b.svgthere's a completely
+            // opaque 100×1 image of a gradient scaled to 100×98 which ends up
+            // transparent almost everywhere without this fix (which it shouldn't).
+            let ptn = surface.to_cairo_pattern();
+            let mut matrix = cairo::Matrix::identity();
+            matrix.translate(-x, -y);
+            ptn.set_matrix(matrix);
+            ptn.set_extend(cairo::Extend::Pad);
+            cr.set_source(&ptn);
+
+            // Clip is needed due to extend being set to pad.
+            cr.rectangle(x, y, width, height);
+            cr.clip();
+
+            cr.paint();
+
+            cr.restore();
+
+            dc.insert_bbox(&bbox);
             Ok(())
-        }
+        })
     }
 }
diff --git a/rsvg_internals/src/svg.rs b/rsvg_internals/src/svg.rs
index a07222f0..a9a3151e 100644
--- a/rsvg_internals/src/svg.rs
+++ b/rsvg_internals/src/svg.rs
@@ -67,13 +67,14 @@ impl Svg {
         self.tree.clone()
     }
 
-    pub fn lookup(&self, fragment: &Fragment) -> Option<RsvgNode> {
+    pub fn lookup(&self, fragment: &Fragment) -> Result<RsvgNode, LoadingError> {
         if fragment.uri().is_some() {
             self.externs
                 .borrow_mut()
                 .lookup(&self.load_options, fragment)
         } else {
             self.lookup_node_by_id(fragment.fragment())
+                .ok_or(LoadingError::BadUrl)
         }
     }
 
@@ -81,13 +82,13 @@ impl Svg {
         self.ids.get(id).map(|n| (*n).clone())
     }
 
-    pub fn lookup_image(&self, href: &str) -> Option<SharedImageSurface> {
+    pub fn lookup_image(&self, href: &str) -> Result<SharedImageSurface, LoadingError> {
         self.images.borrow_mut().lookup(&self.load_options, href)
     }
 }
 
 struct Resources {
-    resources: HashMap<AllowedUrl, Rc<Svg>>,
+    resources: HashMap<AllowedUrl, Result<Rc<Svg>, LoadingError>>,
 }
 
 impl Resources {
@@ -97,36 +98,42 @@ impl Resources {
         }
     }
 
-    pub fn lookup(&mut self, load_options: &LoadOptions, fragment: &Fragment) -> Option<RsvgNode> {
+    pub fn lookup(
+        &mut self,
+        load_options: &LoadOptions,
+        fragment: &Fragment,
+    ) -> Result<RsvgNode, LoadingError> {
         if let Some(ref href) = fragment.uri() {
-            // FIXME: propagate errors from the loader
-            match self.get_extern_svg(load_options, href) {
-                Ok(svg) => svg.lookup_node_by_id(fragment.fragment()),
-
-                Err(()) => None,
-            }
+            self.get_extern_svg(load_options, href).and_then(|svg| {
+                svg.lookup_node_by_id(fragment.fragment())
+                    .ok_or(LoadingError::BadUrl)
+            })
         } else {
             unreachable!();
         }
     }
 
-    fn get_extern_svg(&mut self, load_options: &LoadOptions, href: &str) -> Result<Rc<Svg>, ()> {
-        let aurl = AllowedUrl::from_href(href, load_options.base_url.as_ref()).map_err(|_| ())?;
+    fn get_extern_svg(
+        &mut self,
+        load_options: &LoadOptions,
+        href: &str,
+    ) -> Result<Rc<Svg>, LoadingError> {
+        let aurl = AllowedUrl::from_href(href, load_options.base_url.as_ref())
+            .map_err(|_| LoadingError::BadUrl)?;
 
         match self.resources.entry(aurl) {
-            Entry::Occupied(e) => Ok(e.get().clone()),
+            Entry::Occupied(e) => e.get().clone(),
             Entry::Vacant(e) => {
-                // FIXME: propagate errors
-                let svg = load_svg(load_options, e.key()).map_err(|_| ())?;
-                let rc_svg = e.insert(Rc::new(svg));
-                Ok(rc_svg.clone())
+                let svg = load_svg(load_options, e.key()).map(|s| Rc::new(s));
+                let res = e.insert(svg);
+                res.clone()
             }
         }
     }
 }
 
 struct Images {
-    images: HashMap<AllowedUrl, SharedImageSurface>,
+    images: HashMap<AllowedUrl, Result<SharedImageSurface, LoadingError>>,
 }
 
 impl Images {
@@ -136,17 +143,20 @@ impl Images {
         }
     }
 
-    pub fn lookup(&mut self, load_options: &LoadOptions, href: &str) -> Option<SharedImageSurface> {
-        // FIXME: propagate errors
-        let aurl = AllowedUrl::from_href(href, load_options.base_url.as_ref()).ok()?;
+    pub fn lookup(
+        &mut self,
+        load_options: &LoadOptions,
+        href: &str,
+    ) -> Result<SharedImageSurface, LoadingError> {
+        let aurl = AllowedUrl::from_href(href, load_options.base_url.as_ref())
+            .map_err(|_| LoadingError::BadUrl)?;
 
         match self.images.entry(aurl) {
-            Entry::Occupied(e) => Some(e.get().clone()),
+            Entry::Occupied(e) => e.get().clone(),
             Entry::Vacant(e) => {
-                // FIXME: propagate errors
-                let surface = load_image(load_options, e.key()).ok()?;
+                let surface = load_image(load_options, e.key());
                 let res = e.insert(surface);
-                Some(res.clone())
+                res.clone()
             }
         }
     }