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