Review Request 126725: prevent crash-on-exit in KSelectAction::~KSelectAction

[David Faure]

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


Can I see the backtrace, with a description of how this gets triggered? I'm not sure the issue is fully understood (as shown by the use of "might" in the description...).

Is the action being destroyed by clicking into a submenu of the action? Otherwise I don't get the relation with the fix.


src/kselectaction.cpp (line 101)
<https://git.reviewboard.kde.org/r/126725/#comment62146>

    This seems unnecessary and wasteful (it sends an ActionChanged event). You're not deleting the menu right now anyway, you're using deleteLater. So the action will be gone before the menu is deleted, therefore the action will never have a dangling pointer to the menu.


- David Faure


On Jan. 12, 2016, 1:14 p.m., René J.V. Bertin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/126725/
> -----------------------------------------------------------
> 
> (Updated Jan. 12, 2016, 1:14 p.m.)
> 
> 
> Review request for KDE Frameworks and Christoph Feck.
> 
> 
> Repository: kwidgetsaddons
> 
> 
> Description
> -------
> 
> I was seeing a systematic crash on exitting kdenlive5, on OS X deep under the `delete menu()` instruction in `KSelectAction::~KSelectAction`. The backtrace suggested this might be due to a pending event (or an event due to the menu deletion) being delivered post-mortem to an instance of a QMenu related class.
> 
> My fix is based on 3 principles:
> 1) release the "foreign" member instance (`menu()`) before releasing the own d-ptr
> 2) Remove the QMenu instance from ourselves before deleting it to have one less potential dangling reference to it
> 3) QMenu is a QObject descendent that corresponds to a UI element: on OS X it is safer to dispose these through `deleteLater()` rather than directly.
> 
> 
> Diffs
> -----
> 
>   src/kselectaction.cpp 1381099 
> 
> Diff: https://git.reviewboard.kde.org/r/126725/diff/
> 
> 
> Testing
> -------
> 
> On OS X 10.9.5, Qt 5.5.1 and KF5 Frameworks 5.16.0, built for installation under /opt/local .
> 
> It seems that points 1) and 2) above already solve the crash issue in kdenlive on OS X, but the general principle stands so I suggest keeping 3) in library code like this.
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20160112/bc664ad1/attachment.html>