> 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**. > > Frank Reininghaus wrote: > 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. > > Thomas Braxton wrote: > You're right, QPointer it has to be. > > Thomas Lübking wrote: > I don't wanna be a spoil spot, but if you delete a qobject while one of > its (heaped) children is in a nested eventloop (ie. you're most likely > deleting it from that eventloop), you'll usually hit a segfault just as well > - QPointer or not. > > --> If you need to delete sth. and don't know whether you're in a nested > loop and "something" might be an ancestor: > call ::deleteLater() > > In other cases, you'll have to protect _anything_ on the heap that you're > gonna access after the nested eventloop exited (esp. "this" and the dialog, > but also any other heap data) > > The stack is sane with "this", but as of course increases the memory > footprint (ie. not good for large temporary structures) > > The dialog is /usually/ safe when it exits "Accepted", but there's no > general guarantee (depends on the dialog class) > > Felix Eisele wrote: > Ok i see the problem. As a result of that if we use QDialog and check the > result in that way we should use always qpointer? > > void TagWidget::slotShowAll() > { > QPointer<KEditTagsDialog> dialog = new KEditTagsDialog( > selectedTags(), this); > if (dialog->exec() == QDialog::Accepted) { > setSelectedTags(dialog->tags()); > emit selectionChanged(selectedTags()); > } > > if (dialog){ > dialog->deleteLater(); > } > } > > Thomas Lübking wrote: > void TagWidget::slotShowAll() > { > QPointer<TagWidget> that = this; > QPointer<KEditTagsDialog> dialog = new > KEditTagsDialog(selectedTags(), this); > if (dialog->exec() == QDialog::Accepted && // this is the critical > line. if *this is deleted durig exec(), you lost > dialog && that) { // this is the safety check - you need *this > and dialog to be still sane > setSelectedTags(dialog->tags()); > emit selectionChanged(selectedTags()); > } > delete dialog; // it's ok to delete NULL and you don't need to defer > it since dialog is for sure not an ancestor of *this > } > > If you want to be really sure, don't parent dialog with *this. > Since that's not required for memory management (you delete dialog), the > only good the parent gets you (and that I can think of) is window transiency. > You can gain that with QWindow::setTransientParent(QWindow *parent) in Qt5 > In Qt4, using an ApplicationModal dialog might do. > > Alternatively, do simply not use a nested eventloop, ie. set the dialog > modal, bind its finished() signal to a slot and then show it.
I don't think that you need the 'that' QPointer. Felix' proposal looks fine to me. AFAIK, exec() will never return QDialog::Accepted if the event loop is aborted by the dialog (or both the parent widget and the dialog) being deleted, so the "&& that" part of the if-check is superfluous from my point of view. - 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 <<
