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

[Hugo Pereira Da Costa]

> On Aug. 26, 2016, 4: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();
> 
> Peter Wu wrote:
>     `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?
> 
> Anthony Fieroni wrote:
>     You can test to remove manual deleting with style->setParent(this)
> 
> Peter Wu wrote:
>     Setting `style->setParent(0)` in the two places (the initial `QApplication::style()` call and the internal `QProxyStyle` code from the testcase) prevents the crash on `exit()`. What about removing the `delete style` code, what are the risks for that?

I would vote for removing the "delete code". 
I know it was introduce to fix a crash in e.g. kioclient5, but i don't think this is the right patch. It basically results in ambiguous ownership of the Style objects (QApp, and/or QStylePlugin), which is wrong (and creates all sort of problems. not only this one).
The issue of QApplication not being destroyed on exit(), which was the reason why this code was introduced, should be fixed upstream.

I don't think deleting only the "first" QStyle is a proper fix either. (does not solve the ambiguous ownership, and has no guarantee to fix the original issue either, basically you get the worse of both worlds :))


- Hugo


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


On Aug. 25, 2016, 9: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, 9: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/20160827/b36b9ed7/attachment-0001.html>