Custom managed records with String List causing memory leak

[Gary 18]

Hello all,

 

I am trying to separate my UI from the rest of my application. I want to use an interfaced listener on my main form and pass a reference to it around to parts of the application that need input from the user. I have no problem with the InputBox or MsgDialog as they don't take too many parameters, however I want to use TMSTaskDialog as well (Different listener since requires 3rd party). The problem I am having is that the TaskDialog has so many options that I want to pass a record with all the parameters. I only have a problem with TStrings. I get memory leak on close. 

Work in progress but here are the relevant parts.

TTaskDialogParams = record
    DisplayedButtons : TPsTaskDialogDisplayedButtons;
    Title: String;
    Instruction: string;
    Content: string;
    Options: TPsTaskDialogOptions;
    CustomButtons : TStringList;

    Icon: TPsTaskDialogIcon;

    FResultCallBack: TProc<TTaskDialogReturnParams>;
    procedure Clear;
    class operator Initialize (out Dest: TTaskDialogParams);
    class operator Finalize (var Dest: TTaskDialogParams);
  end;

  IPsFNCTaskDialogUIListener = interface
    ['{BA6ABA84-F3EE-4421-856F-42E76C878D3B}']
    procedure ExecuteDialog(const Params : TTaskDialogParams);
  end;
      
  procedure TTaskDialogParams.Clear;
begin
  CustomButtons.Clear;
  CustomButtons.Free;
end;

class operator TTaskDialogParams.Finalize(var Dest: TTaskDialogParams);
begin
//  Dest.CustomButtons.Free;
end;

class operator TTaskDialogParams.Initialize(out Dest: TTaskDialogParams);
begin
  Dest.CustomButtons := TStringList.Create;
end;

In listener I set string params by enumerating through the recordsStringList.
procedure TPsFNCTaskDialogUIListener.ExecuteDialog(const Params :
    TTaskDialogParams);
begin
  SetDialogParams(Params);
  FTaskDialog.Execute;
  Params.Clear;
//  FTaskDialog.CustomButtons.Clear;
end;
      
Then show it
procedure TPsFNCTaskDialogUIListener.ExecuteDialog(const Params :
    TTaskDialogParams);
begin
  SetDialogParams(Params);
  FTaskDialog.Execute;
  Params.Clear;
//  FTaskDialog.CustomButtons.Clear;
end;      
      

Some code in there is things I have tried to no avail. Any help would be appreciated. I tried this before and gave up and used an Interfaced  Class instead.

[Gary 18]

Sorry copied same block twice Here is the other part

procedure TPsFNCTaskDialogUIListener.SetDialogParams(Params: TTaskDialogParams);
begin
  ClearDialog;

  FTaskDialog.Title := Params.Title;
  FTaskDialog.Instruction := Params.Instruction;
  FTaskDialog.Icon := TTMSFNCTaskDialogIcon(Params.Icon);
  FTaskDialog.Options := TTMSFNCTaskDialogOptions(params.Options);
  for var s : String in params.CustomButtons do
    FTaskDialog.CustomButtons.Add(s);

end;

 

[Brian Evans 105]

"Leak" is a bit vague - usually there would be some indication of what leaked which can help narrow down what to look at.

[Gary 18]

From FastMM

 

--------------------------------2023/12/28 22:32:03--------------------------------
A memory block has been leaked. The size is: 84

This block was allocated by thread 0x6A4, and the stack trace (return addresses) at the time was:
9371E6 [System.pas][System][@GetMem$qqri][4960]
938957 [System.pas][System][TObject.NewInstance][18324]
93905A [System.pas][System][@ClassCreate$qqrpvzc][19654]
9F9C30 [System.Classes.pas][System.Classes][Classes.TStringList.Create][8121]
94C231 [FastMM4.pas][FastMM4][UpdateHeaderAndFooterCheckSums$qqrp29Fastmm4.TFullDebugBlockHeader][9153]
94D214 [FastMM4.pas][FastMM4][DebugGetMem$qqri][9731]
EA116F [int.PsFNCTaskDialogUIListener.pas][Int.PsFNCTaskDialogUIListener][Psfnctaskdialoguilistener.TTaskDialogParams._op_Initialize][66]
108E6E5 [PsFNCTaskDialogUIListener.pas][PsFNCTaskDialogUIListener][TPsFNCTaskDialogUIListener.SetDialogParams][32]
108E827 [PsFNCTaskDialogUIListener.pas][PsFNCTaskDialogUIListener][TPsFNCTaskDialogUIListener.ExecuteDialog][47]
108F3C9 [USender.pas][USender][TfrmSender.tbtnTaskDialogClick][77]
C0CF34 [FMX.Controls.pas][FMX.Controls][Controls.TControl.GetHeight][5512]

The block is currently used for an object of class: System.Classes.TStringList

The allocation number is: 296325

Current memory dump of 256 bytes starting at pointer address 7E6DB770:
20 B9 9C 00 00 00 00 00 50 B9 30 7F 6C E3 1B 7F 00 00 00 00 00 00 00 00 2C 00 22 00 3D 00 0E 00
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 16 8C AF 78 80 80 80 80 80 80 80 80 00 00 00 00 51 BB 6D 7E
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 88 85 04 00 9F A8 93 00 3C AD 93 00 A2 CC 93 00
F2 E6 08 01 27 E8 08 01 C9 F3 08 01 34 CF C0 00 9D AB C0 00 77 D6 CB 00 F4 B2 C0 00 D4 65 E8 00
A4 06 00 00 A4 06 00 00 02 72 93 00 59 A9 93 00 28 C5 93 00 E2 D3 C0 00 39 8B 93 00 6E 89 93 00
A5 90 93 00 2E 9A CB 00 B3 57 E5 00 ED 92 93 00 45 C0 E8 00 48 00 00 00 B0 04 02 00 CB 3D A4 8E
8C 7C 0C 01 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80
         .  .  .  .  .  P     0    l     .    .  .  .  .  .  .  .  .  ,  .  "  .  =  .  .  .
.  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .
.  .  .  .  .  .  .  .  .  .  .  .  .        x                          .  .  .  .  Q     m  ~
.  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .        .  .           .  <        .           .
      .  .  '     .  .        .  .  4        .           .  w        .           .     e     .
   .  .  .     .  .  .  .  r     .  Y        .  (        .           .  9        .  n        .
         .  .        .     W     .           .  E        .  H  .  .  .     .  .  .     =      
   |  .  .                                                                                    

--------------------------------2023/12/28 22:32:03--------------------------------
A memory block has been leaked. The size is: 20

This block was allocated by thread 0x6A4, and the stack trace (return addresses) at the time was:
9371E6 [System.pas][System][@GetMem$qqri][4960]
93A89F [System.pas][System][@NewUnicodeString$qqri][26024]
93AD3C [System.pas][System][@UStrAsg$qqrr20System.UnicodeStringx20System.UnicodeString][26946]
9F6F30 [System.Classes.pas][System.Classes][Classes.TStrings.Create][6729]
9F9C3D [System.Classes.pas][System.Classes][Classes.TStringList.Create][8122]
94D214 [FastMM4.pas][FastMM4][DebugGetMem$qqri][9731]
EA116F [int.PsFNCTaskDialogUIListener.pas][Int.PsFNCTaskDialogUIListener][Psfnctaskdialoguilistener.TTaskDialogParams._op_Initialize][66]
108E6E5 [PsFNCTaskDialogUIListener.pas][PsFNCTaskDialogUIListener][TPsFNCTaskDialogUIListener.SetDialogParams][32]
108E827 [PsFNCTaskDialogUIListener.pas][PsFNCTaskDialogUIListener][TPsFNCTaskDialogUIListener.ExecuteDialog][47]
108F3C9 [USender.pas][USender][TfrmSender.tbtnTaskDialogClick][77]
C0CF34 [FMX.Controls.pas][FMX.Controls][Controls.TControl.GetHeight][5512]

The block is currently used for an object of class: UnicodeString

The allocation number is: 296326

Current memory dump of 256 bytes starting at pointer address 7F1BE360:
B0 04 02 00 01 00 00 00 02 00 00 00 0D 00 0A 00 00 00 AA 4B E7 F7 80 80 00 00 00 00 A1 FF 1B 7F
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 5F BF 04 00 E6 71 93 00 57 89 93 00 5A 90 93 00
7F 21 E8 00 12 EE E8 00 31 8A E8 00 1F 40 E8 00 63 40 E8 00 6E EE E8 00 62 DF A0 00 6B 8A 93 00
A4 06 00 00 A4 06 00 00 02 72 93 00 75 89 93 00 A5 90 93 00 E5 21 E8 00 6B 8A 93 00 35 38 E8 00
02 72 93 00 75 89 93 00 A5 90 93 00 F7 10 A8 00 6B 8A 93 00 14 00 00 00 DC E2 E6 00 4C 29 7E 8F
8C 7C 0C 01 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 80 B3 D6 81 70 00 00 00 00 C1 E9 1B 7F
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 9F BF 04 00 E6 71 93 00 57 89 93 00 5A 90 93 00
73 35 E3 00 1B 35 E3 00 51 C4 E2 00 13 C5 E2 00 3E 3F E8 00 D1 F1 E8 00 0A 3A E8 00 02 72 93 00
   .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .     K              .  .  .  .        .  
.  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  _     .  .     q     .  W        .  Z        .
 !     .  .        .  1        .  .  @     .  c  @     .  n        .  b        .  k        .
   .  .  .     .  .  .  .  r     .  u        .           .     !     .  k        .  5  8     .
.  r     .  u        .           .     .     .  k        .  .  .  .  .           .  L  )  ~   
   |  .  .                                                           p  .  .  .  .        .  
.  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .        .  .     q     .  W        .  Z        .
s  5     .  .  5     .  Q        .  .        .  >  ?     .           .  .  :     .  .  r     .

--------------------------------2023/12/28 22:32:03--------------------------------
This application has leaked memory. The small block leaks are (excluding expected leaks registered by pointer):

13 - 20 bytes: UnicodeString x 1
69 - 84 bytes: System.Classes.TStringList x 1

Note: Memory leak detail is logged to a text file in the same folder as this application. To disable this memory leak check, undefine "EnableMemoryLeakReporting".

[Kas Ob. 121]

2 hours ago, Gary said:

A memory block has been leaked. The size is: 84

This block was allocated by thread 0x6A4, and the stack trace (return addresses) at the time was:
9371E6 [System.pas][System][@GetMem$qqri][4960]
938957 [System.pas][System][TObject.NewInstance][18324]
93905A [System.pas][System][@ClassCreate$qqrpvzc][19654]
9F9C30 [System.Classes.pas][System.Classes][Classes.TStringList.Create][8121]

It is from the above that this is one time leak, which is caused by not freeing the stringlist

class operator TTaskDialogParams.Finalize(var Dest: TTaskDialogParams);
begin
//  Dest.CustomButtons.Free;
end;

So where is the problem ? 

What happen if you did uncomment that line ?

[mvanrijnen 123]

1 hour ago, Kas Ob. said:

It is from the above that this is one time leak, which is caused by not freeing the stringlist

class operator TTaskDialogParams.Finalize(var Dest: TTaskDialogParams);
begin
//  Dest.CustomButtons.Free;
end;

So where is the problem ? 

What happen if you did uncomment that line ?

then he get's an AV, cause he's freeing the CustomButtons in his Clear method.

 

[Dalija Prasnikar 1396]

It is very likely that you would have less issues with interface based class and then you would be able to pass around interface reference without having to worry about memory management too much. 

 

If you still want to use record, you need to fix your Clear procedure. You are calling CustomButtons.Free there which will destroy CustomButtons instance and then you would have crash in Finalize which would try to destroy that same instance. You should just clear the list and not free it there. So that memory management happens solely within Initialize and Finalize methods.

 

procedure TTaskDialogParams.Clear;
begin
  CustomButtons.Clear;
end;

class operator TTaskDialogParams.Finalize(var Dest: TTaskDialogParams);
begin
  Dest.CustomButtons.Free;
end;

 

[Kas Ob. 121]

40 minutes ago, mvanrijnen said:

then he get's an AV, cause he's freeing the CustomButtons in his Clear method.

He was asking about the memory leak and i pointed where the leak came form, an AV is his wrong design/approach.

[Remy Lebeau 1392]

Also, because SetDialogParams() is taking a TTaskDialogParams by value, a copy is made, which will have its own TStringList object, so you must implement the Assign() operator as well to copy the source record's TStringList data to the copied record after it is Initialized:

 

class operator TTaskDialogParams.Assign(var Dest: TTaskDialogParams; const [ref] Src: TTaskDialogParams);
begin
  Dest.CustomButtons.Assign(Src.CustomButtons);
end;

 

class operator TTaskDialogParams.Finalize(var Dest: TTaskDialogParams);
begin
  Dest.CustomButtons.Free;
end;

 

class operator TTaskDialogParams.Initialize(out Dest: TTaskDialogParams);
begin
  Dest.CustomButtons := TStringList.Create;
end;
 

Otherwise, if you don't do this, the default assignment behavior will copy the object pointer instead, causing the copied record to lose its own TStringList object (leaking it) and end up pointing to the original TStringList object, so when the copied record is Finalized then the original TStringList object gets freed, corrupting the original record that was copied from.

 

That being said, there is no good reason to pass the record by value in the code shown. SetDialogParams() should take the record by const reference instead, then no copy is made. 

 

Read the documentation, especially the section on "Passing Managed Records as Parameters" :

 

https://docwiki.embarcadero.com/RADStudio/en/Custom_Managed_Records

 

Edited December 30, 2023 by Remy Lebeau