Leaked objects on program exit. Code review

General JUCE discussion
1

I’m returning to C++ after decades away. I’m familiar with the RAII principles, but evidently not with the implementation.

What am I doing wrong?

class Bindings : public juce::Component, public juce::TableListBoxModel
{
public:

    void setTableValueTree(juce::ValueTree TableValueTree)
    {
        // I've narrowed it down to this routine.
        data = TableValueTree.createXml();
        dataList.reset(data->getChildByName("DATA"));
        columnList.reset(data->getChildByName("HEADERS"));
        numRows = dataList->getNumChildElements();
        data.release();
        setColumns();
    }

private:
    /* Do various TableListBox things here. */

    std::unique_ptr<juce::XmlElement>   data;
    std::unique_ptr<juce::XmlElement>   columnList;
    std::unique_ptr<juce::XmlElement>   dataList;
};

Running this triggers an exception on program exit:
numObjects = {value=1 }
getLeakedObjectClassName = 0x00007ff7cbc47c00 {Mobius.exe!juce::LeakedObjectDetectorjuce::XmlElement::getLeakedObjectClassName(void)}

2

Why are you calling data.release()? After you call that, the unique_ptr no longer manages the XmlElement object.

3

I’m done with the data after that call. The TableListBox has the data and the data is read-only.

4

Then data shouldn’t be a member of your class, it should just be declared locally in the function:

void setTableValueTree(juce::ValueTree TableValueTree)
    {
        const auto data = TableValueTree.createXml();
        
        // now use data like you were before
    }
5

I appreciate the help, but that still doesn’t help me understand where I went wrong in my original code. Let’s say I moved “data.release()” to the destructor. The call to setTableValueTree still causes a leaked object.

6

Calling “release” on a unique_ptr tells the unique_ptr to just forget about the pointer and not delete the managed object. That’s the cause of your leak.

Instead, don’t do anything! The unique_ptr will automatically delete the managed object when Bindings is destroyed.

If you prefer, do this in your destructor:

data = nullptr;

That will explicitly delete the managed object. That can be useful if you need to control the order in which your members are freed.

In any case, I think @benvining’s solution is better; createXml returns a unique_ptr which is kept as a local variable. When the local variable goes out of scope, the managed object is deleted.

Matt

7

don’t call release anywhere!

Your class should look like this:

class Bindings : public juce::Component, public juce::TableListBoxModel
{
public:

    void setTableValueTree(juce::ValueTree TableValueTree)
    {
        auto data = TableValueTree.createXml();
        dataList.reset (data->getChildByName("DATA"));
        columnList.reset (data->getChildByName("HEADERS"));
        numRows = dataList->getNumChildElements();
        setColumns();
    }

private:
    /* Do various TableListBox things here. */

    std::unique_ptr<juce::XmlElement>   columnList;
    std::unique_ptr<juce::XmlElement>   dataList;
};
8

Just to rephrase what was already pointed out:

  • calling release() will return the raw pointer and stop managing the lifetime of the pointee (not what you want)
  • calling reset() will delete the pointee (that’s what you want)
  • calling reset with a new raw pointer will swap the objects and delete the old pointee
  • assigning anything to the unique_ptr is the same as reset, just that it takes a moveable unique_ptr

Hope that clears it up a bit
Here are the docs for std::unique_ptr

2 Likes
9

All that has been said is correct, but I believe there’s another subtle problem in this code:

{
    auto data = TableValueTree.createXml();
    dataList.reset (data->getChildByName("DATA"));
    columnList.reset (data->getChildByName("HEADERS"));
    numRows = dataList->getNumChildElements();
    setColumns();
}

the std::unique_ptr <XmlElement> returned by createXml() owns the whole hierarchy of XML nodes that descend from it.
The two calls to getChildByName() return non-owning raw pointers (XmlElement*) to nodes within that hierarchy, and as soon as data goes out of scope, the whole hierarchy of XML nodes is deleted and both dataList and columnList become dangling pointers.

10

Good shout, I was so focused on the leak that I missed that.

If you want to keep information from the tree I strongly recommend ValueTree. Here the ownership is managed by reference counted sharedObjects wrapped in ValueTrees that you can copy at no cost and pass around easily.
And both boils down to XML internally and can easily be serialized from and to XML.

11

I was over complicating things. Thanks.

12

Yes. Not to mention things fail at numRows with a read access violation. Back to the drawing board. Thanks for your help.

13

Your explanation is way better than the link. Thanks.