[librsvg: 18/48] Rewrite the machinery for nested XML handlers in Rust

[Federico Mena Quintero <federico src gnome org>]

commit d934751b9976708095ad4d314fb2c024a32ed761
Author: Federico Mena Quintero <federico gnome org>
Date:   Fri Sep 7 10:12:11 2018 -0500

    Rewrite the machinery for nested XML handlers in Rust
    
    The C code is incorrect:  the end_element() functions in the vtables
    don't ever get called, because of the way the code checks for nested
    handlers.
    
    We now have a simpler scheme in Rust.
    
    Don't free the element_name_stack by hand from the C code.  Let the
    Rust code deal with that itself.  We'll change from using a stack of
    element names, to a stack of XmlContext.

 librsvg/rsvg-load.c       |  52 +++++----
 rsvg_internals/src/lib.rs |   7 +-
 rsvg_internals/src/xml.rs | 265 +++++++++++++++++++++++-----------------------
 3 files changed, 167 insertions(+), 157 deletions(-)
---
diff --git a/librsvg/rsvg-load.c b/librsvg/rsvg-load.c
index adae4b34..52e75ebb 100644
--- a/librsvg/rsvg-load.c
+++ b/librsvg/rsvg-load.c
@@ -52,10 +52,9 @@ typedef struct RsvgXmlState RsvgXmlState;
 extern RsvgXmlState *rsvg_xml_state_new ();
 extern void rsvg_xml_state_free (RsvgXmlState *xml);
 extern RsvgTree *rsvg_xml_state_steal_tree(RsvgXmlState *xml);
-extern void rsvg_xml_state_free_element_name_stack(RsvgXmlState *xml);
-extern void rsvg_xml_state_standard_element_start(RsvgXmlState *xml, RsvgHandle *handle, const char *name, 
RsvgPropertyBag atts);
-extern void rsvg_xml_state_standard_element_end(RsvgXmlState *xml, RsvgHandle *handle, const char *name);
-extern void rsvg_xml_state_add_characters(RsvgXmlState *xml, const char *characters, gsize len);
+extern void rsvg_xml_state_start_element(RsvgXmlState *xml, RsvgHandle *handle, const char *name, 
RsvgPropertyBag atts);
+extern void rsvg_xml_state_end_element(RsvgXmlState *xml, RsvgHandle *handle, const char *name);
+extern void rsvg_xml_state_characters(RsvgXmlState *xml, const char *unterminated_text, gsize len);
 
 
 /* Holds the XML parsing state */
@@ -280,7 +279,7 @@ xinclude_handler_characters (RsvgSaxHandler * self, const char *ch, gsize len)
     RsvgSaxHandlerXinclude *z = (RsvgSaxHandlerXinclude *) self;
 
     if (z->in_fallback) {
-        rsvg_xml_state_add_characters (z->load->xml.rust_state, ch, len);
+        rsvg_xml_state_characters (z->load->xml.rust_state, ch, len);
     }
 }
 
@@ -294,10 +293,10 @@ xinclude_handler_start (RsvgSaxHandler * self, const char *name, RsvgPropertyBag
             if (!strcmp (name, "xi:include"))
                 start_xinclude (z->load, atts);
             else {
-                rsvg_xml_state_standard_element_start (z->load->xml.rust_state,
-                                                       z->load->handle,
-                                                       (const char *) name,
-                                                       atts);
+                rsvg_xml_state_start_element (z->load->xml.rust_state,
+                                              z->load->handle,
+                                              (const char *) name,
+                                              atts);
             }
         } else if (!strcmp (name, "xi:fallback")) {
             z->in_fallback = TRUE;
@@ -496,7 +495,7 @@ start_xinclude (RsvgLoad *load, RsvgPropertyBag * atts)
                     data_len = text_data_len;
                 }
 
-                rsvg_xml_state_add_characters (load->xml.rust_state, data, data_len);
+                rsvg_xml_state_characters (load->xml.rust_state, data, data_len);
 
                 g_free (data);
 
@@ -553,27 +552,31 @@ sax_start_element_cb (void *data, const xmlChar * name, const xmlChar ** atts)
     RsvgLoad *load = data;
 
     bag = rsvg_property_bag_new ((const char **) atts);
-
+#if 0
     if (load->xml.handler) {
         load->xml.handler_nest++;
         load->xml.handler->start_element (load->xml.handler, (const char *) name, bag);
     } else {
+#endif
         const char *tempname;
         for (tempname = (const char *) name; *tempname != '\0'; tempname++)
             if (*tempname == ':')
                 name = (const xmlChar *) (tempname + 1);
-
+#if 0
         if (!strcmp ((const char *) name, "style"))
             start_style (load, bag);
         else if (!strcmp ((const char *) name, "include"))      /* xi:include */
             start_xinclude (load, bag);
         else {
-            rsvg_xml_state_standard_element_start (load->xml.rust_state,
-                                                   load->handle,
-                                                   (const char *) name,
-                                                   bag);
+#endif
+            rsvg_xml_state_start_element (load->xml.rust_state,
+                                          load->handle,
+                                          (const char *) name,
+                                          bag);
+#if 0
         }
     }
+#endif
 
     rsvg_property_bag_free (bag);
 }
@@ -584,23 +587,27 @@ sax_end_element_cb (void *data, const xmlChar * xmlname)
     RsvgLoad *load =  data;
     const char *name = (const char *) xmlname;
 
+#if 0
     if (load->xml.handler_nest > 0 && load->xml.handler != NULL) {
         load->xml.handler->end_element (load->xml.handler, name);
         load->xml.handler_nest--;
     } else {
+#endif
         const char *tempname;
 
         for (tempname = name; *tempname != '\0'; tempname++)
             if (*tempname == ':')
                 name = tempname + 1;
-
+#if 0
         if (load->xml.handler != NULL) {
             load->xml.handler->free (load->xml.handler);
             load->xml.handler = NULL;
         }
-
-        rsvg_xml_state_standard_element_end (load->xml.rust_state, load->handle, name);
+#endif
+        rsvg_xml_state_end_element (load->xml.rust_state, load->handle, name);
+#if 0
     }
+#endif
 }
 
 static void
@@ -608,12 +615,13 @@ sax_characters_cb (void *data, const xmlChar * ch, int len)
 {
     RsvgLoad *load = data;
 
+#if 0
     if (load->xml.handler) {
         load->xml.handler->characters (load->xml.handler, (const char *) ch, (gsize) len);
         return;
     }
-
-    rsvg_xml_state_add_characters (load->xml.rust_state, (const char *) ch, (gsize) len);
+#endif
+    rsvg_xml_state_characters (load->xml.rust_state, (const char *) ch, (gsize) len);
 }
 
 static xmlEntityPtr
@@ -907,8 +915,6 @@ close_impl (RsvgLoad *load, GError ** error)
         load->xml.ctxt = free_xml_parser_and_doc (load->xml.ctxt);
     }
 
-    rsvg_xml_state_free_element_name_stack (load->xml.rust_state);
-
     load->error = NULL;
 
     if (real_error != NULL) {
diff --git a/rsvg_internals/src/lib.rs b/rsvg_internals/src/lib.rs
index 420ddf8d..227167d3 100644
--- a/rsvg_internals/src/lib.rs
+++ b/rsvg_internals/src/lib.rs
@@ -65,12 +65,11 @@ pub use property_bag::{
 pub use structure::rsvg_node_svg_get_size;
 
 pub use xml::{
-    rsvg_xml_state_add_characters,
+    rsvg_xml_state_characters,
+    rsvg_xml_state_end_element,
     rsvg_xml_state_free,
-    rsvg_xml_state_free_element_name_stack,
     rsvg_xml_state_new,
-    rsvg_xml_state_standard_element_end,
-    rsvg_xml_state_standard_element_start,
+    rsvg_xml_state_start_element,
     rsvg_xml_state_steal_tree,
 };
 
diff --git a/rsvg_internals/src/xml.rs b/rsvg_internals/src/xml.rs
index eb1f1d51..fa49f5a0 100644
--- a/rsvg_internals/src/xml.rs
+++ b/rsvg_internals/src/xml.rs
@@ -15,25 +15,118 @@ use util::utf8_cstr;
 
 /// A trait for processing a certain kind of XML subtree
 ///
-/// In the "normal" state of processing, an `XmlContext` may create an RsvgNode
+/// In the "normal" state of processing, an `XmlHandler` may create an RsvgNode
 /// for each SVG element it finds, and create NodeChars inside those nodes when it
 /// encounters character data.
 ///
 /// There may be other, special contexts for different subtrees, for example,
 /// for the `<style>` element.
-trait XmlContext {
+trait XmlHandler {
     /// Called when the XML parser sees the beginning of an element
-    fn start_element(&mut self, handle: *mut RsvgHandle, name: &str, pbag: &PropertyBag);
+    fn start_element(
+        &self,
+        parent: Option<&Rc<Node>>,
+        handle: *mut RsvgHandle,
+        name: &str,
+        pbag: &PropertyBag,
+    ) -> Box<XmlHandler>;
 
-    /// Called when the XML parser sees the end of an element
-    fn end_element(&mut self, handle: *mut RsvgHandle, name: &str);
+    /// Called when the XML parser sees the end of an element.
+    fn end_element(&self, handle: *mut RsvgHandle, name: &str) -> Rc<Node>;
 
     /// Called when the XML parser sees character data or CDATA
-    fn characters(&mut self, text: &str);
+    fn characters(&self, text: &str);
+
+    fn get_node(&self) -> Rc<Node>;
+}
+
+struct NodeCreationContext {
+    node: Option<Rc<Node>>,
+}
+
+impl XmlHandler for NodeCreationContext {
+    fn start_element(
+        &self,
+        parent: Option<&Rc<Node>>,
+        handle: *mut RsvgHandle,
+        name: &str,
+        pbag: &PropertyBag,
+    ) -> Box<XmlHandler> {
+        let mut defs = handle::get_defs(handle);
+        let mut is_svg = false;
+
+        let new_node = rsvg_load_new_node(name, parent, pbag, &mut defs, &mut is_svg);
+
+        if let Some(parent) = parent {
+            parent.add_child(&new_node);
+        }
+
+        new_node.set_atts(&new_node, handle, pbag);
+
+        // The "svg" node is special; it will parse its style attributes
+        // until the end, in standard_element_end().
+        if new_node.get_type() != NodeType::Svg {
+            new_node.parse_style_attributes(handle, name, pbag);
+        }
+
+        new_node.set_overridden_properties();
+
+        Box::new(NodeCreationContext {
+            node: Some(new_node),
+        })
+    }
+
+    fn end_element(&self, handle: *mut RsvgHandle, _name: &str) -> Rc<Node> {
+        let node = self.node.as_ref().unwrap().clone();
+
+        // The "svg" node is special; it parses its style attributes
+        // here, not during element creation.
+        if node.get_type() == NodeType::Svg {
+            node.with_impl(|svg: &NodeSvg| {
+                svg.parse_style_attributes(&node, handle);
+            });
+        }
+
+        node
+    }
+
+    fn characters(&self, text: &str) {
+        let node = self.node.as_ref().unwrap();
+
+        if text.len() == 0 {
+            return;
+        }
+
+        if node.accept_chars() {
+            let chars_node = if let Some(child) = node.find_last_chars_child() {
+                child
+            } else {
+                let child = node_new(
+                    NodeType::Chars,
+                    Some(&node),
+                    None,
+                    None,
+                    Box::new(NodeChars::new()),
+                );
+                node.add_child(&child);
+                child
+            };
+
+            chars_node.with_impl(|chars: &NodeChars| {
+                chars.append(text);
+            });
+        }
+    }
+
+    fn get_node(&self) -> Rc<Node> {
+        self.node.as_ref().unwrap().clone()
+    }
+}
 
-    /// Called when the context terminates, i.e. when the parent element
-    /// that created this context is closed.
-    fn finish(&mut self);
+/// A concrete parsing context for a surrounding `element_name` and its XML event handlers
+struct Context {
+    element_name: String,
+    handler: Box<XmlHandler>,
 }
 
 // A *const RsvgXmlState is just the type that we export to C
@@ -41,19 +134,15 @@ pub enum RsvgXmlState {}
 
 struct XmlState {
     tree: Option<Box<Tree>>,
-    current_node: Option<Rc<Node>>,
 
-    // Stack of element names while parsing; used to know when to stop
-    // parsing the current element.
-    element_name_stack: Vec<String>,
+    context_stack: Vec<Context>,
 }
 
 impl XmlState {
     fn new() -> XmlState {
         XmlState {
             tree: None,
-            current_node: None,
-            element_name_stack: Vec::new(),
+            context_stack: Vec::new(),
         }
     }
 
@@ -69,119 +158,43 @@ impl XmlState {
         self.tree.take()
     }
 
-    pub fn set_current_node(&mut self, node: Option<Rc<Node>>) {
-        self.current_node = node;
-    }
-
-    pub fn push_element_name(&mut self, name: &str) {
-        self.element_name_stack.push(name.to_string());
-    }
-
-    pub fn pop_element_name(&mut self) {
-        self.element_name_stack.pop();
-    }
-
-    pub fn topmost_element_name_is(&mut self, name: &str) -> bool {
-        let len = self.element_name_stack.len();
-
-        if len > 0 {
-            self.element_name_stack[len - 1] == name
+    pub fn start_element(&mut self, handle: *mut RsvgHandle, name: &str, pbag: &PropertyBag) {
+        let next_context = if let Some(top) = self.context_stack.last() {
+            top.handler
+                .start_element(Some(&top.handler.get_node()), handle, name, pbag)
         } else {
-            false
-        }
-    }
-
-    pub fn free_element_name_stack(&mut self) {
-        self.element_name_stack.clear();
-    }
-
-    /// Starts a node for an SVG element of type `name` and hooks it to the tree.
-    ///
-    /// `pbag` is the set of key/value pairs from the element's XML attributes.
-    pub fn standard_element_start(
-        &mut self,
-        handle: *mut RsvgHandle,
-        name: &str,
-        pbag: &PropertyBag,
-    ) {
-        let mut defs = handle::get_defs(handle);
-        let mut is_svg = false;
-
-        let new_node = rsvg_load_new_node(
-            name,
-            self.current_node.as_ref(),
-            pbag,
-            &mut defs,
-            &mut is_svg,
-        );
-
-        self.push_element_name(name);
-
-        if let Some(ref current_node) = self.current_node {
-            current_node.add_child(&new_node);
-        } else if is_svg {
-            self.set_root(&new_node);
-        }
-
-        self.set_current_node(Some(new_node.clone()));
+            let default_context = NodeCreationContext { node: None };
 
-        new_node.set_atts(&new_node, handle, pbag);
+            default_context.start_element(None, handle, name, pbag)
+        };
 
-        // The "svg" node is special; it will parse its style attributes
-        // until the end, in standard_element_end().
-        if new_node.get_type() != NodeType::Svg {
-            new_node.parse_style_attributes(handle, name, pbag);
-        }
+        let context = Context {
+            element_name: name.to_string(),
+            handler: next_context,
+        };
 
-        new_node.set_overridden_properties();
+        self.context_stack.push(context);
     }
 
-    /// Ends an SVG element for which we create a node.
-    pub fn standard_element_end(&mut self, handle: *mut RsvgHandle, name: &str) {
-        if let Some(ref current_node) = self.current_node.clone() {
-            // The "svg" node is special; it parses its style attributes
-            // here, not during element creation.
-            if current_node.get_type() == NodeType::Svg {
-                current_node.with_impl(|svg: &NodeSvg| {
-                    svg.parse_style_attributes(current_node, handle);
-                });
-            }
-
-            if self.topmost_element_name_is(name) {
-                let parent = current_node.get_parent();
+    pub fn end_element(&mut self, handle: *mut RsvgHandle, name: &str) {
+        if let Some(top) = self.context_stack.pop() {
+            assert!(name == top.element_name);
 
-                self.set_current_node(parent);
+            let node = top.handler.end_element(handle, name);
 
-                self.pop_element_name();
+            if self.context_stack.is_empty() {
+                self.set_root(&node);
             }
+        } else {
+            panic!("end_element: XML handler stack is empty!?");
         }
     }
 
-    pub fn add_characters(&mut self, text: &str) {
-        if text.len() == 0 {
-            return;
-        }
-
-        if let Some(ref current_node) = self.current_node {
-            if current_node.accept_chars() {
-                let chars_node = if let Some(child) = current_node.find_last_chars_child() {
-                    child
-                } else {
-                    let child = node_new(
-                        NodeType::Chars,
-                        self.current_node.as_ref(),
-                        None,
-                        None,
-                        Box::new(NodeChars::new()),
-                    );
-                    current_node.add_child(&child);
-                    child
-                };
-
-                chars_node.with_impl(|chars: &NodeChars| {
-                    chars.append(text);
-                });
-            }
+    pub fn characters(&mut self, text: &str) {
+        if let Some(top) = self.context_stack.last() {
+            top.handler.characters(text);
+        } else {
+            panic!("characters: XML handler stack is empty!?");
         }
     }
 }
@@ -213,15 +226,7 @@ pub extern "C" fn rsvg_xml_state_steal_tree(xml: *mut RsvgXmlState) -> *mut Rsvg
 }
 
 #[no_mangle]
-pub extern "C" fn rsvg_xml_state_free_element_name_stack(xml: *mut RsvgXmlState) {
-    assert!(!xml.is_null());
-    let xml = unsafe { &mut *(xml as *mut XmlState) };
-
-    xml.free_element_name_stack();
-}
-
-#[no_mangle]
-pub extern "C" fn rsvg_xml_state_standard_element_start(
+pub extern "C" fn rsvg_xml_state_start_element(
     xml: *mut RsvgXmlState,
     handle: *mut RsvgHandle,
     name: *const libc::c_char,
@@ -236,11 +241,11 @@ pub extern "C" fn rsvg_xml_state_standard_element_start(
     assert!(!pbag.is_null());
     let pbag = unsafe { &*pbag };
 
-    xml.standard_element_start(handle, name, pbag);
+    xml.start_element(handle, name, pbag);
 }
 
 #[no_mangle]
-pub extern "C" fn rsvg_xml_state_standard_element_end(
+pub extern "C" fn rsvg_xml_state_end_element(
     xml: *mut RsvgXmlState,
     handle: *mut RsvgHandle,
     name: *const libc::c_char,
@@ -251,11 +256,11 @@ pub extern "C" fn rsvg_xml_state_standard_element_end(
     assert!(!name.is_null());
     let name = unsafe { utf8_cstr(name) };
 
-    xml.standard_element_end(handle, name);
+    xml.end_element(handle, name);
 }
 
 #[no_mangle]
-pub extern "C" fn rsvg_xml_state_add_characters(
+pub extern "C" fn rsvg_xml_state_characters(
     xml: *mut RsvgXmlState,
     unterminated_text: *const libc::c_char,
     len: usize,
@@ -270,5 +275,5 @@ pub extern "C" fn rsvg_xml_state_add_characters(
     let bytes = unsafe { std::slice::from_raw_parts(unterminated_text as *const u8, len) };
     let utf8 = unsafe { str::from_utf8_unchecked(bytes) };
 
-    xml.add_characters(utf8);
+    xml.characters(utf8);
 }