> 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 > >