Review Request 122500: Don't delete widgets directly when changing styles

[David Edmundson]

> On Feb. 9, 2015, 5:34 p.m., Hugo Pereira Da Costa wrote:
> > Although I have no objection against the change, I must admit I don't understand what's wrong with the current code, nor the actual description of the patch. 
> > "registerWidget may take an existing widget as a parameter. If so, we
> > don't want to delete it"
> > if I understand my own code right, _widget is not the argument passed as the registerWidget method. It is an internal member, created parentless, at first accepted call to registerWidget. So as such it is not 'explicitly' owned by anyone, and implicitly owned by us. 
> > And then, what is wrong with deleting it in our destructor ? 
> > is it because, though parentless it might get deleted elsewhere ? 
> > or because of a thread issue ? 
> > What do I miss ? 
> > (PS: the reason behind this interal _widget member, is that you can not track palettechanged events on a widget, via event filter, once you set it your own 'altered' palette: it won't recive these events anymore. 
> > So: eventFilter must be installed on either qApp (which is then getting the event for _every_ widget, for which we did not alter the palette, which is quite a lot), or on a widget for which we are sure the pallette is not altered. Hence: our own).
> 
> David Edmundson wrote:
>     oh, you're absolutely right...
>     
>     I just had my valgrind traces and in my haste didn't see the difference between _widget and widget.
>     
>     
>     The crash was in QApplication trying to update the palette on _widget after you change styles in the style KCM.
>     https://paste.kde.org/pvhhfielh
>     
>     According to valgrind, the widget it was trying to update was very much the one deleted in the PaletteHelper destructor.
>     
>     I'll try replacing just delete _widget with _widget->deleteLater() and see if that crash still happens; it might be the more relevant part of the fix. It's generally a bad idea to directly delete a QObject in anything that might be called from a slot.
>     
>     
>     Thanks.
> 
> Hugo Pereira Da Costa wrote:
>     Clear enough. I agree with the delete -> deleteLater change. Keep me posted !  
>     Does the crash you report above happen every time ? (and is there a bug report ?), At least here i could not reproduce so far.
> 
> David Edmundson wrote:
>     There's a bug report on Netrunner's internal issue tracker. 
>     
>     It's only when going from qtcurve->breeze that systemsetting crashes
>     (yes, that way round, even though it seems backwards. There is a preview of some widgets with the Breeze theme in systemsettings which is the app that crashes so it makes sense that some Breeze stuff )
>     
>     It seems QtCurve handles the same dbus call that makes apps switch theme to switch palette ast the same time.
>     
>     
>     changing to deleteLater() seems to work. I'll update this.

My valgrind trace when it was crashing.
https://paste.kde.org/p8cxylcox


- David


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


On Feb. 10, 2015, 1:25 p.m., David Edmundson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122500/
> -----------------------------------------------------------
> 
> (Updated Feb. 10, 2015, 1:25 p.m.)
> 
> 
> Review request for Plasma and Hugo Pereira Da Costa.
> 
> 
> Repository: breeze
> 
> 
> Description
> -------
> 
> Don't delete widgets we don't own when changing styles
> 
> registerWidget may take an existing widget as a parameter. If so, we
> don't want to delete it when our paletteHelper is deleted for example if
> we change style.
> 
> 
> Diffs
> -----
> 
>   kstyle/breezepalettehelper.cpp 31c32c3 
> 
> Diff: https://git.reviewboard.kde.org/r/122500/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> David Edmundson
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20150210/6f74364e/attachment.html>