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

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!

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)
1 Like

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

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

That seems sensible, I’ll add it.