Re: Unresolved Issues with libxml2

[Doug Simons]

Hello Fred,

Thanks for all of your work on the XML classes, and for your summary of the 
areas that still need attention. My original issue #1 (at the beginning of this 
thread) is still outstanding as well. My issues #2 and #3 have been resolved -- 
thanks!

As it happens, I managed to get back to this yesterday, and spent most of 
yesterday and today trying to isolate the memory crash I was seeing. It turns 
out there were (unfortunately) a lot of red herrings in my earlier description 
of the sequence that leads to the crash, and obviously some missing steps or 
you would have been able to reproduce it. I was finally able to reproduce the 
crash myself and create a minimal test case for this problem.

Unfortunately, I've now spent too much time on this to do much more with it. I 
appreciate that you've spent a lot of time on this code, too, but I hope you 
might be able to figure out how to resolve this problem. If not, we will revert 
to a slightly earlier version of the NSXML* code for now, which doesn't crash 
in this way.

Regards,

Doug

Here is my crashTest:

#import "ObjectTesting.h"
#import <Foundation/NSAutoreleasePool.h>
#import <Foundation/NSXMLDocument.h>
#import <Foundation/NSXMLElement.h>

int main()
{
  NSAutoreleasePool *arp = [NSAutoreleasePool new];
  NSXMLDocument *node;
  NSXMLDocument *node2;
  NSXMLElement *elem;
  NSXMLElement *elem2;
  NSXMLNode *child;
  NSXMLNode *child2;

  NSString *simpleXML = @"<num>6</num>";

  // create two documents (containing root elements with the same name ("num") 
-- may be significant?)
  node = [[NSXMLDocument alloc] initWithXMLString:simpleXML
                                          options:0
                                            error:NULL];
  PASS(node != nil, "document was initialized from a string");

  node2 = [[NSXMLDocument alloc] initWithXMLString:simpleXML
                                           options:0
                                             error:NULL];
  PASS(node2 != nil, "document 2 was initialized from a string");

  // detach the root elements from their documents
  elem = [node rootElement];
//  PASS_EQUAL([elem XMLString], simpleXML, "root element is correct");
  [elem detach];

  elem2 = [node2 rootElement];
//  PASS_EQUAL([elem2 XMLString], simpleXML, "root element 2 is correct");
  [elem2 detach];

  // now, simply accessing the text node child of each element leads to a CRASH
  child = [elem childAtIndex:0];
  child2 = [elem2 childAtIndex:0];
  
  [node release];
  [node2 release];

  [arp release];
  arp = nil;

  return 0;
}

On Apr 3, 2012, at 12:58 AM, Fred Kiefer wrote:

> Doug, you never replied to this mail. This hopefully means all your issues 
> were resolved.
> 
> 
> In the meantime I added a few more features to our libxml2 wrapper, but wont 
> have much time to work on this area in the future. I also found another nice 
> implementation of some of these classes: KissXML 
> (https://github.com/robbiehanson/KissXML). They do things quite differently 
> from our approach and in many respects these classes are more complete. (In 
> others I definitely prefer our code)
> 
> There are still some open issues with our NSXML classes. We should do a lot 
> more checks, whether we are actually dealing with the right sort of node. 
> This is especially true for the NSXMLDTDNode class, where only the handling 
> of entity declarations has been implemented. For all other node types this 
> will horribly fail.
> We also need a complete rewrite of the namespace handling. The current code 
> works well for all the simple cases, but it is easy to construct code where 
> we differ from Apples results.
> The namespace nodes don't represent a hierarchy, which means the methods 
> level and index return nonsense.
> The code I have in place for older versions of libxml2 when transferring a 
> node to a different document, wont work when the old document goes away. 
> Somebody needs to write a replacement here. (Maybe even by replacing the DOM 
> functions from libxml2 I am using, because they have a few surprising side 
> effects)
> The isEqual: implementation needs a review.
> The sub-node handling needs a lot more testing, only when running KissXML 
> test code did I learn that setStringValue: will destroy sub-nodes!
> And of course we don't have anythign in place for XML Query.
> 
> Overall you need to be careful when using this code as a lot of memory issues 
> may still lurk there. The code will need plenty of testing and should be 
> taken over by somebody with an actual use case in this area.
> 
> And when all these issues are taken care of, there is still plenty of room to 
> optimise the code.
> 
> All of this sounds overly negative, the code is usable and I hope, it is in a 
> lot better shape than it was before.
> 
> Fred
> 
> On 23.03.2012 10:40, Fred Kiefer wrote:
>> Hi Doug,
>> 
>> thank you for taking the time to test the new XML implementation in detail.
>> 
>> On 23.03.2012 01:24, Doug Simons wrote:
>>> Hi Fred,
>>> 
>>> I finally found some time to try the XML code with your recent
>>> changes. First, I appreciate all of the work you've done to finish up
>>> the parts of this code that we weren't able to complete in the
>>> initial implementation. Also, I can confirm that the change you made
>>> to implement prettyPrint seems to be working for me. The code you
>>> implemented for NSXMLNodeCompactEmptyElement is performing the right
>>> operation but in the opposite direction. So I reversed the logic and
>>> also added a test for the complementary NSXMLNodeExpandEmptyElement
>>> flag that (for some reason) Cocoa also defines. If neither flag is
>>> set, it defaults to the expanded form, which I believe is the default
>>> in Cocoa.
>> 
>> I think you are correct here. Could you please go ahead and commit your
>> changes. I am not sure I did get all the details from your description.
>> 
>>> Some things seem broken, though. The first is that the change you
>>> made in -copyWithZone: (passing a value of 2 rather than 1 to the
>>> xmlCopyNode() function) stops it from copying the node's children. So
>>> I've changed that back to 1, which does a deep copy.
>> 
>> Once again you are right. This change was a bug. I did not understand
>> the results of the different values and relied on the online
>> documentation of libxml2, which doesn't really help here. I made that
>> change in the code myself already.
>> 
>>> More serious is that I am getting a crash when dealloc'ing objects in
>>> some situations. I haven't fully tracked it down but I suspect it
>>> might be related to the code you added in the detach method that
>>> creates a new document and calls xmlDOMWrapAdoptNode(). I'm not sure
>>> what all of that does, but in debugging I noticed that I ended up
>>> with what looks to me like a malformed libxml node tree. I have a
>>> text node that has a parent and a document:
>>> 
>>> (gdb) p *node $24 = {_private = 0x0, type = XML_TEXT_NODE, name =
>>> 0x4650d0 "text", children = 0x0, last = 0x0, parent = 0x88e52f8, next
>>> = 0x0, prev = 0x0, doc = 0x88e4ee0, ns = 0x0, content = 0x88e4f68
>>> "7", properties = 0x0, nsDef = 0x0, psvi = 0x0, line = 1, extra = 0}
>>> 
>>> Its parent is an element with a document, but no parent:
>>> 
>>> (gdb) p *node.parent $27 = {_private = 0x88e4e44, type =
>>> XML_ELEMENT_NODE, name = 0x88e4f58 "num", children = 0x88e5338, last
>>> = 0x88e5338, parent = 0x0, next = 0x0, prev = 0x0, doc = 0x88e4ee0,
>>> ns = 0x0, content = 0x0, properties = 0x0, nsDef = 0x0, psvi = 0x0,
>>> line = 0, extra = 0}
>>> 
>>> And the document that both of them belong to has no children: (gdb) p
>>> *node.doc $26 = {_private = 0x88e1c94, type = XML_DOCUMENT_NODE, name
>>> = 0x0, children = 0x0, last = 0x0, parent = 0x0, next = 0x0, prev =
>>> 0x0, doc = 0x88e4ee0, compression = -1, standalone = -1, intSubset =
>>> 0x0, extSubset = 0x0, oldNs = 0x0, version = 0x88e4f48 "1.0",
>>> encoding = 0x0, ids = 0x0, refs = 0x0, URL = 0x0, charset = 1, dict =
>>> 0x0, psvi = 0x0, parseFlags = 0, properties = 32}
>>> 
>>> This seems like a bad state of affairs. If nothing else, it means
>>> that the rootDocument method will return an object that has no
>>> reference to its children. And I suspect that somehow this is leading
>>> up to the crash which occurs when the NSXMLDocument is dealloc'ed:
>> 
>> I should explain why I am using this private documents on nodes and
>> attributes (although I already send a partial explanation in a previous
>> mail). These private documents are used for two purposes. The first on
>> deals with strings while the second one has to do with namespaces.
>> 
>> libxml2 tries to optimise the memory usage of strings. In XML the same
>> tags get used a lot and they save space by using the same memory for all
>> the strings with the same value in a document. These strings get cached
>> in a dictionary on the document and the nodes only reference these
>> strings. This is all fine as long as a node is connected to its
>> document, but when a node gets detached and maybe later added to a
>> different document things get more complicated. After a call to
>> xmlUnlinkNode the strings still point to the dictionary of the old
>> document. When the document gets freed these strings would point to
>> freed memory. To avoid that, I create a private document for the node
>> and use the libxml2 function xmlDOMWrapAdoptNode to transfer these
>> strings to the new document. (This also handles namespaces, which is
>> just as important) When the node gets reattached to a new document a
>> similar process happens and the private document gets freed. This all
>> requires a libxml2 version > 2.6.20, for older ones we are in deep
>> trouble and the call to xmlSetTreeDoc that I added for this case wont
>> really help.
>> What I should change, as you noted and as a FIXME in the code states, is
>> that we should not return these private documents in any way. I just
>> change the code to no longer return the document, when the parent is NULL.
>> Now you may ask yourself, how all this could ever work with the old
>> implementation. Good question! It just didn't, it seemed to work, but
>> only when all the documents would stay around long enough to be cleaned
>> up after all the nodes. That is, you were lucky and a lot of libxml2
>> memory got leaked. (But leaking memory is still better than crashing as
>> we do now)
>> 
>> The second purpose these private documents get used for is to store
>> unresolved namespaces. Again this gets done to make sure we don't have
>> any memory corruption. I really need to explain the concept here in more
>> detail, but I already know that you are not interested in namespaces and
>> all this should not be relevant in this specific case.
>> 
>>> Program received signal SIGSEGV, Segmentation fault. 0x005db761 in
>>> free () from /lib/tls/i686/cmov/libc.so.6 (gdb) bt 9 #0 0x005db761
>>> in free () from /lib/tls/i686/cmov/libc.so.6 #1 0x003a61f8 in
>>> xmlFreeNode () from /usr/lib/libxml2.so.2 #2 0x00944a11 in
>>> -[NSXMLNode dealloc] (self=0x88e1c94, _cmd=0xb3c9d0) at
>>> NSXMLNode.m:1192 #3 0x0093be62 in -[NSXMLDocument dealloc]
>>> (self=0x88e1c94, _cmd=0xb0e6e8) at NSXMLDocument.m:54 #4 0x008a2cc4
>>> in -[NSObject release] (self=0x88e1c94, _cmd=0xadd8d0) at
>>> NSObject.m:2049 #5 0x007ea77f in -[NSAutoreleasePool emptyPool]
>>> (self=0x881a314, _cmd=0xadd8c0) at NSAutoreleasePool.m:656 #6
>>> 0x007ea5a5 in -[NSAutoreleasePool dealloc] (self=0x881a314,
>>> _cmd=0xadd8b8) at NSAutoreleasePool.m:538 #7 0x007ea0c8 in
>>> -[NSAutoreleasePool release] (self=0x881a314, _cmd=0x2dfa30) at
>>> NSAutoreleasePool.m:531 #8 0x0020d699 in endFrame (self=0x835b18c)
>>> at STRuntime.m:462 (More stack frames follow...)
>> 
>> Here an NSXMLDocument gets freed and this crashes in libxml2. Bad, we
>> really need to fix this.
>> 
>>> I may be wrong about what is causing this -- a quick attempt to
>>> remove the code I was suspicious of in the detach method didn't solve
>>> the problem. I'm afraid I can't take any more time with this right
>>> now, but I can tell you that this occurs after approximately this
>>> sequence of steps (it's hard for me to pin down the exact steps
>>> because this is all invoked by some higher-level code, but I think
>>> this is very close):
>> 
>> Now having a text description of test code is quite nice, but you do
>> understand that having it in code would actually help more?
>> You state that you don't have any more time to spend on this, but what
>> you do is using up my time instead.
>> 
>>> I create 2 NSXMLDocuments by calling [NSXMLDocument
>>> initWithXMLString:options:error:] with the strings "<num>6</num>"
>>> and"<num>7</num>"
>>> 
>>> In each of these Documents my code calls -rootElement to get the root
>>> element and then calls -detach on the element to remove it from its
>>> document. Then I believe -copyWithZone: is called on the root
>>> elements, and these copies are what are used by the rest of the
>>> code.
>>> 
>>> Then, in the first element, the code changes the contents from "6" to
>>> "7" by calling -setStringValue: on the text node.
>>> 
>>> Then the code compares the two detached element trees. All of that
>>> works fine until it releases the pool and crashes.
>> 
>> I wrote some test code that does what you describe (Now in SVN with the
>> name transfer.m):
>> 
>> #import "ObjectTesting.h"
>> #import <Foundation/NSAutoreleasePool.h>
>> #import <Foundation/NSXMLDocument.h>
>> #import <Foundation/NSXMLElement.h>
>> 
>> int main()
>> {
>> NSAutoreleasePool *arp = [NSAutoreleasePool new];
>> NSXMLElement *elem1 = [[NSXMLElement alloc] initWithXMLString:
>> @"<num>6</num>" error: NULL];
>> NSXMLElement *elem2 = [[NSXMLElement alloc] initWithXMLString:
>> @"<num>7</num>" error: NULL];
>> NSXMLElement *copy1 = [elem1 copy];
>> NSXMLElement *copy2 = [elem2 copy];
>> 
>> [copy1 setStringValue: @"7"];
>> PASS_EQUAL(copy1, copy2, "equal after setStringValue:");
>> 
>> [arp drain];
>> arp = nil;
>> 
>> return 0;
>> }
>> 
>> This code works fine, and did so even before I made the changes listed
>> above. (OK, I had to correct the value for the deep copy)
>> The only difference to your description is the usage of
>> initWithXMLString:error:, but the implementation of that method is just
>> what you describe:
>> 
>> - (id) initWithXMLString: (NSString*)string
>> error: (NSError**)error
>> {
>> NSXMLElement *result = nil;
>> NSXMLDocument *tempDoc =
>> [[NSXMLDocument alloc] initWithXMLString: string
>> options: 0
>> error: error];
>> if (tempDoc != nil)
>> {
>> result = RETAIN([tempDoc rootElement]);
>> [result detach]; // detach from document.
>> }
>> [tempDoc release];
>> [self release];
>> 
>> return result;
>> }
>> 
>> I would say that this means that the problem comes from your higher
>> level code. Could it be that this code access the rootDocument of the
>> standalone nodes and tries to free up that? This problem should be fixed
>> by the changes I just made. Please try again and if there are any
>> remaining issue, PLEASE provide actual test code.
>> You see, you are getting paid for what you do here. I am not. This
>> clearly means that you should try to make my work as easy as possible.
>> 
>> Cheers
>> Fred
>> 
>>> This same sequence of steps was working without crashing with the XML
>>> code as it existed a couple weeks ago. So I'm hoping maybe you have a
>>> clue about which changes might lead to this.
>>> 
>>> Thanks,
>>> 
>>> Doug
>>> 
>>> 
>>> 
>>> 
>>> On Mar 14, 2012, at 4:27 PM, Fred Kiefer wrote:
>>> 
>>>> On 29.02.2012 23:28, Doug Simons wrote:
>>>>> Since we've submitted the new implementation of the NSXML...
>>>>> classes based on libxml2 and people are beginning to use them, I
>>>>> thought I would mention some remaining unresolved issues in the
>>>>> hope that other people might have more experience with the
>>>>> libxml2 libraries and have some ideas about how to solve them.
>>>>> These are currently the top issues on my list:
>>>>> 
>>>>> 1. Parsing an XML document generates text nodes in the tree for
>>>>> whitespace between elements even when the XML_PARSE_NOBLANKS
>>>>> option is given. Cocoa doesn't do this.
>>>>> 
>>>>> 2. Find a way to control formatting of "empty" nodes. Cocoa has
>>>>> the options NSXMLNodeExpandEmptyElement and
>>>>> NSXMLNodeCompactEmptyElement to control whether an empty node foo
>>>>> is displayed as "<foo></foo>" or as"<foo/>" . Currently our
>>>>> libxml2 implementation will only display the latter.
>>>>> 
>>>>> 3. Find a way to control "pretty-print" formatting. Cocoa has an
>>>>> option NSXMLNodePrettyPrint to control whether a string
>>>>> representation of a tree will include indentation for enhanced
>>>>> readability or not. Currently our libxml2 implementation always
>>>>> includes indentation.
>>>>> 
>>>>> I have searched the libxml2 documentation for ways to control
>>>>> these issues but haven't come up with anything that works (in
>>>>> particular I tried the xmlKeepBlanksDefault() function for issue
>>>>> 1 without any success).
>>>> 
>>>> I hope that I have solved the last two of your issues. Could you
>>>> please give it a try?