How to avoid function result being destroyed by Free inside function?

Question

This code creates an AV:

function PAIsMainAppWindow(Wnd: THandle): Boolean;
var
  ParentWnd: THandle;
  ExStyle: DWORD;
begin
  if IsWindowVisible(Wnd) then
  begin
    ParentWnd := THandle(GetWindowLongPtr(Wnd, GWLP_HWNDPARENT));
    ExStyle := GetWindowLongPtr(Wnd, GWL_EXSTYLE);
    Result := ((ParentWnd = 0) or (ParentWnd = GetDesktopWindow)) and
      ((ExStyle and WS_EX_TOOLWINDOW = 0) or (ExStyle and WS_EX_APPWINDOW <> 0));
  end
  else
    Result := False;
end;

function PAEnumTaskWindowsProc(Wnd: THandle; List: TStrings): Boolean; stdcall;
var
  Caption: array [0..1024] of Char;
begin
  if PAIsMainAppWindow(Wnd) and (GetWindowText(Wnd, Caption, SizeOf(Caption)) > 0) then
    List.AddObject(ExtractFileName(GetProcessNameFromWnd(Wnd)), Pointer(Wnd));
  Result := True;
end;

function PAGetTaskWindowHandleFromProcess(const AProcessName: string): THandle;
var
  sl: TStringList;
  i: Integer;
begin
  Result := 0;

  sl := TStringList.Create(True); // stringlist owns objects
  try
    if EnumWindows(@PAEnumTaskWindowsProc, LPARAM(sl)) then
    begin
      for i := 0 to sl.Count - 1 do
      begin
        if SameText(AProcessName, sl[i]) then
        begin
          Result := THandle(sl.Objects[i]);
          BREAK;
        end;
      end;
    end;
  finally
    sl.Free; // AV!
  end;
end;

ChromeHandle := PAGetTaskWindowHandleFromProcess('chrome.exe');

It is clear that the AV occurs because freeing the stringlist also destroys the function result. But how can this be avoided?

Answer 1

First of all, let's look at the actual code. The string list does not hold objects. It holds window handles. So OwnsObjects is simply not appropriate. That's going to assume that the things in Objects[] are Delphi instances of classes, and call Free on these instances. That is where the failure occurs.

You don't own these window handles, and you therefore should not attempt to destroy them.

So, don't set OwnsObjects to True and the problem goes away. That is replace this line:

sl := TStringList.Create(True); // stringlist owns objects

with this:

sl := TStringList.Create;

Further, you are casting these objects to THandle. That is wrong, not that it actually matters. Semantically though, these are window handles, so cast them to HWND. Indeed, everywhere you use THandle you should use HWND.

There are other errors too. When you call use GetWindowText you pass the size of the buffer rather than its length. That means you are lying about how long the buffer is. Because these are wide characters, the buffer is half as long as you claim it to be. Looking for windows that are parented to the desktop window feels wrong.

---

Let's assume, for the sake of argument, that your string list really did contain objects. In that case, and in an ideal world, the string list class would offer an Extract method which is the conventional method to remove an object from an owning container without destroying that object. So instead you can perform the OwnsObjects shuffle.

if SameText(AProcessName, sl[i]) then
begin
  sl.OwnsObjects := False;
  Result := TSomeObject(sl.Objects[i]);
  sl.Objects[i] := nil;
  sl.OwnsObjects := True;
  BREAK;
end;

If you'd rather, you can set OwnsObjects to be False when you create the string list, and only set it to True just before you call Free on it.