Problem while serializing (doesn't write XML file when requested to)

[Roberto Rigamonti <ro...@gmail.com>]

Hi, I'm having some troubles with XML serializing. I'm working on a
university project that consists of a 3D visor that should receive its
input via XML files, validate them using a set of XSD files (I've
created this "baroque" architecture because I'd like to implement the
visor using a plugin architecture, where object that can be drawn are
plugins that can be loaded at runtime by the user). The problems arise
when I try to insert/remove from the main XSD schema the <include>
declarations of the schemas of the plugins: I load the XML file using
DOM, modify it as I need and rewrite it back on the file, but something
goes wrong and the XML tree isn't written back for a while and the file
has zero size for a bit, so when I access the file again to perform some
computations the root of the XML tree read from the file is NULL.
Instead of pasting here snippets of code, I've uploaded the entire
project so everything can be compiled and tested without much effort, it
can be downloaded from here:
http://www.roby1984.netsons.org/3DVisor-0.0.1.tar.bz2
I suggest to compile it using the following procedure:
$ configure --prefix=/(path/to/dir)/sandbox
$ make
$ make install
to avoid filling your system with buggy executables ;-)
The files involved in managing XML are XMLParser.cc, XMLParser.hh and
XMLDOMErrorReporter.hh, the program seems to ignore the target_->flush()
command at line 444 in XMLParser.cc (XMLParser::serializeXML()) and
leave an empty file until the next routine invoked by main(), that is,
addSchema(), loads the file and finds it empty.
I'm going mad on those errors, it's about two weeks I'm trying to
resolve this bug...
Thanks in advance to anyone willing to help me!
                                          Roberto

Re: Problem while serializing (doesn't write XML file when requested to)

[Roberto Rigamonti <ro...@gmail.com>]

David Bertoni wrote:
> The first thing I noted with your code is that it's full of memory leaks
> of this variety:
<snip>

You're right! I've totally missed them!

> Also, I would suggest that rather than transcoding these hard-coded
> strings at run-time, that you create versions of these strings that are
> already encoded in UTF-16 at compile time.  You can use the constants
> defined in xerces/util/XMLUniDefs.hpp.  For an example of what I mean,
> take a look at XMLUni.cpp in the same directory.

That's a good suggestion, I did so and now it works perfectly.

> The reason the file is empty is because you never destroy the
> LocalFileFormatTarget, so the underlying file is not closed until the
> process exits.  This is also a memory leak, and you have a memory leak
> when you construct it, because you call XMLString::transcode() and you
> don't release the pointer that's returned:
> 
> target_ = new LocalFileFormatTarget(XMLString::transcode(xmlFile.c_str()));
> 
> You can make your life easier by creating the LocalFileFormatTarget on
> the stack, instead of on the heap.  It's a small object, so there's no
> danger in overflowing the stack.

Same as above!

> A few other comments, while I'm on a roll:
> 
> void
> XMLParser::destroyInstance()
> {
>   if(instance_)
>     delete(instance_);
> 
>   instance_ = 0;
> }
> 
> It's never necessary in C++ to check a pointer for null before deleting
> it.  The compiler already has to do this, so why have your code do it too?

Doh! I read it on Parashift's FAQ too, but I was so busy trying to
resolve segmentation faults that I forgot it!

> if(current->getNodeType() != 0 && current->getNodeType() ==
> DOMNode::ELEMENT_NODE)
> 
> I'm not sure why you're checking for a node type that's not equal to 0
> then checking for one that's equal to DOMNode::ELEMENT_NODE.
> getNodeType() return an enum, and an enum can only have a single value,
> so the only test you need is for DOMNode::ELEMENT_NODE.

It's a remnant of previous experiments, it happens quite often to me
that I start with clean and well structured code and quickly degrade
it... Ok, I'm not a Real Programmer (TM) :P
Thank you very very much for your precious help, I would have gone crazy
trying to fix those bug by myself!!
                                Roberto

Re: Problem while serializing (doesn't write XML file when requested to)

[David Bertoni <db...@apache.org>]

Roberto Rigamonti wrote:
> Hi, I'm having some troubles with XML serializing. I'm working on a
> university project that consists of a 3D visor that should receive its
> input via XML files, validate them using a set of XSD files (I've
...
> Instead of pasting here snippets of code, I've uploaded the entire
> project so everything can be compiled and tested without much effort, it
> can be downloaded from here:
> http://www.roby1984.netsons.org/3DVisor-0.0.1.tar.bz2
> I suggest to compile it using the following procedure:
> $ configure --prefix=/(path/to/dir)/sandbox
> $ make
> $ make install
> to avoid filling your system with buggy executables ;-)
> The files involved in managing XML are XMLParser.cc, XMLParser.hh and
> XMLDOMErrorReporter.hh, the program seems to ignore the target_->flush()
> command at line 444 in XMLParser.cc (XMLParser::serializeXML()) and
> leave an empty file until the next routine invoked by main(), that is,
> addSchema(), loads the file and finds it empty.
> I'm going mad on those errors, it's about two weeks I'm trying to
> resolve this bug...
The first thing I noted with your code is that it's full of memory leaks 
of this variety:

XMLParser.cc:   DOMElement* addedElem = 
xmlDoc->createElement(XMLString::transcode("xsd:element"));

The pointer returned by XMLString::transcode() must be freed through a 
call to XMLString::release().  This is clear if you read the 
documentation for XMLString::transcode().

Also, I would suggest that rather than transcoding these hard-coded 
strings at run-time, that you create versions of these strings that are 
already encoded in UTF-16 at compile time.  You can use the constants 
defined in xerces/util/XMLUniDefs.hpp.  For an example of what I mean, 
take a look at XMLUni.cpp in the same directory.

The reason the file is empty is because you never destroy the 
LocalFileFormatTarget, so the underlying file is not closed until the 
process exits.  This is also a memory leak, and you have a memory leak 
when you construct it, because you call XMLString::transcode() and you 
don't release the pointer that's returned:

target_ = new LocalFileFormatTarget(XMLString::transcode(xmlFile.c_str()));

You can make your life easier by creating the LocalFileFormatTarget on 
the stack, instead of on the heap.  It's a small object, so there's no 
danger in overflowing the stack.

Note also that LocalFileFormatTarget has a constructor that takes a 
string in the local code page, so there's no need to do the transcoding 
yourself:

LocalFileFormatTarget target_(xmlFile.c_str());

A few other comments, while I'm on a roll:

void
XMLParser::destroyInstance()
{
   if(instance_)
     delete(instance_);

   instance_ = 0;
}

It's never necessary in C++ to check a pointer for null before deleting 
it.  The compiler already has to do this, so why have your code do it too?

if(current->getNodeType() != 0 && current->getNodeType() == 
DOMNode::ELEMENT_NODE)

I'm not sure why you're checking for a node type that's not equal to 0 
then checking for one that's equal to DOMNode::ELEMENT_NODE. 
getNodeType() return an enum, and an enum can only have a single value, 
so the only test you need is for DOMNode::ELEMENT_NODE.

DOMElement* element = dynamic_cast< DOMElement* >(current);
if(XMLString::equals(element->getTagName(),
                      XMLString::transcode(tagName.c_str()))) {

The dynamic_cast here is unnecessary, and wastes cycles.  You've already 
checked that it's a DOMElement node.  Also, the call to 
XMLString::transcode() is a memory leak.

Dave