Review Request 120081: [OS X] prevent a crash after finishing a code difference review

René J.V. Bertin rjvbertin at gmail.com
Sun Sep 7 09:33:12 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.
> 
> René J.V. Bertin wrote:
>     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).
> 
> Milian Wolff wrote:
>     Interesting, I learned something new about QObject documentation. Anyhow, just don't write it should always be done and commit this patch :) I gave you the ship it already for that.
>     
>     Regarding the above discussion, I really want to ask about it on the Qt development list. I'm personally quite confused by this, consider e.g. a QObject parent-child relationshipp such as:
>     
>         parent
>         |- child1
>         |- child2
>     
>     If you delete parent, either direct or indirect via `deleteLater()`, the children will always be deleted directly via the QObject destructor (or am I out of the loop there as well and they are also only deleted via `deleteLater`?.
>     
>     Anyhow, unrelated to this patch, really. Please push this in - or don't you have commit rights?
> 
> Marko Käning wrote:
>     He doesn't have the rights, Milian. Can you do it, or shall I try to do so myself?
> 
> Milian Wolff wrote:
>     I talked to David Faure about this yesterday btw. My initial assumption was correct - `deleteLater()` is *not* what you should use by default for deleting `QObject` instances. The documentation is a bit confusing there, but actually correct, it says: "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."
>     
>     Here, no threads should be involved. If there are - then that is the big issue, I doubt the classes themselves are threadsafe.

Is David an expert on Qt's internal functioning on OS X, including older versions like 10.6 (asking, he indeed might be)?

If you google the function where the crash occurs, you'll get hits from projects other than kdevelop with near identical backtraces (i.e. on OS X), and in this particular case I went from near 100% crashes when doing `delete m_plugin` to 0% doing `m_plugin->deleteLater()`. That's sufficient real-world evidence ...

I think the case that applies here is "while pending events are waiting to be delivered". If Qt on Linux/X11 uses something like the X11 context (XContext, XSaveContext, XFindContext, ...), it's more than possible that an object's destructor can ensure that the object being deleted receives no more events through Qt's event handler, and take care of that as the first step.
To my knowledge, Cocoa doesn't have such a mechanism, and as I said earlier, it's very possible to find an object receiving events after you released it (to make things worse, ObjC's release mechanism isn't immediate like C++'s delete). In short, if you have an object that corresponds to a GUI widget, you shouldn't delete it as soon as the user clicks on the widget's close button (esp. if that's not a standard button provided by the OS). The whole idea behind the GUI API is that the user only has to worry about releasing his/her own resources, for which there are "messages" like windowWillClose and windowHasClosed.
This situation can be intricate enough when you're not really used to Cocoa/ObjC programming, it goes to a new level when you're writing a Cocoa backend to a cross-platform GUI framework like Qt.

I know all this sounds vague, and Qt's own comments I copied above probably could have included details like "can cause a crash on OS X but not on Linux" (which I think they left out for good reason).

BTW, I haven't checked, but what's the thread on which the review plugin is deleted here? If it isn't the main thread, there's extra reason on OS X to take precautions...

In short, I think that even if David is correct, it is a *good idea* to use `deleteLater` for deleting QObjects that _might_ have pending events and/or exist on a different thread (or that represent a GUI widget and are deleted off the main thread). At least on multi-platform code paths.
OTOH, what are the drawbacks of using `deleteLater`, apart from the obvious fact that resources don't get released immediately?


- René J.V.


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120081/#review65892
-----------------------------------------------------------


On Sept. 7, 2014, 10:43 a.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. 7, 2014, 10:43 a.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/kdevelop-devel/attachments/20140907/84714ca8/attachment-0001.html>


More information about the KDevelop-devel mailing list