On Wed, Jul 02, 2008 at 09:11:12PM +0200, Johannes Hofmann wrote:
Hi,
while looking for the reason of the rare selection related crashes I found something that seems like a severe bunch of bugs in the iterator code. It might be related to the crash, but I'm not 100% sure yet.
There is this innocent comment in front of the DeepIterator constructor in iterator.cc:
If you want to continue using \em it, pass it->cloneIterator().
This means, that the DeepIterator constructor eats up the Iterator it get's passed a pointer to!
The evil thing is that every method that passes one of it's arguments to the DeepIterator constructor inherits this property, e.g:
DeepIterator::createVariant() and as a consequence also: SelectionState::switchLinkToSelection() and SelectionState::adjustSelection()
In Iterator::scrollTo() Iterators that are used after being passed to the DeepIterator constructor.
In SelectionState::buttonRelease() an iterator that has been passed to switchLinkToSelection() might be used afterwards as argument to adjustSelection().
I would propose to change the behaviour of the DeepIterator constructor and always clone its input Iterator. Better be safe than sorry and the fact that there is already several bugs related to this shows, that the current behaviour - even if it's documented - is really dangerous.
What do you think?
I agree with you. This is an API which already has hard-to-find side effects. Set DeepIterator to always clone, and if there's a performance penalty that's worth addressing, we may add a reference-parameter version of the constructor. -- Cheers Jorge.-