Review Request 113965: Possible fix for bug 321100

[Thomas Lübking]

> On Nov. 20, 2013, 6:02 p.m., Albert Astals Cid wrote:
> > I don't see why this should fix anything. Do you think anyone in the bug can provide a valgrind trace to better understand why it's crashing?
> 
> Christoph Feck wrote:
>     See also https://git.reviewboard.kde.org/r/102981/ which has some discussion and links to related bugs.
> 
> Albert Astals Cid wrote:
>     Why is it related? Who is modifying the entries variable? It's used in 4 places in the file, and actually there's no way to remove stuff from it, so I don't see how it is related to the bug you point.
> 
> Christoph Feck wrote:
>     They are only related because replacing qDeleteAll() with manual deletion fixed the crash for Boudewijn. Since my understanding ended there, I suggested to post a review request.
> 
> Thomas Lübking wrote:
>     See http://lists.kde.org/?t=132194778400005&r=1&w=2
>     Qt 4.8 changed qDeleteAll to rely on the container being immutable.
>     
>     The patch detaches the container, what allows safe operation despite - what's likely happening as it seemed the core issue back than - the container is altered by the deletion of one or more of its items (eg. the items deconstructor delists itself)
>     
>     An alternative solution would be to pass the to-be-deleted objects class a static member flag to skip self delisting and set that around qDeleteAll() (which would be followed by an erase())
> 
> Albert Astals Cid wrote:
>     Please look at the code and tell us where the item deconstructor delists itself from the list.

I said that it seemed the core issue back then, not that it happens here (for sure) or would be the only trigger.

Briefly looking at KArchive, i'd rather bet on a recursion (entries containing a KArchiveEntry being or ultimately pointing down to the same  KArchiveDirectory)
Just a wild guess, though - but testable if one can reproduce the bug (unless you can assure that cannot be the case)


- Thomas


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


On Nov. 20, 2013, 9:40 a.m., Boudewijn Rempt wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/113965/
> -----------------------------------------------------------
> 
> (Updated Nov. 20, 2013, 9:40 a.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Bugs: 321100
>     http://bugs.kde.org/show_bug.cgi?id=321100
> 
> 
> Repository: kdelibs
> 
> 
> Description
> -------
> 
> While I haven't been able to reproduce the issue on Linux, many Krita users encounter a crash in the destructor of  KArchiveDirectoryPrivate, where all entries are removed with qDeleteAll.
> 
> This patch removes the use of qDeleteAll.
> 
> I'm not 100% sure whether this is correct, but it works for me with the Windows build of kdelibs I use.
> 
> 
> Diffs
> -----
> 
>   kdecore/io/karchive.cpp 88e1de0 
> 
> Diff: http://git.reviewboard.kde.org/r/113965/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Boudewijn Rempt
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20131120/7fe8cb02/attachment.htm>