this pointer and QSharedPointer

Question

I have a tree-node like class called Message which looks like this:

class Message
{
public:
    using Ptr = QSharedPointer<Message>;

public:
    explicit Message();
    explicit Message(Message::Ptr parentPtr);
    explicit Message(const Data &data, Message::Ptr parentPtr = Message::Ptr());

    void setParent(Message::Ptr parentPtr);
    Message::Ptr parent() const;
    bool hasParent() const;

    QSet<Message::Ptr> children() const;
    void setChildren(const QSet<Message::Ptr> &children);
    bool hasChildren() const;

    Data data() const;
    void setData(const Data &data);

private:
    void addChild(Message::Ptr childPtr);
    void removeChild(Message::Ptr childPtr);

private:
    Message::Ptr m_parentPtr;
    QSet<Message::Ptr> m_children;
    Data m_data;
};

This class can have a parent and a set of children. I have a problem when I implemented the addChild and setParent member functions:

void Message::addChild(Message::Ptr childPtr)
{
    if (!m_children.contains(childPtr)) {
        m_children.insert(childPtr);
    }

    Message::Ptr thisPtr(this);

    if (childPtr->parent() != thisPtr) {
        childPtr->setParent(thisPtr);
    }
}

void Message::setParent(Message::Ptr parentPtr)
{
    if (m_parentPtr != parentPtr) {
        m_parentPtr = parentPtr;

        m_parentPtr->addChild(Message::Ptr(this));
    }
}

What I expect will happen:

Message::addChild gets called
thisPtr gets created with a reference count of 1
childPtr->parent() != thisPtr will be resolved to true
childPtr->setParent(thisPtr);, Message::setParent gets executed and thisPtr reference count will increase by 1 as a copy of the shared pointer is created. Now thisPtr has a reference count of 2
As Message::setParent gets executed, m_parentPtr = parentPtr; will increase m_parentPtr, parentPtr and thus thisPtr reference counts by 1; these 3 smart pointers now have a reference count of 3.
Execution exits Message::setParent and destroy parentPtr decreasing the reference count of m_parentPtr and thisPtr by 1
Execution returns to Message::addChild. Now reference count of thisPtr is 2.

What actually happens:

When execution exits the if statement in Message::addChild thisPtr reference count decreases again by 1, leaving thisPtr with a reference count of 1. This makes everything break as when execution exists Message::addChild, thisPtr gets destroyed, thus this deleted.

My question:

Why does thisPtr reference count decreases again by when execution exits the if statement in Message::addChild or what actually happens there?...

Here is how it runs in the debugger:

You can't just hand over ownership of `*this` to a `QSharedPointer` (you're even doing it multiple times). Read [the documentation](http://doc.qt.io/qt-5/qsharedpointer.html#QSharedPointer-1). — molbdnilo, Jan 24 '17 at 20:23
Wow, that's an annoying animation - couldn't you have put that into text form? — Jesper Juhl, Jan 24 '17 at 20:48
@JesperJuhl Well that's exactly what I did on those 7 points at *What I expect will happen*... — Jacob Krieg, Jan 24 '17 at 20:49
Ohh, I forgot to say "and then ditch that annoying annimation' ;-) but maybe that's just me.. — Jesper Juhl, Jan 24 '17 at 21:10

Oktalist · Accepted Answer · 2017-01-24T20:37:34.240

As Message::setParent gets executed, m_parentPtr = parentPtr; will increase m_parentPtr, parentPtr and thus thisPtr reference counts by 1; these 3 smart pointers now have a reference count of 3.

5.1. Then, setParent constructs a temporary shared pointer to the child with reference count 1 and calls addChild on the parent:

m_parentPtr->addChild(Message::Ptr(this));

5.2. addChild creates a shared pointer to the parent with reference count 1:

Message::Ptr thisPtr(this);

5.3. addChild returns, destroying that shared pointer of 5.2, which destroys the parent, which destroys the parent's QSet<Message::Ptr> m_children member.

5.4. The temporary shared pointer of 5.1 is destroyed, which destroys the child.

More generally, you have a cyclic reference: parents own children, and children own their parents, which is a recipe for memory leaks and use-after-delete bugs. Constructing new shared pointers owning raw pointers already owned by other shared pointers is a recipe for double-delete and use-after-delete bugs; the shared pointers won't know about each other, their reference counts will vary independently. You should investigate QWeakPointer to break the cycle and QEnableSharedFromThis to safely obtain a shared pointer to *this.

this pointer and QSharedPointer

1 Answers1