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

List:       kfm-devel
Subject:    KDE1.1.2/PATCH Was: Bug#1812: KFM memory leak: updated patch for khtmlw.
From:       Waldo Bastian <bastian () suse ! de>
Date:       1999-08-31 16:48:02
[Download RAW message or body]

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.  

Yes.

> 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.
> 
>     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()
> 
>     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.
> 
>     d) I added checks of whether pixmap was actually unassigned before
>        calling pixmap = new QPixmap.
> 
> 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.  

I just discovered it is c) :-)

> 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
> straightforward, and should cause no adverse effects other than a minor
> 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.
> 
> 2. Is just in the way.	
> 
> 3. Makes handling of GIF images consistant throughout the program, only
>    implementing movie-related stuff on GIF89a images.
>    
> 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.

Yes this does help.

I have attached a patch that consists of 1c) and 4.

If you can check that this patch gives the same test-results as patch7 then we 
should apply this.

That solves bug 1812 I think.

Then we can have a look at the pCache stuff. It would be nice if we could fix it, 
but it isn't very urgent. It is clear that it leaks but not very hard. I think it is worse
if we change something there and break something.

I have commited 5) since it doesn't hurt.

Is 3) related to a bug? 

Cheers,
Waldo



 

["kdelibs.khtmlw.31-08-99.diff2" (text/x-c++)]

Index: htmlobj.cpp
===================================================================
RCS file: /home/kde/kdelibs/khtmlw/Attic/htmlobj.cpp,v
retrieving revision 1.69
diff -u -r1.69 htmlobj.cpp
--- htmlobj.cpp	1999/01/18 10:57:54	1.69
+++ htmlobj.cpp	1999/08/31 16:57:17
@@ -1304,8 +1304,6 @@
     url = _url;
     target = _target;
     
-    cached = TRUE;
-
     predefinedWidth = ( _width < 0 && !_percent ) ? false : true;
     predefinedHeight = _height < 0 ? false : true;
 
@@ -1431,7 +1429,6 @@
     else
     {
       pixmap->load( u.path() );
-      cached = false;
     }
 
     bComplete = true;
@@ -1487,7 +1484,6 @@
   {
     pixmap = new QPixmap();
     pixmap->loadFromData( _buffer.buffer() );	    
-    cached = false;
 
     if ( pixmap == 0 || pixmap->isNull() )
       return false;
@@ -1556,7 +1552,6 @@
     {
 	pixmap = new QPixmap();
 	pixmap->load( _filename );	    
-	cached = false;
 
 	if ( pixmap == 0 || pixmap->isNull() )
 	    return;
@@ -1816,8 +1811,10 @@
     // if ( !imageURL.isEmpty() && !pixmap )
     // htmlWidget->cancelRequestFile( this );
 
-    if ( pixmap && !cached )
+    if ( pixmap )
 	delete pixmap;
+    if ( movieCache )
+        delete movieCache;
 #ifdef USE_QMOVIE
     if ( movie )
     {


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