- Dillo-dev - lists.mailman3.com

CSS leaks: WAS Re: [Dillo-dev] <ol>'s leak the labels of their items

by onepoint＠starurchin.org

On Wed, Jan 28, 2009 at 10:44:11PM +0100, Hofmann Johannes wrote: > On Wed, Jan 28, 2009 at 01:06:51PM +0000, Jeremy Henty wrote: > Went with the former for now, but we will need to switch to the > latter to support "inherit" or properties that can hold strings _or_ > enums. > Please check current tip. I still see a leak. I'm attaching a test case, the valgrind log and a suppression that stops the report. Regards, Jeremy Henty

Jan. 29, 2009

CSS leaks: WAS Re: [Dillo-dev] <ol>'s leak the labels of their items

by Johannes.Hofmann＠gmx.de

On Wed, Jan 28, 2009 at 01:06:51PM +0000, Jeremy Henty wrote: > On Tue, Jan 27, 2009 at 09:08:56AM +0100, Hofmann Johannes wrote: > > On Tue, Jan 27, 2009 at 07:45:42AM +0000, Jeremy Henty wrote: > > > I thought the problem was that CssPropertyList::set() does not > > > free any existing value but I'm not sure I understand how these > > > classes are meant to manage their memory. > > > > CSS values are stored in CssProperty::Value, which is a union. The > > fix is to free Value.strVal, whenever Value contains a strVal (and > > not an enum or length). To know this we could depend on the > > corresponding CssProperty::Name (freeing it for > > CSS_PROPERTY_FONT_FAMILY, but not for others), or add a Type member. > > The latter is cleaner and would not need more memory, as Name and > > Type could fit in two shorts. > > So, which should it be? I favour the former as I think that if we > want to keep the Value struct lean then it is nicer to do it by not > adding fields rather than resorting to packing tricks. But that's > only a mild preference: either seems OK. Went with the former for now, but we will need to switch to the latter to support "inherit" or properties that can hold strings _or_ enums. Please check current tip. Cheers, Johannes

Jan. 28, 2009

dw::Image objects leak their mapKey

by onepoint＠starurchin.org

On Wed, Jan 28, 2009 at 05:48:20PM -0300, Jorge Arellano Cid wrote: > In the second part of the patch, Why not: > > void Image::setUseMap (ImageMapsList *list, object::Object *key) > { > mapList = list; > + if (mapKey && mapKey != key) > + delete mapKey; > mapKey = key; > } Because I'm ridiculously paranoid about data members pointing to free()-ed values for even a nanosecond. You decide whether Dillo needs to be similarly paranoid/ridiculous. :-) Regards, Jeremy Henty

Jan. 28, 2009

dw::Image objects leak their mapKey

by jcid＠dillo.org

On Wed, Jan 28, 2009 at 07:20:38PM +0000, Jeremy Henty wrote: > > This shows up under valgrind as a leak when the HTML includes an <img > usemap="...">. Patch attached. In the second part of the patch, Why not: void Image::setUseMap (ImageMapsList *list, object::Object *key) { mapList = list; + if (mapKey && mapKey != key) + delete mapKey; mapKey = key; } > # HG changeset patch > # User Jeremy Henty <onepoint(a)starurchin.org> > # Date 1233170119 0 > # Node ID 0f662866abb4ab30f50a2da66432eff7fda8ec7a > # Parent ff6f4530c0f4dc880503d73faf5d2d828d577678 > [mq]: fix-images-leak-mapkey > > diff --git a/dw/image.cc b/dw/image.cc > --- a/dw/image.cc > +++ b/dw/image.cc > @@ -140,6 +140,8 @@ > delete altText; > if (buffer) > buffer->unref (); > + if (mapKey) > + delete mapKey; > } > > void Image::sizeRequestImpl (core::Requisition *requisition) > @@ -401,7 +403,12 @@ > void Image::setUseMap (ImageMapsList *list, object::Object *key) > { > mapList = list; > - mapKey = key; > + if (key != mapKey) { > + object::Object *oldKey = mapKey; > + mapKey = key; > + if (oldKey) > + delete oldKey; > + } > } > > } // namespace dw -- Cheers Jorge.- ______________________________________________________________________ This email has been scanned by the MessageLabs Email Security System. For more information please visit http://www.messagelabs.com/email ______________________________________________________________________

Jan. 28, 2009

dw::Image objects leak their mapKey

by onepoint＠starurchin.org

This shows up under valgrind as a leak when the HTML includes an <img usemap="...">. Patch attached. Regards, Jeremy Henty

Jan. 28, 2009

CSS leaks: WAS Re: [Dillo-dev] <ol>'s leak the labels of their items

by onepoint＠starurchin.org

On Tue, Jan 27, 2009 at 09:08:56AM +0100, Hofmann Johannes wrote: > On Tue, Jan 27, 2009 at 07:45:42AM +0000, Jeremy Henty wrote: > > I thought the problem was that CssPropertyList::set() does not > > free any existing value but I'm not sure I understand how these > > classes are meant to manage their memory. > > CSS values are stored in CssProperty::Value, which is a union. The > fix is to free Value.strVal, whenever Value contains a strVal (and > not an enum or length). To know this we could depend on the > corresponding CssProperty::Name (freeing it for > CSS_PROPERTY_FONT_FAMILY, but not for others), or add a Type member. > The latter is cleaner and would not need more memory, as Name and > Type could fit in two shorts. So, which should it be? I favour the former as I think that if we want to keep the Value struct lean then it is nicer to do it by not adding fields rather than resorting to packing tricks. But that's only a mild preference: either seems OK. Jeremy Henty

Jan. 28, 2009

Tools button committed

by jcid＠dillo.org

On Tue, Jan 27, 2009 at 10:47:19PM +0100, Hofmann Johannes wrote: > On Tue, Jan 27, 2009 at 06:38:50PM -0300, Jorge Arellano Cid wrote: > > Hi, > > > > The tools button changeset was just committed. > > > > Please notice the options are global. It's quite useful as is > > and making per bw options seems not worth. > > > > Go check! :-) > > Hey, pretty cool! There's more stuff that should go into the > tool menu: > > load images > increase font size > decrease font size Yes, it's a good place to experiment. > PS: Where did you get the nice icon? Got a free one from internet, and tweaked it to match the style. Good you like it! It took long to polish it. ;) -- Cheers Jorge.- ______________________________________________________________________ This email has been scanned by the MessageLabs Email Security System. For more information please visit http://www.messagelabs.com/email ______________________________________________________________________

Jan. 27, 2009

Tools button committed

by Johannes.Hofmann＠gmx.de

On Tue, Jan 27, 2009 at 06:38:50PM -0300, Jorge Arellano Cid wrote: > Hi, > > The tools button changeset was just committed. > > Please notice the options are global. It's quite useful as is > and making per bw options seems not worth. > > Go check! :-) Hey, pretty cool! There's more stuff that should go into the tool menu: load images increase font size decrease font size Cheers, Johannes PS: Where did you get the nice icon?

Jan. 27, 2009

Tools button committed

by jcid＠dillo.org

Hi, The tools button changeset was just committed. Please notice the options are global. It's quite useful as is and making per bw options seems not worth. Go check! :-) -- Cheers Jorge.- ______________________________________________________________________ This email has been scanned by the MessageLabs Email Security System. For more information please visit http://www.messagelabs.com/email ______________________________________________________________________

Jan. 27, 2009

CSS leaks: WAS Re: [Dillo-dev] <ol>'s leak the labels of their items

by Johannes.Hofmann＠gmx.de

On Tue, Jan 27, 2009 at 07:45:42AM +0000, Jeremy Henty wrote: > On Mon, Jan 26, 2009 at 11:33:43PM +0100, Hofmann Johannes wrote: > > > There is at least one leak I know of. It's font-family names that > > are parsed in CSS style data. > > Aha! I looked at the Valgrind log for that one but didn't know the > cause. Valgrind identifies the dStrdup() in > > val->strVal = dStrdup(parser->tval); > > in Css_parse_value() as the leak. What's the fix? I thought the > problem was that CssPropertyList::set() does not free any existing > value but I'm not sure I understand how these classes are meant to > manage their memory. Not 100% sure either :-). The problem only exists for string values. Most CSS values are just enums or lengths. Actually I only know of font names atm that are strings. CSS values are stored in CssProperty::Value, which is a union. The fix is to free Value.strVal, whenever Value contains a strVal (and not an enum or length). To know this we could depend on the corresponding CssProperty::Name (freeing it for CSS_PROPERTY_FONT_FAMILY, but not for others), or add a Type member. The latter is cleaner and would not need more memory, as Name and Type could fit in two shorts. We also need a way to accommodate the special "inherit" value, which is not supported yet. Regards, Johannes

Jan. 27, 2009

CSS leaks: WAS Re: [Dillo-dev] <ol>'s leak the labels of their items

by onepoint＠starurchin.org

On Mon, Jan 26, 2009 at 11:33:43PM +0100, Hofmann Johannes wrote: > There is at least one leak I know of. It's font-family names that > are parsed in CSS style data. Aha! I looked at the Valgrind log for that one but didn't know the cause. Valgrind identifies the dStrdup() in val->strVal = dStrdup(parser->tval); in Css_parse_value() as the leak. What's the fix? I thought the problem was that CssPropertyList::set() does not free any existing value but I'm not sure I understand how these classes are meant to manage their memory. Regards, Jeremy Henty

Jan. 27, 2009

<ol>'s leak the labels of their items

by Johannes.Hofmann＠gmx.de

On Mon, Jan 26, 2009 at 10:32:42PM +0000, Jeremy Henty wrote: > On Mon, Jan 26, 2009 at 11:03:20PM +0100, Hofmann Johannes wrote: > > On Mon, Jan 26, 2009 at 09:22:03PM +0000, Jeremy Henty wrote: > > > > > > Valgrind reports that <ol>s leak the labels of their items: each > > > of the first 9 <li>s leaks 3 bytes, then they leak 4 bytes per > > > <li> etc. The leaked memory is created by the line > > > > > > list_item->initWithText (dStrdup(buf), wordStyle); > > > > Ah right. The strdup() can just be dropped. Allocation and > > deallocation of text is done in Textblock itself for quite some time > > now. Please check current tip. > > It's fixed! (/me deletes valgrind suppression file) good > > But..., valgrind still reports some "indirectly lost" blocks despite > reporting no "definitely lost" or "possibly lost" blocks. How is that > possible? I am suppressing some leaks that come from the depths of X > windows and other libraries, could that be the reason? (Of course > this may have nothing to do with Dillo.) There is at least one leak I know of. It's font-family names that are parsed in CSS style data. Regards, Johannes

Jan. 26, 2009

<ol>'s leak the labels of their items

by onepoint＠starurchin.org

On Mon, Jan 26, 2009 at 11:03:20PM +0100, Hofmann Johannes wrote: > On Mon, Jan 26, 2009 at 09:22:03PM +0000, Jeremy Henty wrote: > > > > Valgrind reports that <ol>s leak the labels of their items: each > > of the first 9 <li>s leaks 3 bytes, then they leak 4 bytes per > > <li> etc. The leaked memory is created by the line > > > > list_item->initWithText (dStrdup(buf), wordStyle); > > Ah right. The strdup() can just be dropped. Allocation and > deallocation of text is done in Textblock itself for quite some time > now. Please check current tip. It's fixed! (/me deletes valgrind suppression file) But..., valgrind still reports some "indirectly lost" blocks despite reporting no "definitely lost" or "possibly lost" blocks. How is that possible? I am suppressing some leaks that come from the depths of X windows and other libraries, could that be the reason? (Of course this may have nothing to do with Dillo.) Regards, Jeremy Henty

Jan. 26, 2009

<ol>'s leak the labels of their items

by Johannes.Hofmann＠gmx.de

On Mon, Jan 26, 2009 at 09:22:03PM +0000, Jeremy Henty wrote: > > Valgrind reports that <ol>s leak the labels of their items: each of > the first 9 <li>s leaks 3 bytes, then they leak 4 bytes per <li> etc. > The leaked memory is created by the line > > list_item->initWithText (dStrdup(buf), wordStyle); Ah right. The strdup() can just be dropped. Allocation and deallocation of text is done in Textblock itself for quite some time now. Please check current tip. Cheers, Johannes

Jan. 26, 2009

<ol>'s leak the labels of their items

by onepoint＠starurchin.org

Valgrind reports that <ol>s leak the labels of their items: each of the first 9 <li>s leaks 3 bytes, then they leak 4 bytes per <li> etc. The leaked memory is created by the line list_item->initWithText (dStrdup(buf), wordStyle); in Html_tag_open_li (in src/html.cc). The dStrdup()ed memory is never freed. I think that the problem is that the TextBlock destructor only deletes the content of a word if the content type is WIDGET or ANCHOR, whereas it should also handle the case when the content type is TEXT. Perhaps initWithText() should copy the string argument itself with new[] so that ~TextBlock could safely delete[] it? The only two references to initWithText() are in html.cc and one of the widget tests, so this would be an easy change. Regards, Jeremy Henty

Jan. 26, 2009

Re: new font preferences

by Johannes.Hofmann＠gmx.de

On Mon, Jan 26, 2009 at 04:58:55PM -0300, Jorge Arellano Cid wrote: > On Mon, Jan 26, 2009 at 07:14:08PM +0100, Hofmann Johannes wrote: > > Hi Jorge, > > > > do you have an opinion on the (incompatible) changes to the font > > preferences as proposed in > > http://lists.auriga.wearlab.de/pipermail/dillo-dev/2009-January/005746.html > > > > I'd like to commit this if you agree. > > Oh yes, feel free to commit. Committed. > Just let dillo-dev know how to upgrade our custom dillorc! :-) > > At this point you're the most knowledgeable in font handling and > CSS, thus some basic explanation of settings/usage would be welcomed too. >From the checking comment: CSS 2.1 uses the following predefined font names: 'serif', 'sans-serif', 'cursive', 'fantasy', 'monospace'. Add preferences so that the user can set real font names for these. Basically the new "font_sans_serif" option is what "vw_fontame" used to be and "font_monospace" is equivalent to "fw_fontname". The other two options are new. http://www.w3.org/TR/CSS21/fonts.html#generic-font-families has some more information including font examples. Cheers, Johannes

Jan. 26, 2009

dillo CSS and charset

by jcid＠dillo.org

On Thu, Jan 22, 2009 at 06:53:46PM +0000, corvid wrote: > Johannes wrote: > > On Thu, Jan 22, 2009 at 05:57:42PM +0000, corvid wrote: > > > Johannes wrote: > > > > BTW. now you can set vw_fontname in dillorc again. This font will > > > > then be used when CSS specifies "sans" as font, e.g: > > > > vw_fontname="VL Gothic" > > > > > > If I put a serif font in vw_fontname and then use > > > sans in style.css for some things, it'll use > > > the serif font. > > > > True. We need to make new config options > > > > sans-serif_fontname > > serif_fontname > > cursive_fontname > > fantasy_fontname > > monospace_fontname > > > > maybe even dropping the "_fontname" suffix. > > > > But of course that would no longer be compatible with older > > versions of dillorc. > > I would add the new options and remove the old options. > CSS is big and certainly worth some incompatibility. I agree with corvid. And, dillo should be able to pick some sane defaults when we figure how to deal with them right through FLTK. > Related to this, I've been wondering whether we should have > a policy about the dillorc on the website -- whether it should > follow tip, or follow the most recent release, or always add > new options but never delete old options... The current practice is to make it compatible with the last release, and to update the one in the source tree for those using the repository. -- Cheers Jorge.-

Jan. 26, 2009

Character entity parsing regression.

by jcid＠dillo.org

On Mon, Jan 26, 2009 at 07:09:55AM +0000, corvid wrote: > Jorge wrote: > > There's a regression with the attached page. Squared boxes > > appear where nothing should be. I haven't dug since when. > > Attached. Looks like it dates to > changeset: 286:d16cec5095be > user: jcid > date: Fri Jun 20 19:10:04 2008 +0200 > summary: - Adapted the code to the new Textblock::addText() API. > Good! Committed I also committed another fix for an unrelated but similar problem. Use "hg log -v|less" for a test case. -- Cheers Jorge.-

Jan. 26, 2009

CSS parser leak

by onepoint＠starurchin.org

On Sun, Jan 25, 2009 at 06:07:58PM +0100, Hofmann Johannes wrote: > I decided to stick with SimpleVector, but allocate CssSimpleSelector > properly with new/delete. Please check current tip. This seems to have fixed all the big "definitely lost" errors, which is very good. I still see some small "possibly lost" errors. The valgrind docs advise that "possibly lost" really means "definitely lost" unless you are doing something funny. I can't tell whether that applies here! As I understand it, "possibly lost" means that there is still a pointer to the interior of the block. Most likely the original pointer to the block has been lost and it has therefore leaked, but it is possible that the program knows about the interior pointer and is using that to access the block. Valgrind log attached. Regards, Jeremy Henty

Jan. 26, 2009

switch to DILLO_HTML_PARSE_MODE_PRE depending on style

by Johannes.Hofmann＠gmx.de

On Sun, Jan 25, 2009 at 06:10:33PM -0300, Jorge Arellano Cid wrote: > On Thu, Jan 22, 2009 at 02:39:32PM +0100, Hofmann Johannes wrote: > > Hi, > > > > I'm experimenting with attached patch to make switching to > > DILLO_HTML_PARSE_MODE_PRE from CSS possible. > > Some online source browsers like cgit (http://hjemli.net/git/cgit/) > > use this to display sources or diffs. > > > > Please comment. > > Considering the current length of the Html_process_tag function, > and that the proposed patch may have secondary effects if a tag > with automatic close sets PRE, I'd suggest something along the > lines of the attached patch. If it passes your tests, feel free > to commit. Committed in two steps. Thanks, Johannes

Jan. 26, 2009

Character entity parsing regression.

by corvid＠lavabit.com

Jorge wrote: > There's a regression with the attached page. Squared boxes > appear where nothing should be. I haven't dug since when. Attached. Looks like it dates to changeset: 286:d16cec5095be user: jcid date: Fri Jun 20 19:10:04 2008 +0200 summary: - Adapted the code to the new Textblock::addText() API.

Jan. 26, 2009

Character entity parsing regression.

by jcid＠dillo.org

Hi, There's a regression with the attached page. Squared boxes appear where nothing should be. I haven't dug since when. Can somebody please give it a look while I work on the tools button? TIA. -- Cheers Jorge.-

Jan. 26, 2009

switch to DILLO_HTML_PARSE_MODE_PRE depending on style

by jcid＠dillo.org

On Thu, Jan 22, 2009 at 02:39:32PM +0100, Hofmann Johannes wrote: > Hi, > > I'm experimenting with attached patch to make switching to > DILLO_HTML_PARSE_MODE_PRE from CSS possible. > Some online source browsers like cgit (http://hjemli.net/git/cgit/) > use this to display sources or diffs. > > Please comment. Considering the current length of the Html_process_tag function, and that the proposed patch may have secondary effects if a tag with automatic close sets PRE, I'd suggest something along the lines of the attached patch. If it passes your tests, feel free to commit. -- Cheers Jorge.-

Jan. 26, 2009

CSS parser leak

by Johannes.Hofmann＠gmx.de

On Sat, Jan 24, 2009 at 09:01:07PM +0000, Jeremy Henty wrote: > On Sat, Jan 24, 2009 at 06:13:48PM +0100, Hofmann Johannes wrote: > > > ... the problem is that the destructor is not called > > atm. CssSimpleSelector is embedded in struct CombinatorAndSelector > > which in turn lives in a lout::misc::SimpleVector. So the memory is > > allocated with malloc and not properly handled with new/delete. > > This is nasty and has to be fixed somehow. > > ...especially as valgrind reports that this is one of Dillo's biggest > leaks. > > I'm attaching a quick and dirty fix that defines a finalize() method > on selectors. This still misses the case when the SimpleVector is > resize()-ed, but that appears to be much more rare. > > The proper fix would be to replace the SimpleVector with a typed > Vector with ownerOfObjects = true. Then finalize() could be turned > into a proper destructor. That would mean CombinatorAndSelector > inheriting Object. I decided to stick with SimpleVector, but allocate CssSimpleSelector properly with new/delete. Please check current tip. Cheers, Johannes

Jan. 25, 2009

dillo CSS and charset

by furaisanjin＠gmail.com

Please ignore this one. I found that my cache.c was corrupted. My local change still remained after updating and that caused this behaviour. 2009/1/25 furaisanjin <furaisanjin(a)gmail.com>: > After pulling from http://freehg.org/u/dillo/main and updating dillo, > the charset conversion problem appeared again. > > Goto http://headlines.yahoo.co.jp/hl (this page is OK) and click some > of article like > http://headlines.yahoo.co.jp/hl?a=20090125-00000013-yom-soci. Charset > conversion doesn't happen on this page. These pages don't contain meta > but HTTP server specifies charset. > > Regards, > furaisanjin >

Jan. 25, 2009

dillo CSS and charset

by furaisanjin＠gmail.com

After pulling from http://freehg.org/u/dillo/main and updating dillo, the charset conversion problem appeared again. Goto http://headlines.yahoo.co.jp/hl (this page is OK) and click some of article like http://headlines.yahoo.co.jp/hl?a=20090125-00000013-yom-soci. Charset conversion doesn't happen on this page. These pages don't contain meta but HTTP server specifies charset. Regards, furaisanjin

Jan. 25, 2009

CSS parser leak

by onepoint＠starurchin.org

On Sat, Jan 24, 2009 at 06:13:48PM +0100, Hofmann Johannes wrote: > ... the problem is that the destructor is not called > atm. CssSimpleSelector is embedded in struct CombinatorAndSelector > which in turn lives in a lout::misc::SimpleVector. So the memory is > allocated with malloc and not properly handled with new/delete. > This is nasty and has to be fixed somehow. ...especially as valgrind reports that this is one of Dillo's biggest leaks. I'm attaching a quick and dirty fix that defines a finalize() method on selectors. This still misses the case when the SimpleVector is resize()-ed, but that appears to be much more rare. The proper fix would be to replace the SimpleVector with a typed Vector with ownerOfObjects = true. Then finalize() could be turned into a proper destructor. That would mean CombinatorAndSelector inheriting Object. Regards, Jeremy Henty

Jan. 24, 2009

inheriting background color

by Johannes.Hofmann＠gmx.de

On Sat, Jan 24, 2009 at 05:43:52PM +0000, corvid wrote: > Is the idea that we should stick in inheritBackgroundColor > whenever we encounter cases like the following? > (I see the "get rid of this somehow" comment.) The inheritBackgroundColor thing is a workaround for elements (<a>, <small> in the below case) that do not create a new dw widget. Things like: <span style="border: 2px solid black">hello</span> don't work at all. I'd like to see a general solution for this but have no idea yet. It's also somehow related to the CSS property "display" (http://www.w3.org/TR/CSS21/visuren.html#display-prop) which is not yet supported at all. But yes, as long as we stick with inheritBackgroundColor, we will need to add it whenever no dw widget is created. Cheers, Johannes > > <head> > <title></title> > <style type="text/css"> > body {background: #000000; color: #00ff00} > </style> > </head> > <body> > <SPAN STYLE='background:#fff6aa;'>link background dark( > <SMALL><A HREF="/">link</A></SMALL> > )</SPAN> > <p> > <SPAN STYLE='background:#fff6aa;'>link background light( > <A HREF="/">link</A>) > </SPAN> > </body> > > > > _______________________________________________ > Dillo-dev mailing list > Dillo-dev(a)dillo.org > http://lists.auriga.wearlab.de/cgi-bin/mailman/listinfo/dillo-dev

Jan. 24, 2009

inheriting background color

by corvid＠lavabit.com

Is the idea that we should stick in inheritBackgroundColor whenever we encounter cases like the following? (I see the "get rid of this somehow" comment.) <head> <title></title> <style type="text/css"> body {background: #000000; color: #00ff00} </style> </head> <body> <SPAN STYLE='background:#fff6aa;'>link background dark( <SMALL><A HREF="/">link</A></SMALL> )</SPAN> <p> <SPAN STYLE='background:#fff6aa;'>link background light( <A HREF="/">link</A>) </SPAN> </body>

Jan. 24, 2009

CSS parser leak

by Johannes.Hofmann＠gmx.de

On Sat, Jan 24, 2009 at 05:00:02PM +0000, Jeremy Henty wrote: > > The CSS parser gurus will have to verify this for me, but it *seems* > to me that the leak is because the CssSimpleSelector class does not > clean up its id, klass and pseudo members. Furthermore the only > method that sets those members is Css_parse_simple_selector(), and it > always assigns them to new heap-allocated values. So I reckon the fix > is just to add a CssSimpleSelector destructor that dFree()s those > members. Have I missed anything? No, you are right, but the problem is that the destructor is not called atm. CssSimpleSelector is embedded in struct CombinatorAndSelector which in turn lives in a lout::misc::SimpleVector. So the memory is allocated with malloc and not properly handled with new/delete. This is nasty and has to be fixed somehow. Regards, Johannes

Jan. 24, 2009

dillo CSS and charset

by jcid＠dillo.org

On Sat, Jan 24, 2009 at 03:00:33PM +0900, furaisanjin wrote: > Hi. > > This is work around for www.amazon.de. > > diff -r 93de73875a8c src/cache.c > --- a/src/cache.c Thu Jan 22 08:31:44 2009 -0300 > +++ b/src/cache.c Sat Jan 24 14:53:50 2009 +0900 > @@ -747,6 +747,7 @@ > dStr_free(entry->Data, 1); > entry->Data = dStr_sized_new(MIN(entry->ExpectedSize, MAX_INIT_BUF)); > } > + Cache_ref_data(entry); > > /* Get Content-Type */ > if ((Type = Cache_parse_field(header, "Content-Type")) == NULL) { > @@ -762,7 +763,6 @@ > _MSG("TypeMeta {%s}\n", entry->TypeMeta); > dFree(Type); > } > - Cache_ref_data(entry); > } > > /* > > I found that Cache_ref_data position was moved to the bottom. I don't > fully understand the logic but somehow this change helps. You're using repo's tip, right? Cache_ref_data() also updates UTF8Data according to CharsetDecoder (yes it's a trick). That's why moving it after a_Cache_set_content_type() has the effect of initializing the UTF8Data biffer. -- Cheers Jorge.-

Jan. 24, 2009

Valgrind errors: big improvement!

by jcid＠dillo.org

On Sat, Jan 24, 2009 at 03:26:14PM +0000, Jeremy Henty wrote: > > The very latest dillo is vastly more valgrind-clean than before: I got > no errors from my latest browsing session! I'll keep monitoring it > and look at the leak reports too. Good. Yes, there're still some css selector issues and some image structures leaking in border cases. -- Cheers Jorge.-

Jan. 24, 2009

Valgrind errors

by jcid＠dillo.org

On Sat, Jan 24, 2009 at 10:34:45AM +0100, Hofmann Johannes wrote: > On Fri, Jan 23, 2009 at 05:11:54PM -0300, Jorge Arellano Cid wrote: > > On Fri, Jan 23, 2009 at 07:56:33AM +0000, Jeremy Henty wrote: > > > On Fri, Jan 23, 2009 at 03:13:10AM +0000, corvid wrote: > > > > > > > I realized we should really be using entry->Data regardless of > > > > character encoding when showing progress in receiving the > > > > page. Attached. > > > > > > OK, then ignore my earlier patch. Do you expect this to fix both of > > > the valgrind errors, or just the second one as your previous > > > suggestion did? > > > > The new patches added expect to fix the remaining. > > The small patch (2 lines) aims to this problem, and the others are > > cleanups and logic fixes that hope to avoid future troubles. > > > > Please test. > > The amazon.de crash is fixed. > And I don't see any other crashes. Good. (It didn't crash for me, but valgrind complained. Fixing the latter did the trick). -- Cheers Jorge.-

Jan. 24, 2009

CSS parser leak

by onepoint＠starurchin.org

The CSS parser gurus will have to verify this for me, but it *seems* to me that the leak is because the CssSimpleSelector class does not clean up its id, klass and pseudo members. Furthermore the only method that sets those members is Css_parse_simple_selector(), and it always assigns them to new heap-allocated values. So I reckon the fix is just to add a CssSimpleSelector destructor that dFree()s those members. Have I missed anything? Regards, Jeremy Henty

Jan. 24, 2009

CSS parser leak

by onepoint＠starurchin.org

Valgrind log attached. Jeremy Henty

Jan. 24, 2009

Valgrind errors: big improvement!

by onepoint＠starurchin.org

The very latest dillo is vastly more valgrind-clean than before: I got no errors from my latest browsing session! I'll keep monitoring it and look at the leak reports too. Regards, Jeremy Henty

Jan. 24, 2009

Valgrind errors

by Johannes.Hofmann＠gmx.de

On Fri, Jan 23, 2009 at 05:11:54PM -0300, Jorge Arellano Cid wrote: > On Fri, Jan 23, 2009 at 07:56:33AM +0000, Jeremy Henty wrote: > > On Fri, Jan 23, 2009 at 03:13:10AM +0000, corvid wrote: > > > > > I realized we should really be using entry->Data regardless of > > > character encoding when showing progress in receiving the > > > page. Attached. > > > > OK, then ignore my earlier patch. Do you expect this to fix both of > > the valgrind errors, or just the second one as your previous > > suggestion did? > > The new patches added expect to fix the remaining. > The small patch (2 lines) aims to this problem, and the others are > cleanups and logic fixes that hope to avoid future troubles. > > Please test. The amazon.de crash is fixed. And I don't see any other crashes. Thanks, Johannes

Jan. 24, 2009

dillo CSS and charset

by furaisanjin＠gmail.com

Hi. This is work around for www.amazon.de. diff -r 93de73875a8c src/cache.c --- a/src/cache.c Thu Jan 22 08:31:44 2009 -0300 +++ b/src/cache.c Sat Jan 24 14:53:50 2009 +0900 @@ -747,6 +747,7 @@ dStr_free(entry->Data, 1); entry->Data = dStr_sized_new(MIN(entry->ExpectedSize, MAX_INIT_BUF)); } + Cache_ref_data(entry); /* Get Content-Type */ if ((Type = Cache_parse_field(header, "Content-Type")) == NULL) { @@ -762,7 +763,6 @@ _MSG("TypeMeta {%s}\n", entry->TypeMeta); dFree(Type); } - Cache_ref_data(entry); } /* I found that Cache_ref_data position was moved to the bottom. I don't fully understand the logic but somehow this change helps. Regards, furaisanjin

Jan. 24, 2009

dillo CSS and charset

by furaisanjin＠gmail.com

Hi 2009/1/23 Jorge Arellano Cid <jcid(a)dillo.org>: > On Fri, Jan 23, 2009 at 07:31:56AM +0900, furaisanjin wrote: > OK, but I still want to commit the proposed patch. > Please test it and comment. I have browsed several pages and I don't encounter problem. Still www.amazon.de causes dillo crashed but I guess that would be different issue. Regards, furaisanjin

Jan. 23, 2009

Valgrind errors

by jcid＠dillo.org

On Fri, Jan 23, 2009 at 07:56:33AM +0000, Jeremy Henty wrote: > On Fri, Jan 23, 2009 at 03:13:10AM +0000, corvid wrote: > > > I realized we should really be using entry->Data regardless of > > character encoding when showing progress in receiving the > > page. Attached. > > OK, then ignore my earlier patch. Do you expect this to fix both of > the valgrind errors, or just the second one as your previous > suggestion did? The new patches added expect to fix the remaining. The small patch (2 lines) aims to this problem, and the others are cleanups and logic fixes that hope to avoid future troubles. Please test. -- Cheers Jorge.-

Jan. 23, 2009

dillo CSS and charset

by Johannes.Hofmann＠gmx.de

On Fri, Jan 23, 2009 at 03:54:42AM +0000, corvid wrote: > Johannes wrote: > > On Thu, Jan 22, 2009 at 06:53:46PM +0000, corvid wrote: > > > Johannes wrote: > > > > On Thu, Jan 22, 2009 at 05:57:42PM +0000, corvid wrote: > > > > > Johannes wrote: > > > > > > BTW. now you can set vw_fontname in dillorc again. This font will > > > > > > then be used when CSS specifies "sans" as font, e.g: > > > > > > vw_fontname="VL Gothic" > > > > > > > > > > If I put a serif font in vw_fontname and then use > > > > > sans in style.css for some things, it'll use > > > > > the serif font. > > > > > > > > True. We need to make new config options > > > > > > > > sans-serif_fontname > > > > serif_fontname > > > > cursive_fontname > > > > fantasy_fontname > > > > monospace_fontname > > > > > > > > maybe even dropping the "_fontname" suffix. > > > > > > > > But of course that would no longer be compatible with older > > > > versions of dillorc. > > > > > > I would add the new options and remove the old options. > > > CSS is big and certainly worth some incompatibility. > > > > Ok, attached is a patch for discussion. > > You meant to write font_monospace in dialog.cc and plain.cc. Yes, that's what I meant. > > PS I approve of cursive and fantasy defaulting to sans :) :) Cheers, Johannes

Jan. 23, 2009

Valgrind errors

by corvid＠lavabit.com

Jeremy wrote: > On Fri, Jan 23, 2009 at 03:13:10AM +0000, corvid wrote: > > > I realized we should really be using entry->Data regardless of > > character encoding when showing progress in receiving the > > page. Attached. > > OK, then ignore my earlier patch. Do you expect this to fix both of > the valgrind errors, or just the second one as your previous > suggestion did? Just the second one. The other error looks like it's upset that we're trying to look at start and tag after set_content_type made buf go away.

Jan. 23, 2009

Valgrind errors

by onepoint＠starurchin.org

On Fri, Jan 23, 2009 at 03:13:10AM +0000, corvid wrote: > I realized we should really be using entry->Data regardless of > character encoding when showing progress in receiving the > page. Attached. OK, then ignore my earlier patch. Do you expect this to fix both of the valgrind errors, or just the second one as your previous suggestion did? Regards, Jeremy Henty

Jan. 23, 2009

dillo CSS and charset

by corvid＠lavabit.com

Johannes wrote: > On Thu, Jan 22, 2009 at 06:53:46PM +0000, corvid wrote: > > Johannes wrote: > > > On Thu, Jan 22, 2009 at 05:57:42PM +0000, corvid wrote: > > > > Johannes wrote: > > > > > BTW. now you can set vw_fontname in dillorc again. This font will > > > > > then be used when CSS specifies "sans" as font, e.g: > > > > > vw_fontname="VL Gothic" > > > > > > > > If I put a serif font in vw_fontname and then use > > > > sans in style.css for some things, it'll use > > > > the serif font. > > > > > > True. We need to make new config options > > > > > > sans-serif_fontname > > > serif_fontname > > > cursive_fontname > > > fantasy_fontname > > > monospace_fontname > > > > > > maybe even dropping the "_fontname" suffix. > > > > > > But of course that would no longer be compatible with older > > > versions of dillorc. > > > > I would add the new options and remove the old options. > > CSS is big and certainly worth some incompatibility. > > Ok, attached is a patch for discussion. You meant to write font_monospace in dialog.cc and plain.cc. PS I approve of cursive and fantasy defaulting to sans :)

Jan. 23, 2009

Valgrind errors

by corvid＠lavabit.com

Jeremy wrote: > On Thu, Jan 22, 2009 at 10:37:41PM +0000, corvid wrote: > > Jeremy wrote: > > > ==20949== by 0x806065B: a_Cache_process_dbuf (cache.c:865) > > > ==20949== > > > ==20949== Invalid read of size 4 > > > ==20949== at 0x8060E9B: Cache_process_queue (cache.c:1145) > > > > Does this part of it stop complaining if the > > a_UIcmd_set_page_prog(Client_bw, data->len, 1); > > in Cache_process_queue() uses Cache_data(entry)->len instead? > > Yes. Patch attached. I realized we should really be using entry->Data regardless of character encoding when showing progress in receiving the page. Attached.

Jan. 23, 2009

Valgrind error: possible link to Imgbuf crash?

by onepoint＠starurchin.org

Here's another valgrind error that I think might be related to the crashes I have been seeing. The gdb stack trace showed that a_Imgbuf_new() was calling a layout method with an invalid address for the layout object. The valgrind error shows a_Imgbuf_new() reading invalid memory when it calls Widget::getLayout(). It looks to me as though a_Imgbuf_new() is being passed an Image that has already been freed. Regards, Jeremy Henty

Jan. 23, 2009

Valgrind errors

by onepoint＠starurchin.org

On Thu, Jan 22, 2009 at 10:37:41PM +0000, corvid wrote: > Jeremy wrote: > > ==20949== by 0x806065B: a_Cache_process_dbuf (cache.c:865) > > ==20949== > > ==20949== Invalid read of size 4 > > ==20949== at 0x8060E9B: Cache_process_queue (cache.c:1145) > > Does this part of it stop complaining if the > a_UIcmd_set_page_prog(Client_bw, data->len, 1); > in Cache_process_queue() uses Cache_data(entry)->len instead? Yes. Patch attached. Regards, Jeremy Henty

Jan. 23, 2009

dillo CSS and charset

by jcid＠dillo.org

On Fri, Jan 23, 2009 at 07:31:56AM +0900, furaisanjin wrote: > Hi. > > 2009/1/23 Jorge Arellano Cid <jcid(a)dillo.org>: > >> Have you added charset to the content-type given by the bookmarks dpi? > >> or have you added a META or both? > > I did hg revert cache.c to cancel my local modification and then built > dillo again. However I can't reproduce the problem anymore. I don't > add content-type on dpi nor meta on bookmark because if encoding type > is UTF-8, no conversion is required. I assume dillo native encoding is > UTF-8 now. > > Sorry for confusion. Please ignore bookmark problem. OK, but I still want to commit the proposed patch. Please test it and comment. -- Cheers Jorge.-

Jan. 23, 2009

[patch]: control lout's MSG from the preferences

by jcid＠dillo.org

Hi Jeremy, On Sat, Jan 17, 2009 at 06:57:55PM +0000, Jeremy Henty wrote: > > This patch makes the dillorc show_msg preference control lout's MSG > macros as well as dillo's. Please comment. As is, it looks like an overkill to me, but it may be interesting to be able to handle other preferences as well. This is, to have something like: lout::prefs::message({true|false}); henceforth items could be added when neeeded. e.g. lout::prefs::tableDebugRendering({true|false}); Please resubmit like above, but with just message. -- Cheers Jorge.-

Jan. 23, 2009

Valgrind errors

by corvid＠lavabit.com

Jeremy wrote: > ==20949== by 0x806065B: a_Cache_process_dbuf (cache.c:865) > ==20949== > ==20949== Invalid read of size 4 > ==20949== at 0x8060E9B: Cache_process_queue (cache.c:1145) Does this part of it stop complaining if the a_UIcmd_set_page_prog(Client_bw, data->len, 1); in Cache_process_queue() uses Cache_data(entry)->len instead?

Jan. 22, 2009