One more memory leak and FastMM4

Alberto Paganini · June 23, 2020

I have one more memory leak to fix. Here is the report from FastMM4

--------------------------------2020/6/23 14:34:20--------------------------------
A memory block has been leaked. The size is: 116

This block was allocated by thread 0x157C, and the stack trace (return addresses) at the time was:
404DA6 [System.pas][System][@GetMem$qqri][3454]
408BBB [System.pas][System][@NewUnicodeString$qqri][19030]
408DEC [System.pas][System][@UStrFromPWCharLen$qqrr20System.UnicodeStringpbi][19697]
40A0C9 [System.pas][System][@UStrCopy$qqrx20System.UnicodeStringii][24873]
E5CAD3 [Data.DBXPlatform.pas][Data.DBXPlatform][Dbxplatform.TDBXStringBuffer.ToString$qqrv][697]
E8E8BC [Data.DBXJSON.pas][Data.DBXJSON][Dbxjson.TJSONString.Value$qqrv][1541]
E9297C [uBetFair.pas][uBetFair][TBetFairApi.GetToken$qqrv][222]
E92E90 [uTokenBL.pas][uTokenBL][TTokenBL.GetToken$qqrv][30]
E5B593 [Main.pas][Main][TMainForm.FormActivate$qqrp14System.TObject][402]
557ED9 [Vcl.Forms][Forms.TCustomForm.Activate$qqrv]
558F99 [Vcl.Forms][Forms.TCustomForm.CMActivate$qqrr27Winapi.Messages.TWMNoParams]
4B4930 [Vcl.Controls.pas][Vcl.Controls][Controls.TControl.WndProc$qqrr24Winapi.Messages.TMessage][7204]
75C3C431 [Unknown function at gapfnScSendMessage]
75C3C3EE [Unknown function at gapfnScSendMessage]
75C3C380 [Unknown function at gapfnScSendMessage]

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

The allocation number is: 60573

Current memory dump of 256 bytes starting at pointer address 7F2C7580:
B0 04 02 00 01 00 00 00 2C 00 00 00 53 00 31 00 45 00 41 00 66 00 64 00 33 00 33 00 51 00 61 00
57 00 35 00 77 00 36 00 53 00 49 00 62 00 2F 00 69 00 4B 00 4F 00 6E 00 6F 00 32 00 74 00 47 00
36 00 74 00 63 00 36 00 4F 00 51 00 69 00 45 00 74 00 73 00 6C 00 6E 00 70 00 45 00 57 00 45 00
77 00 3D 00 00 00 31 8D A4 0F 80 80 80 80 80 80 80 80 80 80 80 80 80 80 00 00 00 00 21 80 2C 7F
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 E1 0C 01 00 A6 4D 40 00 0F 6E 40 00 7E 74 40 00
2B 97 DB 00 6D 80 DC 00 9B D1 CD 00 7A 3A D5 00 68 39 CD 00 D4 39 CD 00 62 8F DC 00 69 91 DC 00
D7 FE B8 00 06 68 D4 00 1E FD B8 00 37 AC DD 00 7C 15 00 00 7C 15 00 00 C2 4D 40 00 2D 6E 40 00
C9 74 40 00 F1 EF D4 00 73 6E 40 00 1F D2 CD 00 CA D1 CD 00 BE 3A D5 00 73 6E 40 00 86 39 CD 00
°  .  .  .  .  .  .  .  ,  .  .  .  S  .  1  .  E  .  A  .  f  .  d  .  3  .  3  .  Q  .  a  .
W  .  5  .  w  .  6  .  S  .  I  .  b  .  /  .  i  .  K  .  O  .  n  .  o  .  2  .  t  .  G  .
6  .  t  .  c  .  6  .  O  .  Q  .  i  .  E  .  t  .  s  .  l  .  n  .  p  .  E  .  W  .  E  .
w  .  =  .  .  .  1  �  ¤  .  €  €  €  €  €  €  €  €  €  €  €  €  €  €  .  .  .  .  !  €  ,  �
.  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  á  .  .  .  ¦  M  @  .  .  n  @  .  ~  t  @  .
+  —  Û  .  m  €  Ü  .  ›  Ñ  Í  .  z  :  Õ  .  h  9  Í  .  Ô  9  Í  .  b  �  Ü  .  i  ‘  Ü  .
×  þ  ¸  .  .  h  Ô  .  .  ý  ¸  .  7  ¬  Ý  .  |  .  .  .  |  .  .  .  Â  M  @  .  -  n  @  .
É  t  @  .  ñ  ï  Ô  .  s  n  @  .  .  Ò  Í  .  Ê  Ñ  Í  .  ¾  :  Õ  .  s  n  @  .  †  9  Í  .

The culprit seems to be TBetFairApi.GetToken and here is the code:

function TBetFairApi.GetToken: Boolean;
var
  LJSONObject: TJSONObject;
  LJSONValue: TJSONValue;
  s:string;
begin
  Result := False;

  FidHTTP := TidHTTP.Create(nil);
  FidHTTP.HTTPOptions := FidHTTP.HTTPOptions - [hoForceEncodeParams];
  FidHTTP.Intercept := TIdLogFile.Create(FidHTTP);

  s:=ExtractFileName(ParamStr((0)));
  s:=Copy(s,1,Pos('.',s)-1);
  TIdLogFile(FidHTTP.Intercept).Filename := 'c:\' + s + '.log';
  TIdLogFile(FidHTTP.Intercept).Active := true;
  FList := TStringList.Create;

  FIdSSL := TIdSSLIOHandlerSocketOpenSSL.Create;
  FIdSSL.SSLOptions.Method:=sslvTLSv1_2;

  FidHTTP.Request.Accept := 'application/json';
  FidHTTP.Request.CustomHeaders.Clear;
  FidHTTP.Request.CustomHeaders.AddValue('X-Application', cBetfair_AppKey);

  FList.Clear;
  FList.Add('username=' + FUserID);
  FList.Add('password=' + FPassword);

  FidHTTP.Request.ContentType := 'application/x-www-form-urlencoded';
  FidHTTP.HandleRedirects := true;
  {$IFDEF VER230}
  FidHTTP.IOHandler := FIdSSL;
  {$ENDIF}

  LJSONObject := TJSONObject.ParseJSONValue(FidHTTP.Post(URL_LOGIN, FList)) as TJSONObject;
  try
    if Assigned(LJSONObject) then
    begin
       LJSONValue := LJSONObject.{$IFDEF VER230}Get('token').JsonValue{$ELSE}Values['token']{$ENDIF};
       if Assigned(LJSONValue) then
       begin
          FToken := LJSONValue.Value;
          Result := True;
       end;
    end;
  finally
    FreeAndNil(LJSONObject);
  end;

    FreeAndNil(FidHTTP);
  {$IFDEF VER230}
    FreeAndNil(FIdSSL);
  {$ENDIF}
    FreeAndNil(FList);
end;

There are 4 .Creates:

FidHTTP := TidHTTP.Create(nil);

FList := TStringList.Create;

FIdSSL := TIdSSLIOHandlerSocketOpenSSL.Create;

FidHTTP.Intercept := TIdLogFile.Create(FidHTTP);

The first three are freed at the end of the procedure and TIdLogFile.Create(FidHTTP) should be freed automatically when FidHTTP is freed.

I cannot see any memory leak here but obviously I am wrong.

Can you see anything I cannot?

Alberto

Der schöne Günther · June 23, 2020

Your Delphi version would be interesting.

June 23, 2020

You really should start spending some time with stack reading and how it is works !

Quote


E8E8BC [Data.DBXJSON.pas][Data.DBXJSON][Dbxjson.TJSONString.Value$qqrv][1541]
E9297C [uBetFair.pas][uBetFair][TBetFairApi.GetToken$qqrv][222]

From these, FastMM4 already pinned the source, can you figure it out by your own ?

If not then ask again, if you OK with that ?

I want you to start fishing on your own instead of solving simple problems by asking simple questions in forums.

Alberto Paganini · June 23, 2020

4 minutes ago, Der schöne Günther said:

Your Delphi version would be interesting.

Sorry, Delphi XE2

I have added it to my profile now.

Edited June 23, 2020 by Alberto Paganini

David Heffernan · June 23, 2020

Not the cause of the leak, but each of those calls to TSomeObject.Create needs to be protected in a try/finally

June 23, 2020

@Alberto Paganini Been 1 hour, so i will hint one more thing, from one line only in that log

Quote


E9297C [uBetFair.pas][uBetFair][TBetFairApi.GetToken$qqrv][222]

The number after the procedure name "222" is a line number in that unit.

Alberto Paganini · June 23, 2020

️

4

30 minutes ago, Kas Ob. said:

You really should start spending some time with stack reading and how it is works !

From these, FastMM4 already pinned the source, can you figure it out by your own ?

If not then ask again, if you OK with that ?

The line 222 in TBetFairApi.GetToken is

if Assigned(LJSONValue)

I tried FreeAndNil(LJSONValue) after having retrieved the value but this generates additional errors.

1 hour ago, Kas Ob. said:

I want you to start fishing on your own instead of solving simple problems by asking simple questions in forums.

I know and I appreciate that.

June 23, 2020

OK we are getting there, But first i want to made it clear, i never used that TJonsValue so i might be wrong here.

Now you pinned the source to one line which is not the one you listed but the above, may be you changed the source with one line may the FastMM4 is simply reporting the return line from returning address.

Your problem is within this line

Quote


LJSONValue := LJSONObject.{$IFDEF VER230}Get('token').JsonValue{$ELSE}Values['token']{$ENDIF};

You said freeing LJSONValue didn't help, so what about that Get('token') ?? what it does return, an ordinary type or an object ?

Your problem is between those two.

Alberto Paganini · June 23, 2020

22 minutes ago, Kas Ob. said:

OK we are getting there, But first i want to made it clear, i never used that TJonsValue so i might be wrong here.

Now you pinned the source to one line which is not the one you listed but the above, may be you changed the source with one line may the FastMM4 is simply reporting the return line from returning address.

Your problem is within this line

You said freeing LJSONValue didn't help, so what about that Get('token') ?? what it does return, an ordinary type or an object ?

Your problem is between those two.

Get('token') return a TJSONPair object

I tried to add the following after having retrieved the value:

LJSONObject.Free;
LJSONValue.Free;

LJSONObject.Get('token').Free;
LJSONObject.Get('token').JsonValue.Free;

but all of them made things worse

Der schöne Günther · June 23, 2020

I'm puzzled why a UnicodeString is the only thing that leaks. If the problem was a missing disposal of a JSON object, we should be leaking much more.

It's just a stupid idea, but were you running in the debugger? I think sometimes, if you inspect values, the debugger increases the reference count on managed things like arrays or strings and will keep them alive until the very end of the application.

June 23, 2020

5 minutes ago, Alberto Paganini said:

LJSONObject.Get('token').Free;
LJSONObject.Get('token').JsonValue.Free;

Those you should not even think about.

as for LJSONObject and LJSONValue where are you freeing them like other objects at the end or inside if assigned().

David already pointed that you should be more precise of using create and free with try..finally , and that is the right thing to do.

So please fix them all then show code, on other hand the you are checking Assigned(LJSONValue) this means it is a an object, so if you walked with the debugger to the point after where you free LJSONObject and tried to access its Value ( LJSONValue.Value ), what does happen ? and as Gunther pointed what value it does holds.

3 minutes ago, Der schöne Günther said:

I'm puzzled why a UnicodeString is the only thing that leaks. If the problem was a missing disposal of a JSON object, we should be leaking much more.

Same as his last memory leak, his report was only a string from HttpRequest.

so here may be that token is holding the reference to that string and freeing LJSONValue is not releasing the string.

Alberto Paganini · June 23, 2020

4 hours ago, Kas Ob. said:

Those you should not even think about.

as for LJSONObject and LJSONValue where are you freeing them like other objects at the end or inside if assigned().

David already pointed that you should be more precise of using create and free with try..finally , and that is the right thing to do.

So please fix them all then show code, on other hand the you are checking Assigned(LJSONValue) this means it is a an object, so if you walked with the debugger to the point after where you free LJSONObject and tried to access its Value ( LJSONValue.Value ), what does happen ? and as Gunther pointed what value it does holds.

Same as his last memory leak, his report was only a string from HttpRequest.

so here may be that token is holding the reference to that string and freeing LJSONValue is not releasing the string.

Here is the revised code:

function TBetFairApi.GetToken: Boolean;
var
  LJSONObject: TJSONObject;
  LJSONValue: TJSONValue;
  s:string;
  IdLogFile:TIdLogFile;
begin
  try
    Result := False;

    FidHTTP := TidHTTP.Create(nil);
    FidHTTP.HTTPOptions := FidHTTP.HTTPOptions - [hoForceEncodeParams];
    IdLogFile:=TIdLogFile.Create(FidHTTP);
    FidHTTP.Intercept := IdLogFile;

    s:=ExtractFileName(ParamStr((0)));
    s:=Copy(s,1,Pos('.',s)-1);
    TIdLogFile(FidHTTP.Intercept).Filename := 'c:\'+s+'.log';
    TIdLogFile(FidHTTP.Intercept).Active := true;
    FList := TStringList.Create;

    FIdSSL := TIdSSLIOHandlerSocketOpenSSL.Create;
    FIdSSL.SSLOptions.Method:=sslvTLSv1_2;

    FidHTTP.Request.Accept := 'application/json';
    FidHTTP.Request.CustomHeaders.Clear;
    FidHTTP.Request.CustomHeaders.AddValue('X-Application', cBetfair_AppKey);

    FList.Clear;
    FList.Add('username=' + FUserID);
    FList.Add('password=' + FPassword);

    FidHTTP.Request.ContentType := 'application/x-www-form-urlencoded';
    FidHTTP.HandleRedirects := true;
    {$IFDEF VER230}
    FidHTTP.IOHandler := FIdSSL;
    {$ENDIF}

    LJSONObject := TJSONObject.ParseJSONValue(FidHTTP.Post(URL_LOGIN, FList)) as TJSONObject; // This is line 218
    try
      if Assigned(LJSONObject) then
      begin
         LJSONValue := LJSONObject.{$IFDEF VER230}Get('token').JsonValue{$ELSE}Values['token']{$ENDIF};
         try
           if Assigned(LJSONValue) then
           begin
              FToken := LJSONValue.Value;
              Result := True;
           end;
        finally
          FreeAndNil(LJSONValue); // I free the object as soon as I don't need any longer
        end;
      end;
    finally
      FreeAndNil(LJSONObject); 


    end;

  finally
    FreeAndNil(IdLogFile);
    FreeAndNil(FidHTTP);
    {$IFDEF VER230}
      FreeAndNil(FIdSSL);
    {$ENDIF}
    FreeAndNil(FList);
  end;
end;

and this is the leak report from FastMM4

--------------------------------2020/6/23 21:33:53--------------------------------
FastMM has detected an attempt to call a virtual method on a freed object. An access violation will now be raised in order to abort the current operation.

Freed object class: Data.DBXJSON.TJSONString

Virtual method: Offset +16

Virtual method address: E8D644

The allocation number was: 60454

The object was allocated by thread 0xAD4, and the stack trace (return addresses) at the time was:
406E0F [System.pas][System][TObject.NewInstance$qqrv][13000]
40747E [System.pas][System][@ClassCreate$qqrpvzc][14164]
E8E3E7 [Data.DBXJSON.pas][Data.DBXJSON][Dbxjson.TJSONString.$bctr$qqrx20System.UnicodeString][1410]
E8FD6C [Data.DBXJSON.pas][Data.DBXJSON][Dbxjson.TJSONObject.ParseString$qqrpx28Data.Dbxjson.TJSONByteReaderpx26Data.Dbxjson.TJSONAncestor][2235]
E8F78E [Data.DBXJSON.pas][Data.DBXJSON][Dbxjson.TJSONObject.ParseValue$qqrpx28Data.Dbxjson.TJSONByteReaderpx26Data.Dbxjson.TJSONAncestor][2010]
E8F5F5 [Data.DBXJSON.pas][Data.DBXJSON][Dbxjson.TJSONObject.ParsePair$qqrpx28Data.Dbxjson.TJSONByteReaderpx24Data.Dbxjson.TJSONObject][1958]
E8F476 [Data.DBXJSON.pas][Data.DBXJSON][Dbxjson.TJSONObject.Parse$qqrpx28Data.Dbxjson.TJSONByteReader][1892]
E8F577 [Data.DBXJSON.pas][Data.DBXJSON][Dbxjson.TJSONObject.ParseObject$qqrpx28Data.Dbxjson.TJSONByteReaderpx26Data.Dbxjson.TJSONAncestor][1938]
E8F7AA [Data.DBXJSON.pas][Data.DBXJSON][Dbxjson.TJSONObject.ParseValue$qqrpx28Data.Dbxjson.TJSONByteReaderpx26Data.Dbxjson.TJSONAncestor][2024]
E8EE51 [Data.DBXJSON.pas][Data.DBXJSON][Dbxjson.TJSONObject.ParseJSONValue$qqrx25System.%DynamicArray$tuc%xixio][1666]
432D26 [System.SysUtils][Sysutils.TEncoding.GetBytes$qqrx20System.UnicodeString]
E8EDF0 [Data.DBXJSON.pas][Data.DBXJSON][Dbxjson.TJSONObject.ParseJSONValue$qqrx25System.%DynamicArray$tuc%xio][1651]
E8EED1 [Data.DBXJSON.pas][Data.DBXJSON][Dbxjson.TJSONObject.ParseJSONValue$qqrx20System.UnicodeString][1677]
E92947 [uBetFair.pas][uBetFair][TBetFairApi.GetToken$qqrv][218]
E92EE0 [uTokenBL.pas][uTokenBL][TTokenBL.GetToken$qqrv][30]

The object was subsequently freed by thread 0xAD4, and the stack trace (return addresses) at the time was:
404DC2 [System.pas][System][@FreeMem$qqrpv][3502]
406E2D [System.pas][System][TObject.FreeInstance$qqrv][13006]
4074C9 [System.pas][System][@ClassDestroy$qqrp14System.TObject][14206]
E8E455 [Data.DBXJSON.pas][Data.DBXJSON][Dbxjson.TJSONString.$bdtr$qqrv][1419]
406E73 [System.pas][System][TObject.Free$qqrv][13025]
E929CE [uBetFair.pas][uBetFair][TBetFairApi.GetToken$qqrv][230]
E92EE0 [uTokenBL.pas][uTokenBL][TTokenBL.GetToken$qqrv][30]
E5B593 [Main.pas][Main][TMainForm.FormActivate$qqrp14System.TObject][402]
557ED9 [Vcl.Forms][Forms.TCustomForm.Activate$qqrv]
558F99 [Vcl.Forms][Forms.TCustomForm.CMActivate$qqrr27Winapi.Messages.TWMNoParams]
4B4930 [Vcl.Controls.pas][Vcl.Controls][Controls.TControl.WndProc$qqrr24Winapi.Messages.TMessage][7204]
7600C431 [Unknown function at gapfnScSendMessage]
7600C3EE [Unknown function at gapfnScSendMessage]
7600C380 [Unknown function at gapfnScSendMessage]
7600C431 [Unknown function at gapfnScSendMessage]

The current thread ID is 0xAD4, and the stack trace (return addresses) leading to this error is:
406E73 [System.pas][System][TObject.Free$qqrv][13025]
E8F12D [Data.DBXJSON.pas][Data.DBXJSON][Dbxjson.TJSONObject.$bdtr$qqrv][1762]
406E73 [System.pas][System][TObject.Free$qqrv][13025]
E929F6 [uBetFair.pas][uBetFair][TBetFairApi.GetToken$qqrv][234]
E92EE0 [uTokenBL.pas][uTokenBL][TTokenBL.GetToken$qqrv][30]
E5B593 [Main.pas][Main][TMainForm.FormActivate$qqrp14System.TObject][402]
557ED9 [Vcl.Forms][Forms.TCustomForm.Activate$qqrv]
558F99 [Vcl.Forms][Forms.TCustomForm.CMActivate$qqrr27Winapi.Messages.TWMNoParams]
4B4930 [Vcl.Controls.pas][Vcl.Controls][Controls.TControl.WndProc$qqrr24Winapi.Messages.TMessage][7204]
7600C431 [Unknown function at gapfnScSendMessage]
7600C3EE [Unknown function at gapfnScSendMessage]
7600C380 [Unknown function at gapfnScSendMessage]
7600C431 [Unknown function at gapfnScSendMessage]
7600C3EE [Unknown function at gapfnScSendMessage]
76004D0E [Unknown function at GetScrollBarInfo]

Current memory dump of 256 bytes starting at pointer address 7F2FEFC0:
94 B2 F1 00 80 80 80 80 80 80 80 80 80 80 80 80 A9 74 9A 96 80 80 80 80 00 00 00 00 61 E5 2F 7F
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 27 EC 00 00 0F 6E 40 00 7E 74 40 00 6F CA E5 00
18 6E 40 00 7E 74 40 00 04 E4 E8 00 6C FD E8 00 8E F7 E8 00 F5 F5 E8 00 76 F4 E8 00 77 F5 E8 00
AA F7 E8 00 51 EE E8 00 26 2D 43 00 F0 ED E8 00 D4 0A 00 00 D4 0A 00 00 C2 4D 40 00 2D 6E 40 00
C9 74 40 00 66 6E 40 00 73 6E 40 00 3F E4 E8 00 73 6E 40 00 CE 29 E9 00 E0 2E E9 00 93 B5 E5 00
D9 7E 55 00 99 8F 55 00 30 49 4B 00 31 C4 00 76 EE C3 00 76 10 00 00 00 18 C3 E5 00 9F 4C 8D 7C
94 B2 F1 00 80 80 80 80 80 80 80 80 80 80 80 80 60 B3 72 83 80 80 80 80 00 00 00 00 80 B7 2F 7F
00 00 00 00 00 00 00 00 80 47 41 00 00 00 00 00 59 EC 00 00 A6 4D 40 00 0F 6E 40 00 7E 74 40 00
”  ²  ñ  .  €  €  €  €  €  €  €  €  €  €  €  €  ©  t  š  –  €  €  €  €  .  .  .  .  a  å  /  �
.  .  .  .  .  .  .  .  .  .  .  .  .  .  .  .  '  ì  .  .  .  n  @  .  ~  t  @  .  o  Ê  å  .
.  n  @  .  ~  t  @  .  .  ä  è  .  l  ý  è  .  Ž  ÷  è  .  õ  õ  è  .  v  ô  è  .  w  õ  è  .
ª  ÷  è  .  Q  î  è  .  &  -  C  .  ð  í  è  .  Ô  .  .  .  Ô  .  .  .  Â  M  @  .  -  n  @  .
É  t  @  .  f  n  @  .  s  n  @  .  ?  ä  è  .  s  n  @  .  Î  )  é  .  à  .  é  .  “  µ  å  .
Ù  ~  U  .  ™  �  U  .  0  I  K  .  1  Ä  .  v  î  Ã  .  v  .  .  .  .  .  Ã  å  .  Ÿ  L  �  |
”  ²  ñ  .  €  €  €  €  €  €  €  €  €  €  €  €  `  ³  r  ƒ  €  €  €  €  .  .  .  .  €  ·  /  �
.  .  .  .  .  .  .  .  €  G  A  .  .  .  .  .  Y  ì  .  .  ¦  M  @  .  .  n  @  .  ~  t  @  .

2

3

4 hours ago, Kas Ob. said:

on other hand the you are checking Assigned(LJSONValue) this means it is a an object, so if you walked with the debugger to the point after where you free LJSONObject and tried to access its Value ( LJSONValue.Value ), what does happen ? and as Gunther pointed what value it does holds.

I put a breakpoint on the line

FreeAndNil(IdLogFile)

and LJSONValue is nil

David Heffernan · June 23, 2020

Try finslly still wrong. You must acquire the resource immediately before the try

Foo := TMyObject.Create;

try

Foo.Use;

finally

Foo.Free;

end;

As you have it, if an exception is raised before the variable is assigned, you'll call Free on an uninitialized variable.

Edited June 23, 2020 by David Heffernan

June 23, 2020

Here i reformatted it for you, and as David pointed, i will add few points

1) Don't abuse FreeAndNil, it has right or good places to be used but definitely not with local class/object, where should you do this and only this

Quote

Foo := TMyObject.Create;

try

A := TBar.create;

try

Foo.Use(A);

finally

A.Free;

end;

finally

Foo.Free;

end;

Use Free always to call the destructor and leave the value un-nilled this will evade different problems with memory access when you have just AV with address 0000000

2) You are missing the simple and clean way in design, i mean IdHttp is created locally while you can use it on the parent class TBetFairApi as fields and create it once and free it once, better cleaner and simpler, also shorter code, this goes for The IdLogger and SSL handler, i believe it was similar problem yesterday , right ? i mean different place and different code but has something to do with the way you destruct your classes.

3) Every time you have problem with some line ( memory leak or raised exception ) go and split it, make it clear so the debugger or the logger will show you narrower place to look, like you you line 218, load the result into local string then parse it with TJSONObject.

4) What is FToken ? , is it string, WideString, UnicodeString, Utf8String...., it is better to make that function return String than boolean and save the FToken to a field.

function TBetFairApi.GetToken: Boolean;
var
  LJSONObject: TJSONObject;
  LJSONValue: TJSONValue;
  s:string;
  IdLogFile:TIdLogFile;
begin
  Result := False;
  FidHTTP := TidHTTP.Create(nil);
  try
    FidHTTP.HTTPOptions := FidHTTP.HTTPOptions - [hoForceEncodeParams];
    IdLogFile:=TIdLogFile.Create(FidHTTP);
    try
      FidHTTP.Intercept := IdLogFile;

      s:=ExtractFileName(ParamStr((0)));
      s:=Copy(s,1,Pos('.',s)-1);
      TIdLogFile(FidHTTP.Intercept).Filename := 'c:\'+s+'.log';
      TIdLogFile(FidHTTP.Intercept).Active := true;
      FList := TStringList.Create;
      try
        FIdSSL := TIdSSLIOHandlerSocketOpenSSL.Create;
        try
          FIdSSL.SSLOptions.Method:=sslvTLSv1_2;

          FidHTTP.Request.Accept := 'application/json';
          FidHTTP.Request.CustomHeaders.Clear;
          FidHTTP.Request.CustomHeaders.AddValue('X-Application', cBetfair_AppKey);

          //FList.Clear; // it is already empty
          FList.Add('username=' + FUserID);
          FList.Add('password=' + FPassword);

          FidHTTP.Request.ContentType := 'application/x-www-form-urlencoded';
          FidHTTP.HandleRedirects := true;
          {$IFDEF VER230}
          FidHTTP.IOHandler := FIdSSL;
          {$ENDIF}

          LJSONObject := TJSONObject.ParseJSONValue(FidHTTP.Post(URL_LOGIN, FList)) as TJSONObject;
          if Assigned(LJSONObject) then
          begin
            {$IFDEF VER230}
            LJSONValue := LJSONObject.Get('token').JsonValue;
            {$ELSE}
            LJSONValue := LJSONObject.Values['token'];   
            {$ENDIF}
            if Assigned(LJSONValue) then
            begin
              FToken := LJSONValue.Value;
              //FToken := Trim(FToken+' ');
              // or
              //UniqueString(FToken);
              Result := True;

              LJSONValue.Free;
            end;
            LJSONObject.Free;
          end;
        finally
          {$IFDEF VER230}
          FIdSSL.Free;
          {$ENDIF}
        end;
      finally
        FList.Free;
      end;
    finally
      IdLogFile.Free;
    end;
  finally
    FidHTTP.Free;
  end;
end;

5) try to uncomment one of those lines i added, if that worked then your problem is bigger than simple free object and memory leak in this function, the problem might be where FToken is handled and stored.

6) You are using XE2 so why all of those IFDEF's, write your code and when you are finished make it compatible with different compiler/IDE

7) What is LJSONObject.Values['token'] and how to free if that is needed, because documentation doesn't mention if owner ship belongs to TJSONObject or TJSONPair, that is beyond me, someone else can help here, though it might be yo need to add another local variable for this TJSONPair and free it later, like

LJSONPair := LJSONObject.Get('token');

LJSONValue := LJsonPair.JsonValue;

8 ) your last report shows LJSONValue.Free is not needed, while confirming that TJSONValue is using UnicodeString, try to declare FToken a UnicodeString

Remy Lebeau · June 23, 2020

On 6/23/2020 at 1:48 PM, Alberto Paganini said:

Here is the revised code:

DO NOT free the TJSONValue that is returned by TJSONPair.JsonValue or TJSONObject.Values[]. You do not own that TJSONValue, so you are not responsible for freeing it. The parent TJSONObject owns it, and will free it for you when itself is freed. The only JSON object you should be freeing manually in this code is the TJSONValue that is returned by TJSONObject.ParseJSONValue(). You can also remove manual freeing of the Indy SSL and Log objects by utilizing TComponent ownership semantics. Thus, the only objects you really need to free manually in this code are the TIdHTTP and TStringList objects, eg:

function TBetFairApi.GetToken: Boolean;
var
  LJSONObject: TJSONObject;
  LJSONValue, LJSONToken: TJSONValue;
  {$IFDEF VER230}
  LJSONPair: TJSONPair;
  {$ENDIF}
  IdHTTP: TIdHTTP;
  IdSSL: TIdSSLIOHandlerSocketOpenSSL;
  IdLogFile: TIdLogFile;
  sList: TStringList;
begin
  Result := False;
  FToken := '';

  IdHTTP := TIdHTTP.Create(nil);
  try
    IdHTTP.HTTPOptions := IdHTTP.HTTPOptions + [hoForceEncodeParams];
    IdHTTP.HandleRedirects := True;

    IdSSL := TIdSSLIOHandlerSocketOpenSSL.Create(IdHTTP);
    IdSSL.SSLOptions.SSLVersions := [sslvTLSv1, sslvTLSv1_1, sslvTLSv1_2];
    IdHTTP.IOHandler := IdSSL;

    IdLogFile := TIdLogFile.Create(IdHTTP);
    IdLogFile.Filename := 'c:\' + ChangeFileExt(ExtractFileName(ParamStr(0)), '.log');
    IdLogFile.Active := True;
    IdHTTP.Intercept := IdLogFile;

    IdHTTP.Request.Accept := 'application/json';
    IdHTTP.Request.CustomHeaders.Values['X-Application'] := cBetfair_AppKey;
    IdHTTP.Request.ContentType := 'application/x-www-form-urlencoded';

    sList := TStringList.Create;
    try
      sList.Add('username=' + FUserID);
      sList.Add('password=' + FPassword);

      LJSONValue := TJSONObject.ParseJSONValue(IdHTTP.Post(URL_LOGIN, sList));
      try
        if LJSONValue is TJSONObject then
        begin
          LJSONObject := TJSONObject(LJSONValue);
          {$IFDEF VER230}
          LJSONPair := LJSONObject.Get('token');
          if LJSONPair <> nil then
            LJSONToken := LJSONPair.JsonValue
          else
            LJSONToken := nil;
          {$ELSE}
          LJSONToken := LJSONObject.Values['token'];
          {$ENDIF}
          if LJSONToken <> nil then
            FToken := LJSONToken.Value;
        end;
      finally
        LJSONValue.Free;
      end;
    finally
      sList.Free;
    end;
  finally
    IdHTTP.Free;
  end;

  Result := (FToken <> '');
end;

Edited June 24, 2020 by Remy Lebeau

Alberto Paganini · June 24, 2020

16 hours ago, Kas Ob. said:
Here i reformatted it for you, and as David pointed, i will add few points

1) Don't abuse FreeAndNil, it has right or good places to be used but definitely not with local class/object, where should you do this and only this

Use Free always to call the destructor and leave the value un-nilled this will evade different problems with memory access when you have just AV with address 0000000

2) You are missing the simple and clean way in design, i mean IdHttp is created locally while you can use it on the parent class TBetFairApi as fields and create it once and free it once, better cleaner and simpler, also shorter code, this goes for The IdLogger and SSL handler, i believe it was similar problem yesterday , right ? i mean different place and different code but has something to do with the way you destruct your classes.

3) Every time you have problem with some line ( memory leak or raised exception ) go and split it, make it clear so the debugger or the logger will show you narrower place to look, like you you line 218, load the result into local string then parse it with TJSONObject.

4) What is FToken ? , is it string, WideString, UnicodeString, Utf8String...., it is better to make that function return String than boolean and save the FToken to a field.
function TBetFairApi.GetToken: Boolean;
var
  LJSONObject: TJSONObject;
  LJSONValue: TJSONValue;
  s:string;
  IdLogFile:TIdLogFile;
begin
  Result := False;
  FidHTTP := TidHTTP.Create(nil);
  try
    FidHTTP.HTTPOptions := FidHTTP.HTTPOptions - [hoForceEncodeParams];
    IdLogFile:=TIdLogFile.Create(FidHTTP);
    try
      FidHTTP.Intercept := IdLogFile;

      s:=ExtractFileName(ParamStr((0)));
      s:=Copy(s,1,Pos('.',s)-1);
      TIdLogFile(FidHTTP.Intercept).Filename := 'c:\'+s+'.log';
      TIdLogFile(FidHTTP.Intercept).Active := true;
      FList := TStringList.Create;
      try
        FIdSSL := TIdSSLIOHandlerSocketOpenSSL.Create;
        try
          FIdSSL.SSLOptions.Method:=sslvTLSv1_2;

          FidHTTP.Request.Accept := 'application/json';
          FidHTTP.Request.CustomHeaders.Clear;
          FidHTTP.Request.CustomHeaders.AddValue('X-Application', cBetfair_AppKey);

          //FList.Clear; // it is already empty
          FList.Add('username=' + FUserID);
          FList.Add('password=' + FPassword);

          FidHTTP.Request.ContentType := 'application/x-www-form-urlencoded';
          FidHTTP.HandleRedirects := true;
          {$IFDEF VER230}
          FidHTTP.IOHandler := FIdSSL;
          {$ENDIF}

          LJSONObject := TJSONObject.ParseJSONValue(FidHTTP.Post(URL_LOGIN, FList)) as TJSONObject;
          if Assigned(LJSONObject) then
          begin
            {$IFDEF VER230}
            LJSONValue := LJSONObject.Get('token').JsonValue;
            {$ELSE}
            LJSONValue := LJSONObject.Values['token'];   
            {$ENDIF}
            if Assigned(LJSONValue) then
            begin
              FToken := LJSONValue.Value;
              //FToken := Trim(FToken+' ');
              // or
              //UniqueString(FToken);
              Result := True;

              LJSONValue.Free;
            end;
            LJSONObject.Free;
          end;
        finally
          {$IFDEF VER230}
          FIdSSL.Free;
          {$ENDIF}
        end;
      finally
        FList.Free;
      end;
    finally
      IdLogFile.Free;
    end;
  finally
    FidHTTP.Free;
  end;
end;
5) try to uncomment one of those lines i added, if that worked then your problem is bigger than simple free object and memory leak in this function, the problem might be where FToken is handled and stored.

6) You are using XE2 so why all of those IFDEF's, write your code and when you are finished make it compatible with different compiler/IDE

7) What is LJSONObject.Values['token'] and how to free if that is needed, because documentation doesn't mention if owner ship belongs to TJSONObject or TJSONPair, that is beyond me, someone else can help here, though it might be yo need to add another local variable for this TJSONPair and free it later, like

LJSONPair := LJSONObject.Get('token');

LJSONValue := LJsonPair.JsonValue;

8 ) your last report shows LJSONValue.Free is not needed, while confirming that TJSONValue is using UnicodeString, try to declare FToken a UnicodeString

I will answer your questions here below.

1) I didn't know about the FreeAndNil. I will google it and find out the details of when to use it and when not to.
2) FidHTTP, FList, FIdSSL are private variable of TBetFairApi. As these classes were used in two different procedures I declared this way to share them. No problem, declaring them locally will make the code more readable and maintainable.
3) This is very good advice indeed. I will follow it going forward
4) FToken is a private string which is used in a number of procedures in TBetFairApi. This is also linked to the TBetFairApi.Token property. The result of TBetFairApi.GetToken is a boolean because it was easier for me to do unit testing. I may change it in the future tho.
6) I think this is because this function comes from another version of Delphi hence the IFDEFs. I may change it in the future.
😎 FToken is UnicodeString now
5) I tried your code but it always gives me a leak on the line 235 which are the lines you suggested.

UniqueString(FToken);

or

FToken := Trim(FToken+' ');

here is the report

--------------------------------2020/6/24 16:25:39--------------------------------
A memory block has been leaked. The size is: 116

This block was allocated by thread 0x2FA8, and the stack trace (return addresses) at the time was:
404DA6 [System.pas][System][@GetMem$qqri][3454]
408BBB [System.pas][System][@NewUnicodeString$qqri][19030]
4091AF [System.pas][System][InternalUniqueStringU$qqrr20System.UnicodeString][20569]
E929AA [uBetFair.pas][uBetFair][TBetFairApi.GetToken$qqrv][235]
E92E8C [uTokenBL.pas][uTokenBL][TTokenBL.GetToken$qqrv][30]
E5B593 [Main.pas][Main][TMainForm.FormActivate$qqrp14System.TObject][402]
557ED9 [Vcl.Forms][Forms.TCustomForm.Activate$qqrv]
558F99 [Vcl.Forms][Forms.TCustomForm.CMActivate$qqrr27Winapi.Messages.TWMNoParams]
4B4930 [Vcl.Controls.pas][Vcl.Controls][Controls.TControl.WndProc$qqrr24Winapi.Messages.TMessage][7204]
76EAC431 [Unknown function at gapfnScSendMessage]
76EAC3EE [Unknown function at gapfnScSendMessage]
76EAC380 [Unknown function at gapfnScSendMessage]
76EAC431 [Unknown function at gapfnScSendMessage]
76EAC3EE [Unknown function at gapfnScSendMessage]
76EA4D0E [Unknown function at GetScrollBarInfo]

David Heffernan · June 24, 2020

Cut this code down to the minimum that leaks. Then debug that.

Alberto Paganini · June 24, 2020

Hi Remy,

I tried the code you suggests but I still have leaks.

The following is the report:

--------------------------------2020/6/24 16:32:52--------------------------------
A memory block has been leaked. The size is: 116

This block was allocated by thread 0x2D9C, and the stack trace (return addresses) at the time was:
404DA6 [System.pas][System][@GetMem$qqri][3454]
408BBB [System.pas][System][@NewUnicodeString$qqri][19030]
408DEC [System.pas][System][@UStrFromPWCharLen$qqrr20System.UnicodeStringpbi][19697]
40A0C9 [System.pas][System][@UStrCopy$qqrx20System.UnicodeStringii][24873]
E5CAD3 [Data.DBXPlatform.pas][Data.DBXPlatform][Dbxplatform.TDBXStringBuffer.ToString$qqrv][697]
E8E8BC [Data.DBXJSON.pas][Data.DBXJSON][Dbxjson.TJSONString.Value$qqrv][1541]
E92948 [uBetFair.pas][uBetFair][TBetFairApi.GetToken$qqrv][238]
E92E00 [uTokenBL.pas][uTokenBL][TTokenBL.GetToken$qqrv][30]
E5B593 [Main.pas][Main][TMainForm.FormActivate$qqrp14System.TObject][402]
557ED9 [Vcl.Forms][Forms.TCustomForm.Activate$qqrv]
558F99 [Vcl.Forms][Forms.TCustomForm.CMActivate$qqrr27Winapi.Messages.TWMNoParams]
4B4930 [Vcl.Controls.pas][Vcl.Controls][Controls.TControl.WndProc$qqrr24Winapi.Messages.TMessage][7204]
76EAC431 [Unknown function at gapfnScSendMessage]
76EAC3EE [Unknown function at gapfnScSendMessage]
76EAC380 [Unknown function at gapfnScSendMessage]

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

The allocation number is: 60670

and the following is the line 238

FToken := LJSONValue.Value;

Quote

DO NOT free the TJSONValue that is returned by TJSONPair.JsonValue or TJSONObject.Values[]. ....Thus, the only objects you really need to free manually in this code are the TIdHTTP and TStringList objects, eg:

I am confused here. If I don't have to free TJSONValue why is there this line in the code you propose?

      finally
        LJSONValue.Free;
      end;

What am I missing here?

Alberto Paganini · June 24, 2020

35 minutes ago, David Heffernan said:

Cut this code down to the minimum that leaks. Then debug that.

I did more investigation on the leaking instruction and found out that LJSONValue.Value; is UnicodeString. As FToken was a string I changed FToken into a UnicodeString but this didn't change anything.

I don't know if this can help.

Mahdi Safsafi · June 24, 2020

Quote

I am confused here. If I don't have to free TJSONValue why is there this line in the code you propose?

Obviously its a typos (read again Remy explanation). I believe he means :

LJSONValueObj := TJSONObject.ParseJSONValue(IdHTTP.Post(URL_LOGIN, sList));
try
  if LJSONValueObj is TJSONObject then
  begin
    LJSONObject := TJSONObject(LJSONValueObj);
    {$IFDEF VER230}
    LJSONPair := LJSONObject.Get('token');
    if LJSONPair <> nil then
      LJSONValue := LJSONPair.JsonValue
    else
      LJSONValue := nil;
    {$ELSE}
    LJSONValue := LJSONObject.Values['token'];
    {$ENDIF}
    if LJSONValue <> nil then
      FToken := LJSONValue.Value;
  end;
finally
  LJSONValueObj.Free;
end;

June 24, 2020

Let me get this right, when you used

UniqueString(FToken);

There was only one leak and it was reported on that line, One memory leak ?

June 24, 2020

To save you time, i am not waiting to an answer to the above question.

Use TStringlist to save the the result from your HTTP POST then save it to a file, create new project with and try the same json library to parse that data for the token and see if there will be a memory leak too, i have this feeling that JSON library you are using is buggy and broken.

David Heffernan · June 24, 2020

Dude, just make a minimal example and debug that. Post it here if you want. Really good discipline to learn how to make that minimal example.

Remy Lebeau · June 24, 2020

7 hours ago, Alberto Paganini said:

I tried the code you suggests but I still have leaks.

The stack trace you showed is complaining that the character data for the UnicodeString returned by TJSONString.Value is what is being leaked. That UnicodeString is assigned to your private FToken variable. Which means the TBetFairApi object that is holding that UnicodeString is not freeing that UnicodeString properly. Either because itself is being leaked as well, or you have a bug elsewhere in your code that is overwriting the UnicodeString's internal pointer to the allocated character data. Either way, this is happening after TBetFairApi.GetToken() has exited, so the problem is not in TBetFairApi.GetToken() itself. The leak report is merely showing you the call stack leading up to the memory allocation that leaked, not the call stack leading up to the leak itself.

Quote
I am confused here. If I don't have to free TJSONValue why is there this line in the code you propose?
      finally
        LJSONValue.Free;
      end;
What am I missing here?

That was a typo in my example. I meant to free only the TJSONValue that ParseJSONValue() returns, not the TJSONValue of the 'token' field. I have corrected that mistake in my earlier example.

Edited June 24, 2020 by Remy Lebeau

Alberto Paganini · June 24, 2020

️

1

Quote

The stack trace you showed is complaining that the character data for the UnicodeString returned by TJSONString.Value is what is being leaked. That UnicodeString is assigned to your private FToken variable. Which means the TBetFairApi object that is holding that UnicodeString is not freeing that UnicodeString properly. Either because itself is being leaked as well, or you have a bug elsewhere in your code that is overwriting the UnicodeString's internal pointer to the allocated character data. Either way, this is happening after TBetFairApi.GetToken() has exited, so the problem is not in TBetFairApi.GetToken() itself.

TBetFairApi.GetToken works perfectly now, thank you very much! Now I have to debug the rest because, you are right, there is another leak elsewhere. I will work on a minimal example.

Thank you again, everybody. I have learnt a few new things !

Sign In

One more memory leak and FastMM4

Recommended Posts

Alberto Paganini 3

Share this post

Link to post

Der schöne Günther 316

Share this post

Link to post

Guest

Share this post

Link to post

Alberto Paganini 3

Share this post

Link to post

David Heffernan 2345

Share this post

Link to post

Guest

Share this post

Link to post

Alberto Paganini 3

Share this post

Link to post

Guest

Share this post

Link to post

Alberto Paganini 3

Share this post

Link to post

Der schöne Günther 316

Share this post

Link to post

Guest

Share this post

Link to post

Alberto Paganini 3

Share this post

Link to post

David Heffernan 2345

Share this post

Link to post

Guest

Share this post

Link to post

Remy Lebeau 1394

Share this post

Link to post

Alberto Paganini 3

Share this post

Link to post

David Heffernan 2345

Share this post

Link to post

Alberto Paganini 3

Share this post

Link to post

Alberto Paganini 3

Share this post

Link to post

Mahdi Safsafi 225

Share this post

Link to post

Guest

Share this post

Link to post

Guest

Share this post

Link to post

David Heffernan 2345

Share this post

Link to post

Remy Lebeau 1394

Share this post

Link to post

Alberto Paganini 3

Share this post

Link to post

Create an account or sign in to comment

Create an account