Recursive destructor

I have the following two classes:

type
  TItemProp = class
  private
    FItemPropName: string;
    FItemPropValue: string;
  public
    constructor Create(AItemPropName, AItemPropValue: string);
    class function GetItemProp(AHTMLElement: OleVariant; AProp: string): TItemProp;
    property ItemPropName: string read FItemPropName;
    property ItemPropValue: string read FItemPropValue;
 end;

TTest = class
private
  FName: string;
  FProps: TList<TItemProp>;
  FTest: TList<TTest>;
public
  constructor Create(AName: string);
  destructor Destroy();
  property Name: string read FName;
  property ItemProps: TList<TItemProp> read FProps write FProps;
  property Test: TList<TTest> read FTest write FTest;
end;

And here is the code of the constructor and the destructor of TTest class:

constructor TTest.Create(AName: string);
begin
  Self.FName := AName;
  Self.FProps := TList<TItemProp>.Create();
  Self.FTest := TList<TTest>.Create();
end;

destructor TTest.Destroy();
var
  I: Integer;
begin
  for I := 0 to Self.FTest.Count - 1 do
  begin
    Self.FTest[I].Free;
    Self.FTest[I] := nil;
  end;

  Self.FProps.Free;
  Self.FTest.TrimExcess;
  Self.FTest.Free;
  inherited; 
end;

The problem is that this code is leaking memory. How I should rewrite the destructor in order to fix the memory leak?

Solution

The first problem is here:

destructor Destroy();

You need to override the virtual destructor declared in TObject. Like this:

destructor Destroy; override;

Your destructor implementation is needlessly complex, and also fails to destroy the objects owned by FProps. This destructor should be written like so:

destructor TTest.Destroy;
var
  I: Integer;
begin
  for I := 0 to FTest.Count - 1 do
    FTest[I].Free;
  FTest.Free;

  for I := 0 to FProps.Count - 1 do
    FProps[I].Free;
  FProps.Free;

  inherited; 
end;

Your properties that expose these list objects should not have setter methods. So, they should be read only properties like this:

property ItemProps: TList<TItemProp> read FProps;
property Test: TList<TTest> read FTest;

Your code would lead to leaks should any body attempt to write to these properties.

Were you to use TObjectList rather than TList you would be able to delegate lifetime management to the list class. This tends to lead to much simpler code.

Exposing TList<T> instances as public properties does rather expose your class to abuse. The clients of this class can call any public methods of that class and potentially mutate the list in ways that you do not expect, and do not wish to cater for. You should seek to encapsulate these objects more.

As a general rule you should always call the inherited constructor of a class. In your case that constructor is the TObject constructor that does nothing. But, it's good practise to call it anyway. Doing so means that if you change the inheritance hierarchy at a later date, you won't be caught out by not calling the constructor of the new parent class.

constructor TTest.Create(AName: string);
begin
  inherited;
  FName := AName;
  FProps := TList<TItemProp>.Create();
  FTest := TList<TTest>.Create();
end;

You are using Self everywhere. This is fine, but it is not at all idiomatic. I suggest that you refrain from this habit otherwise your code will be very verbose and less easy to read.

There were a very large number of mistakes in the code you posted. There is clearly more code in your program that you have not posted. I would very much expect that code to contain bugs too, so do not be surprised if leaks remain even after you apply all the changes above.