Jump to content
Alberto Paganini

One more memory leak and FastMM4

Recommended Posts

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

Share this post


Link to post
Guest

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.

Share this post


Link to post
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 by Alberto Paganini

Share this post


Link to post

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

  • Thanks 1

Share this post


Link to post
Guest

@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.

Share this post


Link to post
 
 
 
 
 
✌
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.

Share this post


Link to post
Guest

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.

Share this post


Link to post
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

Share this post


Link to post

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.

Share this post


Link to post
Guest
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. 

Share this post


Link to post
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

Share this post


Link to post

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 by David Heffernan
  • Like 1

Share this post


Link to post
Guest

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

 

Share this post


Link to post
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 by Remy Lebeau

Share this post


Link to post
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]
 

Share this post


Link to post

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?

 

Share this post


Link to post
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.

 

Share this post


Link to post
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;

 

Share this post


Link to post
Guest

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 ?

 

Share this post


Link to post
Guest

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.

Share this post


Link to post

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. 

  • Like 2

Share this post


Link to post
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 by Remy Lebeau
  • Like 1
  • Thanks 1

Share this post


Link to post
 
 
✌
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 !

Share this post


Link to post

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

×