Recent

Author Topic: The LCL isn't impressing me lately. LEAKS with Tbitmap and TCursorImage.  (Read 2360 times)

Martin_fr

  • Administrator
  • Hero Member
  • *
  • Posts: 5801
    • wiki
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]
  1. 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)