- Dillo-dev - lists.mailman3.com

More Cache memory corruption

by jcid＠dillo.org

On Thu, Sep 11, 2008 at 10:31:37PM +0000, corvid wrote: > Jeremy wrote: > > Now the bad news: this HTML almost invariably crashes Dillo: > > > > <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" > > "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd"> > > <html xmlns="http://www.w3.org/1999/xhtml"> > > <head> > > <meta > > http-equiv="Content-Type" > > content="application/xhtml+xml; charset=iso-8859-1" > > /> > > </head> > > <body> > > </body> > > </html> > > > > The culprit is the <meta...> tag. Dillo always crashes in one of two > > ways: segmentation fault or glibc detecting memory corruption and > > aborting. In each case the backtrace from gdb is the same (verified > > by running Dillo 100 times (it only failed to crash twice)). The > > backtraces show that the Cache_* functions are freeing things they > > should not. Valgrind confirms this. I'm attaching backtraces and a > > valgrind log. > > That's not bad news. Getting to fix a bug is a good thing. > Try the attached patch. > > Meta causes a repush, which eventually finds its way to > Cache_delayed_process_queue_callback(). In Cache_process_queue(), the fact > that the content type is not viewable causes an abort and the cache entry > is removed. This is bad for my subsequent Cache_unref_data(entry). > (The entries currently have reference counting so that I know when I can > get rid of the UTF8-ized data.) > > > If you put some text above the meta tag, you still have some text visible > that you can right-click to pop up the page menu. Trying the menu options > doesn't crash anything, but...hmmm...are there are implications to not > having a cache entry backing the page? Hmm, that needs to be investigated... I'll hold the patch a bit just in case you find something. -- Cheers Jorge.-

Sept. 12, 2008

[PATCH] Missing include directive for OpenBSD?

by jcid＠dillo.org

On Fri, Sep 12, 2008 at 08:39:10AM +0200, Christian Kellermann wrote: > * Jorge Arellano Cid <jcid(a)dillo.org> [080908 17:18]: > > > > Yes, in dillo2's tree src/colors.h has a check that OpenBSD 4.3 > > is happy with. Please test it. > > Ok, so I duplicated this check to the dw2 tree and rebuild. I can > confirm that this works for OpenBSD 4.3. Committed. -- Cheers Jorge.-

Sept. 12, 2008

More Cache memory corruption

by onepoint＠starurchin.org

On Thu, Sep 11, 2008 at 10:31:37PM +0000, corvid wrote: > Try the attached patch. That fixes it. Thanks! I'm afraid I don't have any answers to your questions: I'm not familiar enough with the cache code. Regards, Jeremy Henty

Sept. 12, 2008

[PATCH] Missing include directive for OpenBSD?

by Christian.Kellermann＠nefkom.net

* Jorge Arellano Cid <jcid(a)dillo.org> [080908 17:18]: > > Yes, in dillo2's tree src/colors.h has a check that OpenBSD 4.3 > is happy with. Please test it. Ok, so I duplicated this check to the dw2 tree and rebuild. I can confirm that this works for OpenBSD 4.3. The file now looks like this (older patches removed): $ cvs diff -bup dw/style.hh Index: dw/style.hh =================================================================== RCS file: /sfhome/cvs/dillo/dw2/dw/style.hh,v retrieving revision 1.5 diff -b -u -p -r1.5 style.hh --- dw/style.hh 18 Jun 2008 13:19:24 -0000 1.5 +++ dw/style.hh 12 Sep 2008 06:39:38 -0000 @@ -1,6 +1,14 @@ #ifndef __DW_STYLE_HH__ #define __DW_STYLE_HH__ +#ifdef HAVE_INTTYPES_H +#include <inttypes.h> +#else +#ifdef HAVE_STDINT_H +#include <stdint.h> +#endif +#endif + #ifndef __INCLUDED_FROM_DW_CORE_HH__ # error Do not include this file directly, use "core.hh" instead. #endif Kind regards, Christian -- You may use my gpg key for replies: pub 1024D/47F79788 2005/02/02 Christian Kellermann (C-Keen)

Sept. 12, 2008

More Cache memory corruption

by corvid＠lavabit.com

Jeremy wrote: > Now the bad news: this HTML almost invariably crashes Dillo: > > <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" > "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd"> > <html xmlns="http://www.w3.org/1999/xhtml"> > <head> > <meta > http-equiv="Content-Type" > content="application/xhtml+xml; charset=iso-8859-1" > /> > </head> > <body> > </body> > </html> > > The culprit is the <meta...> tag. Dillo always crashes in one of two > ways: segmentation fault or glibc detecting memory corruption and > aborting. In each case the backtrace from gdb is the same (verified > by running Dillo 100 times (it only failed to crash twice)). The > backtraces show that the Cache_* functions are freeing things they > should not. Valgrind confirms this. I'm attaching backtraces and a > valgrind log. That's not bad news. Getting to fix a bug is a good thing. Try the attached patch. Meta causes a repush, which eventually finds its way to Cache_delayed_process_queue_callback(). In Cache_process_queue(), the fact that the content type is not viewable causes an abort and the cache entry is removed. This is bad for my subsequent Cache_unref_data(entry). (The entries currently have reference counting so that I know when I can get rid of the UTF8-ized data.) If you put some text above the meta tag, you still have some text visible that you can right-click to pop up the page menu. Trying the menu options doesn't crash anything, but...hmmm...are there are implications to not having a cache entry backing the page? (BTW, I have begun to wonder whether UTF-8 text/* decoding and dicache/imgbuf image/* decoding should develop similarities. Haven't thought about it in any detail, though.)

Sept. 12, 2008

More Cache memory corruption

by onepoint＠starurchin.org

Hi everyone! No, I'm not dead. I am sorry I haven't been able to spend much time on Dillo recently. I've been lurking and it's good to see Dillo getting lots of developer love. Now the bad news: this HTML almost invariably crashes Dillo: <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd"> <html xmlns="http://www.w3.org/1999/xhtml"> <head> <meta http-equiv="Content-Type" content="application/xhtml+xml; charset=iso-8859-1" /> </head> <body> </body> </html> The culprit is the <meta...> tag. Dillo always crashes in one of two ways: segmentation fault or glibc detecting memory corruption and aborting. In each case the backtrace from gdb is the same (verified by running Dillo 100 times (it only failed to crash twice)). The backtraces show that the Cache_* functions are freeing things they should not. Valgrind confirms this. I'm attaching backtraces and a valgrind log. I'm not sure how to debug this further. If anyone can suggest ways to trace this further I'll have a go. Looking forward to the dill2 release! This will be a great milestone. Jeremy Henty

Sept. 11, 2008

patch: more widget coloring

by jcid＠dillo.org

On Wed, Sep 10, 2008 at 11:07:02PM +0200, Johannes Hofmann wrote: > Hi Jorge, > > On Wed, Sep 10, 2008 at 04:50:35PM -0400, Jorge Arellano Cid wrote: > > On Wed, Sep 10, 2008 at 05:05:34PM +0200, Johannes Hofmann wrote: > > > On Wed, Sep 10, 2008 at 04:56:47AM +0000, corvid wrote: > > > > > By the way, I was looking at that big page of inputs with > > > > > differently-colored backgrounds that I made in February... > > > > > At the time, I wrote that you guys had gotten the page down to > > > > > 0:06 to load with clear_double_buffer(), > > > > > immediate to load without it, and > > > > > 0:09 to destroy the page. > > > > > > > > > > The good news is that it now takes the same amount of time > > > > > with or without clear_double_buffer(), but > > > > > the bad news is that it takes 0:12 to load and 0:48 to close. > > > > > > > > If I assume that you can only have one instance of a widget in a view > > > > and add a break to the loop in FltkWidgetView::removeFltkWidget() > > > > once it finds it, the 0:48 becomes 0:03. > > > > > > Ah, interesting. In that case I'd ditch the canvasWidgets list > > > entirely as in attached patch. > > > > I don't know well that part of the code, but IIRC Sebastian > > used to render the same page into different widgets at the same > > time (e.g. also to thumbnail-sized one). > > > > AFAIU, this patch removes that functionality, which dillo doesn't > > use by now. Please correct me if I'm wrong. > > I don't think this patch removes any functionality. Not even an > unused one. > It simply relies on fltk to remember all the child widgets instead > of storing them again in a separate list. > > It's rather the recent FltkViewBase::drawRectangle() changes that > affect a scaled down display of the page contents. > The initial fix has now been reverted again (Commit messages > "reverted a patch in FltkViewBase::drawRectangle"). > > > > > BTW, why was the children drawing order reversed? > > > > That's because fltk does it that way. Group::find() searches > children backwards, so it's faster to e.g remove widgets in reverse > order (see comment in Group::find() in the fltk sources). > Also Group::children() get's only called once that way. > > I think it's safe to apply this patch - after some testing on other > platforms. Thanks for the explanation. Committed. -- Cheers Jorge.-

Sept. 11, 2008

patch: more widget coloring

by Johannes.Hofmann＠gmx.de

Hi Jorge, On Wed, Sep 10, 2008 at 04:50:35PM -0400, Jorge Arellano Cid wrote: > On Wed, Sep 10, 2008 at 05:05:34PM +0200, Johannes Hofmann wrote: > > On Wed, Sep 10, 2008 at 04:56:47AM +0000, corvid wrote: > > > > By the way, I was looking at that big page of inputs with > > > > differently-colored backgrounds that I made in February... > > > > At the time, I wrote that you guys had gotten the page down to > > > > 0:06 to load with clear_double_buffer(), > > > > immediate to load without it, and > > > > 0:09 to destroy the page. > > > > > > > > The good news is that it now takes the same amount of time > > > > with or without clear_double_buffer(), but > > > > the bad news is that it takes 0:12 to load and 0:48 to close. > > > > > > If I assume that you can only have one instance of a widget in a view > > > and add a break to the loop in FltkWidgetView::removeFltkWidget() > > > once it finds it, the 0:48 becomes 0:03. > > > > Ah, interesting. In that case I'd ditch the canvasWidgets list > > entirely as in attached patch. > > I don't know well that part of the code, but IIRC Sebastian > used to render the same page into different widgets at the same > time (e.g. also to thumbnail-sized one). > > AFAIU, this patch removes that functionality, which dillo doesn't > use by now. Please correct me if I'm wrong. I don't think this patch removes any functionality. Not even an unused one. It simply relies on fltk to remember all the child widgets instead of storing them again in a separate list. It's rather the recent FltkViewBase::drawRectangle() changes that affect a scaled down display of the page contents. The initial fix has now been reverted again (Commit messages "reverted a patch in FltkViewBase::drawRectangle"). > > BTW, why was the children drawing order reversed? > That's because fltk does it that way. Group::find() searches children backwards, so it's faster to e.g remove widgets in reverse order (see comment in Group::find() in the fltk sources). Also Group::children() get's only called once that way. I think it's safe to apply this patch - after some testing on other platforms. Cheers, Johannes.

Sept. 10, 2008

patch: more widget coloring

by jcid＠dillo.org

On Wed, Sep 10, 2008 at 05:05:34PM +0200, Johannes Hofmann wrote: > On Wed, Sep 10, 2008 at 04:56:47AM +0000, corvid wrote: > > > By the way, I was looking at that big page of inputs with > > > differently-colored backgrounds that I made in February... > > > At the time, I wrote that you guys had gotten the page down to > > > 0:06 to load with clear_double_buffer(), > > > immediate to load without it, and > > > 0:09 to destroy the page. > > > > > > The good news is that it now takes the same amount of time > > > with or without clear_double_buffer(), but > > > the bad news is that it takes 0:12 to load and 0:48 to close. > > > > If I assume that you can only have one instance of a widget in a view > > and add a break to the loop in FltkWidgetView::removeFltkWidget() > > once it finds it, the 0:48 becomes 0:03. > > Ah, interesting. In that case I'd ditch the canvasWidgets list > entirely as in attached patch. I don't know well that part of the code, but IIRC Sebastian used to render the same page into different widgets at the same time (e.g. also to thumbnail-sized one). AFAIU, this patch removes that functionality, which dillo doesn't use by now. Please correct me if I'm wrong. BTW, why was the children drawing order reversed? -- Cheers Jorge.-

Sept. 10, 2008

[patch] remove asap parameter

by jcid＠dillo.org

On Wed, Sep 10, 2008 at 04:21:50PM +0200, Johannes Hofmann wrote: > On Tue, Sep 09, 2008 at 06:26:05PM -0400, Jorge Arellano Cid wrote: > > On Tue, Sep 09, 2008 at 06:07:16PM +0200, Johannes Hofmann wrote: > > > Hi Jorge, > > > > > > any objections to remove the asap parameter as in attached patch? > > > It has been disabled for quite some time now. > > > > I read it, made some tests and it looks like the cleanup > > can even reduce the number of resize calls! > > Not sure wether I'm happy about that. It should not change > anything... > I will have a further look. You'll find: //static int calls = 0; //printf(" Layout::resizeIdle calls = %d\n", ++calls); both in layout.cc and fltkviewbase.cc. That's what I used to test. Note that networking may affect the counters. -- Cheers Jorge.-

Sept. 10, 2008