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

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

Dec. 6, 2007

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

by onepoint＠starurchin.org

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

Dec. 6, 2007

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

by Johannes.Hofmann＠gmx.de

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

Dec. 6, 2007

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

by Johannes.Hofmann＠gmx.de

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

Dec. 6, 2007

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

by jcid＠dillo.org

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

Dec. 6, 2007

BUG: CCC corrupts memory when the Content-Type is wrong

by jcid＠dillo.org

On Wed, Dec 05, 2007 at 06:18:22PM +0000, Jeremy Henty wrote: > On Wed, Dec 05, 2007 at 12:34:18PM -0300, Jorge Arellano Cid wrote: > > > The problem is reentrancy. > > Aha! I did some testing and noticed that calls to a_IO_ccc were > sometimes nested. > > > This bug was produced because a CCC operation was reentering the > > same chain after it was aborted. I added an extra check as a > > workaround to avoid this situation and a comment that a more generic > > solution should be designed. > > Hmm, perhaps the CCC Chainlink structures could have an aborted flag > and the chaining functions could call the callbacks only if the link > was not aborted? Yes. I gave that idea a quick try and realized I had to decide better whether to allow some reentrancy, why/when and how, and also to check if that was already happening... > Also, I noticed that a_Chain_unlink is never used. Is that > intentional? It's just an API idea. -- Cheers Jorge.-

Dec. 6, 2007

[patch] fix crashes after panel size changes

by jcid＠dillo.org

On Thu, Dec 06, 2007 at 11:05:43AM +0100, Johannes Hofmann wrote: > Hello, > > now that was a weird bug! > I was experiencing crashes when I had changed the panel size via the > right mouse on the search button. > The problem is that FltkWidgetView::prepareCreateFltkWidget() was > calling fltk::Group::begin(). > Therefore after visiting pages with forms, the newly created panel > was getting a child of Main instead of TopGroup! > > Btw. did I mention that this fltk begin()/end() stuff is evil? Great you commented this here. Side effects are sometimes very hard to detect and fix. Probably the begin/end pair is less side-effect prone when the GUI is static and built once and in a single function. For dynamic content (as HTML rendering, resizable panels, etc.) I'd also prefer explicit add() calls. > One could expect that Group::replace() should disconnect the > new widget properly from its old parent (if there is any). But > that's not the case. I wrote a mail to fltk.general about this. Yes, I'd also expect a reparent. If that's not the case, it should be mentioned in FLTK's docs. Which AFAIS is not the case. <quote> void fltk::Group::replace(int index, fltk::Widget&) Remove the indexed widget and insert the passed widget in it's place. void fltk::Group::replace(fltk::Widget& old, fltk::Widget&) Find the old widget and remove it and insert the new one. If the widget is not a child, the new widget is appended to the end of the list. </quote> > > So in the end Main still had a reference to the panel and once it > was deleted -- bang. > > The patch below uses Group::add() directly in > FltkWidgetView::addFltkWidget() instead of > FltkWidgetView::prepareCreateFltkWidget(). Committed. > Does anyone remember why FltkWidgetView::prepareCreateFltkWidget() > was added in the first place? I don't know, but while developing dillo's UI I added a similar function to group different combinations of calls until finding one that worked as I wanted. At that time the API wasn't as stable as now, so maybe Sebastian did the same. -- Cheers Jorge.-

Dec. 6, 2007

[patch] fix crashes after panel size changes

by Johannes.Hofmann＠gmx.de

Hello, now that was a weird bug! I was experiencing crashes when I had changed the panel size via the right mouse on the search button. The problem is that FltkWidgetView::prepareCreateFltkWidget() was calling fltk::Group::begin(). Therefore after visiting pages with forms, the newly created panel was getting a child of Main instead of TopGroup! Btw. did I mention that this fltk begin()/end() stuff is evil? One could expect that Group::replace() should disconnect the new widget properly from its old parent (if there is any). But that's not the case. I wrote a mail to fltk.general about this. So in the end Main still had a reference to the panel and once it was deleted -- bang. The patch below uses Group::add() directly in FltkWidgetView::addFltkWidget() instead of FltkWidgetView::prepareCreateFltkWidget(). Does anyone remember why FltkWidgetView::prepareCreateFltkWidget() was added in the first place? Cheers, Johannes diff --git a/dw/fltkplatform.cc b/dw/fltkplatform.cc --- a/dw/fltkplatform.cc +++ b/dw/fltkplatform.cc @@ -83,10 +83,6 @@ FltkColor::FltkColor (int color, core::s colors[SHADING_DARK] = shadeColor (color, SHADING_DARK) << 8; colors[SHADING_LIGHT] = shadeColor (color, SHADING_LIGHT) << 8; } -} - -void FltkView::prepareCreateFltkWidget () -{ } void FltkView::addFltkWidget (::fltk::Widget *widget, diff --git a/dw/fltkplatform.hh b/dw/fltkplatform.hh --- a/dw/fltkplatform.hh +++ b/dw/fltkplatform.hh @@ -40,7 +40,6 @@ public: public: virtual bool usesFltkWidgets () = 0; - virtual void prepareCreateFltkWidget (); virtual void addFltkWidget (::fltk::Widget *widget, core::Allocation *allocation); virtual void removeFltkWidget (::fltk::Widget *widget); diff --git a/dw/fltkui.cc b/dw/fltkui.cc --- a/dw/fltkui.cc +++ b/dw/fltkui.cc @@ -103,7 +103,6 @@ void FltkResource::attachView (FltkView ViewAndWidget *viewAndWidget = new ViewAndWidget(); viewAndWidget->view = view; - view->prepareCreateFltkWidget (); viewAndWidget->widget = createNewWidget (&allocation); viewAndWidget->view->addFltkWidget (viewAndWidget->widget, &allocation); viewsAndWidgets->append (viewAndWidget); diff --git a/dw/fltkviewbase.cc b/dw/fltkviewbase.cc --- a/dw/fltkviewbase.cc +++ b/dw/fltkviewbase.cc @@ -375,17 +375,12 @@ bool FltkWidgetView::usesFltkWidgets () return true; } - -void FltkWidgetView::prepareCreateFltkWidget () -{ - begin (); -} - void FltkWidgetView::addFltkWidget (::fltk::Widget *widget, core::Allocation *allocation) { canvasWidgets->append (new TypedPointer < ::fltk::Widget> (widget)); allocateFltkWidget (widget, allocation); + add (widget); } void FltkWidgetView::removeFltkWidget (::fltk::Widget *widget) diff --git a/dw/fltkviewbase.hh b/dw/fltkviewbase.hh --- a/dw/fltkviewbase.hh +++ b/dw/fltkviewbase.hh @@ -82,7 +82,6 @@ public: int x, int y, int width, int height); bool usesFltkWidgets (); - void prepareCreateFltkWidget (); void addFltkWidget (::fltk::Widget *widget, core::Allocation *allocation); void removeFltkWidget (::fltk::Widget *widget); void allocateFltkWidget (::fltk::Widget *widget,

Dec. 6, 2007

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

by onepoint＠starurchin.org

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

Dec. 6, 2007

BUG: CCC corrupts memory when the Content-Type is wrong

by onepoint＠starurchin.org

On Wed, Dec 05, 2007 at 12:34:18PM -0300, Jorge Arellano Cid wrote: > The problem is reentrancy. Aha! I did some testing and noticed that calls to a_IO_ccc were sometimes nested. > This bug was produced because a CCC operation was reentering the > same chain after it was aborted. I added an extra check as a > workaround to avoid this situation and a comment that a more generic > solution should be designed. Hmm, perhaps the CCC Chainlink structures could have an aborted flag and the chaining functions could call the callbacks only if the link was not aborted? Also, I noticed that a_Chain_unlink is never used. Is that intentional? > Patch is in CVS. Problem solved. Yay! Regards, Jeremy Henty

Dec. 5, 2007