DevHeads.net

Review Request 113965: Possible fix for bug 321100

Review request for kdelibs.

Bugs: 321100
<a href="http://bugs.kde.org/show_bug.cgi?id=321100" title="http://bugs.kde.org/show_bug.cgi?id=321100">http://bugs.kde.org/show_bug.cgi?id=321100</a>

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: <a href="http://git.reviewboard.kde.org/r/113965/diff/" title="http://git.reviewboard.kde.org/r/113965/diff/">http://git.reviewboard.kde.org/r/113965/diff/</a>

Testing

Thanks,

Boudewijn Rempt

Comments

Re: Review Request 113965: Possible fix for bug 321100

By Boudewijn Rempt at 11/23/2013 - 04:51

(Updated Nov. 23, 2013, 8:51 a.m.)

Status
This change has been discarded.

Review request for kdelibs.

Bugs: 321100
<a href="http://bugs.kde.org/show_bug.cgi?id=321100" title="http://bugs.kde.org/show_bug.cgi?id=321100">http://bugs.kde.org/show_bug.cgi?id=321100</a>

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: <a href="http://git.reviewboard.kde.org/r/113965/diff/" title="http://git.reviewboard.kde.org/r/113965/diff/">http://git.reviewboard.kde.org/r/113965/diff/</a>

Testing

Thanks,

Boudewijn Rempt

Re: Review Request 113965: Possible fix for bug 321100

By Albert Astals Cid at 11/20/2013 - 14:02

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?

- Albert Astals Cid

On Nov. 20, 2013, 9:40 a.m., Boudewijn Rempt wrote:

Re: Review Request 113965: Possible fix for bug 321100

By Friedrich W. H.... at 11/22/2013 - 19:03

Took it myself ;)

Please see & review <a href="https://git.reviewboard.kde.org/r/114048/" title="https://git.reviewboard.kde.org/r/114048/">https://git.reviewboard.kde.org/r/114048/</a> for a proposal how to fix that problem in KZip::doPrepareWriting().

- Friedrich W. H.

On Nov. 20, 2013, 9:40 a.m., Boudewijn Rempt wrote:

Re: Review Request 113965: Possible fix for bug 321100

By =?utf-8?Q?Thoma... at 11/20/2013 - 16:41

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

On Nov. 20, 2013, 9:40 a.m., Boudewijn Rempt wrote:

Re: Review Request 113965: Possible fix for bug 321100

By Albert Astals Cid at 11/20/2013 - 16:18

Please look at the code and tell us where the item deconstructor delists itself from the list.

- Albert

On Nov. 20, 2013, 9:40 a.m., Boudewijn Rempt wrote:

Re: Review Request 113965: Possible fix for bug 321100

By =?utf-8?Q?Thoma... at 11/20/2013 - 16:08

See <a href="http://lists.kde.org/?t=132194778400005&amp;r=1&amp;w=2" title="http://lists.kde.org/?t=132194778400005&amp;r=1&amp;w=2">http://lists.kde.org/?t=132194778400005&amp;r=1&amp;w=2</a>
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())

- Thomas

On Nov. 20, 2013, 9:40 a.m., Boudewijn Rempt wrote:

Re: Review Request 113965: Possible fix for bug 321100

By Christoph Feck at 11/20/2013 - 14:46

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.

- Christoph

On Nov. 20, 2013, 9:40 a.m., Boudewijn Rempt wrote:

Re: Review Request 113965: Possible fix for bug 321100

By Albert Astals Cid at 11/20/2013 - 14:39

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.

- Albert

On Nov. 20, 2013, 9:40 a.m., Boudewijn Rempt wrote:

Re: Review Request 113965: Possible fix for bug 321100

By Christoph Feck at 11/20/2013 - 14:34

See also <a href="https://git.reviewboard.kde.org/r/102981/" title="https://git.reviewboard.kde.org/r/102981/">https://git.reviewboard.kde.org/r/102981/</a> which has some discussion and links to related bugs.

- Christoph

On Nov. 20, 2013, 9:40 a.m., Boudewijn Rempt wrote: