Why there's a mem leak and how to fix it?

Question

unit Unit7;

interface

uses Classes;

type
  TListener = class(TThread)
    procedure Execute; override;
  end;

  TMyClass = class
    o1,o2: Tobject;
    procedure FreeMyObject(var obj: TObject);
    constructor Create;
    destructor Destroy; override;
  end;

implementation

uses Windows, SysUtils;

var l: TListener;
    my: TMyClass;

procedure TListener.Execute;
var msg:TMsg;
begin
  while(GetMessage(msg, Cardinal(-1), 0, 0)) do
    if(msg.message=6) then begin
      TMyClass(msg.wParam).FreeMyObject(TObject(msg.lParam));
      Exit;
    end;
end;

constructor TMyClass.Create;
begin
  inherited;
  o1:=TObject.Create;
  o2:=Tobject.Create; // Invalid pointer operation => mem leak
end;

destructor TMyClass.Destroy;
begin
  if(Assigned(o1)) then o1.Free;
  if(Assigned(o2)) then o2.Free;
  inherited;
end;

procedure TMyClass.FreeMyObject(var obj: TObject);
begin
  FreeAndNil(obj);
end;

initialization
  l:= TListener.Create();
  my:=TMyClass.Create;

  sleep(1000); //make sure the message loop is set
  PostThreadMessage(l.ThreadID, 6, Integer(my), Integer(my.o2));
finalization
  l.Free;
  my.Free;
end.

I used the message handler to illustrate my problem as is so you understand it. The real design is a lot more complicated. The function 'FreeMyObject' actually Frees AND creates an instance using polymorphism paradigm, but this here is not needed. I only want to point out that the design should stay the same.

Now the question and problem - why it happens AND how to fix it? It seems 'if Assigned(o2)' doesn't fit it.

What I think of: Sending a pointer to my.o2 would free and nil o2 and I tries to do so, but I couldn't convert from pointer to object in the message handler, got no idea why.

Could anybody give a hand? Thanks

Answer 1

You free o2 twice. Once as a result of the message and once from the destructor.

You think you are setting o2 to nil when you call FreeMyObject but you are not. You are in fact setting msg.lParam to 0.

o2 is a variable holding a reference to an object. You are passing the value of o2 and when you pass by value you cannot modify the variable whose value you passed. So you need to pass a reference to o2. To do so you need to add an extra level of redirection and pass a pointer to o2, like so:

if(msg.message=6) then begin
  FreeAndNil(PObject(msg.lParam)^);
  Exit;
end;

...

PostThreadMessage(l.ThreadID, 6, 0, LPARAM(@my.o2));

You don't need FreeMyObject, you can just call FreeAndNil directly. And you don't need to pass an instance in the message.

I hope your real code isn't quite as weird as this! ;-)

Answer 2

If you want to FreeAndNil an object sending just object reference Integer(my.o2) is not enough - you need Integer(@my.o2). You should also make corresponding changes in your code.

Since your code is difficult to debug I have written a simple demo to give an idea of necessary code changes:

type
  PObject = ^TObject;

procedure FreeObj(PObj: PObject);
var
  Temp: TObject;

begin
  Temp:= PObj^;
  PObj^:= nil;
  Temp.Free;
end;

procedure TForm17.Button1Click(Sender: TObject);
var
  Obj: TList;
  PObj: PObject;

begin
  Obj:= TList.Create;
  PObj:= @Obj;
  Assert(Obj <> nil);
  FreeObj(PObj);
  Assert(Obj = nil);
end;