> On Juli 30, 2014, 8:54 vorm., 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. > > Frank Reininghaus wrote: > 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.
As mentioned, "Accepted" /usually/ can be assumed to be safe (for the dialog) If you want to access members of the calling object and cannot guarantee that it has not been deleted before (ie. your initial concern), protecting *this with a QPointer is required. Please notice that the new suggestion would not provide any security beyond the stack solution. Putting casual stuff on the stack is anything but ideal, but once more: When a QObject dies while one of it's children is in a nested eventloop you will segfault. Stack, Heap, QPointer does not matter. Try if you don't believe it. On the heap, you'll likely end up somewhere in QApplicationPrivate::dispatch*() accessing a member of the removed child from QEventLoop::processEvents() in QDialog::exec() - Thomas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/119543/#review63489 ----------------------------------------------------------- On Juli 30, 2014, 6:05 nachm., Felix Eisele wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/119543/ > ----------------------------------------------------------- > > (Updated Juli 30, 2014, 6:05 nachm.) > > > 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 <<
