- Dillo-dev - lists.mailman3.com

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

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

by jcid＠dillo.org

Hi Jeremy, On Tue, Dec 04, 2007 at 02:03:07PM +0000, Jeremy Henty wrote: > > I am hoping that someone who understands Dillo's CCC better than I can > point me in the right direction and save me some time debugging this. Well, ehmmmm, I know CCC, but this was a hard-to-find bug! > Dillo almost always crashes if the HTML contains the stumbleupon.com > favicon link, ie.'<img src="http://www.stumbleupon.com/favicon.ico">'. > (This is very annoying as it makes Dillo unusable for one of my > favourite blogs.) If it doesn't crash right away then clicking Reload > crashes it. > > Examining core dumps with gdb suggests that memory is being corrupted, > and running dillo under valgrind shows that the CCC functions are > reading, writing and freeing already freed memory. (Details > attached.) > > A local HTTP server triggers the bug when it server returns a > Content-Type of "text/plain; charset=iso-8859-1". If I change it to > "image/x-icon" there is no corruption. Accessing the image directly > via a "file:" URL does not trigger the bug. (Strangely, using a local > HTTP server does not hit the bug until I open a second window on the > page, although sometimes it crashes first time.) > > I'm attaching a summary of the invalid memory accesses that valgrind > reported, and the entire valgrind log (which includes stack traces). > > Any suggestions for where I should start instrumenting the CCC to find > out why it is prematurely freeing pointers? The problem is reentrancy. The CCC handles parallelism and serves to propagate operations in a managed environment. 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. Patch is in CVS. Please test and comment. Hopefully it will work. ;-) -- Cheers Jorge.-

Dec. 5, 2007

[patch] initialize mustQueueResize in Textblock constructor

by jcid＠dillo.org

On Wed, Dec 05, 2007 at 10:59:24AM +0100, Johannes Hofmann wrote: > Hello, > > this patch initializes mustQueueResize in the Textblock constructor. > Valgrind was complaining about a jump depending on uninitialized > memory. Committed. -- Cheers Jorge.-

Dec. 5, 2007

[patch] add missing style->ref ()

by jcid＠dillo.org

On Wed, Dec 05, 2007 at 10:57:04AM +0100, Johannes Hofmann wrote: > Hello, > > the tiny patch below adds a missing style->ref() which was causing > a lot of crashes on my system. Committed. -- Cheers Jorge.-

Dec. 5, 2007

[patch] initialize mustQueueResize in Textblock constructor

by Johannes.Hofmann＠gmx.de

Hello, this patch initializes mustQueueResize in the Textblock constructor. Valgrind was complaining about a jump depending on uninitialized memory. Cheers, Johannes diff --git a/dw/textblock.cc b/dw/textblock.cc --- a/dw/textblock.cc +++ b/dw/textblock.cc @@ -39,6 +39,7 @@ Textblock::Textblock (bool limitTextWidt line1Offset = 0; line1OffsetEff = 0; ignoreLine1OffsetSometimes = false; + mustQueueResize = false; /* * The initial sizes of lines and words should not be

Dec. 5, 2007

[patch] add missing style->ref ()

by Johannes.Hofmann＠gmx.de

Hello, the tiny patch below adds a missing style->ref() which was causing a lot of crashes on my system. cheers, Johannes diff --git a/dw/fltkui.cc b/dw/fltkui.cc --- a/dw/fltkui.cc +++ b/dw/fltkui.cc @@ -159,6 +159,7 @@ void FltkResource::setStyle (core::style this->style->unref (); this->style = style; + style->ref (); for (Iterator <ViewAndWidget> it = viewsAndWidgets->iterator (); it.hasNext(); ) {

Dec. 5, 2007