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

[Martin_fr]

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.

[jamie]

Code: Pascal  [Select][+][-]

1.  
2.   if img^.Data = AImage.Data then begin
3.       img^.Data := nil;
4.       img^.DataSize := 0;
5.     end;
6.    if img^.Mask = AImage.Mask then begin
7.       img^.Mask := nil;
8.       img^.MaskSize := 0;
9.     end;
10.  

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..

[Martin_fr]

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

[BrunoK]

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.

[jamie]

martin_Fr

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

Code: Pascal  [Select][+][-]

1. procedure TRasterImage.LoadFromRawImage(const AImage: TRawImage; ADataOwner: Boolean);
2. 580 var
3. 581   img: PRawImage;
4. 582 begin
5. 583   BeginUpdate;
6. 584   try
7. 585     Clear;
8. 586     if AImage.Description.Format = ricfNone then Exit; // empty image
9. 587  
10. 588     img := GetRawImagePtr;
11. 589  
12. 590     // Make sure, we do not free AImage.Data or Mask
13. 591     if img^.Data = AImage.Data then begin
14. 592       img^.Data := nil;
15. 593       img^.DataSize := 0;
16. 594     end;
17. 595     if img^.Mask = AImage.Mask then begin
18. 596       img^.Mask := nil;
19. 597       img^.MaskSize := 0;
20. 598     end;
21. 599     img^.FreeData;
22. 600  
23. 601     img^.Description := AImage.Description;
24. 602     if ADataOwner
25. 603     then begin
26. 604       img^.DataSize := AImage.DataSize;
27. 605       img^.Data := AImage.Data;
28. 606       img^.MaskSize := AImage.MaskSize;
29. 607       img^.Mask := AImage.Mask;
30. 608       img^.PaletteSize := AImage.PaletteSize;
31.  
32.  
33.  

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][+][-]

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

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

[jamie]

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][+][-]

1. procedure TForm1.Button1Click(Sender: TObject);
2. var
3.   P:Pbyte;
4. begin
5.   P := GetMem(1000);
6.   Caption := IntPtr(P).tostring;  //just to make the compiler happy.
7.   P := Nil; // This does not RELEASE memory!
8.   //Turn on heap trace Please.
9.  
10. end;                                
11.  

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.

[Martin_fr]

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

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

Yes palette is needed too. Done.

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.

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.

[jamie]

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][+][-]

1. procedure TForm1.Button1Click(Sender: TObject);
2. var
3.   I:Tbitmap;
4. begin
5.   I := Tbitmap.Create;
6.   I.PixelFormat := Pf8bit;
7.   I.SetSize(100,100);
8.   I.LoadFromRawImage(I.RawImage, False);
9.   I.Free;
10. end;
11.  
12.  

Please test that with your code with heapTrc Please!

[Martin_fr]

As I said in my last post

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.

[jamie]

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.

[Martin_fr]

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.

[Martin_fr]

Btw, I have the strong feeling that your example

Code: Pascal  [Select][+][-]

1. 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][+][-]

1. SomeRawImage := GetRawImage;
2. I.LoadFromRawImage(SomeRawImage, False);
3. RawImage.ClearData;
4. I.Free;
5.  

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.

[Martin_fr]

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.

[jamie]

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.

[User137]

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][+][-]

1. img := GetRawImagePtr;
2.  
3. // Make sure, we do not free AImage.Data or Mask
4. 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][+][-]

1. img := @GetRawImage;

Even after that there may be much potential for cleaning.