[Ocs] Review Request: Crashfix for Attica when the network connection is lost after fetching GHNS data.

[Laszlo Papp]

> On July 2, 2012, 7:50 p.m., Laszlo Papp wrote:
> > I would prefer if you did not submit this. First, why are you trying to work on an unrelated method? Backtraces write about the abort method. Second, I would not like to get anything committed in a rush to a stable module without clear logic why the crash is happening, and why the fix would address that.
> > 
> > I am afraid, this might require more investigation from your side.
> 
> Mark Gaiser wrote:
>     Sorry for not mentioning that.
>     
>     Please don't start with "why are you trying to work on an unrelated method?" ... I took the time to backtrace it, found out that abort itself isn't doing anything that is (or seems) wrong so i searched deeper in that class to see what could cause abort to crash.
>     
>     The crash is happening because the d pointer is gone and abort tries to call an object on the d pointer. Yes, you could argue that it needs to check for d as well, but i think that's wrong because the GHNS list (for wallpapers as an example) is working fine the first time, but crashing the second time like i described in #251871 (last comment by my with the steps) thus something else must have been wrong. That d pointer is gone because deleteLayer(); is called in void BaseJob::dataFinished() which removes it's current object http://qt-project.org/doc/qt-4.8/qobject.html#deleteLater thus you get a crash in the abort function because d->m_reply simply doesn't exist because d is gone.
>     
>     I'm not quite sure anymore why i removed "d->m_reply->deleteLater();" as well.
> 
> Laszlo Papp wrote:
>     Your problem is basically that, knewstuff has been trying to abort a non-running process without even having any smart pointer or destroyed signal handling underneath. This causes that the pointer has been tried to be deleted twice. The second try is essentially the deletion of a dangling pointer, which is not good.
>     
>     Well, to be honest, abort() should be an abort, and not deletion in any circumstances. Means, it would abort the running job (not finished). Abort now seems more like a delayed deletion which is a suboptimal concept in my opinion. I would personally change the behavior in this case for good which does not mean API or ABI break. Once, the users complain (if any), then it can be reverted like the QUrl change in Qt 4.8.
>     
>     Also, if you take a look at the KJob implementation, they also have a hack for the auto/manual deletion for finished. Again, IMO, the definition of "abort" is different to me, what is now in there. A finished job is not a running job, so nothing to abort.
> 
> Mark Gaiser wrote:
>     Your reasoning does sound good. If you want i can take a look and see if i can fix it in a better way. Please do note that i'm only trying to prevent a crash which is open in bugzilla for quite some time. I'm not the attica maintainer and everything in there is new for me. If you're more experienced in attica then i would prefer you to fix this issue since i have no clue about how it was designed. Until a week ago i didn't even know something called "attica" existed within the KDE world.
>     
>     Also a thing i really don't understand but that looks very dangerous to me is the following.
>     On line 119 it does: emit finished(this); and in the current code it does deleteLayer(); right after that. Isn't that dangerous? I mean, you emit the this pointer which you then schedule for deletion.

Could you please verify this patch? It is not complete, but should be a good base.

diff --git a/knewstuff/knewstuff3/attica/atticaprovider.h b/knewstuff/knewstuff3/attica/atticaprovider.h
index e0231be..38aacfa 100644
--- a/knewstuff/knewstuff3/attica/atticaprovider.h
+++ b/knewstuff/knewstuff3/attica/atticaprovider.h
@@ -18,6 +18,7 @@
 #define KNEWSTUFF3_ATTICA_PROVIDER_H
 
 #include <QtCore/QSet>
+#include <QtCore/QPointer>
 
 #include <attica/providermanager.h>
 #include <attica/provider.h>
@@ -104,7 +105,7 @@ namespace KNS3
         QHash<Attica::BaseJob*, QPair<EntryInternal, int> > mDownloadLinkJobs;
         
         // keep track of the current request
-        Attica::BaseJob* mEntryJob;
+        QPointer<Attica::BaseJob> mEntryJob;
         Provider::SearchRequest mCurrentRequest;
         
         QSet<Attica::BaseJob*> m_updateJobs;


- Laszlo


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/105367/#review15317
-----------------------------------------------------------


On June 26, 2012, 9:04 p.m., Mark Gaiser wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105367/
> -----------------------------------------------------------
> 
> (Updated June 26, 2012, 9:04 p.m.)
> 
> 
> Review request for Attica.
> 
> 
> Description
> -------
> 
> Get hot new stuff could easily crash when filtering the retrieved data. The attached bug report shows a lot of backtraces that gave me a nice starting point in fixing this. I'm not quite sure if i fixed it the proper way. Somehow the original code was emitting the current class pointer followed by removing the same class. I simply removed the remove lines (deleteLayer) since the abort function is also cleaning the stuff up. I hope i'm not introducing a memory leak now?
> 
> 
> This addresses bug 251871.
>     http://bugs.kde.org/show_bug.cgi?id=251871
> 
> 
> Diffs
> -----
> 
>   lib/atticabasejob.cpp c4f0bc8 
> 
> Diff: http://git.reviewboard.kde.org/r/105367/diff/
> 
> 
> Testing
> -------
> 
> Tested it out with the descriptions in the provided bug. It seems to be working just fine now. No more crashes.
> 
> 
> Thanks,
> 
> Mark Gaiser
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/ocs/attachments/20120703/a191b2b5/attachment-0001.html>