Memory management of QStates added to QStateMachine

Question

The below code causes a crash due to memory corruption. I am assuming that it is because of delete pTestStateMachine trying to delete the memory which is not allocated in heap. Is that correct?

If so, does it imply that QStateMachine::addState(QAbstractState * state) must always be passed an dynamically allocated memory? Unfortunately Qt docs doesen't specify any such condition. What am I missing here?

class CTestClass
{
public:
    QState m_pTestState;
};

int main(int argc, char *argv[])
{
    QCoreApplication a(argc, argv);

    QStateMachine *pTestStateMachine;
    CTestClass  TestClass;

    pTestStateMachine = new QStateMachine();
    pTestStateMachine->addState(&(TestClass.m_pTestState));
    pTestStateMachine->setInitialState(&(TestClass.m_pTestState));
    pTestStateMachine->start();

    pTestStateMachine->stop();
    delete pTestStateMachine;

    return a.exec();
}

Kuba hasn't forgotten Monica · Accepted Answer · 2015-10-13T19:59:42.433

6

does it imply that QStateMachine::addState(QAbstractState * state) must always be passed an dynamically allocated memory?

Not at all. The QState is not special in any way, the same caveats apply to any QObject. Recall that QObject is a container for other QObjects: it owns them, and unless they are separately destroyed first, will attempt to delete child objects in QObject::~QObject.

Your code can be fixed in several ways - in all cases, the objective is not to let the ~QObject delete the child states it shouldn't be deleting.

It all gets really simple if you let the compiler do the job it's supposed to be doing. Your code style of using raw owning pointers and not initializing them at the point of definition is non-idiomatic and often inspires the bug you've run into. If you have an owning pointer, use std::unique_ptr or QScopedPointer. delete and manual memory management belongs only within a single-purpose resource-managing class. It simply does not belong in general-purpose code: consider every explicit delete to be a bug. You don't need them.

class CTestClass
{
public:
  QState m_pTestState;
};

// Fix 1: Don't mix automatic storage duration with dynamic storage duration
int main(int argc, char *argv[])
{
  QCoreApplication a(argc, argv);
  {
    QStateMachine TestStateMachine;
    CTestClass    TestClass;
    TestStateMachine.addState(&TestClass.m_pTestState);
    TestStateMachine.setInitialState(&TestClass.m_pTestState);
    TestStateMachine.start();
    TestStateMachine.stop();
  } // <-- here the compiler emits
    // TestClass.~TestClass()
    // ...
    // TestStateMachine.~QStateMachine()
    //   ...
    //   TestStateMachine.~QObject()
}

// Fix 2: Make sure that the child doesn't outlive the parent.
int main(int argc, char *argv[])
{
  QCoreApplication a(argc, argv);
  {
    QScopedPointer<QStateMachine> TestStateMachine(new QStateMachine);
    CTestClass                    TestClass;
    TestStateMachine->addState(&TestClass.m_pTestState);
    TestStateMachine->setInitialState(&TestClass.m_pTestState);
    TestStateMachine->start();
    TestStateMachine->stop();
  } // <-- here the compiler emits
    // TestClass.~TestClass()
    // ...
    // TestStateMachine.~QScopedPointer()
    // delete data;
    //   data->~QStateMachine
    //   ...
    //   data->~QObject
    //   free(data)
}

edited Oct 13 '15 at 19:59

answered Oct 13 '15 at 18:18

Kuba hasn't forgotten Monica

95,931
16
151
313

Based on your answer, I understand that if application deletes the `QObject` say `QObjChild`, before it's parent (say `QObjParent`) goes out of scope. Then the destruction of `QObjParent` is smart enough to identify that it's child `QObjChild` has been already destructed and will not perform any destruction. Is that correct ? – Vivek Maran Oct 13 '15 at 18:46
But what I see as a trap here is, If application relies on `QObjParent` to delete it's child QObjects then all the parent does is `delete pChildQObject` and that would be a problem if `pChildQObject` is not dynamically allocated. Ideally, isn't it good to have deleters registered while adding child objects – Vivek Maran Oct 13 '15 at 18:50
@Vivek If the child `QObject` is not dynamically allocated, then it's on you to ensure that the parent outlives the child. It's really simple - you're always in control, you just have to do the right thing. "isn't it good to have deleters registered while adding child object" -- I have no idea what that means. What deleters? Registered where? ?? – Kuba hasn't forgotten Monica Oct 13 '15 at 18:55
1

"Then the destruction of QObjParent is smart enough to identify that it's child QObjChild has been already destructed and will not perform any destruction. Is that correct ?" That's a rather convoluted way of thinking about it. When you delete the child object, it doesn't exist anymore past that point. The parent has nothing to identify: when it gets destructed, it doesn't have that child, so how would it attempt to delete it? The parent's child list is modified **immediately** as each child is added or removed. The `~QObject()` literally does `for (auto child : children()) delete child;` – Kuba hasn't forgotten Monica Oct 13 '15 at 18:57
Thanks, I got your point. I am newbie to C++ and qt (so pardon me if I am silly. By deleters I meant parent `QObject` shouldn't just call `delete` rather also should have an option to register custom delete functions for child objects, one example of custom deleter for unique_ptr http://stackoverflow.com/questions/8274451/well-how-does-the-custom-deleter-of-stdunique-ptr-work – Vivek Maran Oct 13 '15 at 19:04
@Vivek `QObject` does not support custom deleters. You're not generally forced into using `QObject` as a sub-object container. The exceptions are `QState` and `QWidget` hierarchies, where sub-states and sub-widgets must be also `QObject` children. You're completely free to add `QScopedPointer`, `std::unique_ptr`, or any other smart pointer *or* smart container as a member within a `QObject` to hold the children. As long as the smart pointer/container disposes of the children before `~QObject` is invoked, and it will if it's a RAII class, then everything will work without custom deleters. – Kuba hasn't forgotten Monica Oct 13 '15 at 19:57
@Vivek Note how in my example code there isn't a single custom deleter necessary, or even any sort of manual resource management at all. That's how your code should look. Frankly, I dread the mess that custom deleter support in `QObject` would bring about. If you want custom resource management, add a member of a type that does the resource management for you. – Kuba hasn't forgotten Monica Oct 13 '15 at 19:58

score 3 · Answer 2 · answered Oct 13 '15 at 17:52

From the wording of the docs

If the state is already in a different machine, it will first be removed from its old machine, and then added to this machine.

The QStateMachine takes ownership of the QState that means it'll try to delete all the states it owns on destruction, you can either pass a dynamically allocated pointer or you can use QStateMachine::removeState() which:

Removes the given state from this state machine. The state machine releases ownership of the state.

So this should work:

class CTestClass
{
public:
    QState m_pTestState;
};

int main(int argc, char *argv[])
{
    QCoreApplication a(argc, argv);

    QStateMachine *pTestStateMachine;
    CTestClass  TestClass;

    pTestStateMachine = new QStateMachine();
    pTestStateMachine->addState(&(TestClass.m_pTestState));
    pTestStateMachine->setInitialState(&(TestClass.m_pTestState));
    pTestStateMachine->start();

    pTestStateMachine->stop();
    pTestStateMachine->removeState(&(TestClass.m_pTestState)); //removing state before deletion
    delete pTestStateMachine;

    return a.exec();
}

Memory management of QStates added to QStateMachine

2 Answers2