Print Page - The LCL isn't impressing me lately. LEAKS with Tbitmap and TCursorImage.

Programming => LCL => Topic started by: jamie on May 10, 2019, 03:07:23 am

Title: The LCL isn't impressing me lately. LEAKS with Tbitmap and TCursorImage.
Post by: jamie on May 10, 2019, 03:07:23 am

I am using the Laz 2.0.2 now and TCursorImage was fixed partially however, Tbitmap still have major issues and I think it
has to do with the LoadFromRamImage method which is bugging a lot of my code!

I am getting frustrated and the work flow is diminishing greatly.

I short example below demonstrates my frustration with this..

Code: Pascal [Select][+]

procedure TForm1.Button5Click(Sender: TObject);
Var
  B:TbitMap;
  C:TCursorImage;
begin
  B := TBitMap.Create;
  C := TCursorImage.Create;
  B.SetSize(320,240);
  B.Canvas.CopyRect(Rect(0,0,319,239),Canvas,Rect(0,0,319,239));{Comment this line, leaks go away}
  C.Assign(B); {or comment this line, leaks go away}
  B.Free;
  C.Free;
end;                                                   
 

I am sure that TBitmap.LoadFromRamImage is part of the problem, a problem I report before 2.0.2 was released , it never
got fixed.

I can make the above TcursorImage code work if I get an image from a TimageList.GetBitMap(0, B); Some where along the
line things are differently handled but this should still work!
Can someone please test this on their 2.0.0 or 2.0.2 release for LEAKS!

Title: Re: The LCL isn't impressing me lately. LEAKS with Tbitmap and TCursorImage.
Post by: Zvoni on May 10, 2019, 09:08:06 am

Why do you use unqualified second Argument in CopyRect?
From: http://docs.embarcadero.com/products/rad_studio/delphiAndcpp2009/HelpUpdate2/EN/html/delphivclwin32/Graphics_TCanvas_CopyRect.html

Quote

Use CopyRect to transfer part of the image on another canvas to the image of the TCanvas object. Dest specifies the rectangle on the canvas where the source image will be copied. The Canvas parameter specifies the canvas with the source image. Source specifies a rectangle bounding the portion of the source canvas that will be copied.

As i understood it: Your Destination is B, your Source is C
I would have expected:

Code: [Select]

B.Canvas.CopyRect(Rect(0,0,319,239),C.Canvas,Rect(0,0,319,239));

Title: Re: The LCL isn't impressing me lately. LEAKS with Tbitmap and TCursorImage.
Post by: BrunoK on May 10, 2019, 11:58:59 am

It leaks also here, the leaked block is getmem'ed in C.Assign(B).

Here after a memory trail of allocations during Button1.Click. Semi-column text can be pasted to a spread sheet to better visualise the trail.

Code: [Select]

B := TBitMap.Create;;;;;getmem_incr;738
073068C4;TBitmap;getmem;size;72;getmem_incr;739
07306C04;TSharedBitmap;getmem;size;124;getmem_incr;740
C := TCursorImage.Create;;;;;getmem_incr;740
07306D0C;TCursorImage;getmem;size;72;getmem_incr;741
072FD4D4;TSharedCursorImage;getmem;size;24;getmem_incr;742
07306DDC;TFPList;getmem;size;16;getmem_incr;743
B.SetSize(320,240);;;;;getmem_incr;743
B.Canvas.CopyRect(...);;;;;getmem_incr;743
07306E74;TBitmapCanvas;getmem;size;172;getmem_incr;744
07306FAC;TList;getmem;size;16;getmem_incr;745
07307044;TFont;getmem;size;108;getmem_incr;746
0730713C;memory;getmem;size;32;getmem_incr;747
073071E4;memory;getmem;size;16;getmem_incr;748
0730727C;TPen;getmem;size;92;getmem_incr;749
07307364;TBrush;getmem;size;204;getmem_incr;750
073074BC;TRegion;getmem;size;32;getmem_incr;751
09BD00B4;memory;getmem;size;230400;getmem_incr;752
07307564;memory;getmem;size;9600;getmem_incr;753
09BD00B4;memory;freemem;size;230400;getmem_incr;752
07307564;memory;freemem;size;9600;getmem_incr;753
C.Assign(B);;;;;getmem_incr;753
07307564;TCursorImageImage;getmem;size;128;getmem_incr;754
0730766C;memory;getmem;size;16;getmem_incr;755
09BD00B4;memory;getmem;size;230400;getmem_incr;756
07307704;memory;getmem;size;9600;getmem_incr;757
07309D0C;memory;getmem;size;9600;getmem_incr;758
07307704;memory;freemem;size;9600;getmem_incr;757
09C0853C;TCursorImageImage;getmem;size;128;getmem_incr;759
09C08644;memory;getmem;size;230400;getmem_incr;760
07307704;memory;getmem;size;9600;getmem_incr;761
0730C314;memory;getmem;size;9600;getmem_incr;762
07307704;memory;freemem;size;9600;getmem_incr;761
07307704;memory;getmem;size;9600;getmem_incr;763
0730E91C;memory;getmem;size;9600;getmem_incr;764
07307704;memory;freemem;size;9600;getmem_incr;763
07307704;memory;getmem;size;9600;getmem_incr;765
0730C314;memory;freemem;size;9600;getmem_incr;762
07307564;TCursorImageImage;freemem;size;128;getmem_incr;754
B.Free;;;;;getmem_incr;765
09BD00B4;memory;freemem;size;230400;getmem_incr;756
07309D0C;memory;freemem;size;9600;getmem_incr;758
07306C04;TSharedBitmap;freemem;size;124;getmem_incr;740
073074BC;TRegion;freemem;size;32;getmem_incr;751
073071E4;memory;freemem;size;16;getmem_incr;748
0730713C;memory;freemem;size;32;getmem_incr;747
07307044;TFont;freemem;size;108;getmem_incr;746
07307364;TBrush;freemem;size;204;getmem_incr;750
0730727C;TPen;freemem;size;92;getmem_incr;749
07306FAC;TList;freemem;size;16;getmem_incr;745
07306E74;TBitmapCanvas;freemem;size;172;getmem_incr;744
073068C4;TBitmap;freemem;size;72;getmem_incr;739
C.Free;;;;;getmem_incr;765
09C08644;memory;freemem;size;230400;getmem_incr;760
07307704;memory;freemem;size;9600;getmem_incr;765
09C0853C;TCursorImageImage;freemem;size;128;getmem_incr;759
0730766C;memory;freemem;size;16;getmem_incr;755
07306DDC;TFPList;freemem;size;16;getmem_incr;743
072FD4D4;TSharedCursorImage;freemem;size;24;getmem_incr;742
07306D0C;TCursorImage;freemem;size;72;getmem_incr;741
Heap dump by heaptrc unit
Max trace depth : 16
775 memory blocks allocated : 2309708/2310520
774 memory blocks freed     : 2300108/2300920
1 unfreed memory blocks : 9600
Call trace for block $0730E91C size 9600  getmem_incr=764
  $00410A26 line 280 of ../inc/heap.inc
  $00459183 line 306 of graphtype.pp
  $0054356A line 1322 of win32/win32proc.pp
  $004DF582 line 1064 of win32/win32lclintf.inc
  $004AA150 line 172 of include/lclintf.inc
  $004A2B68 line 444 of include/icon.inc
  $004A28CF line 383 of include/icon.inc
  $0042BB17 line 71 of frmdebugleaks_1.pas
  $004F8128 line 2995 of include/control.inc
  $0050EB6D line 55 of include/buttoncontrol.inc
  $0050F18D line 169 of include/buttons.inc
  $0050EA99 line 21 of include/buttoncontrol.inc
  $004F3652 line 78 of include/control.inc
  $004F71B0 line 2313 of include/control.inc
  $004ED9F1 line 5467 of include/wincontrol.inc
  $005505D9 line 112 of lclmessageglue.pas

line 306 of graphtype.pp is in CopyImageData

Title: Re: The LCL isn't impressing me lately. LEAKS with Tbitmap and TCursorImage.
Post by: Martin_fr on May 10, 2019, 04:08:54 pm

Bug report please...

Title: Re: The LCL isn't impressing me lately. LEAKS with Tbitmap and TCursorImage.
Post by: jamie on May 11, 2019, 12:55:41 am

I've done some tinkering and the TBitmap.LoadFromRamImage leak does not seem to be directly related to to the TCursorImage
leak because when I step that code LoadFromRAwImage does not get called however, I did find something in this
Tbitmap code that could be related and before I submit a patch suggestion I would like to first review it here.

Code: Pascal [Select][+]

procedure TBitmap.LoadFromRawImage(const AImage: TRawImage; ADataOwner: Boolean);
var
  img: PRawImage;
begin
  BeginUpdate;
  try
    Clear;  // Note from Jamie. This does not call RawImage.FreeData ?
 
    if AImage.Description.Format = ricfNone then Exit; // empty image
 
    img := GetRawImagePtr;
 { + }   img^.FreeData;   //Added to prevent leaks. Note From Jamie, Calling this corrects the issue.
    img^.Description := AImage.Description;
    if ADataOwner
    then begin
      img^.DataSize := AImage.DataSize;
      img^.Data := AImage.Data;
      img^.MaskSize := AImage.MaskSize;
      img^.Mask := AImage.Mask;
      img^.PaletteSize := AImage.PaletteSize;
      img^.Palette := AImage.Palette;
    end
    else begin
      // copy needed
      img^.DataSize := AImage.DataSize;
      if img^.DataSize > 0
      then begin
        GetMem(img^.Data, img^.DataSize);
        Move(AImage.Data^, img^.Data^, img^.DataSize);
      end
      else img^.Data := nil;
 
      img^.MaskSize := AImage.MaskSize;
      if img^.MaskSize > 0
      then begin
        GetMem(img^.Mask, img^.MaskSize);
        Move(AImage.Mask^, img^.Mask^, img^.MaskSize);
      end
      else img^.Mask := nil;
 
      img^.PaletteSize := AImage.PaletteSize;
      if img^.PaletteSize > 0
      then begin
        GetMem(img^.Palette, img^.PaletteSize);
        Move(AImage.Palette^, img^.Palette^, img^.PaletteSize);
      end
      else img^.Palette := nil;
    end;
  finally
    EndUpdate;
  end;
 
End;
 
 

As you can see with the NOTES from JAMIE, I inserted the Img^.Freedata to correct this issue..

the use of "Clear" however in my mind should be doing this don't you think ?

"Clear" is used in many places to clean up an existing image, and since TCursorImage also hits on this, I was thinking that
maybe correcting the "Clear" override code to free the raw image data may correct this and the other?

So before I submit the Tbitmap.LoadFromRawImage issue, I would like some opinions on if we should actually fix this
in the "Clear"

Title: Re: The LCL isn't impressing me lately. LEAKS with Tbitmap and TCursorImage.
Post by: BrunoK on May 11, 2019, 10:40:28 am

Seems it was already discussed in your message https://forum.lazarus.freepascal.org/index.php/topic,45016.msg317235.html#msg317235

Title: Re: The LCL isn't impressing me lately. LEAKS with Tbitmap and TCursorImage.
Post by: Martin_fr on May 11, 2019, 01:23:33 pm

Created a report https://bugs.freepascal.org/view.php?id=35562

I got a fix already, but I need the report.

In future please create reports (and see my notes on the other thread).

Bugreports (with sample code to reproduce the issue, if available) are important. They will later act as a reference for the fix. So it is possible to check why the code was changed/fixed.

The other issue is not related

Title: Re: The LCL isn't impressing me lately. LEAKS with Tbitmap and TCursorImage.
Post by: Martin_fr on May 11, 2019, 01:36:50 pm

Fix:
https://svn.freepascal.org/cgi-bin/viewvc.cgi?view=revision&root=lazarus&revision=61200
https://svn.freepascal.org/cgi-bin/viewvc.cgi?view=revision&root=lazarus&revision=61201

Title: Re: The LCL isn't impressing me lately. LEAKS with Tbitmap and TCursorImage.
Post by: jamie on May 11, 2019, 05:50:00 pm

I apologize for the confusing or mixing things.

I like your resolution on the loadfromRawImage fix, but did you test the code I presented here at the start of the
this threat to see if it resolved the TCursorImage leak ?

I am not setup with the SNV here, maybe I should be doing this to present patches.

Title: Re: The LCL isn't impressing me lately. LEAKS with Tbitmap and TCursorImage.
Post by: Martin_fr on May 11, 2019, 06:16:43 pm

Both the code samples (from this thread, and from the other) did no longer leak on my setup (Though I test against trunk, it should be the same with fixes 2.0.x).

If you follow the links, you can see the changes I made in each files.
You can simple edit the files of your Lazarus installation and make the same changes. The changes are only a few lines.
Then rebuild your Lazarus (Tools menu). No need for svn.

Title: Re: The LCL isn't impressing me lately. LEAKS with Tbitmap and TCursorImage.
Post by: jamie on May 11, 2019, 06:47:32 pm

Ok, I didn't know I was able to recompile my own install.

I wasn't aware that making changes to the current Graphics file would be allowed..

I'll experiment with that and see how that works out,. thanks

Title: Re: The LCL isn't impressing me lately. LEAKS with Tbitmap and TCursorImage.
Post by: Martin_fr on May 11, 2019, 06:58:46 pm

If you only want it for your app, you do not even need to rebuild the IDE.

Just make the changes, and run your app.

Title: Re: The LCL isn't impressing me lately. LEAKS with Tbitmap and TCursorImage.
Post by: jamie on May 11, 2019, 09:09:25 pm

I made the changes and it works :D

But I used my method of image duplication because testing the address of img and Aimage does not seem to
ever match if I pass the same rawimage to it. I instead compared the DATA pointers of each and that works great.

As I noted In the other thread..

Thanks.

Title: Re: The LCL isn't impressing me lately. LEAKS with Tbitmap and TCursorImage.
Post by: Martin_fr on May 11, 2019, 09:27:36 pm

Penny dropped...

It is an old style object, not a class instance. So yes.

---
Need to compare Date and Mask (individually)
This should do:

Code: Pascal [Select][+]

    if img^.Data = AImage.Data then begin
      img^.Data := nil;
      img^.DataSize := 0;
    end;
    if img^.Mask = AImage.Mask then begin
      img^.Mask := nil;
      img^.MaskSize := 0;
    end;
 

If either is equal to the source, then prevent it from being freed.

Thanks for spotting this.

Title: Re: The LCL isn't impressing me lately. LEAKS with Tbitmap and TCursorImage.
Post by: jamie on May 11, 2019, 11:24:07 pm

I don't know what Is happening to the post here, I just wrote a long post and it never made it to the server..

Please don't do that!!!!!!!!!!!!!!!!!!!!!!!!!!

We must have a language barrier problem here because you are not understanding the issue...

the line
if Img = @IAmage then exit; // Never works over here.

Try this.

MyBitmap.LoadFromRawImage(MyBitmap, False);

you see that above ^^^^^^^^^^^^^^^^^^^

Now break point the LoadFromRawImage code in the INC file and walk throught it.

when you get to the duplication check point, the two do not match but they are the same!!!!!!!!!

If Img = @Aimage then Exit never is True over here...

The two addresses are never the same!!!!!!!!!

Maybe we are not using the same compiler? I am using 3.0.4

All you need to do is fix the Duplcation check point via the DATA field not the Address of the structure.

Again, the ADdresses over here NEVER match each other. I can't say why but that is the case here.

I made the changes and rebuilt the IDE with how I explained it to you, it works.

Title: Re: The LCL isn't impressing me lately. LEAKS with Tbitmap and TCursorImage.
Post by: Martin_fr on May 11, 2019, 11:56:26 pm

Yes " if Img = @IAmage then exit; " does NOT work. As you say.

I did not realize that this was not a class, but a TP-object (object like record).

So I did change the code. See my last post.
It now compares the data and the mask.

Title: Re: The LCL isn't impressing me lately. LEAKS with Tbitmap and TCursorImage.
Post by: jamie on May 12, 2019, 03:49:46 am

Code: Pascal [Select][+]

 
  if img^.Data = AImage.Data then begin
      img^.Data := nil;
      img^.DataSize := 0;
    end;
   if img^.Mask = AImage.Mask then begin
      img^.Mask := nil;
      img^.MaskSize := 0;
    end; 
 

I assume you first did this.
img^.FreeData ?

FreeData already clears those fields.

Please point to the Source so I can see what you did..

Title: Re: The LCL isn't impressing me lately. LEAKS with Tbitmap and TCursorImage.
Post by: Martin_fr on May 12, 2019, 11:25:55 am

You can browse the log of changes
https://svn.freepascal.org/cgi-bin/viewvc.cgi/trunk/?view=log&root=lazarus

And this is the one in question:
https://svn.freepascal.org/cgi-bin/viewvc.cgi?view=revision&root=lazarus&revision=61205

And yes, it is before FreeData, in order to prevent FreeData

Title: Re: The LCL isn't impressing me lately. LEAKS with Tbitmap and TCursorImage.
Post by: BrunoK on May 12, 2019, 02:34:05 pm

In 61205 (lines 590 - 599), shouldn't the palette also be treated like Data and Mask.

This procedure TRasterImage.LoadFromRawImage is called in Lazarus only once by \components\ideintf\imagelisteditor.pp in function CreateGlyphSplit (which itself is never called) so it is difficult to know, for example, what ADataOwner is supposed to mean.

That was the reason I suggested in https://forum.lazarus.freepascal.org/index.php/topic,45016.msg317235.html#msg317235 April 11, 2019, 03:43:45 pm a Patch using ReallocMem only when ADataOwner=False.

Title: Re: The LCL isn't impressing me lately. LEAKS with Tbitmap and TCursorImage.
Post by: jamie on May 12, 2019, 02:59:29 pm

martin_Fr

Please changed that, You have actually erased the pointers before calling the FreeData;

Code: Pascal [Select][+]

procedure TRasterImage.LoadFromRawImage(const AImage: TRawImage; ADataOwner: Boolean); 
var 
  img: PRawImage; 
begin 
  BeginUpdate; 
  try 
    Clear; 
    if AImage.Description.Format = ricfNone then Exit; // empty image 
 
    img := GetRawImagePtr; 
 
    // Make sure, we do not free AImage.Data or Mask 
    if img^.Data = AImage.Data then begin 
      img^.Data := nil; 
      img^.DataSize := 0; 
    end; 
    if img^.Mask = AImage.Mask then begin 
      img^.Mask := nil; 
      img^.MaskSize := 0; 
    end; 
    img^.FreeData; 
 
    img^.Description := AImage.Description; 
    if ADataOwner 
    then begin 
      img^.DataSize := AImage.DataSize; 
      img^.Data := AImage.Data; 
      img^.MaskSize := AImage.MaskSize; 
      img^.Mask := AImage.Mask; 
      img^.PaletteSize := AImage.PaletteSize; 
 
 
 

You just basically put it back to where it was...!
You need to clean up the existing img^.... You need to call FreeMem on those pointers.

DO NOT nil the pointers and then call Img^.FreeData because it will never free it!

Look, all you need is this..

Code: Pascal [Select][+]

if Img^.Data = Aimage.Data then Exit;
Img^.FreeData;  // Must free the current object because we are overwriting it and can't have leaks!
 

I will post exactly what I have done here which works like it should be if you'd like?

Title: Re: The LCL isn't impressing me lately. LEAKS with Tbitmap and TCursorImage.
Post by: jamie on May 12, 2019, 03:14:56 pm

For those that don't know any better;
Here is a simple demo of what happens when you NIL a pointer before using FREEMEM!

Code: Pascal [Select][+]

procedure TForm1.Button1Click(Sender: TObject);
var
  P:Pbyte;
begin
  P := GetMem(1000);
  Caption := IntPtr(P).tostring;  //just to make the compiler happy.
  P := Nil; // This does not RELEASE memory!
  //Turn on heap trace Please.
 
end;                                
 

If you don't correct what you did, then it negates the whole bug report, fix and we are back to square one!

What you did was simply erase both source and destination pointers so FReeData will never work!

This will only happen if a user passes the same RawImage but what you did was in fact corrupt the rawimage and
caused a memory leak when this takes place!
all you want to do is simply EXIT when the two Data pointers are the same..

The Mask Pointer does not matter because not all images have mask so you only need to test for the DATA pointer because they
all have it and you don't want to be nilling out the pointers and leaving it dry. It should simply EXIT so to leave the
duplicating RawImage still living and accessible to the code that presented it!

if the two are not the same then you call the img^.FreeData and continue on with the code.

Title: Re: The LCL isn't impressing me lately. LEAKS with Tbitmap and TCursorImage.
Post by: Martin_fr on May 12, 2019, 04:41:41 pm

That happens, when one tries to squeeze in a "simple looking" bug fix....

Quote from: BrunoK on May 12, 2019, 02:34:05 pm

In 61205 (lines 590 - 599), shouldn't the palette also be treated like Data and Mask.

Yes palette is needed too. Done.

Quote

This procedure TRasterImage.LoadFromRawImage is called in Lazarus only once by \components\ideintf\imagelisteditor.pp in function CreateGlyphSplit (which itself is never called) so it is difficult to know, for example, what ADataOwner is supposed to mean.

That was the reason I suggested in https://forum.lazarus.freepascal.org/index.php/topic,45016.msg317235.html#msg317235 April 11, 2019, 03:43:45 pm a Patch using ReallocMem only when ADataOwner=False.

I am not sure about ADataOwner neither. You can suggest additional patches on the bugtracker. Maybe someone who knows the code better will pick it up.

Quote

Please changed that, You have actually erased the pointers before calling the FreeData;

You just basically put it back to where it was...!
You need to clean up the existing img^.... You need to call FreeMem on those pointers.

DO NOT nil the pointers and then call Img^.FreeData because it will never free it!

Setting the pointers to NIL before FreeData is full intention.

Normally the target (img^) has probably either no data/mask/palette or different data/mask/palette.

- If it has none, then nothing needs to be done, and the nil setting does not matter.

- if it has different, then the conditions are false, and there is no setting to nil.

=> So those two cases are fine.

- If either one, two or all of data/mask/palette are the same as the source on AImage:

Then calling FreeData *before* setting to nil, would be a problem.

If "img^.Data = AImage.Data" and I call img^.FreeData, then I release the memory.
After that Image.Data is a dangling pointer, and the memory that it points to, can contain any random trash.
Assigning that dangling pointer to img^.Data makes it worse. Copying the trashed content of this memory block to newly allocated memory is not better either.
And worst case the memory was returned to the OS, and it causes a SigSegV.

So if "img^.Data = AImage.Data" and I call img^.FreeData, then I *prevent* that Data from being freed.
I may still have to free the img^.Mask, because for the same Data, there could be a different Mask.

From review of the code:

* With DataOwner = False

The OLD code would always assign copied memory to img^.
The OLD code would leak memory, in some cases.

The NEW code will always assign copied memory to img^.
The NEW code will free the old memory af the target img^, *if* that memory would otherwise be leaked (have no (known) reference left)

* With DataOwner = True

Same as above, but instead of copies, a pointer to existing memory will be made.

DataOwner = True
Means that the memory of the source AImage.Date/Mask/Palette must not (is not allowed to) be freed, as long as the target "img^" is in use.
At the end, only one of the 2 is allowed to free the memory.

Title: Re: The LCL isn't impressing me lately. LEAKS with Tbitmap and TCursorImage.
Post by: jamie on May 12, 2019, 05:55:57 pm

Look, you have 3 pointers here..

DATA
MASK
PALETTE

FreeData clears all of that.

currently you are Niling the Data and MASK but you still call FreeData which just does the same thing but it also
frees the Palette.
So there is no dangling pointers however, you then follow through with the code which does nothing but copy the
same image back to those pointers!

But because you didn't NIL the palette too, FreeData does that and when you copy the image back over which is/was the
same image to start with, the Palette pointer is no longer! because the pointer is now NIL! due to what FreeData did

And even if you were to clear the Palette pointer so FreeData won't, you still end up with a Performance hit because you are
just copying back the original data!

You need to test these fields for being equal and if so, simply EXIT, please don't slow down the app!. We need all the
speed we can get!

Code: Pascal [Select][+]

procedure TForm1.Button1Click(Sender: TObject);
var
  I:Tbitmap;
begin
  I := Tbitmap.Create;
  I.PixelFormat := Pf8bit;
  I.SetSize(100,100);
  I.LoadFromRawImage(I.RawImage, False);
  I.Free;
end;
 
 

Please test that with your code with heapTrc Please!

Title: Re: The LCL isn't impressing me lately. LEAKS with Tbitmap and TCursorImage.
Post by: Martin_fr on May 12, 2019, 06:39:07 pm

As I said in my last post

Quote from: Martin_fr on May 12, 2019, 04:41:41 pm

Quote from: BrunoK on May 12, 2019, 02:34:05 pm
In 61205 (lines 590 - 599), shouldn't the palette also be treated like Data and Mask.
Yes palette is needed too. Done.

I tried your code. With heaptrc. No leak.

I also explained the dangling pointer

If "img^.Data = AImage.Data"
=> that is, if both Data pointers refer to the same memory
=> then that memory has only been allocated once (but is used/referred by both)
=> then if I call "img^.FreeData" without "img^.DAta:=nil"
=> the memory will be freed.

Again "AImage.Data" points to the same memory. The memory that now would be freed.
So "AImage.Data" at that moment would be a dangling pointer.

Title: Re: The LCL isn't impressing me lately. LEAKS with Tbitmap and TCursorImage.
Post by: jamie on May 12, 2019, 07:41:44 pm

the example I gave you for some reason has the Img^,Data already at nil although I gave it a copy of itself?

so your code side effects that I am seeing and you are not, are not showing at this time but in a larger project I can
assure you that the Img^.data isn't always nil

Lets say they are the same for the moment..

You Nil The pointers so that FreeData will not release the memory. that's fine. Now the memory that is the same
in the Aimage is still available and will be copied over..

However, you keep overlooking the obvious here.

Down below in the code you will see the use of GETMEM! That is now creating another chunk of memory for all three pointers.

Since you didn't release any memory by setting the pointers to NIL, you have now recreated the memory
leak this all started from.

This only happens if both the img and Amage hold the same pointers, currently with simple do nothing project it appears that
img has NIL pointers already so you are not seeing the effects I am.

Its a rare case where this is going to happen, having both the img and Aimage holding the same image. It really shouldn't happen
but in the case where it does a simple EXIT is warranted and no harm and no fuss.

As I said..

if Img^.Data = Aimage.Data then Exit;

Img^.Freedata;
// and the rest of the code as is.

Do what you want, I'll just fix it here when the nest release comes out so I don't get side effects and leaks when the
next release comes out.

I think I'll make a Patch folder for this operation.

Title: Re: The LCL isn't impressing me lately. LEAKS with Tbitmap and TCursorImage.
Post by: Martin_fr on May 12, 2019, 08:04:25 pm

Quote from: jamie on May 12, 2019, 07:41:44 pm

Since you didn't release any memory by setting the pointers to NIL, you have now recreated the memory
leak this all started from.

This only happens if both the img and Amage hold the same pointers, currently with simple do nothing project it appears that
img has NIL pointers already so you are not seeing the effects I am.

Yes, and if they hold the same pointer, the old memory is still owned by AImage. And it will at some point be freed by AImage.
Hence, no leak.

If the memory is not copied and duplicated, then AImage and the resulting img^ will at some later point both free it. But they would free the same data. And a double free is a critical error. Usually leading to a crash. (Or undefined/random behaviour).

If DataOwner is not set, then the final img^ is obviously NOT allowed to end up with the same pointer as AImage. So a copy is needed.

Title: Re: The LCL isn't impressing me lately. LEAKS with Tbitmap and TCursorImage.
Post by: Martin_fr on May 12, 2019, 08:15:42 pm

Btw, I have the strong feeling that your example

Code: Pascal [Select][+]

I.LoadFromRawImage(I.RawImage, False);

Should have ADataOwner = True.

The image is loaded from the data owned by I.

And on review: Yes your example should leak. (Not sure why it does not).
But that is the think, it *should* leak. Because you are calling it with False, where it should (in all probability) be True for ADataOwner.

Your suggestion would change that. But it would introduce other issues. Such as double frees.

Lets modify your example

Code: Pascal [Select][+]

SomeRawImage := GetRawImage;
I.LoadFromRawImage(SomeRawImage, False);
RawImage.ClearData;
I.Free;
 

If in that case "I" already has the same Data, then I.Free will crash. Because it tries to free the data that was freed a line before.

The Problem is the method is not documented. And without that there is no way of fixing it. Because it can always be called with params, that either will leak or fail otherwise.

Title: Re: The LCL isn't impressing me lately. LEAKS with Tbitmap and TCursorImage.
Post by: Martin_fr on May 12, 2019, 08:35:54 pm

Now we have 2 examples.

- Yours leaks with my current fix.
- Mine works, but would crash with your solution.

Lets go back to what we had at the start.

We had a leak, in a different situation.
We had a leak, if Self.GetRawImagePtr^.Data (or mask or palette) already had data.

We wanted to fix that leak, without changing any other behaviour.

I think my fix does that. Because for I.LoadFromRawImage(SomeRawImage, False); the old code would have created a copy.
The new one still does.

As my example shows, there is a use-case for that behaviour (limited or not, not more limited than you leak example).

From that follows that the fix for the ORIGINAL leak, is correct.

As for the leak in your last example. This could be
- your code is wrong. It calls with wrong params
- there is an unfixed 2nd leak in the LoadFromRawImage

It depends on what the original intend (for which we lack the docs) of the code was.

If we assume (and I do) that the implementation of that code (before my changes) matched the indent for which it was written, then the mem-copy is correct.

I looked at the history. The code was written (and not changed since)
Revision: 18328 Date: 18 January 2009 16:47:42
Message: * Added loading form rawimage

So there is no clue towards any answer.
And I doubt anyone will remember.

Btw, the reason your example did not leak may be (but I have not fully traced it) that LoadFromRawImage starts with: Self.Clear;
Though that cannot always be clearing the image, or we would never have seen the initial leak.

Title: Re: The LCL isn't impressing me lately. LEAKS with Tbitmap and TCursorImage.
Post by: jamie on May 12, 2019, 11:26:38 pm

the GetRawimagePtr is not always returning ready made image, It depends on what is happening in code and also I never
suggested putting in a check point for duplicates, just calling FreeData was enough. That was your idea but your approach was
failing and so I suggested using the data pointers to compare with which work great for me over here but you kind of went off
on the deep end and blew it out of proportion..

I can say for sure If two images using the same pointers come in it will generate a LEAK as you have it now, because the
following code in the block is recreating the memory, after that you now have a leak, again.

Don't worry about it Martin, if you think that is how it should be so be it, I know how to fix it over here, I'll do so on next
release. Currently its working great with the more simplistic way I did it.

Thanks for the afford.

Title: Re: The LCL isn't impressing me lately. LEAKS with Tbitmap and TCursorImage.
Post by: User137 on May 13, 2019, 03:25:36 am

So what i can tell from... is this always final version?
https://svn.freepascal.org/cgi-bin/viewvc.cgi/trunk/lcl/include/rasterimage.inc?view=markup&root=lazarus&pathrev=61205

Code: Pascal [Select][+]

img := GetRawImagePtr;
 
// Make sure, we do not free AImage.Data or Mask
if img^.Data = AImage.Data then begin

GetRawImagePtr can return a nil. Therefore if it's not checked, asking for img^.Data can be invalid (and even worse for all the other code that follows). It is always nil tested elsewhere in the code.

You propably want to change it to this one, as it will init the variables

Code: Pascal [Select][+]

img := @GetRawImage;

Even after that there may be much potential for cleaning.

Title: Re: The LCL isn't impressing me lately. LEAKS with Tbitmap and TCursorImage.
Post by: Martin_fr on May 13, 2019, 02:50:54 pm

Quote from: User137 on May 13, 2019, 03:25:36 am

So what i can tell from... is this always final version?
https://svn.freepascal.org/cgi-bin/viewvc.cgi/trunk/lcl/include/rasterimage.inc?view=markup&root=lazarus&pathrev=61205

Actually 61210

Within this forum thread, actually the thread to which this issue belongs is: https://forum.lazarus.freepascal.org/index.php/topic,45016.0.html
But the 2 threads got mixed up.
So within those threads, several issues were brought forward.

As far as the issue described by the example at the very start of https://forum.lazarus.freepascal.org/index.php/topic,45016.0.html goes: This is the fix.
- It fixes that memory leak
- It does not change any other behaviour.

The latter is to the best of my knowledge. If you have an example that shows that other(1) behaviour was altered(2), then let me know.

(1) functionality that did/does not rely upon the leak)
(2) changed between before my patch and after my patch. Independent of it being correct or wrong behaviour.

If I preserved/kept another pre-existing bug, then that does not affect this particular fix.

There are other things in that procedure that are unclear. They can be considered right or wrong. Depending on how they are considered more fixes may be needed. (But that is nothing to do with above leak).

I am not the maintainer of this code. Therefore I only got into that leak, as it could be done without changing anything else.
Admittedly I managed to overlook quite a few things initially. (I meant to spend 5 minutes on it, and return to my other stuff. But due to my initial oversights... my fault that it took longer)

On those other issues, I have expressed my opinion (as to whether I think they are intended or buggy). This is my personal opinion. If anyone from the team, more familiar with this code, believes or knows otherwise, then that is still open to change.

Quote

Code: Pascal [Select][+][-]
img := GetRawImagePtr;
GetRawImagePtr can return a nil. Therefore if it's not checked, asking for img^.Data can be invalid (and even worse for all the other code that follows). It is always nil tested elsewhere in the code.

Ok, that is a bug. The problem existed before the leak fix. So it is unrelated.

It will be ok to get fixed in LoadFromRawImage.

Though it may be that the bug is in GetRawImagePointer. It is not clear if this should always return none nil.
- TBitmap.GetRawImagePtr seems to be intended to be always none nil (from a cursory look)
- TCustomIcon.GetRawImagePtr indeed returns nil

However please report this on the bugtracker.

Someone needs to decide if the rawimage should be forcefully created. After all, the caller expects (at least I assume he does) an image to exists after the call. So just aborting would be equally wrong.

If no-one picks it up, then ping me in a few weeks (with a link to this post of mine), and I will put in an
if img = nil then raise Exception.Create('unimplemented - rawimage is missing');
That way, at least there is a controlled error. (And it should be clear, that the behaviour is still subject to unannounced change)

I will return to the other stuff I was doing.
That is unless you can show that my changes introduce none necessary behavioural changes compared to the old code. (And I mean that in the strictest way possible)