Changes in SqlQueryMaker.cpp

[RISHABH GUPTA]

From: Michael Pyne <mpyne at kde.org>
Date: Thu, Oct 29, 2015 at 7:23 AM
Subject: Re: Debug ThreadWeaver::IdDecorator
To: kde-devel at kde.org


On Wed, October 28, 2015 18:45:40 Olivier Churlaud wrote:
> Le 28/10/2015 16:08, Thomas Lübking a écrit :
> > On Mittwoch, 28. Oktober 2015 14:33:02 CEST, Olivier Churlaud wrote:
> >> Hello
> >>
> >> I finally managed to install the Threadweaver in debug mod.
> >>
> >> Fulltrace in gdb : https://paste.kde.org/pdnashan2
> >>
> >>                  in valgrind https://paste.kde.org/pwooi9alf
> >>
> >> Thx for your hints
> >
> > W/o even looking at the code (is it pushed somewhere at all?) - you're
> > not explicitly deleting and autodelete (by default) IdDecorator, are
you?
> >
> > Cheers,
> > Thomas
>
> The code is pushed on amarok, branch kf5.
>
> IdDecorator is never mentioned anywhere in the code. Only derived classes.
> It works well in the KDE4 environnement (since the amarok devs are able
> to compile it and running it well) but not on the KF5 one. That's why I
> asked it there, because it might be a modification in the library that
> I'm not aware of.

It seems to me that there is a double-deletion going on, likely due to a
manual deletion of the ThreadWeaver job after it is completed at src / core-
impl / collections / db / sql / SqlQueryMaker.cpp:234, and likely also at
line
160 with similar reasoning.

Line 234 contains

> void
> SqlQueryMaker::done( ThreadWeaver::JobPointer job )
> {
>
>     ThreadWeaver::QObjectDecorator *qs = new
>     ThreadWeaver::QObjectDecorator(job.data()); qs->deleteLater();
>     d->worker = 0; // d->worker *is* the job, prevent stale pointer
>     emit queryDone();
>
> }

In this case ThreadWeaver::JobPointer is a QSharedPointer which means that
the
QSharedPointer controls the object lifetime for this sql worker thread.
However the pointer is manually pulled out of the QSharedPointer (by
job.data()) and then manually deleted.

By the time the actual QSharedPointer is ready to delete the pointer, it has
already been deleted thanks to the qs->deleteLater(), which causes the crash
you see.

This is also where you see the "IdDecorator" stuff from: IdDecorator is a
superclass of the QObjectDecorator that was wrapped around d->worker.

>From the moment that d->worker was enqueue()'d into ThreadWeaver (in line
212)
Amarok lost control of its object lifetime, and therefore should not attempt
to delete it manually.

Instead just let ThreadWeaver control the object lifetime. If you wish for
the
code to work in both the ThreadWeaver and non-ThreadWeaver cases you should
at
least also check for d->blocking when manually deleting d->worker.

Regards,
 - Michael Pyne


As we couldn't  figure out how this threadweaver issue could be solved,
olivier wrote a mail to kde-devel regarding the same
. Michael pyne pointed out wherer the issue was .Please refer to the above
message ,he has explained the reason of why we were getting the
segmentation fault .


Thank you ,
rishabh

On Fri, Oct 30, 2015 at 12:27 AM, Aroonav Mishra <aroonav11 at gmail.com>
wrote:

> Hello all,
>
> KF5::ThreadWeaver is not very well documented and the docs available for
> it's porting is inadequate. And due to this, porting this part was tricky &
> took up quite a bit of my time.
>
> I will be explaining all the changes that I made in here
> <https://quickgit.kde.org/?p=amarok.git&a=blobdiff&h=15c1b0b28d8865a56b37da04e919e6e803a3454d&hp=b4427f2db1126f05bb10b57216a47070f14acfe3&f=src%2Fcore-impl%2Fcollections%2Fdb%2Fsql%2FSqlQueryMaker.cpp&hb=adee016bdeb5ea747e45f88beb42c58f4521d492>.
> (which also includes some of the changes that I have mentioned here
> <https://mail.kde.org/pipermail/amarok-devel/2015-July/013486.html>)
>
> In the constructor call,ThreadWeaver::Job no longer inherits QObject in
> KF5. Hence it only seemed logical to add QObject into the inheritance
> hierarchy(and subsequently the constructor call too).
>
> The pure virtual function ThreadWeaver::Job::run() prototype has changed
> and hence 2 new formal parameters have been added in the SqlWorkerThread. I
> had set a default value to each of these 2 new parameters. I neither saw a
> benefit nor a way of using these new parameters. The first default
> parameter is QSharedPointer<TheradWeaver::Job>() which returns a
> QSharedPointer pointing to null. The second default parameter is
> self-explanatory. The Q_UNUSED has been added to suppress the warnings of
> non-usage of these parameters.
>
> defaultBegin(), defaultEnd() and the signals(started, done, failed) have
> been removed in KF5::ThreadWeaver::Job and these have been moved into
> QObjectDecorator class. QObjectDecorator is a wrapper class that wraps
> around the Job object. I faced problems in using QObjectDecorator & hence I
> had directly added these(the functions and the signals) into the
> SqlWorkerThread class itself.
>
> d->worker->deleteLater(); This is one part which I hadn't paid much
> attention before until now. Rishabh has identified the issue in the recent
> review request #125872 <https://git.reviewboard.kde.org/r/125872/> to be
> here. I have suggested the modifications in the comments.
>
> Thank you,
> Aroonav Mishra.
>
> _______________________________________________
> Amarok-devel mailing list
> Amarok-devel at kde.org
> https://mail.kde.org/mailman/listinfo/amarok-devel
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/amarok-devel/attachments/20151030/e786b7cd/attachment-0001.html>