> 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. > > Thomas Lübking wrote: > 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) > > Boudewijn Rempt wrote: > I haven't been able to reproduce it myself on Linux. On Windows it was a > lot easier, but there I use an old and completely hacked-up version of > kdelibs. However, when I was giving a workshop at LGM, pretty much half of > the Linux users present (most of them *buntu) users had to disable autosave > to prevent this crash from happening. > > I'm puzzled... And I would love a better fix than mine, but that will > have to come from someone who understands karchive -- which I don't, not > really. > > Friedrich W. H. Kossebau wrote: > I do not have a better fix yet, but I think I found the root of the > problem: > in case of a duplicated name for an entry in KZip::doPrepareWriting() the > old entry is removed from d->m_fileList, but not from the parentDir directory > holding it: > > https://projects.kde.org/projects/kde/kdelibs/repository/revisions/ee5dea835039c8a8f765321378dbed398826f368/entry/kdecore/io/kzip.cpp#L1026 > > Thus on destruction of that dir the qDeleteAll will try to delete an > entry that is already deleted. Seems that triggers not always a crash, > perhaps because the memory might still be available? > > Sadly I do not have a kdelibs development environment setup currently and > also short of disk space, so cannot come up with a patch/unittest. Anyone? > But so far I already see the problem that KArchiveDirectory has a method > "addEntry( KArchiveEntry* )" (which currently in case of adding an entry with > the same name qwarns about that, and ignores the new entry), but not some > "removeEntry( KArchiveEntry* )". This needs some more thinking what the > proper behaviour should be and how to solve that. Perhaps "addEntry( > KArchiveEntry* )" should just overwrite the old entry, that would at least > match the behaviour in KZip::doPrepareWriting()... > > Any takers?
Took it myself ;) Please see & review https://git.reviewboard.kde.org/r/114048/ for a proposal how to fix that problem in KZip::doPrepareWriting(). - Friedrich W. H. ----------------------------------------------------------- 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 > >
