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? Cheers, Johannes
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.-
Hi, now I have a reliable way to reproduce the problem. On DragonFly I use debug malloc options, but valgrind should also work. Even without any debug features I can crash dillo that way, but it takes longer. * Choose a rather long page that does not fit on the screen with lot's of links. I use my bookmarks page. * Start the search dialog and enter a very common string e.g. 2 chars and hold the Enter key down. Dillo now scrolls through the page over and over again. * Now click the 1. mouse button and move the mouse like crazy over the scrolling page so arbitrary text gets selected and deselected. That crashes dillo pretty soon for me. Attached patch fixes the problem here. Please test and check for potential memory leaks. BTW, the patch fixes one leak in Iterator::scrollTo(). Cheers, Johannes
On Thu, Jul 03, 2008 at 04:09:19PM +0200, Johannes Hofmann wrote:
Hi,
now I have a reliable way to reproduce the problem. On DragonFly I use debug malloc options, but valgrind should also work. Even without any debug features I can crash dillo that way, but it takes longer.
* Choose a rather long page that does not fit on the screen with lot's of links. I use my bookmarks page.
* Start the search dialog and enter a very common string e.g. 2 chars and hold the Enter key down. Dillo now scrolls through the page over and over again.
* Now click the 1. mouse button and move the mouse like crazy over the scrolling page so arbitrary text gets selected and deselected.
That crashes dillo pretty soon for me.
Funny as it may look, I can't reproduce it here. Maybe it's not strange because we've already noticed DragonFly handles memory in a different way than GNU/Linux.
Attached patch fixes the problem here. Please test and check for potential memory leaks. BTW, the patch fixes one leak in Iterator::scrollTo().
Committed both. I gave it a review: [OK] new DeepIterator [OK] DeepIterator *DeepIterator::createVariant(Iterator *it) [OK] object::Object *DeepIterator::clone () OK cloneDeepIterator() TableIterator() \ Not reviewed because thay don't inherit TextblockIterator() / from DeepIterator [OK][2] SelectionState::buttonPress() BTW, it's in things like this that I really dislike C++... -- Cheers Jorge.-
On Fri, Jul 04, 2008 at 11:19:43AM -0400, Jorge Arellano Cid wrote:
On Thu, Jul 03, 2008 at 04:09:19PM +0200, Johannes Hofmann wrote:
Hi,
now I have a reliable way to reproduce the problem. On DragonFly I use debug malloc options, but valgrind should also work. Even without any debug features I can crash dillo that way, but it takes longer.
* Choose a rather long page that does not fit on the screen with lot's of links. I use my bookmarks page.
* Start the search dialog and enter a very common string e.g. 2 chars and hold the Enter key down. Dillo now scrolls through the page over and over again.
* Now click the 1. mouse button and move the mouse like crazy over the scrolling page so arbitrary text gets selected and deselected.
That crashes dillo pretty soon for me.
Funny as it may look, I can't reproduce it here. Maybe it's not strange because we've already noticed DragonFly handles memory in a different way than GNU/Linux.
I could crash it like this under valgrind on a linux box.
Attached patch fixes the problem here. Please test and check for potential memory leaks. BTW, the patch fixes one leak in Iterator::scrollTo().
Committed both.
I gave it a review:
[OK] new DeepIterator [OK] DeepIterator *DeepIterator::createVariant(Iterator *it) [OK] object::Object *DeepIterator::clone () OK cloneDeepIterator() TableIterator() \ Not reviewed because thay don't inherit TextblockIterator() / from DeepIterator [OK][2] SelectionState::buttonPress()
Thanks for reviewing.
BTW, it's in things like this that I really dislike C++...
Yes, it can get tricky. On the other hand C++ in dillo is pretty harmless compared to other projects :-) Cheers, Johannes
On Fri, Jul 04, 2008 at 05:27:21PM +0200, Johannes Hofmann wrote:
On Fri, Jul 04, 2008 at 11:19:43AM -0400, Jorge Arellano Cid wrote:
On Thu, Jul 03, 2008 at 04:09:19PM +0200, Johannes Hofmann wrote:
Hi,
now I have a reliable way to reproduce the problem. On DragonFly I use debug malloc options, but valgrind should also work. Even without any debug features I can crash dillo that way, but it takes longer.
* Choose a rather long page that does not fit on the screen with lot's of links. I use my bookmarks page.
* Start the search dialog and enter a very common string e.g. 2 chars and hold the Enter key down. Dillo now scrolls through the page over and over again.
* Now click the 1. mouse button and move the mouse like crazy over the scrolling page so arbitrary text gets selected and deselected.
That crashes dillo pretty soon for me.
Funny as it may look, I can't reproduce it here. Maybe it's not strange because we've already noticed DragonFly handles memory in a different way than GNU/Linux.
I could crash it like this under valgrind on a linux box.
I also tried with: valgrind --tool=memcheck --leak-check=yes ./dillo-fltk but I believe you! :-)
BTW, it's in things like this that I really dislike C++...
Yes, it can get tricky. On the other hand C++ in dillo is pretty harmless compared to other projects :-)
8-P -- Cheers Jorge.-
participants (2)
-
jcid＠dillo.org -
Johannes.Hofmann＠gmx.de