January 2009 - Dillo-dev - lists.mailman3.com

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

by Johannes.Hofmann＠gmx.de

On Thu, Jan 29, 2009 at 01:03:29PM +0000, Jeremy Henty wrote: > On Thu, Jan 29, 2009 at 01:25:29PM +0100, Hofmann Johannes wrote: > > > Can you please retry with the user-agent style parsing disabled: > > > > diff -r 4b5125e3ca89 src/css.cc > > --- a/src/css.cc Thu Jan 29 12:38:20 2009 +0100 > > +++ b/src/css.cc Thu Jan 29 13:24:16 2009 +0100 > > @@ -327,7 +327,7 @@ CssContext::CssContext () { > > sheet[CSS_PRIMARY_USER] = userStyle; > > sheet[CSS_PRIMARY_USER_IMPORTANT] = userImportantStyle; > > > > - buildUserAgentStyle (); > > + //buildUserAgentStyle (); > > buildUserStyle (); > > } > > That takes out most of the "indirectly lost" memory but doesn't affect > the "definitely lost" stuff that valgrind generates the suppression > for. Log attached. Next try. Please check attached patch. Thanks for testing this, Johannes

Jan. 29, 2009

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

by Johannes.Hofmann＠gmx.de

On Thu, Jan 29, 2009 at 09:37:31AM -0300, Jorge Arellano Cid wrote: > 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: > > > 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. > > These patches produce lots of warnings in the test directory: I can't reproduce the warnings, but I think I know the reason. It's changeset 9a7684395cca. Can you please check attached patch which tries to fix the underlying issue in an other way. Comments are welcome! We could also backout 9a7684395cca and leave things as they were before, but deleting memory allocated with strdup() is a bug. Cheers, Johannes > > make[2]: Entering directory `/home/jcid/C/Dillo/d2/dillo-wc/test' > dw_anchors_test.cc: In function 'int main(int, char**)': > dw_anchors_test.cc:120: warning: deprecated conversion from string constant to > 'char*' > dw_example.cc: In function 'int main(int, char**)': > dw_example.cc:49: warning: deprecated conversion from string constant to > 'char*' > dw_find_test.cc: In function 'int main(int, char**)': > dw_find_test.cc:93: warning: deprecated conversion from string constant to > 'char*' > dw_links.cc: In function 'int main(int, char**)': > dw_links.cc:88: warning: deprecated conversion from string constant to 'char*' > dw_links2.cc: In function 'int main(int, char**)': > dw_links2.cc:116: warning: deprecated conversion from string constant to > 'char*' > dw_images_simple.cc: In function 'int main(int, char**)': > dw_images_simple.cc:110: warning: deprecated conversion from string constant to > 'char*' > dw_images_scaled.cc: In function 'int main(int, char**)': > dw_images_scaled.cc:112: warning: deprecated conversion from string constant to > 'char*' > dw_images_scaled2.cc: In function 'int main(int, char**)': > dw_images_scaled2.cc:87: warning: deprecated conversion from string constant to > 'char*' > dw_lists.cc: In function 'int main(int, char**)': > dw_lists.cc:53: warning: deprecated conversion from string constant to 'char*' > dw_table_aligned.cc: In function 'int main(int, char**)': > dw_table_aligned.cc:56: warning: deprecated conversion from string constant to > 'char*' > dw_table.cc: In function 'int main(int, char**)': > dw_table.cc:61: warning: deprecated conversion from string constant to 'char*' > dw_border_test.cc: In function 'int main(int, char**)': > dw_border_test.cc:57: warning: deprecated conversion from string constant to > 'char*' > dw_resource_test.cc: In function 'int main(int, char**)': > dw_resource_test.cc:54: warning: deprecated conversion from string constant to > 'char*' > dw_ui_test.cc: In function 'int main(int, char**)': > dw_ui_test.cc:60: warning: deprecated conversion from string constant to > 'char*' > make[2]: Leaving directory `/home/jcid/C/Dillo/d2/dillo-wc/test' > > when compiled with: > CFLAGS="-g -O0 -W" CXXFLAGS="-g -O0 -W" ./configure --enable-ssl > > Please update test/ too. > > -- > Cheers > Jorge.- > > ______________________________________________________________________ > This email has been scanned by the MessageLabs Email Security System. > For more information please visit http://www.messagelabs.com/email > ______________________________________________________________________ > > _______________________________________________ > Dillo-dev mailing list > Dillo-dev(a)dillo.org > http://lists.auriga.wearlab.de/cgi-bin/mailman/listinfo/dillo-dev

Jan. 29, 2009

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

by onepoint＠starurchin.org

On Thu, Jan 29, 2009 at 01:25:29PM +0100, Hofmann Johannes wrote: > Can you please retry with the user-agent style parsing disabled: > > diff -r 4b5125e3ca89 src/css.cc > --- a/src/css.cc Thu Jan 29 12:38:20 2009 +0100 > +++ b/src/css.cc Thu Jan 29 13:24:16 2009 +0100 > @@ -327,7 +327,7 @@ CssContext::CssContext () { > sheet[CSS_PRIMARY_USER] = userStyle; > sheet[CSS_PRIMARY_USER_IMPORTANT] = userImportantStyle; > > - buildUserAgentStyle (); > + //buildUserAgentStyle (); > buildUserStyle (); > } That takes out most of the "indirectly lost" memory but doesn't affect the "definitely lost" stuff that valgrind generates the suppression for. Log attached. Regards, Jeremy Henty

Jan. 29, 2009

dw::Image objects leak their mapKey

by jcid＠dillo.org

On Wed, Jan 28, 2009 at 09:25:02PM +0000, Jeremy Henty wrote: > 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. :-) Interesting. With multithreading, if mapKey was a shared resource, it could be a problem. In that case a mutex solves the problem, making the operation "atomic". AFAIS, with: > if (key != mapKey) { > object::Object *oldKey = mapKey; > mapKey = key; > if (oldKey) > delete oldKey; > } If oldKey was a shared resource, there's still the problem of being preempted in the middle of the destructor (delete oldKey). If another thread uses the object then, BUM! Again, a mutex solves the problem. The simpler version was committed. ;-) -- Cheers Jorge.- ______________________________________________________________________ This email has been scanned by the MessageLabs Email Security System. For more information please visit http://www.messagelabs.com/email ______________________________________________________________________

Jan. 29, 2009

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

by jcid＠dillo.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: > > 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. These patches produce lots of warnings in the test directory: make[2]: Entering directory `/home/jcid/C/Dillo/d2/dillo-wc/test' dw_anchors_test.cc: In function 'int main(int, char**)': dw_anchors_test.cc:120: warning: deprecated conversion from string constant to 'char*' dw_example.cc: In function 'int main(int, char**)': dw_example.cc:49: warning: deprecated conversion from string constant to 'char*' dw_find_test.cc: In function 'int main(int, char**)': dw_find_test.cc:93: warning: deprecated conversion from string constant to 'char*' dw_links.cc: In function 'int main(int, char**)': dw_links.cc:88: warning: deprecated conversion from string constant to 'char*' dw_links2.cc: In function 'int main(int, char**)': dw_links2.cc:116: warning: deprecated conversion from string constant to 'char*' dw_images_simple.cc: In function 'int main(int, char**)': dw_images_simple.cc:110: warning: deprecated conversion from string constant to 'char*' dw_images_scaled.cc: In function 'int main(int, char**)': dw_images_scaled.cc:112: warning: deprecated conversion from string constant to 'char*' dw_images_scaled2.cc: In function 'int main(int, char**)': dw_images_scaled2.cc:87: warning: deprecated conversion from string constant to 'char*' dw_lists.cc: In function 'int main(int, char**)': dw_lists.cc:53: warning: deprecated conversion from string constant to 'char*' dw_table_aligned.cc: In function 'int main(int, char**)': dw_table_aligned.cc:56: warning: deprecated conversion from string constant to 'char*' dw_table.cc: In function 'int main(int, char**)': dw_table.cc:61: warning: deprecated conversion from string constant to 'char*' dw_border_test.cc: In function 'int main(int, char**)': dw_border_test.cc:57: warning: deprecated conversion from string constant to 'char*' dw_resource_test.cc: In function 'int main(int, char**)': dw_resource_test.cc:54: warning: deprecated conversion from string constant to 'char*' dw_ui_test.cc: In function 'int main(int, char**)': dw_ui_test.cc:60: warning: deprecated conversion from string constant to 'char*' make[2]: Leaving directory `/home/jcid/C/Dillo/d2/dillo-wc/test' when compiled with: CFLAGS="-g -O0 -W" CXXFLAGS="-g -O0 -W" ./configure --enable-ssl Please update test/ too. -- Cheers Jorge.- ______________________________________________________________________ This email has been scanned by the MessageLabs Email Security System. For more information please visit http://www.messagelabs.com/email ______________________________________________________________________

Jan. 29, 2009

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

by Johannes.Hofmann＠gmx.de

On Thu, Jan 29, 2009 at 11:43:36AM +0000, Jeremy Henty wrote: > 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. Not sure about those. Maybe it's reporting the strings allocated for the user-agent style? Those are allocated once and never freed. Can you please retry with the user-agent style parsing disabled: diff -r 4b5125e3ca89 src/css.cc --- a/src/css.cc Thu Jan 29 12:38:20 2009 +0100 +++ b/src/css.cc Thu Jan 29 13:24:16 2009 +0100 @@ -327,7 +327,7 @@ CssContext::CssContext () { sheet[CSS_PRIMARY_USER] = userStyle; sheet[CSS_PRIMARY_USER_IMPORTANT] = userImportantStyle; - buildUserAgentStyle (); + //buildUserAgentStyle (); buildUserStyle (); } Cheers, Johannes

Jan. 29, 2009

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