> On July 30, 2014, 8:54 a.m., Vishesh Handa wrote: > > src/tagwidget.cpp, line 180 > > <https://git.reviewboard.kde.org/r/119543/diff/1/?file=294417#file294417line180> > > > > Any reason you've combined the too functions? I don't remember why, but > > there was a point when it split up. > > > > Also, if one really doesn't need it to be split up, then we should > > probably use a QScopedPointer instead of deleting it outself. > > Felix Eisele wrote: > i did googling yesterday and i think it is not needed. > I am not a fan of class members. you didnt see the dataflow. > We should i use a QScopedPointer? > > Thomas Braxton wrote: > Or you could just make it not a pointer and it cleans itself up. > > Felix Eisele wrote: > Good idea! > > Frank Reininghaus wrote: > No, this is a **very bad** idea. Creating the dialog on the stack as a > child of "this" and then executing its nested event loop will cause crashes > like the ones that are fixed by https://git.reviewboard.kde.org/r/118858/ . > You should use a QPointer for the dialog and delete it after calling exec() > and checking that the QPointer is not null. > > See the links that Dominik added in a comment to that review request of > mine for further info. > > Thomas Braxton wrote: > Since the whole purpose of this function is to get a list of tags using a > dialog, the dialog must exist when it returns from exec(). Otherwise the > whole function is **moot**.
Unfortunately, you can't really control what happens inside the nested event loop that is started by exec(). Anything can happen in there, including events that cause the deletion of the parent widget of the dialog. This parent widget will then try to delete the dialog, which lives on the stack. This will cause a crash. The easiest way to reproduce this is the "quit by D-Bus"-trick, as described in, e.g., https://blogs.kde.org/2009/03/26/how-crash-almost-every-qtkde-application-and-how-fix-it-0 http://kate-editor.org/2012/04/06/crash-through-d-bus-calls/ https://bugs.kde.org/show_bug.cgi?id=293863#c13 And even if you say that the user is unlikely to do this D-Bus thing: there are other ways to achieve the same effect, even if they are much harder to reproduce. You really cannot assume that the dialog still exists when exec() returns. Definitely not. Code that assumes this is buggy (even if the Qt docs for QDialog still suggest this approach - maybe I should try to get that changed). And no, I am really not making this stuff up. We do get bug reports about crashes that are caused by problems like this. Please do not add new versions of these bugs when modifying code. BTW, your statement that "the whole function is **moot**" if the dialog does not exist anymore is wrong. After all, the function cannot know what will happen during exec(). Most of the time, the dialog will still be there, stuff will get done with the tags, and everything is all right. But sometimes, the application will quit during exec(), and then the sole remaining purpose of the function is to return gracefully, without a crash. And this graceful return is prevented by creating the dialog on the stack. - Frank ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119543/#review63489 ----------------------------------------------------------- On July 30, 2014, 6:05 p.m., Felix Eisele wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/119543/ > ----------------------------------------------------------- > > (Updated July 30, 2014, 6:05 p.m.) > > > Review request for Baloo and Vishesh Handa. > > > Repository: baloo-widgets > > > Description > ------- > > Removed all KDialogs from balooWidgets. Removed not needed includes > > > Diffs > ----- > > src/kedittagsdialog.cpp c83ce9d > src/kedittagsdialog_p.h 0bcf744 > src/tagwidget.h 2843acd > src/tagwidget.cpp f2c3601 > src/tagwidget_p.h 045a185 > > Diff: https://git.reviewboard.kde.org/r/119543/diff/ > > > Testing > ------- > > > Thanks, > > Felix Eisele > >
>> Visit http://mail.kde.org/mailman/listinfo/kde-devel#unsub to unsubscribe <<
