[KDE/Mac] Review Request 120081: [OS X] prevent a crash after finishing a code difference review
René J.V. Bertin
rjvbertin at gmail.com
Sat Sep 6 14:29:55 UTC 2014
> On Sept. 6, 2014, 2:26 p.m., Milian Wolff wrote:
> > Technically, it should not be a requirement. to delete every qobject via deletelater (and note that we do not do this acutally, most of the time).
> >
> > Please rephrase the comment to something like:
> >
> > // HACK: workaround a crash on OS X, see bug: ...
> >
> > Then it's OK with me to push this patch. Also, did you test this bug on frameworks/qt5? Is it also occurring there or is the hack not required there?
>
> Marko Käning wrote:
> Sorry, Milian, I haven't KF5-kdevelop avtually RUNNING on the OSX/CI system yet (due to the QStandardsPath issue). The CI system currently only checks whether projects build successfully. So, I can't say whether this is still an issue for Qt5. Perhaps we shouldn't patch KF5's kdevelop for now.
>
> René J.V. Bertin wrote:
> I've preferred calling it a tweak rather than a hack as the patch *does* follow (official?) guidelines, even if they're apparently no technical requirement.
>
> As Marko indicated, it is not currently feasible to test this on KF/Qt5. I planned to ask for more details on the Qt forums, but they're down (once again...)
>
> Milian Wolff wrote:
> No Rene, the comment on the Qt forum is utterly misleading. Always calling deleteLater is **not** an official guideline. It is a *hack*. Please don't take everything you read on the Qt forum at face value, but with a grain of salt. If in doubt, ask on the Qt development mailing list.
I don't want to start an argument with someone who cannot but know Qt much better than I do, but the Qt forum is not the only source where I found the ... suggestion to use deleteLater. In fact, browsing `qtbase/src/corelib/kernel/qobject.cpp` *from Qt5*, one can read:
/*!
Destroys the object, deleting all its child objects.
All signals to and from the object are automatically disconnected, and
any pending posted events for the object are removed from the event
queue. However, it is often safer to use deleteLater() rather than
deleting a QObject subclass directly.
\warning All child objects are deleted. If any of these objects
are on the stack or global, sooner or later your program will
crash. We do not recommend holding pointers to child objects from
outside the parent. If you still do, the destroyed() signal gives
you an opportunity to detect when an object is destroyed.
\warning Deleting a QObject while pending events are waiting to
be delivered can cause a crash. You must not delete the QObject
directly if it exists in a different thread than the one currently
executing. Use deleteLater() instead, which will cause the event
loop to delete the object after all pending events have been
delivered to it.
\sa deleteLater()
*/
QObject::~QObject()
{/**/}
/*!
Schedules this object for deletion.
The object will be deleted when control returns to the event
loop. If the event loop is not running when this function is
called (e.g. deleteLater() is called on an object before
QCoreApplication::exec()), the object will be deleted once the
event loop is started. If deleteLater() is called after the main event loop
has stopped, the object will not be deleted.
Since Qt 4.8, if deleteLater() is called on an object that lives in a
thread with no running event loop, the object will be destroyed when the
thread finishes.
Note that entering and leaving a new event loop (e.g., by opening a modal
dialog) will \e not perform the deferred deletion; for the object to be
deleted, the control must return to the event loop from which
deleteLater() was called.
\b{Note:} It is safe to call this function more than once; when the
first deferred deletion event is delivered, any pending events for the
object are removed from the event queue.
\sa destroyed(), QPointer
*/
void QObject::deleteLater()
{
QCoreApplication::postEvent(this, new QDeferredDeleteEvent());
}
So while it seems you're right it's not an obligation to discard QObject instances through deleteLater, it apparently is a very good idea to do so if there is a chance that "pending events are waiting to be delivered [because direct deletion] can cause a crash."
I really think therefore that my modification is *not* a hack; it is actualle more a precaution even than a tweak (and a necessary one at that on OS X)
BTW, I can corroborate this with experience from Cocoa programming (and with pure X11, from longer ago), where it is indeed quite possible to end up in an object's event handler *after* having deleted ("released") the object, and thus cause a crash. Rather than checking everywhere if an object's resources are still available, one uses deferred release where necessary (and it's great Qt provides its own mechanism; Cocoa doesn't to my knowledge).
- René J.V.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120081/#review65892
-----------------------------------------------------------
On Sept. 6, 2014, 3:51 p.m., René J.V. Bertin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120081/
> -----------------------------------------------------------
>
> (Updated Sept. 6, 2014, 3:51 p.m.)
>
>
> Review request for KDE Software on Mac OS X and KDevelop.
>
>
> Repository: kdevplatform
>
>
> Description
> -------
>
> On OS X, kdevelop would crash after clicking on the "Finish Review" button, at least when the path review tool was started through a project's git->differences. This crash occurred with the current stable release (kdevplatform 1.6.0) through the 4.7ß up to and including kdevplatform git/kde4-legacy.
>
> The crash would be deep inside Qt, seemingly mouse action/event related. Googling the actual function in which the crash occurred led to the hypothesis a QObject derived object might be disposed of via `delete m_obj` instead of `m_obj->deleteLate()` as appears to be the correct way of doing things (http://qt-project.org/forums/viewthread/38406/#162801).
>
> More details, with referring links and backtraces are here: https://bugs.kde.org/show_bug.cgi?id=338829
>
> A single line change in the PatchReviewPlugin destructor, following this knowledge, seems indeed to be the required fix to prevent the crash. I have not made this a platform-specific change as the online information I found doesn't say anything about whether or not the requirement is valid for OS X only.
>
> I ignore to what extent Qt5 has relaxed the disposal requirements on QObjects but presume it will continue to be a good idea to use ->deleteAll() . I presume Qt has some kind of garbage collection, if so, one should leave object disposal to the GC and not do it the hard way.
>
>
> Diffs
> -----
>
> plugins/patchreview/patchreview.cpp 2977c40
>
> Diff: https://git.reviewboard.kde.org/r/120081/diff/
>
>
> Testing
> -------
>
> Works (i.e. no more crashing) under OS X 10.6.8, with kdelibs git/4.14 and the other dependencies at KDE/MacPorts 4.12.5 . kdevplatform and kdevelop are built using gcc 4.8.3 from MacPorts (due to the C++11 requirement).
> The patch does not interfere with functionality on Linux.
>
>
> Thanks,
>
> René J.V. Bertin
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-mac/attachments/20140906/bddffa6b/attachment-0001.html>
More information about the kde-mac
mailing list