Jump to content
aehimself

TTreeNode leak when VCL styles are active

Recommended Posts

Yet another evidence: It's sufficient simply to reorder children in Z-order for bug to disappear. And Z-order is the order in which EnumChildWindows function reports child windows. And so, it's most probably the order in which DestroyWindow function processes child windows.

 

function MoveScrollWindows(hwnd: HWND; lParam: LPARAM): BOOL; stdcall;
var
  c: TWinControl;
begin
  c := FindControl(hwnd);
  if c is TScrollingStyleHook.TScrollWindow then
    SetWindowPos(hwnd, HWND_TOP, 0, 0, 0, 0, SWP_NOMOVE + SWP_NOSIZE);
  Result := True;
end;

procedure TMainForm.FormClose(Sender: TObject; var Action: TCloseAction);
begin
  EnumChildWindows(ClientPanel.Handle, @MoveScrollWindows, 0);
end;

 

  • Like 2

Share this post


Link to post
  On 3/8/2021 at 9:36 AM, balabuev said:

With VCL style:

Detail noticed: Order is initially ok, IF the default style is not Windows (but for example Iceberg Classico) and only becomes problematic after switching to another VCL Style (other than Windows).
 

// order with default VCL style <> Windows, immediately after startup
TScrollingStyleHook.TScrollWindow
TScrollingStyleHook.TScrollWindow
TScrollingStyleHook.TScrollWindow
TScrollingStyleHook.TScrollWindow
TListView
TTreeView

// order after switching to VCL style <> Windows
TListView
TScrollingStyleHook.TScrollWindow
TScrollingStyleHook.TScrollWindow
TScrollingStyleHook.TScrollWindow
TScrollingStyleHook.TScrollWindow
TTreeView

( The order we always get with the workaround is the same as the initial order without workaround. )
 

  • Like 2

Share this post


Link to post
  On 3/8/2021 at 11:22 AM, Gustav Schubert said:

Detail noticed: Order is initially ok, IF the default style is not Windows (but for example Iceberg Classico) and only becomes problematic after switching to another VCL Style (other than Windows).
 

// order with default VCL style <> Windows, immediately after startup
TScrollingStyleHook.TScrollWindow
TScrollingStyleHook.TScrollWindow
TScrollingStyleHook.TScrollWindow
TScrollingStyleHook.TScrollWindow
TListView
TTreeView

// order after switching to VCL style <> Windows
TListView
TScrollingStyleHook.TScrollWindow
TScrollingStyleHook.TScrollWindow
TScrollingStyleHook.TScrollWindow
TScrollingStyleHook.TScrollWindow
TTreeView

( The order we always get with the workaround is the same as the initial order without workaround. )
 

The question still remains why the change in the order results in the Treeview not receiving the WM_DESTROY message.  Why is the order important anyway in this issue?

Edited by pyscripter

Share this post


Link to post

It's the scrollbars, in TScrollingStyleHook.InitScrollBars changing

 

     SetWindowPos(FVertScrollWnd.Handle, HWND_TOP, Control.Left + Left, Control.Top + Top,
       Right - Left, Bottom - Top, SWP_NOREDRAW);

to

 

     SetWindowPos(FVertScrollWnd.Handle, HWND_TOPMOST, Control.Left + Left, Control.Top + Top,
       Right - Left, Bottom - Top, SWP_NOREDRAW);

(both, vertical and horizontal) 

 

also solves the problem without reordering the windows nor blocking the VCL to recreate the wnd's with stopping the CM_STYLECHANGED broadcast.

However the re-broadcasting is still results in multiple recreatewnd's which is slowing the theme-change down massively, the solution is not as easy as just

stopping re-broadcasting. 
 

Yes, the question remains still the same. Are the handles screwed up on the nested recreatewnd's or are there any Z-Order logic in Windows' internal wndproc or

are those WM_DESTROY messages with invalid handles which are arriving was one of the lost TListView.

Share this post


Link to post

Just a note: HWND_TOPMOST only applies to top level windows, not child windows. So, it's not clear, what SetWindowPos really do in this case.

Edited by balabuev

Share this post


Link to post

I see this in TScrollingStyleHook.TScrollWindow.CreateParams:

 

Params.Style := Params.Style or WS_CHILDWINDOW ...

 

  • Like 1

Share this post


Link to post

can we test it on other than W10?

it's very strange

 

 

  Reveal hidden contents
Edited by Attila Kovacs

Share this post


Link to post

If I stop broadcasting the CM_STYLECHANGED it looks similar at the end as with HWND_TOPMOST.

I'm just wondering what regression that pulls with it.

 

The double destroy on the same handle appearing here too.

 

  Reveal hidden contents

 

Edited by Attila Kovacs

Share this post


Link to post
Guest

in this moment a "spoiler tag" help us 🤔 

 

hug

Share this post


Link to post
Guest

just type

SPOILER]

you post or img etc..

/SPOILER]

 

valid for: CODE, QUOTE, IMG, URL, and others command on post / edit

valid for all forums using basic concept on post messages.

 

note: for formated text sometimes is necessary remove the control-formatation... just copy to Notepad and Copy/Paste here

hug

Edited by Guest

Share this post


Link to post
Guest

no. you dont, for sure.

now, go back to the subject... if dont, Daniel appears here :classic_cheerleader:

 

hug

Share this post


Link to post

I've solved this puzzle :classic_tongue:

 

In short - this is not about handles recreation, this is about handles destruction. And the order of destruction plays essential role here.

 

First, I want to recall again, that the process starts in form's destructor, which calls DestroyWindow function. And as specified in MSDN, DestroyWindow will actually destory all children window handles recursively, and each of destorying window should receive WM_DESTROY message. It vital to understand that the destruction of a handle does not imply the destruction of the corresponding control. Control can exist without its handle. Opposite is not possible - each control destroys its own handle during the control destruction.

 

So, from this viewpoint, form destruction is a two stage process:

  1. A single call to DestroyWindow in a form destructor deallocates all handles of all child controls on the form.
  2. And only after that, child controls are destroyed recursively.

 

So, what is happening in the demo. Given the following panel child order:

 

image.png.0ebcb5f3d44660b19a6f60ed60160b5a.png.070331aa9709c351a8cd674fdbcf8bdf.png

 

  • List view is the first child control of the panel, which will receive the WM_DESTROY message due to form's DestroyWindow call.
  • List view's TWinControl.WMDestroy message handler will eventually call the following: TStyleManager.Notification(snControlDestroyed, Self);
  • The above notification will force the style hook to destroy two owned TScrollWindow controls, which at this time have its handles allocated, and so, each of the TScrollWindow control, will call DestroyWindow recursively.

 

So, this recursive calls of the DestroyWindow (where the nested function call destroys the window, which was already enqueued for destruction in outer function call) is the cause of the bug.

 

This may be either: illegal usage of DestroyWindow function by Delphi or Windows bug. I don't know.

 

Part two is comming, in which I'll reproduce this strange behavior with a simple three buttons placed on a panel (no relation to VCL Styles at all).

 

 

 

Edited by balabuev
  • Like 2
  • Thanks 1

Share this post


Link to post

 some of the trouble is the windows need destroyed downto in for loop. Destroying the top window first changes the indexes of the window. So with for

LItem in LItem in FRegisteredStyleHooks do
    LItem.Value.Free;  We need a "downin" 

source VCl.themes line 7510.  My take.  

Edited by Pat Foley
add source

Share this post


Link to post
  On 3/8/2021 at 8:34 PM, balabuev said:

Given the following panel child order:

Shouldn't the ListView TScrollWindows be above the ListView after it gets recreated? This is what happens when it is originally created Why are they not on top?

 

By the way readers of this thread may be interested in a related old post of mine: 

 

Edited by pyscripter

Share this post


Link to post

Part 2:

 

The demo with simple three buttons on a panel.

  • RunTestClick destroys the panel handle using DestroyWindow function.
  • Button1 is the first in Z-order, and thats why it will first receive WM_DESTROY message. In the message handler it will destroy Button2 recursively.
  • And the test shows, that in this case Button3 will not receive WM_DESTROY.

ButtonsTest.zip

 

 

Edited by balabuev
  • Like 4

Share this post


Link to post

@balabuev Nice work! I replaced the buttons with TListViews with some Nodes and the leak appears. Do we know where the dog is buried or is it time to ask Mr. Chen?

 

Looks like it's only happening if we are destroying the next window in the order, then it's stops all the propagation of WM_DESTROY's, like if there were an exception in the window manager.

 

If we skip one control it's fine again (4 buttons, freeing Nr. 3 on Tag1, not Nr.2).

Edited by Attila Kovacs
  • Like 1

Share this post


Link to post
  On 3/9/2021 at 5:26 AM, balabuev said:

Part 2:

 

The demo with simple three buttons on a panel.

  • RunTestClick destroys the panel handle using DestroyWindow function.
  • Button1 is the first in Z-order, and thats why it will first receive WM_DESTROY message. In the message handler it will destroy Button2 recursively.
  • And the test shows, that in this case Button3 will not receive WM_DESTROY.

ButtonsTest.zip

 

 

We now understand the cause of this issue.  Great work @balabuev.  But now we need to come up with a good fix to propose to Embarcadero. 

 

My earlier question "Shouldn't the ListView TScrollWindows be above the ListView after it gets recreated?" still stands.  If the order was not messed up, the error would not occur.   Or can the recursive calling of DestroyWindow window be avoided without introducing other bugs?

 

Although in the case of the ListView the RecreateWnd as a response ot CM_STYLECHANGED the call to RecreateWnd appears to be redundant, RecreateWnd happens often with many controls and for all sort of reasons (e.g. a property change).  So Vcl should be able to handle such calls robustly.

 

Edited by pyscripter

Share this post


Link to post

Update:  This does not work well.:classic_blush:

 

One suggestion I have is the following:

 

Change TStyleEngine.DoRemoveControl(Control: TWinControl) from:

 

class procedure TStyleEngine.DoRemoveControl(Control: TWinControl);
var
  I: Integer;
  LControl: TControl;
begin
  if (FControls <> nil) and FControls.ContainsKey(Control) then
  begin
    for I := 0 to Control.ControlCount - 1 do
    begin
      LControl := Control.Controls[I];
      if LControl is TWinControl then
        DoRemoveControl(TWinControl(LControl));
    end;
    FControls.Items[Control].Free;
    FControls.Remove(Control);
  end;
end;

to

class procedure TStyleEngine.DoRemoveControl(Control: TWinControl);
var
  I: Integer;
  LControl: TControl;
begin
  if not (csRecreating in Control.ControlState) and (FControls <> nil) and FControls.ContainsKey(Control) then
  begin
    for I := 0 to Control.ControlCount - 1 do
    begin
      LControl := Control.Controls[I];
      if LControl is TWinControl then
        DoRemoveControl(TWinControl(LControl));
    end;
    FControls.Items[Control].Free;
    FControls.Remove(Control);
  end;
end;

or even to:

class procedure TStyleEngine.DoRemoveControl(Control: TWinControl);
begin
  if not (csRecreating in Control.ControlState) and (FControls <> nil) and FControls.ContainsKey(Control) then
  begin
    FControls.Items[Control].Free;
    FControls.Remove(Control);
  end;
end;

There seems to be no reason to destroy the hook if the control is recreating. Also hooks of child controls would be removed when they get destroyed, so there is no need to do this in advance.

Do you see anything wrong with the above?  With the above change the test app runs fine and without memory leaks.  And the change would save a lot of hook destruction and reconstruction.

 

To test copy Vcl.Styles.pas, StyleAPI.inc and StyleUtils.inc to the project source directory, make the change and re-build the project.

 

Edited by pyscripter

Share this post


Link to post
  On 3/9/2021 at 8:17 AM, Attila Kovacs said:

If we skip one control it's fine again (4 buttons, freeing Nr. 3 on Tag1, not Nr.2).

 

I can imagine a linked list iteration inside DestroyWindow function, like:

cur := FirstChild;
while cur <> nil do
begin
  nxt := cur.Next;
  ProcessNode(cur);
  cur := nxt;
end;

 

If ProcessNode will (as in our case) recursively delete the next node from the list - the function will fail, because it already fetched the pointer to the next node into its local variable.

However, if ProcessNode will delete the next next node (the node after next) - nothing bad will happen.

 

  On 3/9/2021 at 12:05 PM, pyscripter said:

Shouldn't the ListView TScrollWindows be above the ListView after it gets recreated?

 

I think it's not necessary. For example, when scrollbars are hidden these controls may stay untouched. On the other hand this may be the simplest fix.

 

 

 

  • Like 1

Share this post


Link to post

Looks like on RecreateWnd it's necessary to free the two themed scrollbars, but at the moment the window manager kicks in with destroying all controls, it becomes a problem.

I've tested on W7, it's the same.

 

I like the workaround to eliminate the message broadcasting like @pyscripter suggested as this seems to have more advantage.

 

Otherwise, creating a transparent container between TListView and its Parent could eventually solve the issue.

I'd say give it as-is to Emba, let's see how they think about it, maybe they have more comments in the VCL.

 

Share this post


Link to post

I did a bit of research to assess whether other parts of Vcl could be suffering from the same issue.  The only controls that recreate themselves in response to the CM_STYLECHANGED message are the TListView and the ActionManager controls (ActionToolbar etc.).  I think there are not scrollbars and the like there, but I wonder why there is a need to do that.  So the issue may be confined to the ListView and if the call to RecreateWnd can be avoided that would be the simplest solution.

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

×