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