[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kfm-devel
Subject:    Re: FW: Bug#1812: KFM memory leak: updated patch for khtmlw.
From:       Lars Knoll <Lars.Knoll () mpi-hd ! mpg ! de>
Date:       1999-08-31 16:56:10
[Download RAW message or body]

Just to throw in my opinion once again...

On Tue, 31 Aug 1999, Bjarni R. Einarsson wrote:

> On Tue, Aug 31, 1999 at 05:53:30PM +0200, Waldo Bastian wrote:
> > On Tue, 31 Aug 1999, you wrote:
> > > Sorry if this doesn't thread correctly, I'm pasting from
> > > http://www.lists.kde.org/, since I'm not actually subscribed to kfm-devel
> > > (I'm just a power user, not a KDE developer). :-)
> > 
> > No problem. Do you want CC's?
> 
> Yes please.  That makes quoting much easier. :-)
> 
> I understand your resistance to my patch - it's a bit bigger than you'd
> like, right before release time, and you aren't sure what it is fixing.  
> You want to know which is doing what, I guess.  
> 
> So... my patch explained:
> 
> The patch implements the following changes.
> 
> 	1. pCache cleanup code
> 	2. cosmetics
> 	3. A GIF/Gif98a typo fixed	
> 	4. The movieCache one-liner.
> 	5. Minor paranoia.
> 
> 1. Consists of the following changes:
> 
>     a) A global definition (PCACHESIZE) replacing the constant 503.

Not really needed, but ok.

>     b) Use of pCache->setAutoDelete(TRUE) where appropriate.
>        A destructor for the HTMLCachedImage type, which deletes the pixmap.
>        Brute force cache-flushing, via pCache->count() and pCache->clear()

ok, except for the cache flushing, because of overlay images. I only found
it after another close look at the code. HTMLImage::setOverlay() 
only looks in the cache. If we clear the cache at a certain point,
overlays will get cleared too. But these pixmaps are needed and provide
important information to the user, which views a directory with kfm. 
See also my other mail.

>     c) The "cached" boolean variable in the HTMLImage type is ignored,
>        destroying the pixmap whether it is cached or not.  This is a good
>        thing, because the pixmap is already implicitly shared by Qt, and
>        /must/ always be deleted by this destructor, for b) to work.
>        This means the "cached" variable isn't really used.

OK too. I checked all assignments to the pixmaps once again.

>     d) I added checks of whether pixmap was actually unassigned before
>        calling pixmap = new QPixmap.

fine too. plugs a possible memory leak.
> 
> I think that if you're correct that the pCache is never used on web pages,
> that d) or c) might be the one-liner you are looking for.  

That might be, these pieces of code might introduce memory leaks.

> But.. I feel compelled to harp on this some more - how can you be sure that
> everybody uses cacheImage() the right way?  The changes in b) are very

I'm not sure, but kfm is the only program using it, and there are only 4
calls to cacheImage() in it.

> straightforward, and should cause no adverse effects other than a minor

See overlays...

> performance hit - if that.  cacheImage() is available to anyone using the
> HTML widget, so fixing this is protecting us from abuse by future KDE 1.1.x
> compatible programs which may not "do the right thing", no matter what kfm
> is doing today.

Yes and no... Since the call to clear might break overlays, I'm against
it.

> 
> 2. Is just in the way.	

cosmetics are not really needed, so I removed them from my version of the
patch.
> 
> 3. Makes handling of GIF images consistant throughout the program, only
>    implementing movie-related stuff on GIF89a images.

This is good. It's easy to verify this part is correct, it makes things
consistent (HTMLImage uses "GIF89a" also in one other place), reduces
memory usage and makes displaying of images faster.

> 4. The one-liner everyone seems happy with. :-)

:-)

> 5. I changed a "--refcount == 0" comparison to "--refcount <= 0", just
>    to be on the safe side.  It probably doesn't help, probably doesn't
>    matter, but it made me feel better.

No need for that. 

Bye,
Lars

[prev in list] [next in list] [prev in thread] [next in thread]