September 2008 - Dillo-dev - lists.mailman3.com

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

[patch] remove asap parameter

by jcid＠dillo.org

On Wed, Sep 10, 2008 at 04:20:50PM +0200, Johannes Hofmann wrote: > On Tue, Sep 09, 2008 at 07:32:14PM -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 made a mistake during the dw2 commit, then fixed it, and it > > looks OK, but please check it. > > Looks good. Just a comment replacing a ifdef'd block is missing. > See attached patch. Committed. -- Cheers Jorge.-

Sept. 10, 2008

patch: more widget coloring

by corvid＠lavabit.com

Johannes 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. > > Cheers, > Johannes > > PS: Can you share that test page? I just meant the one generated from the code on: http://lists.auriga.wearlab.de/pipermail/dillo-dev/2008-February/003760.html

Sept. 10, 2008

patch: more widget coloring

by Johannes.Hofmann＠gmx.de

ok, here's the patch ... 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. > > Cheers, > Johannes > > PS: Can you share that test page? >

Sept. 10, 2008

patch: more widget coloring

by Johannes.Hofmann＠gmx.de

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. Cheers, Johannes PS: Can you share that test page?

Sept. 10, 2008