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

[René J.V. Bertin]

> On Sept. 12, 2014, 12:59 a.m., Milian Wolff wrote:
> > No really, this just shows that something else is wrong here. Maybe fixed the bug in Qt then. But doing this kind of hit-and-run patching, blindly trying to workaround a bug is really bad as it greatly increases the maintenance cost.
> > 
> > Could you please ensure that no threads are involved? Add stuff like Q_ASSERT(QThread::currentThread() == thread()) to the dtor of the class.
> > If that does not help, then certainly there is a bug elsewhere, maybe inside Qt, and should be fixed there then.
> 
> René J.V. Bertin wrote:
>     I take it you're talking about the descructor for the classes I use `deleteLater` on in this 2nd patch?
>     
>     Is there a Qt call to check if an object has pending events or (better yet?) flush events for an objects fromt the queue?
> 
> Milian Wolff wrote:
>     Yes, and also in the first patch which should be reverted, I guess.
>     
>     And Qt removes pending events from the event queue when an object is deleted. *Only* when threads are involved there might be race conditions. But here, there should not be any threads.

We keep circling around this issue about pending events. Frankly, I shouldn't have asked about detecting or removing them, because my whole previous experience suggests that this is just not doable on OS X. That, and/or it's impossible to avoid getting what I'd recursive events, meaning you get in an event handler when in any number of previous/higher stack frames you're also in an event handler for that same object. Those are pending events too, after all - they haven't been handled and so theoretically at least they're still on the queue. And of course you cannot pop events that are already being handled.

So what would you do? Check after every function call that might have resulted in event processing if your object is still valid, while there is no feature in C/C++ to check if a pointer points to valid memory? Also, remember how I said ObjC doesn't delete objects immediately when they're deleted, but instead leaves them in the NSAutoReleasePool *that goes with the current event loop*? The crash I've been seeing occurs in a function *that gets called from _unavoidable_ ObjC code*. And it's quite possible btw that the code that ought to do this "is this pointer still alive" check is part of that ObjC code.

I really think we shouldn't be second-guessing the Qt authors and their knowledge of their own product. If they put a warning about preferring `deleteLater` when there are or might be pending events, it is because it's somehow impossible (reliably) to remove pending events for that object from the queue while deleting it. I'm tempted to add that `deleteLater` basically schedules the delete for after the exit of the current event loop - and that loop probably isn't so different from that other "loop", the one concerning threads, that is not managed by Qt ... 

I will do the thread checking, and I indeed planned to revert my earlier patch for that. We'll see what that teaches me. That aside, I suppose I'll wrap my modifications in Q_OS_MAC tags . I frankly don't see the problem doing a delete this way or that way, and I guess the Qt authors also would have preferred to use a direct rather than a deferred delete. But sometimes you just have to go with what you have, whether or not that leaves you wondering about whether you're dealing with design bugs or features ...

BTW, I've asked about this on stackoverflow, no answer. And the Qt forum tells me it's in maintenance as soon as I log in, so that place is apparently off limits to me.


- René J.V.


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


On Sept. 12, 2014, 12:22 a.m., René J.V. Bertin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120150/
> -----------------------------------------------------------
> 
> (Updated Sept. 12, 2014, 12:22 a.m.)
> 
> 
> Review request for KDE Software on Mac OS X and KDevelop.
> 
> 
> Repository: kdevplatform
> 
> 
> Description
> -------
> 
> In https://reviewboard.kde.org/r/120081/ I proposed an (accepted) approach to prevent kdevelop from crashing after closing the patch review ("git/show differences") toolview. I had another of those crashes after heavier-than-usual perusal of the toolview, despite the previous patch. The attached patch replaces all `delete`s of `QObject` derived class instances with `deleteLater()`.
> 
> 
> Diffs
> -----
> 
>   plugins/patchreview/patchreview.cpp 18b63db 
> 
> Diff: https://git.reviewboard.kde.org/r/120150/diff/
> 
> 
> Testing
> -------
> 
> kdevplatform git/kde4-legacy on KDE/MacPorts OS X 10.6.8 . 
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20140912/45da16bf/attachment-0001.html>