December 2007 - Dillo-dev - lists.mailman3.com

BUG: assertion failed: "no widget": in dw::core::DeepIterator::searchSideward

by Johannes.Hofmann＠gmx.de

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(a)dillo.org > http://lists.auriga.wearlab.de/cgi-bin/mailman/listinfo/dillo-dev

Dec. 10, 2007

Re: Colspan and table percentages

by Johannes.Hofmann＠gmx.de

On Sun, Dec 09, 2007 at 05:58:04PM -0300, Jorge Arellano Cid wrote: > On Sun, Dec 09, 2007 at 02:07:48PM -0300, Jorge Arellano Cid wrote: > > Hi! > > > > After a lot of work, the patch for better table rendering > > (regarding colspan and percentages) was good enough to be > > committed. Please enjoy and send feedback (or patches) ;-). > > > > - Implemented a weighted apportionment algorithm for table colspan. > > - Implemented percentage widths in tables (better rendering!). > > - Fixed an initialization bug in hruler. > > + Minor fix to table-wide percentage apportion. > > Now about:splash renders OK. Excellent, dpi:/bm/ renders fine too! Cheers, Johannes > > -- > Cheers > Jorge.- > > _______________________________________________ > Dillo-dev mailing list > Dillo-dev(a)dillo.org > http://lists.auriga.wearlab.de/cgi-bin/mailman/listinfo/dillo-dev

Dec. 10, 2007

BUG: assertion failed: "no widget": in dw::core::DeepIterator::searchSideward

by jcid＠dillo.org

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.-

Dec. 10, 2007

Re: Colspan and table percentages

by jcid＠dillo.org

On Sun, Dec 09, 2007 at 02:07:48PM -0300, Jorge Arellano Cid wrote: > Hi! > > After a lot of work, the patch for better table rendering > (regarding colspan and percentages) was good enough to be > committed. Please enjoy and send feedback (or patches) ;-). > > - Implemented a weighted apportionment algorithm for table colspan. > - Implemented percentage widths in tables (better rendering!). > - Fixed an initialization bug in hruler. + Minor fix to table-wide percentage apportion. Now about:splash renders OK. -- Cheers Jorge.-

Dec. 9, 2007

BUG: assertion failed: "no widget": in dw::core::DeepIterator::searchSideward

by jcid＠dillo.org

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.-

Dec. 9, 2007

Colspan and table percentages

by jcid＠dillo.org

Hi! After a lot of work, the patch for better table rendering (regarding colspan and percentages) was good enough to be committed. Please enjoy and send feedback (or patches) ;-). - Implemented a weighted apportionment algorithm for table colspan. - Implemented percentage widths in tables (better rendering!). - Fixed an initialization bug in hruler. If you: #define DBG In table.cc. Verbose debugging output is enabled. I tried to make special and ill cases decision-making similar to firefox, but frankly, some cases are really weird! For instance, take a look here and compare: http://www.dillo.org/test/cs1.html http://www.dillo.org/test/cs7.html // e.g. look at th 20% table Well, the main point is for you to test and to report how it works! -- Cheers Jorge.-

Dec. 9, 2007

BUG: assertion failed: "no widget": in dw::core::DeepIterator::searchSideward

by onepoint＠starurchin.org

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) {

Dec. 7, 2007

BUG: assertion failed: "no widget": in dw::core::DeepIterator::searchSideward

by onepoint＠starurchin.org

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

Dec. 7, 2007

BUG: assertion failed: "no widget": in dw::core::DeepIterator::searchSideward

by onepoint＠starurchin.org

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

Dec. 7, 2007

BUG: assertion failed: "no widget": in dw::core::DeepIterator::searchSideward

by Johannes.Hofmann＠gmx.de

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(a)dillo.org > http://lists.auriga.wearlab.de/cgi-bin/mailman/listinfo/dillo-dev

Dec. 7, 2007