BUG: assertion failed: "no widget": in dw::core::DeepIterator::searchSideward
Dillo aborts with an assertion failure if you click to the right of an image that has a line all to itself. For example, go to http://www.thenation.com/doc/20071203/pollitt and click to the right of the "Continued" logo. What happens is that the DeepIterator constructor wants an iterator with non-widget content. If necessary it calls searchDownward and searchSideward until it finds one. Both these methods raise an assertion failure if the iterator already has non-widget content, so the content must be checked before calling *either* of them. However, DeepIterator checks the content before calling searchDownward but *not* before calling searchSideward . Effectively, the code assumes that if searchDownward fails then the content type won't have changed. Unfortunately searchDownward can fail *and* change the content of the iterator to something non-widget. If this happens DeepIterator then calls searchSideward, which triggers the assertion. (NB: DeepIterator is in the dw::core namespace.) I'm trying to fix this but so far I'm just generating more crashes. What exactly should DeepIterator::DeepIterator do if searchDownward fails and changes the content of the iterator? If I know that I'll have another go. Regards, Jeremy Henty
Hi, On Wed, Dec 05, 2007 at 11:45:53PM +0000, Jeremy Henty wrote:
Dillo aborts with an assertion failure if you click to the right of an image that has a line all to itself. For example, go to http://www.thenation.com/doc/20071203/pollitt and click to the right of the "Continued" logo.
What happens is that the DeepIterator constructor wants an iterator with non-widget content. If necessary it calls searchDownward and searchSideward until it finds one. Both these methods raise an assertion failure if the iterator already has non-widget content, so the content must be checked before calling *either* of them. However, DeepIterator checks the content before calling searchDownward but *not* before calling searchSideward . Effectively, the code assumes that if searchDownward fails then the content type won't have changed.
Unfortunately searchDownward can fail *and* change the content of the iterator to something non-widget. If this happens DeepIterator then calls searchSideward, which triggers the assertion.
(NB: DeepIterator is in the dw::core namespace.)
I'm trying to fix this but so far I'm just generating more crashes. What exactly should DeepIterator::DeepIterator do if searchDownward fails and changes the content of the iterator? If I know that I'll have another go.
Sorry, I can't help you on this one. Sebastian may answer later, or you can continue your research on the problem... -- Cheers Jorge.-
Hi Jeremy, On Wed, Dec 05, 2007 at 11:45:53PM +0000, Jeremy Henty wrote:
Dillo aborts with an assertion failure if you click to the right of an image that has a line all to itself. For example, go to http://www.thenation.com/doc/20071203/pollitt and click to the right of the "Continued" logo.
Great you are looking into that. It was next on my list...
What happens is that the DeepIterator constructor wants an iterator with non-widget content. If necessary it calls searchDownward and searchSideward until it finds one. Both these methods raise an assertion failure if the iterator already has non-widget content, so the content must be checked before calling *either* of them. However, DeepIterator checks the content before calling searchDownward but *not* before calling searchSideward . Effectively, the code assumes that if searchDownward fails then the content type won't have changed.
Unfortunately searchDownward can fail *and* change the content of the iterator to something non-widget. If this happens DeepIterator then calls searchSideward, which triggers the assertion.
(NB: DeepIterator is in the dw::core namespace.)
I think both DeepIterator::searchDownward() and DeepIterator::searchSideward() are not supposed to modify their argument it. So without really understanding what's going on, I would say it's a typo and should read: diff -r 8edae7f02bd7 dw/iterator.cc --- a/dw/iterator.cc Thu Dec 06 13:30:01 2007 +0100 +++ b/dw/iterator.cc Thu Dec 06 15:23:20 2007 +0100 @@ -343,7 +343,7 @@ Iterator *DeepIterator::searchDownward ( return NULL; } - while (fromEnd ? it->prev () : it->next ()) { + while (fromEnd ? it2->prev () : it2->next ()) { //DEBUG_MSG (1, "%*sexamining %s\n", // indent, "", a_Dw_iterator_text (it2)); @@ -384,7 +384,7 @@ Iterator *DeepIterator::searchSideward ( misc::assert (it->getContent()->type == Content::WIDGET, "no widget"); it2 = it->cloneIterator (); - while (fromEnd ? it->prev () : it->next ()) { + while (fromEnd ? it2->prev () : it2->next ()) { if (it2->getContent()->type == Content::WIDGET) { // Search downwards in this widget. it3 = searchDownward (it2, mask, fromEnd); Unfortunately it now crashes somewhere else related to text selection... Regards, Johannes
I'm trying to fix this but so far I'm just generating more crashes. What exactly should DeepIterator::DeepIterator do if searchDownward fails and changes the content of the iterator? If I know that I'll have another go.
Regards,
Jeremy Henty
_______________________________________________ Dillo-dev mailing list Dillo-dev@dillo.org http://lists.auriga.wearlab.de/cgi-bin/mailman/listinfo/dillo-dev
Ok, I think I found it. At least diff -r 8edae7f02bd7 dw/iterator.cc --- a/dw/iterator.cc Thu Dec 06 13:30:01 2007 +0100 +++ b/dw/iterator.cc Thu Dec 06 17:16:42 2007 +0100 @@ -343,7 +343,7 @@ Iterator *DeepIterator::searchDownward ( return NULL; } - while (fromEnd ? it->prev () : it->next ()) { + while (fromEnd ? it2->prev () : it2->next ()) { //DEBUG_MSG (1, "%*sexamining %s\n", // indent, "", a_Dw_iterator_text (it2)); @@ -351,7 +351,7 @@ Iterator *DeepIterator::searchDownward ( // Another widget. Search in it downwards. it3 = searchDownward (it2, mask, fromEnd); if (it3 != NULL) { - it->unref (); + it2->unref (); return it3; } // Else continue in this widget. @@ -384,7 +384,7 @@ Iterator *DeepIterator::searchSideward ( misc::assert (it->getContent()->type == Content::WIDGET, "no widget"); it2 = it->cloneIterator (); - while (fromEnd ? it->prev () : it->next ()) { + while (fromEnd ? it2->prev () : it2->next ()) { if (it2->getContent()->type == Content::WIDGET) { // Search downwards in this widget. it3 = searchDownward (it2, mask, fromEnd); fixes the crashes for me. Please test. Cheers, Johannes On Thu, Dec 06, 2007 at 03:26:40PM +0100, Johannes Hofmann wrote:
Hi Jeremy,
On Wed, Dec 05, 2007 at 11:45:53PM +0000, Jeremy Henty wrote:
Dillo aborts with an assertion failure if you click to the right of an image that has a line all to itself. For example, go to http://www.thenation.com/doc/20071203/pollitt and click to the right of the "Continued" logo.
Great you are looking into that. It was next on my list...
What happens is that the DeepIterator constructor wants an iterator with non-widget content. If necessary it calls searchDownward and searchSideward until it finds one. Both these methods raise an assertion failure if the iterator already has non-widget content, so the content must be checked before calling *either* of them. However, DeepIterator checks the content before calling searchDownward but *not* before calling searchSideward . Effectively, the code assumes that if searchDownward fails then the content type won't have changed.
Unfortunately searchDownward can fail *and* change the content of the iterator to something non-widget. If this happens DeepIterator then calls searchSideward, which triggers the assertion.
(NB: DeepIterator is in the dw::core namespace.)
I think both DeepIterator::searchDownward() and DeepIterator::searchSideward() are not supposed to modify their argument it.
So without really understanding what's going on, I would say it's a typo and should read:
diff -r 8edae7f02bd7 dw/iterator.cc --- a/dw/iterator.cc Thu Dec 06 13:30:01 2007 +0100 +++ b/dw/iterator.cc Thu Dec 06 15:23:20 2007 +0100 @@ -343,7 +343,7 @@ Iterator *DeepIterator::searchDownward ( return NULL; }
- while (fromEnd ? it->prev () : it->next ()) { + while (fromEnd ? it2->prev () : it2->next ()) { //DEBUG_MSG (1, "%*sexamining %s\n", // indent, "", a_Dw_iterator_text (it2));
@@ -384,7 +384,7 @@ Iterator *DeepIterator::searchSideward ( misc::assert (it->getContent()->type == Content::WIDGET, "no widget"); it2 = it->cloneIterator ();
- while (fromEnd ? it->prev () : it->next ()) { + while (fromEnd ? it2->prev () : it2->next ()) { if (it2->getContent()->type == Content::WIDGET) { // Search downwards in this widget. it3 = searchDownward (it2, mask, fromEnd);
Unfortunately it now crashes somewhere else related to text selection...
Regards, Johannes
I'm trying to fix this but so far I'm just generating more crashes. What exactly should DeepIterator::DeepIterator do if searchDownward fails and changes the content of the iterator? If I know that I'll have another go.
Regards,
Jeremy Henty
_______________________________________________ Dillo-dev mailing list Dillo-dev@dillo.org http://lists.auriga.wearlab.de/cgi-bin/mailman/listinfo/dillo-dev
_______________________________________________ Dillo-dev mailing list Dillo-dev@dillo.org http://lists.auriga.wearlab.de/cgi-bin/mailman/listinfo/dillo-dev
On Thu, Dec 06, 2007 at 05:19:34PM +0100, Johannes Hofmann wrote:
[snip] fixes the crashes for me. Please test.
Possibly only a partial fix, I'm afraid. The crash on the page I sent you has gone, but I have a test file containing just an image, and when I click to the right of that image dillo segfaults. gdb says: Program terminated with signal 11, Segmentation fault. #0 0x080989c3 in dw::core::SelectionState::correctCharPos (it=0x810dfe8, charPos=1073741824) at selection.cc:362 362 if (top->getContent()->type == Content::TEXT) (gdb) print top $1 = (class dw::core::Iterator *) 0x19 (gdb) print top->getContent() Cannot access memory at address 0x19 (gdb) print it $2 = (class dw::core::DeepIterator *) 0x810dfe8 That 0x19 pointer looks like memory corruption, and valgrind confirms that dillo is accessing freed memory. I'll dig into its output and see what sense I can make. Of course this might be a completely different bug from the one your patch fixes. Perhaps you could assume that for the moment and commit anyway? Regards, Jeremy Henty
On Thu, Dec 06, 2007 at 06:12:47PM +0000, Jeremy Henty wrote:
On Thu, Dec 06, 2007 at 05:19:34PM +0100, Johannes Hofmann wrote:
[snip] fixes the crashes for me. Please test.
Possibly only a partial fix, I'm afraid. The crash on the page I sent you has gone, but I have a test file containing just an image, and when I click to the right of that image dillo segfaults. gdb says:
Program terminated with signal 11, Segmentation fault. #0 0x080989c3 in dw::core::SelectionState::correctCharPos (it=0x810dfe8, charPos=1073741824) at selection.cc:362 362 if (top->getContent()->type == Content::TEXT) (gdb) print top $1 = (class dw::core::Iterator *) 0x19 (gdb) print top->getContent() Cannot access memory at address 0x19 (gdb) print it $2 = (class dw::core::DeepIterator *) 0x810dfe8
That 0x19 pointer looks like memory corruption, and valgrind confirms that dillo is accessing freed memory. I'll dig into its output and see what sense I can make.
I can reproduce this here. But I won't have much time the next days. Regards, Johannes
Of course this might be a completely different bug from the one your patch fixes. Perhaps you could assume that for the moment and commit anyway?
Regards,
Jeremy Henty
_______________________________________________ Dillo-dev mailing list Dillo-dev@dillo.org http://lists.auriga.wearlab.de/cgi-bin/mailman/listinfo/dillo-dev
On Thu, Dec 06, 2007 at 06:12:47PM +0000, Jeremy Henty wrote:
On Thu, Dec 06, 2007 at 05:19:34PM +0100, Johannes Hofmann wrote:
[snip] fixes the crashes for me. Please test.
Possibly only a partial fix, I'm afraid. The crash on the page I sent you has gone, but I have a test file containing just an image, and when I click to the right of that image dillo segfaults. gdb says:
Program terminated with signal 11, Segmentation fault. #0 0x080989c3 in dw::core::SelectionState::correctCharPos (it=0x810dfe8, charPos=1073741824) at selection.cc:362 362 if (top->getContent()->type == Content::TEXT) (gdb) print top $1 = (class dw::core::Iterator *) 0x19 (gdb) print top->getContent() Cannot access memory at address 0x19 (gdb) print it $2 = (class dw::core::DeepIterator *) 0x810dfe8
That 0x19 pointer looks like memory corruption, and valgrind confirms that dillo is accessing freed memory. I'll dig into its output and see what sense I can make.
The stack seems to be empty. With the following patch I get at least consistently: (gdb) p top $1 = (class dw::core::Iterator *) 0x0 diff -r 7817ef0b4b13 lout/container.hh --- a/lout/container.hh Fri Dec 07 08:33:56 2007 +0100 +++ b/lout/container.hh Fri Dec 07 08:38:50 2007 +0100 @@ -116,7 +116,7 @@ public: void insert(object::Object *newElement, int pos); void remove(int pos); inline object::Object *get(int pos) - { return pos < numElements ? array[pos] : NULL; } + { return (pos >= 0 && pos < numElements) ? array[pos] : NULL; } inline int size() { return numElements; } void clear(); void sort(); Not sure however whether the stack is supposed to be empty here... Regards, Johannes PS: Compiling the whole thing with -fno-inline makes debugging much easier.
Of course this might be a completely different bug from the one your patch fixes. Perhaps you could assume that for the moment and commit anyway?
Regards,
Jeremy Henty
_______________________________________________ Dillo-dev mailing list Dillo-dev@dillo.org http://lists.auriga.wearlab.de/cgi-bin/mailman/listinfo/dillo-dev
On Fri, Dec 07, 2007 at 08:45:00AM +0100, Johannes Hofmann wrote:
The stack seems to be empty. With the following patch I get at least consistently:
(gdb) p top $1 = (class dw::core::Iterator *) 0x0 [snip]
OK, thanks for that. I'll try getting my head round iterators today.
PS: Compiling the whole thing with -fno-inline makes debugging much easier.
I'm already using -g -O0 as the valgrind docs recommend. Doesn't -O0 imply -fno-inline? The gcc info describes -O0 as "Do not optimize." and says under -fno-inline "Note that if you are not optimizing, no functions can be expanded inline." which suggests that -O0 makes -fno-inline redundant. I certainly noticed functions disappearing from the stack trace before I selected -O0. Regards, Jeremy Henty
On Fri, Dec 07, 2007 at 08:45:00AM +0100, Johannes Hofmann wrote:
The stack seems to be empty. With the following patch I get at least consistently: [...] Not sure however whether the stack is supposed to be empty here...
Eyeballing the code it *seems* to me that: * The constructor will create a DeepIterator with an empty stack if there is no non-widget content in the page. In that case it sets hasContents = false . * In all other cases the stack will always remain non-empty, and the top iterator will always point to the same widget it was created with. * Your fix for the DeepIterator search methods is the correct fix for the original problem. * The correct fix for *this* problem is for SelectionState objects to check any DeepIterator objects they create and reset the link/selection if the new DeepIterator object is empty. This is because the methods that adjust selections assume that the from and to members are either NULL or non-empty. Let me work on a patch for this. Comments? Jeremy Henty
On Fri, Dec 07, 2007 at 06:57:26PM +0000, Jeremy Henty (ie. me) wrote:
* [Johannes Hofmann's] fix for the DeepIterator search methods is the correct fix for the original problem.
* The correct fix for *this* problem is for SelectionState objects to check any DeepIterator objects they create and reset the link/selection if the new DeepIterator object is empty.
OK, I think I have this working. Please test! Johannes' patch for DeepIterator::{searchDownward,searchSideward}() is required. His patch for lout/container.hh is not (I think) required, but I feel it should go in anyway; it would have been nice to have been faced with an obvious NULL rather than memory corruption. Here's the patch to SelectionState::buttonPress() (which also adds DeepIterator::isEmpty() for convenience). Comments are welcome! Jeremy Henty Only in dw2-cur: .in Only in dw2-cur: Makefile Only in dw2-cur: Makefile.in Only in dw2-cur: aclocal.m4 Only in dw2-cur: autom4te.cache Only in dw2-cur: config.guess Only in dw2-cur: config.log Only in dw2-cur: config.status Only in dw2-cur: config.sub Only in dw2-cur: configure Only in dw2-cur: depcomp Only in dw2-cur/doc: Makefile Only in dw2-cur/doc: Makefile.in Only in dw2-cur/dw: .deps Only in dw2-cur/dw: Makefile Only in dw2-cur/dw: Makefile.in Only in dw2-cur/dw: findtext.o diff -pru -- dw2-ref/dw/iterator.cc dw2-cur/dw/iterator.cc --- dw2-ref/dw/iterator.cc 2007-12-07 19:07:09.000000000 +0000 +++ dw2-cur/dw/iterator.cc 2007-12-07 19:07:09.000000000 +0000 @@ -591,6 +591,10 @@ DeepIterator *DeepIterator::createVarian return new DeepIterator (it); } +bool DeepIterator::isEmpty () { + return ! hasContents; +} + /** * \brief Move iterator forward and store content it. * diff -pru -- dw2-ref/dw/iterator.hh dw2-cur/dw/iterator.hh --- dw2-ref/dw/iterator.hh 2007-10-06 23:03:01.000000000 +0100 +++ dw2-cur/dw/iterator.hh 2007-12-07 19:07:09.000000000 +0000 @@ -171,6 +171,8 @@ public: inline Iterator *getTopIterator () { return stack.getTop(); } inline Content *getContent () { return &content; } + bool isEmpty (); + bool next (); bool prev (); inline DeepIterator *cloneDeepIterator() { return (DeepIterator*)clone(); } Only in dw2-cur/dw: iterator.o Only in dw2-cur/dw: layout.o diff -pru -- dw2-ref/dw/selection.cc dw2-cur/dw/selection.cc --- dw2-ref/dw/selection.cc 2007-11-19 15:58:52.000000000 +0000 +++ dw2-cur/dw/selection.cc 2007-12-07 19:08:36.000000000 +0000 @@ -122,15 +122,20 @@ bool SelectionState::buttonPress (Iterat resetLink (); linkState = LINK_PRESSED; linkButton = event->button; - link = new DeepIterator (it); - - // It may be that the user has pressed on something activatable - // (linkNo != -1), but there is no contents, e.g. with images - // without ALTernative text. - if (link) { - linkChar = correctCharPos (link, charPos); - linkNumber = linkNo; - } + DeepIterator *newLink = new DeepIterator (it); + if (newLink->isEmpty ()) { + delete newLink; + resetLink (); + } else { + link = newLink; + // It may be that the user has pressed on something activatable + // (linkNo != -1), but there is no contents, e.g. with images + // without ALTernative text. + if (link) { + linkChar = correctCharPos (link, charPos); + linkNumber = linkNo; + } + } // We do not return the value of the signal method, // but we do actually process this event. ret = true; @@ -142,10 +147,16 @@ bool SelectionState::buttonPress (Iterat resetSelection (); selectionState = SELECTING; - from = new DeepIterator (it); - fromChar = correctCharPos (from, charPos); - to = from->cloneDeepIterator (); - toChar = correctCharPos (to, charPos); + DeepIterator *newFrom = new DeepIterator (it); + if (newFrom->isEmpty ()) { + delete newFrom; + resetSelection (); + } else { + from = newFrom; + fromChar = correctCharPos (from, charPos); + to = from->cloneDeepIterator (); + toChar = correctCharPos (to, charPos); + } ret = true; } else { if (event && event->button == 3) {
On Fri, Dec 07, 2007 at 09:02:39PM +0000, Jeremy Henty wrote:
On Fri, Dec 07, 2007 at 06:57:26PM +0000, Jeremy Henty (ie. me) wrote:
* [Johannes Hofmann's] fix for the DeepIterator search methods is the correct fix for the original problem.
* The correct fix for *this* problem is for SelectionState objects to check any DeepIterator objects they create and reset the link/selection if the new DeepIterator object is empty.
OK, I think I have this working. Please test! Johannes' patch for DeepIterator::{searchDownward,searchSideward}() is required. His patch for lout/container.hh is not (I think) required, but I feel it should go in anyway; it would have been nice to have been faced with an obvious NULL rather than memory corruption. Here's the patch to SelectionState::buttonPress() (which also adds DeepIterator::isEmpty() for convenience).
Comments are welcome!
OK, then I'll wait for Johannes comments on this before a commit. You seem to have investigated it to a good depth. If you finally make it into the bottom and are confident enough of how it is done, just let us know to commit. BTW, the last table rendering patch, fixes a problem with horizontal rulers (unsetting the HAS_CONTENTS flags). This may have lead an iterator to traverse uninitialized memory. -- Cheers Jorge.-
Hi Jeremy, On Fri, Dec 07, 2007 at 09:02:39PM +0000, Jeremy Henty wrote:
On Fri, Dec 07, 2007 at 06:57:26PM +0000, Jeremy Henty (ie. me) wrote:
* [Johannes Hofmann's] fix for the DeepIterator search methods is the correct fix for the original problem.
* The correct fix for *this* problem is for SelectionState objects to check any DeepIterator objects they create and reset the link/selection if the new DeepIterator object is empty.
OK, I think I have this working. Please test! Johannes' patch for DeepIterator::{searchDownward,searchSideward}() is required. His patch for lout/container.hh is not (I think) required, but I feel it should go in anyway; it would have been nice to have been faced with an obvious NULL rather than memory corruption. Here's the patch to SelectionState::buttonPress() (which also adds DeepIterator::isEmpty() for convenience).
Comments are welcome!
Yes great. Your patch fixes the problem here! Just one thing. I would rather have tested the return value of getTopIterator() for NULL. I think the code is generally a bit optimistic here by assuming non-NULL return value from getTopIterator(). Even if this is now always the case with your fix, it may change again in the future. What do you think, if we add checks for NULL at those three points in the code where getTopIterator() is called, do we still need to check with isEmpty() to call resetLink() / resetSelection() in buttonPress()? Regards, Johannes
Jeremy Henty
Only in dw2-cur: .in Only in dw2-cur: Makefile Only in dw2-cur: Makefile.in Only in dw2-cur: aclocal.m4 Only in dw2-cur: autom4te.cache Only in dw2-cur: config.guess Only in dw2-cur: config.log Only in dw2-cur: config.status Only in dw2-cur: config.sub Only in dw2-cur: configure Only in dw2-cur: depcomp Only in dw2-cur/doc: Makefile Only in dw2-cur/doc: Makefile.in Only in dw2-cur/dw: .deps Only in dw2-cur/dw: Makefile Only in dw2-cur/dw: Makefile.in Only in dw2-cur/dw: findtext.o diff -pru -- dw2-ref/dw/iterator.cc dw2-cur/dw/iterator.cc --- dw2-ref/dw/iterator.cc 2007-12-07 19:07:09.000000000 +0000 +++ dw2-cur/dw/iterator.cc 2007-12-07 19:07:09.000000000 +0000 @@ -591,6 +591,10 @@ DeepIterator *DeepIterator::createVarian return new DeepIterator (it); }
+bool DeepIterator::isEmpty () { + return ! hasContents; +} + /** * \brief Move iterator forward and store content it. * diff -pru -- dw2-ref/dw/iterator.hh dw2-cur/dw/iterator.hh --- dw2-ref/dw/iterator.hh 2007-10-06 23:03:01.000000000 +0100 +++ dw2-cur/dw/iterator.hh 2007-12-07 19:07:09.000000000 +0000 @@ -171,6 +171,8 @@ public: inline Iterator *getTopIterator () { return stack.getTop(); } inline Content *getContent () { return &content; }
+ bool isEmpty (); + bool next (); bool prev (); inline DeepIterator *cloneDeepIterator() { return (DeepIterator*)clone(); } Only in dw2-cur/dw: iterator.o Only in dw2-cur/dw: layout.o diff -pru -- dw2-ref/dw/selection.cc dw2-cur/dw/selection.cc --- dw2-ref/dw/selection.cc 2007-11-19 15:58:52.000000000 +0000 +++ dw2-cur/dw/selection.cc 2007-12-07 19:08:36.000000000 +0000 @@ -122,15 +122,20 @@ bool SelectionState::buttonPress (Iterat resetLink (); linkState = LINK_PRESSED; linkButton = event->button; - link = new DeepIterator (it); - - // It may be that the user has pressed on something activatable - // (linkNo != -1), but there is no contents, e.g. with images - // without ALTernative text. - if (link) { - linkChar = correctCharPos (link, charPos); - linkNumber = linkNo; - } + DeepIterator *newLink = new DeepIterator (it); + if (newLink->isEmpty ()) { + delete newLink; + resetLink (); + } else { + link = newLink; + // It may be that the user has pressed on something activatable + // (linkNo != -1), but there is no contents, e.g. with images + // without ALTernative text. + if (link) { + linkChar = correctCharPos (link, charPos); + linkNumber = linkNo; + } + } // We do not return the value of the signal method, // but we do actually process this event. ret = true; @@ -142,10 +147,16 @@ bool SelectionState::buttonPress (Iterat resetSelection ();
selectionState = SELECTING; - from = new DeepIterator (it); - fromChar = correctCharPos (from, charPos); - to = from->cloneDeepIterator (); - toChar = correctCharPos (to, charPos); + DeepIterator *newFrom = new DeepIterator (it); + if (newFrom->isEmpty ()) { + delete newFrom; + resetSelection (); + } else { + from = newFrom; + fromChar = correctCharPos (from, charPos); + to = from->cloneDeepIterator (); + toChar = correctCharPos (to, charPos); + } ret = true; } else { if (event && event->button == 3) {
_______________________________________________ Dillo-dev mailing list Dillo-dev@dillo.org http://lists.auriga.wearlab.de/cgi-bin/mailman/listinfo/dillo-dev
On Mon, Dec 10, 2007 at 05:44:08PM +0100, Johannes Hofmann wrote:
Just one thing. I would rather have tested the return value of getTopIterator() for NULL.
Heh, that *was* my original implementation of isEmpty() . Then I noticed the hasContents member so I used that since it shortened the code. I believe hasContents could be eliminated since it is true when and only when the stack is non-empty, though I haven't exhaustively checked this.
I think the code is generally a bit optimistic here by assuming non-NULL return value from getTopIterator(). Even if this is now always the case with your fix, it may change again in the future.
Yes it may. But I think much of the code assumes this, so if this changes in the future then a lot of code will need to be reviewed, preferably by someone who understands the design.
What do you think, if we add checks for NULL at those three points in the code where getTopIterator() is called, do we still need to check with isEmpty() to call resetLink() / resetSelection() in buttonPress()?
Yes, otherwise you get a crash in DeepIterator::compareTo, which also assumes that the stack is non-empty. (In fact the comments suggest that it assumes the tops of the stacks of the two DeepIterator objects point to the same widget.) When I found that crash I decided to give up on rewriting lots of code to handle empty DeepIterators and instead simply prevent empty DeepIterators appearing in Selection objects. Even if other code ought to handle empty DeepIterators better, I think it's reasonable to argue that Selection objects should never use them. After all, what could it possible mean for a selection to begin or end at an empty DeepIterator? So I think my bugfix makes Selections do the right thing, even if other code should be checking getTopIterator() as you suggest. Regards, Jeremy Henty
On Tue, Dec 11, 2007 at 12:06:37PM +0000, Jeremy Henty wrote:
On Mon, Dec 10, 2007 at 05:44:08PM +0100, Johannes Hofmann wrote:
Just one thing. I would rather have tested the return value of getTopIterator() for NULL.
Heh, that *was* my original implementation of isEmpty() . Then I noticed the hasContents member so I used that since it shortened the code. I believe hasContents could be eliminated since it is true when and only when the stack is non-empty, though I haven't exhaustively checked this.
I think the code is generally a bit optimistic here by assuming non-NULL return value from getTopIterator(). Even if this is now always the case with your fix, it may change again in the future.
Yes it may. But I think much of the code assumes this, so if this changes in the future then a lot of code will need to be reviewed, preferably by someone who understands the design.
What do you think, if we add checks for NULL at those three points in the code where getTopIterator() is called, do we still need to check with isEmpty() to call resetLink() / resetSelection() in buttonPress()?
Yes, otherwise you get a crash in DeepIterator::compareTo, which also assumes that the stack is non-empty. (In fact the comments suggest that it assumes the tops of the stacks of the two DeepIterator objects point to the same widget.) When I found that crash I decided to give up on rewriting lots of code to handle empty DeepIterators and instead simply prevent empty DeepIterators appearing in Selection objects.
Even if other code ought to handle empty DeepIterators better, I think it's reasonable to argue that Selection objects should never use them. After all, what could it possible mean for a selection to begin or end at an empty DeepIterator? So I think my bugfix makes Selections do the right thing, even if other code should be checking getTopIterator() as you suggest.
Yes, I fully agree with you. Selection is fixed. We may need to come back to DeepIterator later. Regards, Johannes
Regards,
Jeremy Henty
_______________________________________________ Dillo-dev mailing list Dillo-dev@dillo.org http://lists.auriga.wearlab.de/cgi-bin/mailman/listinfo/dillo-dev
On Thu, Dec 06, 2007 at 06:12:47PM +0000, Jeremy Henty wrote:
On Thu, Dec 06, 2007 at 05:19:34PM +0100, Johannes Hofmann wrote:
[snip] fixes the crashes for me. Please test.
Possibly only a partial fix, I'm afraid. The crash on the page I sent you has gone, but I have a test file containing just an image, and when I click to the right of that image dillo segfaults. gdb says:
Program terminated with signal 11, Segmentation fault. #0 0x080989c3 in dw::core::SelectionState::correctCharPos (it=0x810dfe8, charPos=1073741824) at selection.cc:362 362 if (top->getContent()->type == Content::TEXT) (gdb) print top $1 = (class dw::core::Iterator *) 0x19 (gdb) print top->getContent() Cannot access memory at address 0x19 (gdb) print it $2 = (class dw::core::DeepIterator *) 0x810dfe8
That 0x19 pointer looks like memory corruption, and valgrind confirms that dillo is accessing freed memory. I'll dig into its output and see what sense I can make.
Of course this might be a completely different bug from the one your patch fixes. Perhaps you could assume that for the moment and commit anyway?
Well, at least that patch fixes the problem with this test case: <html><body> <table width="100%" border="1"> <tr> <td><img src="continued_a.gif" alt="CONTINUED BELOW" height="22" width="59" border="0"></td> </table> </body></html> (clicking to the right of text, inside the table, made dillo exit). and as it really looks like a couple of typos, it was committed! :-) The other patch fixes this test case: <html><body> <img src="continued_a.gif" alt="CONTINUED BELOW" height="22" width="59" border="0"> </body></html> I also don't know whether this is the correct fix for the design, but as it fixes a nasty bug, I preferred to commit. -- Cheers Jorge.-
participants (3)
-
jcid＠dillo.org -
Johannes.Hofmann＠gmx.de -
onepoint＠starurchin.org