[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Unresolved Issues with libxml2
From: |
Doug Simons |
Subject: |
Re: Unresolved Issues with libxml2 |
Date: |
Tue, 3 Apr 2012 16:56:41 -0600 |
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?