Bug 74108 – [Qt] Crash in QtNetworkReplyHandler::finish when using custom QNetworkReply

• Component: WebKit Qt
• Status: RESOLVED INVALID

[Dawit A.]

QNetworkReply's documentation explicitly states that it should not be directly deleted from a SLOT connected to its finished signal. If the reply has to be deleted, then QObject::deleteLater should be used instead. Unfortunately, that is not what happens in QNetworkReplyHandler::finish. The reply wrapper object is simply set to 0, which results in the QNetworkReply object being deleted immediately. This causes crashes like the one reported downstream at https://bugs.kde.org/show_bug.cgi?id=287778.

Please note that the crash does not happen when using QtTestBrowser or for that matter anything that uses Qt's own networking classes. However, it reliably crashes in KDE QNetworkAccessManager integration classes.

[Dawit A.]

Created attachment 118432 [details]
proposed patch I

[Dawit A.]

Created attachment 118437 [details]
proposed patch II

[Simon Hausmann]

Comment on attachment 118437 [details]
proposed patch II

View in context: https://bugs.webkit.org/attachment.cgi?id=118437&action=review

> Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:442
> -        m_replyWrapper = nullptr;
> +        m_replyWrapper->deleteLater();

Isn't this going to cause more crashes? The QObject will be deleted later, but the OwnPtr doesn't know that and in its destructor it will try to call the m_replyWrapper's destructor again, resulting in double deletion.

Perhaps a better fix would be something along the lines of m_replyWrapper.leakPtr()->deleteLater(); - as ugly as it is...

Or simply not use an OwnPtr at all.

[Dawit A.]

(In reply to comment #3)
> (From update of attachment 118437 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118437&action=review
> 
> > Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:442
> > -        m_replyWrapper = nullptr;
> > +        m_replyWrapper->deleteLater();
> 
> Isn't this going to cause more crashes? The QObject will be deleted later, but the OwnPtr doesn't know that and in its destructor it will try to call the m_replyWrapper's destructor again, resulting in double deletion.

That is indeed a problem. My mind was simply substituted the behavior of one of the Qt Pointer handling classes for OwnPtr. Anyhow, it is still easy enough resolve this issue. See below.

> Perhaps a better fix would be something along the lines of m_replyWrapper.leakPtr()->deleteLater(); - as ugly as it is...

QNetworkReplyWrapper already comes with its own release() function ; so I can change the code to call release() first and do a deleteLater() on the QNetworkReply pointer it returns. That way everything is cleaned up proper.

> Or simply not use an OwnPtr at all.

That is another alternative, but I think the above method would suffice.

[Dawit A.]

Never mind! The problem is not caused by what I described, but something completely different. The deletion of the QNetworkReply object is handled properly when QNetworkReplyWrapper is deleted. Will investigate further and update this bug report once my system finishes compiling in debug mode.

[Dawit A.]

Follow frame #11 back to #6 and you will see the QNetworkReplyWrapper being deleted in QNetworkReplyHander::finish. Unfortunately, the call to QNetworkReplyHandler::finish originated from the very same QNetworkReplyWrapper (QNetworkReplyWrapper::didReceiveFinished) that was deleted!! Nasty. No clue why this does not cause more crashes.

Thread 1 (Thread 0x7f449159f760 (LWP 27531)):
[KCrash Handler]
#6  0x00007f447d7831f8 in WTF::deleteOwnedPtr<WebCore::QNetworkReplyWrapper> (ptr=0x154fcd8) at /usr/local/src/Misc/webkit/Source/JavaScriptCore/wtf/OwnPtrCommon.h:59
#7  0x00007f447d782bfb in WTF::OwnPtr<WebCore::QNetworkReplyWrapper>::operator= (this=0x154fcd0, o=...) at /usr/local/src/Misc/webkit/Source/JavaScriptCore/wtf/OwnPtr.h:136
#8  0x00007f447d77ff95 in WebCore::QNetworkReplyHandler::finish (this=0x154fcc0) at /usr/local/src/Misc/webkit/Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:468
#9  0x00007f447d77e4a1 in WebCore::QNetworkReplyHandlerCallQueue::flush (this=0x154fcf8) at /usr/local/src/Misc/webkit/Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:195
#10 0x00007f447d77e387 in WebCore::QNetworkReplyHandlerCallQueue::push (this=0x154fcf8, method=(void (WebCore::QNetworkReplyHandler::*)(WebCore::QNetworkReplyHandler * const)) 0x7f447d77f980 <WebCore::QNetworkReplyHandler::finish()>) at /usr/local/src/Misc/webkit/Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:164
#11 0x00007f447d77f37c in WebCore::QNetworkReplyWrapper::didReceiveFinished (this=0x1815710) at /usr/local/src/Misc/webkit/Source/WebCore/platform/network/qt/QNetworkReplyHandler.cpp:349
#12 0x00007f447d782380 in WebCore::QNetworkReplyWrapper::qt_metacall (this=0x1815710, _c=QMetaObject::InvokeMetaMethod, _id=1, _a=0x7fffc5b007c0) at ./moc_QNetworkReplyHandler.cpp:80
#13 0x00007f448e1fa5ea in QMetaObject::activate(QObject*, QMetaObject const*, int, void**) () from /usr/lib/libQtCore.so.4
#14 0x00007f449014274c in KDEPrivate::AccessManagerReply::slotResult (this=0x18c8a10, kJob=0x18f30c0) at /usr/local/src/KDE/git/kdelibs/kio/kio/accessmanagerreply_p.cpp:366
#15 0x00007f44901428a3 in KDEPrivate::AccessManagerReply::qt_metacall (this=0x18c8a10, _c=QMetaObject::InvokeMetaMethod, _id=<optimized out>, _a=0x7fffc5b00970) at /usr/local/build/KDE/git/kdelibs/kio/accessmanagerreply_p.moc:84
#16 0x00007f448e1fa5ea in QMetaObject::activate(QObject*, QMetaObject const*, int, void**) () from /usr/lib/libQtCore.so.4
#17 0x00007f448e662022 in KJob::result (this=<optimized out>, _t1=0x18f30c0) at /usr/local/build/KDE/git/kdelibs/kdecore/kjob.moc:194
#18 0x00007f448e662060 in KJob::emitResult (this=0x18f30c0) at /usr/local/src/KDE/git/kdelibs/kdecore/jobs/kjob.cpp:312
#19 0x00007f449017a894 in KIO::SimpleJob::slotFinished (this=0x18f30c0) at /usr/local/src/KDE/git/kdelibs/kio/kio/job.cpp:494
#20 0x00007f449018233d in KIO::TransferJob::slotFinished (this=0x18f30c0) at /usr/local/src/KDE/git/kdelibs/kio/kio/job.cpp:1081
#21 0x00007f4490180ca1 in KIO::TransferJob::qt_metacall (this=0x18f30c0, _c=QMetaObject::InvokeMetaMethod, _id=<optimized out>, _a=0x7fffc5b00d20) at /usr/local/build/KDE/git/kdelibs/kio/jobclasses.moc:369
#22 0x00007f448e1fa5ea in QMetaObject::activate(QObject*, QMetaObject const*, int, void**) () from /usr/lib/libQtCore.so.4
#23 0x00007f4490228b81 in KIO::SlaveInterface::dispatch (this=<optimized out>, _cmd=104, rawdata=...) at /usr/local/src/KDE/git/kdelibs/kio/kio/slaveinterface.cpp:172
#24 0x00007f4490225a35 in KIO::SlaveInterface::dispatch (this=<optimized out>) at /usr/local/src/KDE/git/kdelibs/kio/kio/slaveinterface.cpp:88
#25 0x00007f4490218c0e in KIO::Slave::gotInput (this=0x130bbf0) at /usr/local/src/KDE/git/kdelibs/kio/kio/slave.cpp:344
#26 0x00007f449021936c in KIO::Slave::qt_metacall (this=0x130bbf0, _c=QMetaObject::InvokeMetaMethod, _id=<optimized out>, _a=0x7fffc5b01150) at /usr/local/build/KDE/git/kdelibs/kio/slave.moc:82
#27 0x00007f448e1fa5ea in QMetaObject::activate(QObject*, QMetaObject const*, int, void**) () from /usr/lib/libQtCore.so.4
#28 0x00007f449014b357 in dequeue (this=<optimized out>) at /usr/local/src/KDE/git/kdelibs/kio/kio/connection.cpp:82
#29 KIO::ConnectionPrivate::dequeue (this=0x131da40) at /usr/local/src/KDE/git/kdelibs/kio/kio/connection.cpp:71
#30 0x00007f449014b3fd in KIO::Connection::qt_metacall (this=0x13129a0, _c=QMetaObject::InvokeMetaMethod, _id=<optimized out>, _a=0x142f3e0) at /usr/local/build/KDE/git/kdelibs/kio/connection.moc:79
#31 0x00007f448e1fe18e in QObject::event(QEvent*) () from /usr/lib/libQtCore.so.4
#32 0x00007f448d128ae4 in QApplicationPrivate::notify_helper(QObject*, QEvent*) () from /usr/lib/libQtGui.so.4
#33 0x00007f448d12d951 in QApplication::notify(QObject*, QEvent*) () from /usr/lib/libQtGui.so.4
#34 0x00007f448eebce86 in KApplication::notify (this=0x7fffc5b01b60, receiver=0x13129a0, event=0x1807340) at /usr/local/src/KDE/git/kdelibs/kdeui/kernel/kapplication.cpp:311
#35 0x00007f448e1e789c in QCoreApplication::notifyInternal(QObject*, QEvent*) () from /usr/lib/libQtCore.so.4
#36 0x00007f448e1eac2f in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) () from /usr/lib/libQtCore.so.4
#37 0x00007f448e2121a3 in ?? () from /usr/lib/libQtCore.so.4
#38 0x00007f4487aa984d in g_main_context_dispatch () from /usr/lib/libglib-2.0.so.0
#39 0x00007f4487aaa048 in ?? () from /usr/lib/libglib-2.0.so.0
#40 0x00007f4487aaa219 in g_main_context_iteration () from /usr/lib/libglib-2.0.so.0
#41 0x00007f448e212606 in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/lib/libQtCore.so.4
#42 0x00007f448d1cbeee in ?? () from /usr/lib/libQtGui.so.4
#43 0x00007f448e1e6a92 in QEventLoop::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/lib/libQtCore.so.4
#44 0x00007f448e1e6c97 in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/lib/libQtCore.so.4
#45 0x00007f448e1eaeab in QCoreApplication::exec() () from /usr/lib/libQtCore.so.4
#46 0x00007f449119af72 in kdemain (argc=<optimized out>, argv=<optimized out>) at /usr/local/src/KDE/git/kdebase/kde-baseapps/konqueror/src/konqmain.cpp:227
#47 0x00007f448b2c717d in __libc_start_main () from /lib/libc.so.6
#48 0x00000000004007f1 in _start ()

[Luiz Agostini]

(In reply to comment #6)
> Follow frame #11 back to #6 and you will see the QNetworkReplyWrapper being deleted in QNetworkReplyHander::finish. Unfortunately, the call to QNetworkReplyHandler::finish originated from the very same QNetworkReplyWrapper (QNetworkReplyWrapper::didReceiveFinished) that was deleted!! 

You mean that a QNetworkReplyWrapper object is been deleted in a SLOT connected to the finish signal of a QNetworkReply object. I do not see why it would be a problem because, as you said, the QNetworkReply object's destruction is handled properly.

> Nasty. No clue why this does not cause more crashes.

This does not cause crashes because the queue (QNetworkReplyHandlerCallQueue) makes it sure that all the methods will get called in proper sequence and that the QNetworkReplyWrapper object will not be used after been destroyed. The object gets destroyed inside of QNetworkReplyWrapper::didReceiveFinished only if the queue is empty.

It is not clear for me now if the problem that you have is indeed related to the destruction of the QNetworkReplyWrapper. You mentioned that there is no problem related to the destruction of the QNetworkReply object as the bug description states. It seems to me now that both, bug title and bug description, are incorrect.

Could you provide more information?

[Dawit A.]

(In reply to comment #7)
> (In reply to comment #6)
> > Follow frame #11 back to #6 and you will see the QNetworkReplyWrapper being deleted in QNetworkReplyHander::finish. Unfortunately, the call to QNetworkReplyHandler::finish originated from the very same QNetworkReplyWrapper (QNetworkReplyWrapper::didReceiveFinished) that was deleted!! 
> 
> You mean that a QNetworkReplyWrapper object is been deleted in a SLOT connected to the finish signal of a QNetworkReply object. I do not see why it would be a problem because, as you said, the QNetworkReply object's destruction is handled properly.

No. I actually meant a QNetworkReplyWrapper object is getting deleted from a function that is contains within itself. IOW, the object is indvertantly getting deleted before the calling function can return.

> > Nasty. No clue why this does not cause more crashes.
> 
> This does not cause crashes because the queue (QNetworkReplyHandlerCallQueue) makes it sure that all the methods will get called in proper sequence and that the QNetworkReplyWrapper object will not be used after been destroyed. The object gets destroyed inside of QNetworkReplyWrapper::didReceiveFinished only if the queue is empty.

Even if the Queue is not empty once you call m_queue->push from QNetworkReplyHandler, all the calls in the queue will immediately be executed because push calls flush. The only way to prevent ::flush from iterating over each function and executing them is through the explicit use of QueueLocker or directly locking the queue yourself. Unfortunately, the queue locking seems to be done correctly everywhere except in QNetworkReplyHandler::didReceiveFinished, where it is completely missing. Because of that QNetworkReplyHandler::finish is called from didReceiveFinished and hence the statement I made above. Why is there no queue locking done in didReceiveFinished as done in all the other places where QNetworkReplyHandler::finish was added to the queue ?

> It is not clear for me now if the problem that you have is indeed related to the destruction of the QNetworkReplyWrapper. You mentioned that there is no problem related to the destruction of the QNetworkReply object as the bug description states. It seems to me now that both, bug title and bug description, are incorrect.

> Could you provide more information?

What more information can I provide other than the detailed backtrace I already provided ?

[Luiz Agostini]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > Follow frame #11 back to #6 and you will see the QNetworkReplyWrapper being deleted in QNetworkReplyHander::finish. Unfortunately, the call to QNetworkReplyHandler::finish originated from the very same QNetworkReplyWrapper (QNetworkReplyWrapper::didReceiveFinished) that was deleted!! 
> > 
> > You mean that a QNetworkReplyWrapper object is been deleted in a SLOT connected to the finish signal of a QNetworkReply object. I do not see why it would be a problem because, as you said, the QNetworkReply object's destruction is handled properly.
> 
> No. I actually meant a QNetworkReplyWrapper object is getting deleted from a function that is contains within itself. IOW, the object is indvertantly getting deleted before the calling function can return.

Please, just look at the QNetworkReplyWrapper methods that appear in your backtrace to see that it is not a problem. The QNetworkReplyWrapper is been destroyed inside one of its methods, actually a slot connected to the signal finish of a QNetworkReply, but it is not a problembecause no member of the object and no method of that object is called after destroying it.

It is not "inadvertantly" and it will not cause any crash.

> 
> > > Nasty. No clue why this does not cause more crashes.
> > 
> > This does not cause crashes because the queue (QNetworkReplyHandlerCallQueue) makes it sure that all the methods will get called in proper sequence and that the QNetworkReplyWrapper object will not be used after been destroyed. The object gets destroyed inside of QNetworkReplyWrapper::didReceiveFinished only if the queue is empty.
> 
> Even if the Queue is not empty once you call m_queue->push from QNetworkReplyHandler, all the calls in the queue will immediately be executed because push calls flush. The only way to prevent ::flush from iterating over each function and executing them is through the explicit use of QueueLocker or directly locking the queue yourself. Unfortunately, the queue locking seems to be done correctly everywhere except in QNetworkReplyHandler::didReceiveFinished, where it is completely missing. Because of that QNetworkReplyHandler::finish is called from didReceiveFinished and hence the statement I made above. Why is there no queue locking done in didReceiveFinished as done in all the other places where QNetworkReplyHandler::finish was added to the queue ?

If the queue is flushing then the method been pushed will not be executed immediately, with or without the lock. If the queue is flushing then the method will wait in the queue until its turn.

The lock is used when it is needed to push more then one method in sequence to the queue or if it is needed to something else before flushing the queue. The locker is not been used in didReceiveFinished because it would not help at all (see that flush would be called by the locker destructor).

I see no mystery here.

> 
> > It is not clear for me now if the problem that you have is indeed related to the destruction of the QNetworkReplyWrapper. You mentioned that there is no problem related to the destruction of the QNetworkReply object as the bug description states. It seems to me now that both, bug title and bug description, are incorrect.
> 
> > Could you provide more information?
> 
> What more information can I provide other than the detailed backtrace I already provided ?

By looking at the bug title, bug description and any of the comments in this bug (including the backtrace that you provided), I am still seeing no obvious problem related to the QNetworkReplyWrapper and/or QNetworkReply destruction.

It would be very nice if you could provide a test case.

[Dawit A.]

(In reply to comment #9)
> > What more information can I provide other than the detailed backtrace I already provided ?
> 
> By looking at the bug title, bug description and any of the comments in this bug (including the backtrace that you provided), I am still seeing no obvious problem related to the QNetworkReplyWrapper and/or QNetworkReply destruction.
> 
> It would be very nice if you could provide a test case.

Never mind again. This is actually a bug upstream. We did not properly handle the case of KIO Job being deleted in our custom network reply class. Sorry about the noise.