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
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
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
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
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
participants (2)
-
Johannes.Hofmann＠gmx.de -
onepoint＠starurchin.org