> On Авг. 26, 2016, 7:22 преди обяд, 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?
You can test to remove manual deleting with style->setParent(this) - Anthony ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128760/#review98667 ----------------------------------------------------------- On Авг. 26, 2016, 12:56 преди обяд, Peter Wu wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/128760/ > ----------------------------------------------------------- > > (Updated Авг. 26, 2016, 12:56 преди обяд) > > > 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 > >