Removing a child that doesn't belong to XmlElement corrupts memory


#1

Ok, I get that this is a bug, but it would be nice if it threw and assert. Took me forever to track down.

juce::XmlElement xmlCopy (*xml);
xmlCopy.removeChildElement (xml.getChildElement (0), true);

Pretty easy mistake to make. Now child is deleted but xml still has a pointer to it. When xml gets deleted it crashes. Would be nice if it asserted and said Hey! You aren’t even in this tree, wtf are you doing!


#2

Something like this?

diff --git a/modules/juce_core/xml/juce_XmlElement.cpp b/modules/juce_core/xml/juce_XmlElement.cpp
index 5939d9f12..80fe09777 100644
--- a/modules/juce_core/xml/juce_XmlElement.cpp
+++ b/modules/juce_core/xml/juce_XmlElement.cpp
@@ -697,6 +697,8 @@ void XmlElement::removeChildElement (XmlElement* const childToRemove,
 {
     if (childToRemove != nullptr)
     {
+        jassert (containsChildElement (childToRemove));
+
         firstChildElement.remove (childToRemove);

         if (shouldDeleteTheChild)

#3

is this because xmlCopy and xml both point to the same underlying ReferenceCountedObject or whatever they’re using internally?


#4

No, xmlCopy and xml don’t point to the same object. xmlCopy is a “deep” copy.

The issue comes from how XmlElement::removeChildElement is structured:

void XmlElement::removeChildElement (XmlElement* const childToRemove,
                                     const bool shouldDeleteTheChild) noexcept
{
    if (childToRemove != nullptr)
    {
        firstChildElement.remove (childToRemove);

        if (shouldDeleteTheChild)
            delete childToRemove;
    }
}

delete childToRemove; is called as long as shouldDeleteTheChild is true, without caring about whether firstChildElement.remove (childToRemove); actually did something.

I guess the desired behavior is:

void XmlElement::removeChildElement (XmlElement* const childToRemove,
                                     const bool shouldDeleteTheChild) noexcept
{
    if (childToRemove != nullptr)
    {
        if (firstChildElement.remove (childToRemove) && shouldDeleteTheChild)
            delete childToRemove;
    }
}

but we can’t write that, since LinkedListPointer::remove returns void


#5

That seems sensible, I’ll add it.