[PATCH] user32: Call driver's DestroyWindow in destroy_thread_windows().

[Paul Gofman]

On 10/13/21 14:28, Rémi Bernon wrote:
> On 10/13/21 12:39 PM, Paul Gofman wrote:
>> index 5e89f4c2c97..bafd0a3a7f0 100644
>> --- a/dlls/user32/win.c
>> +++ b/dlls/user32/win.c
>> @@ -1218,7 +1218,6 @@ void destroy_thread_windows(void)
>>       while ((wndPtr = next_thread_window( &hwnd )))
>>       {
>>           /* destroy the client-side storage */
>> -
>>           list = WIN_ListChildren( hwnd );
>>           menu = ((wndPtr->dwStyle & (WS_CHILD | WS_POPUP)) != 
>> WS_CHILD) ? (HMENU)wndPtr->wIDmenu : 0;
>>           sys_menu = wndPtr->hSysMenu;
>> @@ -1235,6 +1234,8 @@ void destroy_thread_windows(void)
>>               window_surface_release( surface );
>>           }
>>   +        USER_Driver->pDestroyWindow( hwnd );
>> +
>>           /* free child windows */
>>             if (!list) continue;
>>
>
> One thing I wondered was whether we should call the driver only after 
> releasing the child windows, and what would happen if these child 
> windows are owned by a different thread.
>
> If the windows are owned by the current thread, we'll maybe destroy 
> the child window driver data (ie: client windows I think) after the 
> parent. I'm not sure if it's a problem, but WIN_DestroyWindow does it 
> the other way around.

Well, even if that is not a problem I agree that at very least this 
leaves things too messy. I think I will try to redo 
destroy_thread_windows() so that it just uses WIN_DestroyWindow(). Not 
sure if WM_NCDESTROY message still needs to be send in this case, will 
add a test. There is also other stuff skipped in 
destroy_thread_windows() like wndPtr->hIconSmall2 or wndPtr->text which 
seem to be leaked. Also, notifying the server about window destroy 
(server currently destroys window only when the thread is fully 
terminated so even after destroy_thread_windows freed wnddata the 
queries about hwnd still succeed through server and report the window as 
alive).


>
> In the other case I believe there's already a pre-existing race 
> condition (and I have a test exhibiting it), so maybe we don't need to 
> worry too much about it:
>
>   We send the WM_WINE_DESTROYWINDOW to the child windows, and expect 
> their owning thread to peek their message and destroy them.
>
>   These thread may not be peeking their messages, and we may terminate 
> the current thread before, which will make wineserver detach the child 
> windows from their owning thread, losing the message forever.

I suppose this is a different issue from what I am trying to address, it 
is preexisting and the one concerned here happens without any race 
involved. So I guess that one may be addressed separately.