Review Request 128760: Fix crash when using QProxyStyle and exit()

[Peter Wu]

> On Aug. 26, 2016, 6:22 a.m., Anthony Fieroni wrote:
> > kstyle/breezestyleplugin.cpp, line 44
> > <https://git.reviewboard.kde.org/r/128760/diff/1/?file=475410#file475410line44>
> >
> >     This must be not (!inited).
> >     However this is not proper fix. Correct and test patch in this way
> >     
> >     QPointer<Qstyle> style = new Style;
> >     
> >     Below unchanged, so when QPointer got delete it hold nullptr by itself and delete will be safe.
> 
> Peter Wu wrote:
>     This does not work since the interface requires a QStyle pointer. If I use this, I guess the caller unwraps it into a raw pointer and the issue is still triggered.
>     
>     Good point about `if (!inited)`, forgot to change this while renaming. I'll fix it for the next version. Do you know the root cause of the original issue that required the use of this? It is not documented by Qt.
> 
> Anthony Fieroni wrote:
>     QPointer track QObject and he knows when it's life or not, so issue must be fixed, at end
>     
>     return style.data();

`style.data()` should not be needed because the cast operator of QPointer does this implicitly. (I did test it though and it makes no difference.)

Digging further, I am not convinced that this patch (or the previous code) is correct. While the result of `QApplication::style()` is ignored, its parent is set to a `QApplication` instance. So when a program exits normally, QApplication will be taking care of the QStyle instance. When `exit()` is invoked, the QApplication destructor does not seem to be called which probably led to the code in the first place.

I am tempted to remove the delete code completely, it seems a hack/workaround at the wrong level. Thoughts?


- Peter


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


On Aug. 25, 2016, 11:56 p.m., Peter Wu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128760/
> -----------------------------------------------------------
> 
> (Updated Aug. 25, 2016, 11:56 p.m.)
> 
> 
> Review request for Plasma, David Edmundson, David Faure, and Hugo Pereira Da Costa.
> 
> 
> Bugs: 356940
>     https://bugs.kde.org/show_bug.cgi?id=356940
> 
> 
> Repository: breeze
> 
> 
> Description
> -------
> 
> Do not delete all style instances which we create, restrict ourselves to
> the first instance. I have no idea if the delete hack is still needed,
> but let's keep it until it is certain that it is unneeded.
> 
> 
> Diffs
> -----
> 
>   kstyle/breezestyleplugin.cpp 083100e 
> 
> Diff: https://git.reviewboard.kde.org/r/128760/diff/
> 
> 
> Testing
> -------
> 
> Used "Testcase (with ASAN)" from bug https://bugs.kde.org/show_bug.cgi?id=356940. Run directly, no more crashes. Double-checked with a breakpoint on Breeze::StylePlugin::create that the second instance is called through QProxyStyle.
> 
> 
> Thanks,
> 
> Peter Wu
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20160826/8badf266/attachment-0001.html>