How is this a memory leak?

Question

FastMM reports this line as the source of a memory leak : ``` StrClassName := MidStr (curLine, length(START_OF_CLASSNAME)+1, length(curline)+1) ``` What's up with `Copy` and `MidStr`? Is this only a Delphi 2007 compiler bug, or do the later versions also have this problem? Here is the link to the copy of the FastMM report, and an image of how my applications displays those kinds of reports. See, in order to display the nodes in `VirtualTreeView` I need a new data type. I call it TMemoryLeak. When I parse the report, I give my `TMemoryLeak` a class name, the callstack, it's size etc. But when the app shuts down, and FastMM kicks in, the copy line from above seems to leak memory. I deallocate the callstack the size, the whole object, but the ClassName field which is a string always leeks memory. Update (from comment) Here are the declarations and constructors and deconstructors. As for the lifetime- the objects' deconstructors are called as soon as the objects are used to display the no…

Accepted Answer

The explanation that makes sense is that you have a memory leak.

I think you have a mis-understanding of how FastMM leak reporting works. You seem to infer from the leak report that `Copy`, `MidStr` etc. are responsible for the leak. That's not the case. A leak is reported when memory is allocated but not subsequently deallocated. In the case of functions like `Copy` and `MidStr`, their job is to create new strings which naturally involves allocation of memory. The leak is reported because the memory that was created to hold the string buffer was not deallocated. It is not the fault of `Copy` or `MidStr`, the allocating function, when the rest of the code fails to deallocate that memory.

Delphi 2007 is a mature product and the memory management for strings is known to be correct. Perhaps you do some manual memory copying that bypasses the reference counting for strings. Are you setting some variables/fields to `nil` with a call to `FillChar`? Are you disposing of a record with `FreeMem` instead of `Dispose`? The former will not decrement the strings reference count. Something like this is the likely cause of the leak.

Looking at extracts of the code you posted, this is a problem:

```
destructor TMemoryLeakList.destroy;
begin
  Clear;
end;
```

You have failed to call the inherited destructor. Which means that the members of the list won't be destroyed. Which explains why your string is not destroyed.

In fact you don't need to provide a destructor for the list class. Simply remove it and let the inherited `TObjectList` destructor do the work. Since `OwnsObjects`defaults to `True`, any members of the list are destroyed as soon as they are removed from the list, and when the list itself is destroyed.

If your `Clear` method actually cleared the list, then that would happen. But your `Clear` isn't a real `Clear`. A real `Clear` in a container is expected to remove all members. You should remove your `Clear` and rely on the inherited version.

In `TMemoryLeak`, you also fail to call the inherited destructor. And also fail to destroy the string list instance that is owned by that class.

To summarise, I'd write these constructors and destructors like this:

```
constructor TMemoryLeak.Create;
begin
  inherited;
  fCallStack := TStringList.Create;
end;

destructor TMemoryLeak.Destroy;
begin
  fCallStack.Free;
  inherited;
end;

constructor TMemoryLeakList.Create;
begin
  inherited;//by default OwnsObjects is set to True, list manages member lifetime
  fSortType :=stID;
  fSortDirection :=sdAsc;
end;
```

And then remove the `destructor`, and remove the `Clear` method. The versions inherited from `TObjectList` suffice.

In the comments you state:

The objects' destructors are called as soon as the objects are used to display the node tree. After that they are obsolete, and are deallocated (I hope).

I'd say there's a good chance that this is not helping. Since you created the object list in `OwnsObjects` mode, you should not be destroying the members of the list at all. You've asked the list itself to do that. You can't both do it. And the "I hope" comment doesn't fill me with much confidence that this code is correct.

Since we can't see all of your code, I'm far from sure that this is the entirety of the problems with it.

The bottom line is that your code has leaks. Trust in FastMM!

How is this a memory leak?

Solution

Problem