TTreeNode leak when VCL styles are active

[balabuev 102]

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;

 

[Gustav Schubert 25]

1 hour ago, 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. )
 

[pyscripter 694]

58 minutes ago, 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 March 8, 2021 by pyscripter

[Attila Kovacs 631]

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.

[balabuev 102]

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 March 8, 2021 by balabuev

[Attila Kovacs 631]

@balabuev The themed scollbar is just a parented window, not a child. Or am I wrong on that? 

[balabuev 102]

I see this in TScrollingStyleHook.TScrollWindow.CreateParams:

 

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

 

[Attila Kovacs 631]

can we test it on other than W10?

it's very strange

 

 

 

 

themed HWND_TOP                                               themed HWND_TOPMOST
run
Creating   : 31984546  MainForm - TMainForm                   Creating   : 74388998  MainForm - TMainForm                            
Creating   : 9639484   ClientPanel - TPanel                   Creating   : 36507224  ClientPanel - TPanel                            
Creating   : 73539832  ListView1 - TListView                  Creating   : 51584518  ListView1 - TListView                           
Creating   : 17500942  ImageView2 - TListView                 Creating   : 12720820  ImageView2 - TListView                          
Creating   : 26088278  Panel1 - TPanel                        Creating   : 74388594  Panel1 - TPanel                                 
Creating   : 33232884   - TGroupButton                        Creating   : 35981286   - TGroupButton                                 
Creating   : 42935832  SelectThemeRadioGroup - TRadioGroup    Creating   : 15803724  SelectThemeRadioGroup - TRadioGroup             
Creating   : 27731964   - TGroupButton                        Creating   : 39589402   - TGroupButton                                 
Creating   : 59448192   - TGroupButton                        Creating   : 14879916   - TGroupButton                                 
                                                                                                                                                                 
theme change (broadcast storm)                                theme change                                                                         
Destroying : 33232884   - TGroupButton                        Destroying : 35981286   - TGroupButton                                 
Destroying : 27731964   - TGroupButton                        Destroying : 39589402   - TGroupButton                                 
Destroying : 59448192   - TGroupButton                        Destroying : 14879916   - TGroupButton                                 
Destroying : 42935832  SelectThemeRadioGroup - TRadioGroup    Destroying : 15803724  SelectThemeRadioGroup - TRadioGroup             
Destroying : 26088278  Panel1 - TPanel                        Destroying : 74388594  Panel1 - TPanel                                 
Destroying : 73539832  ListView1 - TListView                  Destroying : 51584518  ListView1 - TListView                           
Destroying : 17500942  ImageView2 - TListView                 Destroying : 12720820  ImageView2 - TListView                          
Destroying : 42798600  ??                                     Destroying : 39461266  ??                                              
Destroying : 9639484   ClientPanel - TPanel                   Destroying : 36507224  ClientPanel - TPanel                            
Destroying : 31984546  MainForm - TMainForm                   Destroying : 74388998  MainForm - TMainForm                            
                                                                                                                                                                 
Creating   : 32050082  MainForm - TMainForm                   Creating   : 74454534  MainForm - TMainForm                            
Creating   : 9705020   Panel1 - TPanel                        Creating   : 36572760  Panel1 - TPanel                                 
Creating   : 42864136   - TGroupButton                        Creating   : 39526802   - TGroupButton                                 
Creating   : 20647324   - TGroupButton                        Creating   : 33496018   - TGroupButton                                 
Creating   : 73605368   - TGroupButton                        Creating   : 51650054   - TGroupButton                                 
Creating   : 17566478  SelectThemeRadioGroup - TRadioGroup    Creating   : 12786356  SelectThemeRadioGroup - TRadioGroup             
Creating   : 34279264  ClientPanel - TPanel                   Creating   : 28256252  ClientPanel - TPanel                            
Creating   : 26153814  ListView1 - TListView                  Creating   : 74454130  ListView1 - TListView                           
Creating   : 59513728  ImageView2 - TListView                 Creating   : 14945452  ImageView2 - TListView                          
Creating   : 40573006   - TScrollingStyleHook.TScrollWindow   Creating   : 24972548   - TScrollingStyleHook.TScrollWindow            
Creating   : 29364520   - TScrollingStyleHook.TScrollWindow   Creating   : 69866168   - TScrollingStyleHook.TScrollWindow            
                                                                                                                                                                 
Destroying : 29364520  - TScrollingStyleHook.TScrollWindow    Destroying : 69866168   - TScrollingStyleHook.TScrollWindow            
Destroying : 26153814  ListView1 - TListView                  Destroying : 74454130  ListView1 - TListView                           
                                                                                                                                                                 
Creating   : 26219350  ListView1 - TListView                  Creating   : 74519666  ListView1 - TListView                           
Creating   : 29430056   - TScrollingStyleHook.TScrollWindow   Creating   : 69931704   - TScrollingStyleHook.TScrollWindow            
                                                                                                                                                                 
Destroying : 40573006   - TScrollingStyleHook.TScrollWindow   Destroying : 24972548   - TScrollingStyleHook.TScrollWindow            
Destroying : 59513728  ImageView2 - TListView                 Destroying : 14945452  ImageView2 - TListView                          
Destroying : 33298420  ??                                     Destroying : 36046822  ??                                              
                                                                                                                                                                 
Creating   : 59579264  ImageView2 - TListView                 Creating   : 15010988  ImageView2 - TListView                          
Creating   : 40638542   - TScrollingStyleHook.TScrollWindow   Creating   : 25038084   - TScrollingStyleHook.TScrollWindow            
                                                                                                                                                                 
Destroying : 29430056   - TScrollingStyleHook.TScrollWindow   Destroying : 69931704   - TScrollingStyleHook.TScrollWindow            
Destroying : 26219350  ListView1 - TListView                  Destroying : 74519666  ListView1 - TListView                           
                                                                                                                                                                 
Creating   : 26284886  ListView1 - TListView                  Creating   : 74585202  ListView1 - TListView                           
Creating   : 29495592   - TScrollingStyleHook.TScrollWindow   Creating   : 69997240   - TScrollingStyleHook.TScrollWindow            
                                                                                                                                                                 
Destroying : 40638542   - TScrollingStyleHook.TScrollWindow   Destroying : 25038084   - TScrollingStyleHook.TScrollWindow            
Destroying : 59579264  ImageView2 - TListView                 Destroying : 15010988  ImageView2 - TListView                          
Destroying : 27863036  ??                                     Destroying : 39720474  ??                                              
                                                                                                                                                                 
Creating   : 59644800  ImageView2 - TListView                 Creating   : 15076524  ImageView2 - TListView                          
Creating   : 40704078   - TScrollingStyleHook.TScrollWindow   Creating   : 25103620   - TScrollingStyleHook.TScrollWindow            
                                                                                                                                                                 
close                                                         close                                                                                
Destroying : 69735096  ??                                               
Destroying : 32050082  MainForm - TMainForm                   Destroying : 74454534  MainForm - TMainForm                            
Destroying : 34279264  ClientPanel - TPanel                   Destroying : 28256252  ClientPanel - TPanel                            
Destroying : 40704078   - TScrollingStyleHook.TScrollWindow   Destroying : 25103620   - TScrollingStyleHook.TScrollWindow            
Destroying : 59644800  ImageView2 - TListView                 Destroying : 15076524  ImageView2 - TListView                          
Destroying : 33429492  ??                                     Destroying : 36177894  ??                                                            
                                                                           
                                                              Destroying : 69997240  ?? - TScrollingStyleHook.TScrollWindow          
                                                              Destroying : 69997240  ?? - TScrollingStyleHook.TScrollWindow                        
                                                                                                                                      
                                                              Destroying : 74585202  ListView1 - TListView

Destroying : 9705020   Panel1 - TPanel                        Destroying : 36572760  Panel1 - TPanel                                 
Destroying : 17566478  SelectThemeRadioGroup - TRadioGroup    Destroying : 12786356  SelectThemeRadioGroup - TRadioGroup             
Destroying : 73605368  - TGroupButton                         Destroying : 51650054   - TGroupButton                                 
Destroying : 20647324  - TGroupButton                         Destroying : 33496018   - TGroupButton                                 
Destroying : 42864136  - TGroupButton                         Destroying : 39526802   - TGroupButton                                 
Destroying : 35130968  ??                                     Destroying : 19340526  ??                                              
Destroying : 51256838  ??                                     Destroying : 14812448  ??                   
                                                                                                                                                                                                                                                                                     

 

Edited March 8, 2021 by Attila Kovacs

[Attila Kovacs 631]

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.

 

 

Creating   : 34678622  MainForm - TMainForm                   
Creating   : 21433990  ClientPanel - TPanel                   
Creating   : 38472226  ListView1 - TListView                  
Creating   : 39394236  ImageView2 - TListView                 
Creating   : 30085416  Panel1 - TPanel                        
Creating   : 24716040   - TGroupButton                        
Creating   : 40310298  SelectThemeRadioGroup - TRadioGroup    
Creating   : 70587064   - TGroupButton                        
Creating   : 15338668   - TGroupButton                        

Destroying : 24716040   - TGroupButton                        
Destroying : 70587064   - TGroupButton                        
Destroying : 15338668   - TGroupButton                        
Destroying : 40310298  SelectThemeRadioGroup - TRadioGroup    
Destroying : 30085416  Panel1 - TPanel                        
Destroying : 38472226  ListView1 - TListView                  
Destroying : 39394236  ImageView2 - TListView                 
Destroying : 15476570  ??                                     
Destroying : 21433990  ClientPanel - TPanel                   
Destroying : 34678622  MainForm - TMainForm                   

Creating   : 34744158  MainForm - TMainForm                   
Creating   : 21499526  Panel1 - TPanel                        
Creating   : 15542106   - TGroupButton                        
Creating   : 57547500   - TGroupButton                        
Creating   : 38537762   - TGroupButton                        
Creating   : 39459772  SelectThemeRadioGroup - TRadioGroup    
Creating   : 38017968  ClientPanel - TPanel                   
Creating   : 30150952  ListView1 - TListView                  
Creating   : 15404204  ImageView2 - TListView                 
Creating   : 74847346   - TScrollingStyleHook.TScrollWindow   
Creating   : 28518396   - TScrollingStyleHook.TScrollWindow   

Destroying : 34744158  MainForm - TMainForm                   
Destroying : 38017968  ClientPanel - TPanel                   
Destroying : 28518396   - TScrollingStyleHook.TScrollWindow   

Destroying : 74847346  ?? - TScrollingStyleHook.TScrollWindow 
Destroying : 74847346  ?? - TScrollingStyleHook.TScrollWindow 

Destroying : 15404204  ImageView2 - TListView                 

Destroying : 21499526  Panel1 - TPanel                        
Destroying : 33888244  ??                                     
Destroying : 39459772  SelectThemeRadioGroup - TRadioGroup    
Destroying : 38537762   - TGroupButton                        

Destroying : 55775352  ??

 

 

 

Edited March 8, 2021 by Attila Kovacs

[Guest]

in this moment a "spoiler tag" help us 🤔 

 

hug

[Attila Kovacs 631]

@emailx45 I don't know how does it work. Was it introduced to the community?

[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 March 8, 2021 by Guest

[Attila Kovacs 631]

@emailx45 does not work with code or am I dumb

[Guest]

no. you dont, for sure.

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

 

hug

[balabuev 102]

I've solved this puzzle 

 

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:

 

 

• 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 March 8, 2021 by balabuev

[Pat Foley 51]

 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 March 8, 2021 by Pat Foley
add source

[pyscripter 694]

1 hour ago, 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 March 8, 2021 by pyscripter

[balabuev 102]

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 March 9, 2021 by balabuev

[Attila Kovacs 631]

@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 March 9, 2021 by Attila Kovacs

[pyscripter 694]

6 hours ago, 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 March 9, 2021 by pyscripter

[pyscripter 694]

Update:  This does not work well.

 

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 March 9, 2021 by pyscripter

[balabuev 102]

4 hours ago, 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.

 

56 minutes ago, 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.

 

 

 

[Attila Kovacs 631]

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.

 

[Gustav Schubert 25]

deleted

 

Edited March 10, 2021 by Gustav Schubert
irrelevant

[pyscripter 694]

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.