setshape memory leak
lazarus / 39-Graphics

----------------------------------------------------------------------
djdjdjole · 2017-12-30 09:45

My application relies on frequent call to TWinControl.setShape method.
After some time, application reports exception and call Stack points
to that method. To be sure, I compiled Lazarus project
(../examples/shapedcontrols), which I changed a little bit. Instead of
a single call to setShape I inserted "for" loop to call it 100000
times. After some time I received the same error as in my original
application. Specifically- Lazarus debugger breaks at BitmapToRegion
method in win32proc unit, after calling GlobalAlloc (which expresses
not enough memory).
Windows10, Lazarus 1.6.

Thanks

----------------------------------------------------------------------
Thaddy · 2017-12-30 12:53

Simple and compilable example code plz? We will fix such things in
minutes.

But let me guess: your "changed a little bit.." uses the same
variable....10.000 times?, then you are creating 9999 leaks!...Because
you did not free them in between. The alternative is to use 10.000
different instances and properly free them.

----------------------------------------------------------------------
djdjdjole · 2017-12-30 15:18

I am thankful to my friend, who found a bug in BitmapToRegion
function. What should be added to the end of it is :
GlobalUnlock(hData);
GlobalFree(hData);

Thanks

----------------------------------------------------------------------
avra · 2018-01-01 10:44

We are still waiting for a simple compilable example that could help
us to determine if that is really a memory leak that should be fixed,
or misuse on your side.

----------------------------------------------------------------------
Thaddy · 2018-01-01 11:32

Quote from: djdjdjole on December 30, 2017, 04:18:30 pmI am thankful
to my friend, who found a bug in BitmapToRegion function. What should
be added to the end of it is :
GlobalUnlock(hData);
GlobalFree(hData);

Thanks

That can only be the case if you added a GlobalLock yourself... which
is YOUR bug....Unless you can prove that the lock is called at least
twice and te release is only once. Which I can't....I can only
disprove the issue under normal behavior.

----------------------------------------------------------------------
djdjdjole · 2018-01-01 11:39

Excuse me- you are right. I can hardly present complete source code
from my application, although TPump.Paint is (TPump.Invalidate is
called 50 times for different instances of TPump, twice in second):

procedure TPump.Paint;  //I know that "Self" here is redundant, but it
stands in my source code
var
  Bitmap: TBitmap;
begin
  Bitmap := MyShape;
  try
    Self.SetShape(Bitmap);  // this is where debugger breaks after 20
minutes approx. (after GlobalAlloc I wrote about)
  finally
    Bitmap.Free;
  end;
  Bitmap := MyBitmap;
  try
    Self.Canvas.Draw(0,0,Bitmap);
  finally
    Bitmap.Free;
  end;
  inherited Paint;
end;

Of course, I know that for complete example, I should send MyShape and
MyBitmap implementation and details about TPump class (which inherits
from TCustomControl),  but I don't know how to submit everything
needed, yet to be enough for this kind of correspondence.

Regarding Lazarus example, I wrote about and that I changed "a little
bit", it is:

procedure TForm1.Button1Click(Sender: TObject);
begin
  ShapeControl(Self);
end;

procedure TForm1.FormCreate(Sender: TObject);
begin
  Button1.Handle;
  ShapeControl(Button1);
end;

procedure TForm1.ShapeControl(AControl: TWinControl);
var
  ABitmap: TBitmap;
  Points: array of TPoint;      i: integer;  // I added variable "i"
begin
  ABitmap := TBitmap.Create;
  ABitmap.Monochrome := True;
  ABitmap.Width := AControl.Width;
  ABitmap.Height := AControl.Height;
  SetLength(Points, 6);
  Points[0] := Point(0, ABitmap.Height div 2);
  Points[1] := Point(10, 0);
  Points[2] := Point(ABitmap.Width - 10, 0);
  Points[3] := Point(ABitmap.Width, ABitmap.Height div 2);
  Points[4] := Point(ABitmap.Width - 10, ABitmap.Height);
  Points[5] := Point(10, ABitmap.Height);
  with ABitmap.Canvas do
  begin
    Brush.Color := clBlack; // transparent color
    FillRect(0, 0, ABitmap.Width, ABitmap.Height);
    Brush.Color := clWhite; // mask color
    Polygon(Points);
  end;
// next line- I added "for" loop - originally only AControl.SetShape
exists in example
  for i:= 1 to 100000 do AControl.SetShape(ABitmap);  // debugger
breaks after minute approx.
  ABitmap.Free;
end;

Regards

----------------------------------------------------------------------
Thaddy · 2018-01-01 12:18

That is exactly what I predicted before I actually saw your source!
Quote from: thaddyBut let me guess: your "changed a little bit.." uses
the same variable....10.000 times?, then you are creating 9999
leaks!...Because you did not free them in between. The alternative is
to use 10.000 different instances and properly free them.Code: Pascal
 [Select][+][-]window.onload = function(){var x1 =
document.getElementById("main_content_section"); if (x1) { var x =
document.getElementsByClassName("geshi");for (var i = 0; i { THIS IS
COMPLETELY WRONG!! }// next line- I added "for" loop - originally only
AControl.SetShape exists in example  for i:= 1 to 100000 do
AControl.SetShape(ABitmap);  // debugger breaks after minute approx. 
ABitmap.Free;end;

----------------------------------------------------------------------
djdjdjole · 2018-01-01 12:29

Well, it seemed to me that it was ok to use ABitmap many times, after
it was created (beginning of proc) and then free it (last line).
Obviously my mistake.

Regards

----------------------------------------------------------------------
djdjdjole · 2018-01-01 13:09

Although it is probably Object programming oriented question, I am
interested if this would be OK regarding memory issue:

procedure TForm1.ShapeControl(AControl: TWinControl);
var
  ABitmap: TBitmap;
  Points: array of TPoint;      i: integer;
begin
  for i:= 1 to 100000 do  // for loop inserted here
  begin

  ABitmap := TBitmap.Create;
  ABitmap.Monochrome := True;
  ABitmap.Width := AControl.Width;
  ABitmap.Height := AControl.Height;
  SetLength(Points, 6);
  Points[0] := Point(0, ABitmap.Height div 2);
  Points[1] := Point(10, 0);
  Points[2] := Point(ABitmap.Width - 10, 0);
  Points[3] := Point(ABitmap.Width, ABitmap.Height div 2);
  Points[4] := Point(ABitmap.Width - 10, ABitmap.Height);
  Points[5] := Point(10, ABitmap.Height);
  with ABitmap.Canvas do
  begin
    Brush.Color := clBlack; // transparent color
    FillRect(0, 0, ABitmap.Width, ABitmap.Height);
    Brush.Color := clWhite; // mask color
    Polygon(Points);
  end;
  AControl.SetShape(ABitmap);
  ABitmap.Free;
  end;
end;

Regards

----------------------------------------------------------------------
djdjdjole · 2018-01-01 15:09

Well, let me try to clarify. The example I rewrote in my last reply
could still be simplified  like:

procedure TForm1.ShapeControl(AControl: TWinControl);
var
  ABitmap: TBitmap;
     i: integer;
begin
  for i:= 1 to 100000 do  // for loop inserted here
  begin
  ABitmap := TBitmap.Create;
  ABitmap.Monochrome := True;
  ABitmap.Width := AControl.Width;
  ABitmap.Height := AControl.Height;
  with ABitmap.Canvas do
  begin
    Brush.Color := clBlack; // transparent color
    FillRect(0, 0, ABitmap.Width, ABitmap.Height);
    Brush.Color := clWhite; // mask color
  end;
  AControl.SetShape(ABitmap);
  ABitmap.Free;
  end;
end;

Run this example and you will still have memory leak at SetShape
method after a minute.
ABitmap is now created and freed, the way Thaddy suggested (if I
understood him right).

Regards

----------------------------------------------------------------------
balazsszekely · 2018-01-01 17:00

The problem is that GlobalAlloc api at some point fails, the author of
the function "BitmapToRegion" did not check the return value of
GlobalAlloc, if it's zero, the function should exit immediately, but
not before freeing other data. The correct implementation should look
like this:
Code: Pascal  [Select][+][-]window.onload = function(){var x1 =
document.getElementById("main_content_section"); if (x1) { var x =
document.getElementsByClassName("geshi");for (var i = 0; i function
BitmapToRegion(hBmp: HBITMAP; cTransparentColor: COLORREF = 0;
cTolerance: COLORREF  = $101010): HRGN;begin  //...  hData :=
GlobalAlloc(GMEM_MOVEABLE, sizeof(RGNDATAHEADER) + (sizeof(TRECT) *
maxRects));  if hData = 0 then  begin    Freemem(Data);    Exit; 
end;  //...                end;Make the necessary modifications,
rebuild the IDE, after your program no longer leaks.
Other then this, creating 100000 bitmaps, then set the controls shape
100000 times makes no sense at all. What exactly are you trying to
achieve?

----------------------------------------------------------------------
djdjdjole · 2018-01-01 18:11

It wasn't my point to create 100000 bitmaps. It was something that
helped me explain (to forum people) that something was wrong with the
method. My real application just makes frequent calls to setShape and
after some time (it is in fact few days of on field working) I
received end user feedback that it "stops working". After many
attempts, I finally was able to put the finger on setShape.
Anyway thanks for explanation.

Regards

----------------------------------------------------------------------
balazsszekely · 2018-01-01 18:23

@djdjdjole
QuoteIt wasn't my point to create 100000 bitmaps. It was something
that helped me explain (to forum people) that something was wrong with
the methodOK. Fair enough. 

QuoteI received end user feedback that it "stops working". After many
attempts, I finally was able to put the finger on setShape. Anyway
thanks for explanation.If you modify BitmapToRegion function, your
application won't leak anyomore(just tested).