Recent

Author Topic: TCustomShellTreeView.GetFilesInDir memory leak?  (Read 5016 times)

circular

  • Hero Member
  • *****
  • Posts: 4196
    • Personal webpage
TCustomShellTreeView.GetFilesInDir memory leak?
« on: July 03, 2016, 05:23:10 pm »
Hello,

I have noticed a memory leak in LazPaint. After investigating, it seems that it comes from a call to GetFilesInDir of the class TCustomShellTreeView. It populates a TStringList that is freed by my program however there are many little memory blocks that seem to remain allocated.
Conscience is the debugger of the mind

lainz

  • Hero Member
  • *****
  • Posts: 4460
    • https://lainz.github.io/
Re: TCustomShellTreeView.GetFilesInDir memory leak?
« Reply #1 on: July 03, 2016, 05:47:25 pm »
Hi, this is the solution: you need to free the objects in the TStringList.

Code: Pascal  [Select][+][-]
  1. var
  2.   s: TStringList;
  3.   i: integer;
  4. begin
  5.   s := TStringList.Create();
  6.   ShellTreeView1.GetFilesInDir('C:\Temp\', '*.*', [otFolders, otNonFolders], s);
  7.  
  8.   for i:=0 to s.Count-1 do
  9.     s.Objects[i].Free;
  10.   s.Free;  

In line 623 of ShellCtrls:
"AResult will contain TFileItem objects upon return, make sure to free them in the calling routine"

Bart

  • Hero Member
  • *****
  • Posts: 5275
    • Bart en Mariska's Webstek
Re: TCustomShellTreeView.GetFilesInDir memory leak?
« Reply #2 on: July 03, 2016, 05:51:15 pm »
GetFilesInDirs adds objects (TFileItems) to the AResult: TStrings parameter.
It is the responsability of the caller of GetFilesInDir to free these items:

Code: [Select]
    //don't free the TFileItems here, they will freed by the calling routine
    Files.Free;

Maybe setting the TSringList's OwnsObjects property to TRUE will fix the memory leak.

[Edit] Lainz beat me to it.
Bart

wp

  • Hero Member
  • *****
  • Posts: 11858
Re: TCustomShellTreeView.GetFilesInDir memory leak?
« Reply #3 on: July 03, 2016, 06:14:28 pm »
GetFilesInDirs adds objects (TFileItems) to the AResult: TStrings parameter.
It is the responsability of the caller of GetFilesInDir to free these items:
I think this is a hackish, quick-and-dirty approach. A clean dedicated list type for the TFileItems which automatically cleans up would be safer in my eyes.

Bart

  • Hero Member
  • *****
  • Posts: 5275
    • Bart en Mariska's Webstek
Re: TCustomShellTreeView.GetFilesInDir memory leak?
« Reply #4 on: July 03, 2016, 06:23:57 pm »
The problem is the the TFileItems are attached (not copied) to the tree-items, so they should not be automatically freed.
A separate list woul need to be guaranteed to be in sync with the AResult parameter, otherwise the TFileItem objects of the tree items don't match up.

An optional parameter AResultOwnsFileItems: Boolean = False may be of help?

Bart

wp

  • Hero Member
  • *****
  • Posts: 11858
Re: TCustomShellTreeView.GetFilesInDir memory leak?
« Reply #5 on: July 03, 2016, 07:44:12 pm »
The problem is the the TFileItems are attached (not copied) to the tree-items
Maybe I don't understand the code correctly. But PopulateTreeNodeWithFiles and PopulateWithRoot are the two methods of TCustomShellTreeview and TCustomShellListView which call  GetFilesInDir. They use a local stringlist for GetFilesInDir; the stringlist has OwnsObjects=true and is destroyed at the end of these methods. Therefore, if the FileItems were only attached to the treenodes/listitems they would be gone here. This can't be.

This is part of what really happens in TCustomShellTreeView.PopulateTreeNodeWithFiles:
Code: Pascal  [Select][+][-]
  1.       NewNode := Items.AddChildObject(ANode, Files.Strings[i], nil);
  2.       TShellTreeNode(NewNode).FFileInfo := TFileItem(Files.Objects[i]).FileInfo;
This does not attach the TFileItem to the treenode, it copies the TFileItem.FileInfo to the TShellTreeNode.FFileInfo (both of them are TSearchRec). Therefore it is safe to destroy the TFileInfo objects when the stringlist is destroyed.

To be on the safe side an exception should be raised if the stringlist passed to GetFilesInDir has OwnsObjects = false. Of course, this would break existing code where the programmer frees the objects manually in a for loop.

Bart

  • Hero Member
  • *****
  • Posts: 5275
    • Bart en Mariska's Webstek
Re: TCustomShellTreeView.GetFilesInDir memory leak?
« Reply #6 on: July 03, 2016, 08:35:43 pm »
Maybe I don't understand the code correctly. But PopulateTreeNodeWithFiles and PopulateWithRoot are the two methods of TCustomShellTreeview and TCustomShellListView which call  GetFilesInDir. They use a local stringlist for GetFilesInDir; the stringlist has OwnsObjects=true and is destroyed at the end of these methods. Therefore, if the FileItems were only attached to the treenodes/listitems they would be gone here. This can't be.

Yes, you are right.
The TShellTreeNode class just has some extra fields, none of them a class or pointer as I mistakenly believed. My mistake.
It's a long time since I wrote that.


To be on the safe side an exception should be raised if the stringlist passed to GetFilesInDir has OwnsObjects = false.

At the time I considered setting OwnsObjects to True inside TCustomShellTreeView.GetFilesInDir, but that cannot be done, since the parameter is defined as being a TStrings, which does not have this property!
So, neither can an exception be raised (nor can my previous suggestion be implemented).

The behaviour is well documented in the code.
Hoovering the mouse over a call to TCustomShellTreeView.GetFilesInDir however does not show this warning in the hint, whereas it propery display all the other text that is in the comments right above it's definition.

Maybe the proper xml file (in docs/xml) should be updated?

Bart

wp

  • Hero Member
  • *****
  • Posts: 11858
Re: TCustomShellTreeView.GetFilesInDir memory leak?
« Reply #7 on: July 03, 2016, 08:56:43 pm »
Just some brainstorming:

Deprecate the GetFilesInDir and overload it with a new variant which gets a "TFileItemList" (a TObjectList of TFileItem) as a parameter? The TFileItemList could have a readonly property FileName[AIndex] and/or a method ExtractFileNames which writes the filenames to a TStrings for those who need the bare stringlist from GetFilesInDir.

Bart

  • Hero Member
  • *****
  • Posts: 5275
    • Bart en Mariska's Webstek
Re: TCustomShellTreeView.GetFilesInDir memory leak?
« Reply #8 on: July 03, 2016, 09:45:09 pm »
IIRC then this class function uses TStrings for it's parameter to make it as generic as possible?

Better ask on devel list.
I don't mind implementing it.

Bart

circular

  • Hero Member
  • *****
  • Posts: 4196
    • Personal webpage
Re: TCustomShellTreeView.GetFilesInDir memory leak?
« Reply #9 on: October 22, 2016, 06:17:10 pm »
Somehow I had missed this answer. Thank you very much for the explanation. Learning something new every day.  :)
Conscience is the debugger of the mind

 

TinyPortal © 2005-2018