Review Request 128016: [OS X] Prevent a crash in the IdealDockWidget's context menu

[René J.V. Bertin]

> On May 26, 2016, 1:19 a.m., Aleix Pol Gonzalez wrote:
> > Can't you use a smart pointer (i.e. QScopedPointer)? code filled with deletes is scary.
> > 
> > Will we need such a patch for every QMenu in the code?

Heh, I don't have that issue with pointer management, I almost don't know any better being mainly a C user since longer than I care to remember :)

I guess QScopedPointer would be fine as long as we can be sure it uses `deleteLater` or an equivalent method. I'll try to figure that out, from the looks of it there's a custom cleanup handler that should have that effect (`QScopedPointerDeleteLater`, for *pointers to QObject's that are actively participating in a QEventLoop*).

>From what I remember this particular instance was rather easy to find and correct as crashing was sufficiently reproducible. I haven't continued to test without the patch for each new Qt or KDevelop version, though.
Anyway, I think that in theory every location that uses a temporary QMenu object is a potential candidate for this kind of patch, if there is no guarantee that all underlying objects can be released safely when the QMenu object goes out of scope. This is related to the changes I've had to make to the patch review plugin, of course.
The Qt documentation makes it clear (but not abundantly so) that it's a good idea to use `deleteLater` on QObjects that participate in an event loop. That doesn't mean that using `delete` will lead to disaster systematically, unfortunately. The presence of pending events or not may even depend on CPU speed or load, so we can probably get away with making this kind of change on a case-by-case basis, tackling issues as they present themselves.

There may also be other workarounds, like doing a forced event handler sync (`sendPostedEvents()`, `processEvents()`). I'm not really sure if that's really preferable ...


- René J.V.


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


On May 25, 2016, 10:54 p.m., René J.V. Bertin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128016/
> -----------------------------------------------------------
> 
> (Updated May 25, 2016, 10:54 p.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Repository: kdevplatform
> 
> 
> Description
> -------
> 
> OS X can be capricious when instances corresponding to a widget are deleted, if the class in question uses "native" ObjC SDKs behind the scenes. Pending events can in that case be (generated and) delivered to objects that were already deleted.
> According to the documentation, one should prefer to use `QObject::deleteLater()` rather than the regular, direct `delete` whether it be explicit or implicit.
> 
> I've long used a local patch that uses this approach in order to prevent a recurring crash after using the context menu of the "ideal dock widget". Somehow I never put it up for review here, apparently.
> 
> 
> Diffs
> -----
> 
>   sublime/idealdockwidget.cpp dae0ea2 
> 
> Diff: https://git.reviewboard.kde.org/r/128016/diff/
> 
> 
> Testing
> -------
> 
> Builds and permits reliable behaviour on both OS X and Linux.
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

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