Crash when deleting object after terminating thread in Qt 5.4.1?

[Thuan_Firelight]

Hi all,

I encountered a crash in QObject::~Object() which indicates the d->currentSender is either invalid or corrupted and I just like a second opinion if this is a legit bug.

// dummy class required to setTerminationEnabled to true since connecting to started() seems to prevent
// the call that reenable termination from happening.
class TerminationThread : public QThread
{
public:
    static void enableTermination() { QThread::setTerminationEnabled(true); }
};

class TestObject : public QObject
{
    Q_OBJECT
public:
    TestObject( QObject* parent = 0 )
    {
        connect(&mThread, SIGNAL(started()), this, SLOT(processOperationQueue()));    

        moveToThread(&mThread);
        mThread.start();
    }

    void terminate()
    {
        mThread.terminate();
        mThread.wait();
    }
    
private slots:
    void processOperationQueue()
    {
        TerminationThread::enableTermination();
        forever
        {
            // simulating work getting done. In my production code, I am using a 3rd party API
            // which has no function to allow it to timeout and forcefully terminating the thread
            // was the recommended approach from the 3rd party developer
            QThread::sleep(10);
            int dummy = 0;
            for(int i=0; i<1000; i++)
                dummy += i;
        }
    }

private:
    QThread mThread;
};

TestClass::TestClass()
{
    TestObject* obj = new TestObject;
    QThread::sleep(100);
    obj->terminate();
    delete obj; // <-- Crash
}

I tried debugging the issue, and it seems like calling sender() on obj was returning mThread as it was the last qobject to emit a signal (i.e. the started() signal). So I suspect the d->currentSender is related to the mThread that I was forcefully terminating. Anyone has any insights to this or if this is something that is already addressed in newer releases?

At the moment I am planning to rewrite the code to inherit QThread and override run instead.

[ambershark]

@Thuan_Firelight Change delete obj to obj->deleteLater() and it should fix your crash. You are deleting the object before it's event queue is empty.

[Thuan_Firelight]

@ambershark said in Crash when deleting object after terminating thread in Qt 5.4.1?:

@Thuan_Firelight Change delete obj to obj->deleteLater() and it should fix your crash. You are deleting the object before it's event queue is empty.

I tried that and it seems like doing so would lead to a leak as delete is never called (verified this by implementing the destructor for TestObject with a qDebug() statement for its body). I think because the thread has ended and so has its event loop? Or is there something else I need to do? I tried moving obj to QThread::currentThread() but that failed because I am not able to call that function on the thread obj is on.

[ambershark]

@Thuan_Firelight Hmm... Can I see the stack trace for the crash? deleteLater should have been called even if it was moved to another thread. That is weird that it is not being called. I might write some test code for this.

One thing to note, calling terminate on a thread is "bad". It almost always causes issues. That is why it is highly recommended never to terminate threads. You may be experiencing one of these issues. If you exit the thread normally, does the delete work then?

When you terminate you are leaving the thread in a broken state basically, and when it comes to an event loop, it may have outstanding unprocessed events, which sounds like the crash you are seeing on delete.

[Thuan_Firelight]

@ambershark said in Crash when deleting object after terminating thread in Qt 5.4.1?:

@Thuan_Firelight Hmm... Can I see the stack trace for the crash? deleteLater should have been called even if it was moved to another thread. That is weird that it is not being called. I might write some test code for this.

One thing to note, calling terminate on a thread is "bad". It almost always causes issues. That is why it is highly recommended never to terminate threads. You may be experiencing one of these issues. If you exit the thread normally, does the delete work then?

When you terminate you are leaving the thread in a broken state basically, and when it comes to an event loop, it may have outstanding unprocessed events, which sounds like the crash you are seeing on delete.

I am not sure what can be observed from the stack but here it is:

>	Qt5Cored.dll!QObject::~QObject() Line 924	C++
 	xxx.dll!TestObject::~TestObject() Line 90	C++
 	xxx!TestObject::`scalar deleting destructor'(unsigned int)	C++
 	xxx.dll!ABC::ABC(QObject * parent) Line 120	C++
 	xxx.dll!AppController::launchNewWorkspace() Line 91	C++
 	xxx.dll!Application::initGUI() Line 222	C++
 	xxx.dll!Application::exec() Line 83	C++
 	xxx.dll!XXXTool::launchGUI(int & argc, char * * argv) Line 28	C++
 	ABC.exe!wWinMain(HINSTANCE__ * hInstance, HINSTANCE__ * hPrevInstance, wchar_t * pCmdLine, int nCmdShow) Line 7	C++
 	ABC.exe!__tmainCRTStartup() Line 528	C
 	ABC.exe!wWinMainCRTStartup() Line 377	C

QObject::~QObject()
{
    Q_D(QObject);
    d->wasDeleted = true;
    d->blockSig = 0; // unblock signals so we always emit destroyed()

    QtSharedPointer::ExternalRefCountData *sharedRefcount = d->sharedRefcount.load();
    if (sharedRefcount) {
        if (sharedRefcount->strongref.load() > 0) {
            qWarning("QObject: shared QObject was deleted directly. The program is malformed and may crash.");
            // but continue deleting, it's too late to stop anyway
        }

        // indicate to all QWeakPointers that this QObject has now been deleted
        sharedRefcount->strongref.store(0);
        if (!sharedRefcount->weakref.deref())
            delete sharedRefcount;
    }

    if (!d->isWidget && d->isSignalConnected(0)) {
        QT_TRY {
            emit destroyed(this);
        } QT_CATCH(...) {
            // all the signal/slots connections are still in place - if we don't
            // quit now, we will crash pretty soon.
            qWarning("Detected an unexpected exception in ~QObject while emitting destroyed().");
            QT_RETHROW;
        }
    }

    if (d->declarativeData) {
        if (static_cast<QAbstractDeclarativeDataImpl*>(d->declarativeData)->ownedByQml1) {
            if (QAbstractDeclarativeData::destroyed_qml1)
                QAbstractDeclarativeData::destroyed_qml1(d->declarativeData, this);
        } else {
            if (QAbstractDeclarativeData::destroyed)
                QAbstractDeclarativeData::destroyed(d->declarativeData, this);
        }
    }

    // set ref to zero to indicate that this object has been deleted
    if (d->currentSender != 0)
        d->currentSender->ref = 0;  // <-- crash
    d->currentSender = 0;

Attempting to inspect d->currentSender shows:

currentSender 0x0612f844 {sender=??? signal=??? ref=??? } QObjectPrivate::Sender *
sender <Unable to read memory>
signal <Unable to read memory>
ref <Unable to read memory>

[ambershark]

@Thuan_Firelight said in Crash when deleting object after terminating thread in Qt 5.4.1?:

if (d->currentSender != 0)
d->currentSender->ref = 0; // <-- crash
d->currentSender = 0;

^-- that was why I wanted to see the stack trace. :) So I could see where it was crashing, and why.

So basically it's as I figured. The event handler is trying to process a message from an object that was already cleaned up. This is why deleteLater was implemented and should be used. It allows objects to finish events before being cleaned up.

I'm not sure of an easy way to fix this. Again it's why you really shouldn't terminate threads.

Straight from the Qt docs:

Warning: This function is dangerous and its use is discouraged. The thread can be terminated at any point in its code path. Threads can be terminated while modifying data. There is no chance for the thread to clean up after itself, unlock any held mutexes, etc. In short, use this function only if absolutely necessary.

If you absolutely have to terminate though what I think I would do is try to clean up the events from the thread before terminating it. If that isn't possible then you could change how you delete the thread. Since the thread is already cleaned up by the time you hit the destructor code in TestObject that is what is causing the issue. So you need to keep that thread in scope until the events are processed. You will need to dynamically allocate it instead and then either use a deleteLater on it, or delete it yourself after the events are processed.

Another thing you could do is just leak the memory on the QThread. This is a bad idea but calling terminate almost guarantees memory leaks anyway as the thread can be killed at any point in the code. If you did that the memory would be valid and the signal would work properly. Again I really don't recommend this approach as it goes against everything we know/do as c++ developers.

Finally, if you are finding the need to regularly terminate your threads, you should re-evaluate your threading model. This comes down to bad design of your threads. You should be able to signal them to quit and not have them hung up in a forever or tight loop that never checks for/handles events. I can't remember the last time I had to terminate a thread. I always do a wait with a timeout and if it hits that timeout for some reason then terminate. However usually by that point in the code the app is in it's shutdown phase and it doesn't matter that much if I terminate. I still try not to though as it tends to cause crashes (as you've found out).

[Thuan_Firelight]

@ambershark

Unfortunately I am calling into a 3rd party api (Perforce C++ API to be specific) and it doesn't have a mechanism for me to signal it to stop hence the need for forced thread termination. It stalls whenever it is running on a crappy WIFI network it seems. I welcome any other threading approach/design suggestion when dealing with a 3rd party API that doesn't provide a way to safely terminate or timeout.

For the moment, I am looking at rewriting the code anyway to have a derived class of QThread instead so I can delete the TestObject on the main thread independent of how I clean up the thread. Overriding the run() function would bypass the need for signal and slot.

[ambershark]

@Thuan_Firelight Well that makes sense. It sounds like you're stuck with bad code on their end which is forcing you to have bad thread design on your end. Not much you can do about that.

I would just drop the signaling completely and use QThread's overridden run. Then you shouldn't have an issue any more.

For that matter you could use native C++ threading since you don't really seem to need the threads for Qt stuff. That would help avoid the crash as well.

And of course any of the memory related suggestions I had would probably fix the crash as well.