> 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(); > > 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? > > Hugo Pereira Da Costa wrote: > 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 :))
There are no outstanding issues, would it be possible to ship this patch? After upgrading breeze to v5.7.5 on Arch Linux, things started crashing again without this patch. - Peter ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128760/#review98667 ----------------------------------------------------------- On Aug. 27, 2016, 11:04 a.m., Peter Wu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/128760/ > ----------------------------------------------------------- > > (Updated Aug. 27, 2016, 11:04 a.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 > ------- > > Since Qt 5.6.0, Qt5 applications started crashing on exit. All signs > point to this delete-on-destroy hack which was added to avoid outliving > the plugin lifetime. > > This method is wrong because the returned style is owned by the caller > (QApplication, QProxyStyle, etc) and will cleaned up when those users > are destructed. > > > Diffs > ----- > > kstyle/breezestyleplugin.cpp 083100e > > Diff: https://git.reviewboard.kde.org/r/128760/diff/ > > > Testing > ------- > > Ran the updated test.sh from "Testcase (ASAN) with normal QApplication::quit > and exit()" from bug https://bugs.kde.org/show_bug.cgi?id=356940, no longer > crashes. Tested with Qt 5.7.0. > > > Thanks, > > Peter Wu > >
