Jump to content
aehimself

TTreeNode leak when VCL styles are active

Recommended Posts

9 hours ago, balabuev said:

I've replaced the treeview with a frame to remove the mess with the second pair of unneeded scrollbar windows. I've simulated some resource allocation on WM_CREATE in a frame, which is then freed on WM_DESTROY, similar to tree view.

 

I guess the issue would still be present if you now replace the ListView with anything that has scrollbars including a treeview.  Is that right?

Share this post


Link to post
1 hour ago, pyscripter said:

How about this one?

Yes, something like this. However, I'll still prefer to leave DoRemoveControl as is, and deal with TScrollingStyleHook.Destroy instead to defer the destruction of two TScrollWindow objects only.

 

And, since TScrollWindow objects (controls) uses CreateParented constructor, which utilizes ParentWindow property (instead of Parent property), they cannot be occasionnaly destroyed by their parents. So, this simplifies the task a bit, and we can ommit usual book keeping involving FreeNotification, etc.

 

procedure FreeScrollWindowAsync(W: TScrollingStyleHook.TScrollWindow);
begin
  if W.HandleAllocated then
  begin
    ShowWindow(W.Handle, SW_HIDE);
    TThread.ForceQueue(nil, procedure
    begin
      W.Free;
    end);
  end
  else
    W.Free;
end;

destructor TScrollingStyleHook.Destroy;
begin
  FInitingScrollBars := True;
  if FVertScrollWnd <> nil then
  begin
    FVertScrollWnd.StyleHook := nil;
    FreeScrollWindowAsync(FVertScrollWnd);
  end;
  if FHorzScrollWnd <> nil then
  begin
    FHorzScrollWnd.StyleHook := nil;
    FreeScrollWindowAsync(FHorzScrollWnd);
  end;
  FInitingScrollBars := False;
  inherited;
end;


 

Share this post


Link to post
1 hour ago, pyscripter said:

I guess the issue would still be present if you now replace the ListView with anything that has scrollbars including a treeview.  Is that right?

I think similarly.

Share this post


Link to post
6 minutes ago, Attila Kovacs said:

Destroying a window from a different thread? Does it work?

AFAIK, this internally executed in the context of the main thread (via PostMessage).

Share this post


Link to post
26 minutes ago, balabuev said:

AFAIK, this internally executed in the context of the main thread (via PostMessage).

Code queued with ForceQueue is executed in the main thread with the same mechanism as Thread.Synchronize and not with PostMessage.

Share this post


Link to post
40 minutes ago, pyscripter said:

Code queued with ForceQueue is executed in the main thread with the same mechanism as Thread.Synchronize and not with PostMessage.

 

It's looks like carefully hidden stuff, but PostMessage is there:

 

class procedure TThread.Synchronize(...);
begin
  //...
  if Assigned(WakeMainThread) then
    WakeMainThread(SyncProcPtr.SyncRec.FThread);
  //...
end;

procedure TApplication.WakeMainThread(Sender: TObject);
begin
  PostMessage(Handle, WM_NULL, 0, 0);
end;

procedure TApplication.WndProc(var Message: TMessage);
begin
  //...
  WM_NULL:
    CheckSynchronize; // Executes accumulated tasks.
  //...
end;

 

There simply no other ways, main thread executes message loop infinitely.

 

  • Like 2

Share this post


Link to post
Posted (edited)

What we now need is a bug report at Embarcadero, with a detailed description of the issue reproduction steps and the potential fixes.  @balabuevAre you planning to submit one?

Edited by pyscripter

Share this post


Link to post
1 hour ago, pyscripter said:

Are you planning to submit one?

Ohh, no. I can vote if someone will report.

Share this post


Link to post
Posted (edited)

Is it tested now?

Btw. interestingly WM_NCDESTROY's are still coming for all of the windows.

 

The chained list theory fails? also as switching the freeing order of the scrollbars (vertical,horizontal to horizontal,vertical) free's the horizontal fine, then vertical becomes the next in the z-order and freeing it breaks the message chain again. If it has actually something to do with the Z-Order.

 

Edit: Just to make sure, I've compiled the testapp with D2007 and D5, and run it even on XP, it's always the same.

Edited by Attila Kovacs

Share this post


Link to post
Posted (edited)
4 hours ago, Attila Kovacs said:

Is it tested now?

 

Not really works, because enqueued via ForceQueue tasks are not executed after the main form close. So, need to replace it with some explicit implementation:

 

type
  TScrollingStyleHook = class(TMouseTrackControlStyleHook)
  public type
    //...
    TAsyncDeletion = class
    private
    class var
      FItems: TList;
      class procedure FreeItems;
    public
      class destructor Destroy;
      class procedure  FreeAsync(O: TObject);
    end;
    //...
  end;

class destructor TScrollingStyleHook.TAsyncDeletion.Destroy;
begin
  TThread.RemoveQueuedEvents(nil, FreeItems);
  FreeItems;
end;

class procedure TScrollingStyleHook.TAsyncDeletion.FreeAsync(O: TObject);
begin
  if FItems = nil then
  begin
    FItems := TList.Create;
    TThread.ForceQueue(nil, FreeItems);
  end;
  FItems.Add(O);
end;

class procedure TScrollingStyleHook.TAsyncDeletion.FreeItems;
var
  itm: TObject;
begin
  if FItems <> nil then
  begin
    for itm in FItems do
      TObject(itm).Free;
    FreeAndNil(FItems);
  end;
end;

 

This version works fine.

 

 

 

Edited by balabuev
  • Like 3

Share this post


Link to post
On 3/10/2021 at 4:36 PM, balabuev said:

This version works fine.

I am quite sure that, for the TAsyncDeletion trick to work in Tokyo, you need to make one more change, in TControls.Destroy.

{ in Vcl.Controls }
destructor TControl.Destroy;
begin
  if Application <> nil then // <-- check needed
    Application.ControlDestroyed(Self);

  // ...
end;

Otherwise, when closing application, you will have access violation and different memory leak (TScrollWindows) because

  • TAsyncDeletion.FreeItems is called when Application is already nil,
  • and App will crash when first item in the list is destroyed.
     

Share this post


Link to post
Guest

voted

 

hug

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

×