[PATCH 1/2 v2] winex11.drv: Clean up client window pointer when destroying Vulkan surface

[Andrew Eikum]

Signed-off-by: Andrew Eikum 
---

v2: Use gdi_display during XDestroyWindow to mirror the XCreateWindow
in create_client_window. Patch 2/2 is unchanged.

Without this, we destroy the client_window but leave the client_window
pointer dangling in the win_data struct. Later, during
destroy_whole_window, we try to re-parent the destroyed client_window,
causing a BadWindow X error.

We do store the HWND for the client_window with XSaveContext, but it
is saved on the per-thread win_data->display handle. Since we might
not be on that same thread during destroy_client_window, we can't use
it to look up the HWND for the given client_window. So instead we have
to store the HWND in the Vulkan struct, unfortunately. The same goes
for OpenGL in the next patch.

dlls/winex11.drv/vulkan.c | 15 +++++++++------
 dlls/winex11.drv/window.c | 16 ++++++++++++++++
 dlls/winex11.drv/x11drv.h |  1 +
 3 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/dlls/winex11.drv/vulkan.c b/dlls/winex11.drv/vulkan.c
index 7e252326b5..d67ad1a564 100644
--- a/dlls/winex11.drv/vulkan.c
+++ b/dlls/winex11.drv/vulkan.c
@@ -46,7 +46,7 @@ typedef VkFlags VkXlibSurfaceCreateFlagsKHR;

struct wine_vk_surface
 {
-    Window window;
+    HWND hwnd;
     VkSurfaceKHR surface; /* native surface */
 };

@@ -179,8 +179,8 @@ static void wine_vk_surface_destroy(VkInstance instance, struct wine_vk_surface
     /* vkDestroySurfaceKHR must handle VK_NULL_HANDLE (0) for surface. */
     pvkDestroySurfaceKHR(instance, surface->surface, NULL /* allocator */);

-    if (surface->window)
-        XDestroyWindow(gdi_display, surface->window);
+    if (surface->hwnd)
+        destroy_client_window(surface->hwnd);

heap_free(surface);
 }
@@ -246,6 +246,7 @@ static VkResult X11DRV_vkCreateWin32SurfaceKHR(VkInstance instance,
     VkResult res;
     VkXlibSurfaceCreateInfoKHR create_info_host;
     struct wine_vk_surface *x11_surface;
+    Window window;

TRACE("%p %p %p %p\n", instance, create_info, allocator, surface);

@@ -263,8 +264,8 @@ static VkResult X11DRV_vkCreateWin32SurfaceKHR(VkInstance instance,
     if (!x11_surface)
         return VK_ERROR_OUT_OF_HOST_MEMORY;

-    x11_surface->window = create_client_window(create_info->hwnd, &default_visual);
-    if (!x11_surface->window)
+    window = create_client_window(create_info->hwnd, &default_visual);
+    if (!window)
     {
         ERR("Failed to allocate client window for hwnd=%p\n", create_info->hwnd);

@@ -273,11 +274,13 @@ static VkResult X11DRV_vkCreateWin32SurfaceKHR(VkInstance instance,
         goto err;
     }

+    x11_surface->hwnd = create_info->hwnd;
+
     create_info_host.sType = VK_STRUCTURE_TYPE_XLIB_SURFACE_CREATE_INFO_KHR;
     create_info_host.pNext = NULL;
     create_info_host.flags = 0; /* reserved */
     create_info_host.dpy = gdi_display;
-    create_info_host.window = x11_surface->window;
+    create_info_host.window = window;

res = pvkCreateXlibSurfaceKHR(instance, &create_info_host, NULL /* allocator */, &x11_surface->surface);
     if (res != VK_SUCCESS)
diff --git a/dlls/winex11.drv/window.c b/dlls/winex11.drv/window.c
index 5fecd9a17b..f16964cdac 100644
--- a/dlls/winex11.drv/window.c
+++ b/dlls/winex11.drv/window.c
@@ -1497,6 +1497,22 @@ Window create_client_window( HWND hwnd, const XVisualInfo *visual )
 }

+/**********************************************************************
+ *		destroy_client_window
+ */
+void destroy_client_window( HWND hwnd )
+{
+    struct x11drv_win_data *data = get_win_data( hwnd );
+    if (data)
+    {
+        XDeleteContext( data->display, data->client_window, winContext );
+        XDestroyWindow( gdi_display, data->client_window );
+        data->client_window = 0;
+        release_win_data( data );
+    }
+}
+
+
 /**********************************************************************
  *		create_whole_window
  *
diff --git a/dlls/winex11.drv/x11drv.h b/dlls/winex11.drv/x11drv.h
index 85a05a904a..a4df2b2fab 100644
--- a/dlls/winex11.drv/x11drv.h
+++ b/dlls/winex11.drv/x11drv.h
@@ -594,6 +594,7 @@ extern void read_net_wm_states( Display *display, struct x11drv_win_data *data )
 extern void update_net_wm_states( struct x11drv_win_data *data ) DECLSPEC_HIDDEN;
 extern void make_window_embedded( struct x11drv_win_data *data ) DECLSPEC_HIDDEN;
 extern Window create_client_window( HWND hwnd, const XVisualInfo *visual ) DECLSPEC_HIDDEN;
+extern void destroy_client_window( HWND hwnd ) DECLSPEC_HIDDEN;
 extern void set_window_visual( struct x11drv_win_data *data, const XVisualInfo *vis, BOOL use_alpha ) DECLSPEC_HIDDEN;
 extern void change_systray_owner( Display *display, Window systray_window ) DECLSPEC_HIDDEN;
 extern void update_systray_balloon_position(void) DECLSPEC_HIDDEN;

[Andrew Eikum]

Hm, I guess this breaks the case where the whole window is destroyed
before the client window, since the HWND is no longer valid. I'll keep
working on it.

Andrew

On Wed, Jun 13, 2018 at 10:05:18AM -0500, Andrew Eikum wrote:
> Signed-off-by: Andrew Eikum
>
> v2: Use gdi_display during XDestroyWindow to mirror the XCreateWindow
> in create_client_window. Patch 2/2 is unchanged.
>
> Without this, we destroy the client_window but leave the client_window
> pointer dangling in the win_data struct. Later, during
> destroy_whole_window, we try to re-parent the destroyed client_window,
> causing a BadWindow X error.
>
> We do store the HWND for the client_window with XSaveContext, but it
> is saved on the per-thread win_data->display handle. Since we might
> not be on that same thread during destroy_client_window, we can't use
> it to look up the HWND for the given client_window. So instead we have
> to store the HWND in the Vulkan struct, unfortunately. The same goes
> for OpenGL in the next patch.
>
> dlls/winex11.drv/vulkan.c | 15 +++++++++------
>  dlls/winex11.drv/window.c | 16 ++++++++++++++++
>  dlls/winex11.drv/x11drv.h |  1 +
>  3 files changed, 26 insertions(+), 6 deletions(-)
>
> diff --git a/dlls/winex11.drv/vulkan.c b/dlls/winex11.drv/vulkan.c
> index 7e252326b5..d67ad1a564 100644
> --- a/dlls/winex11.drv/vulkan.c
> +++ b/dlls/winex11.drv/vulkan.c
> @@ -46,7 +46,7 @@ typedef VkFlags VkXlibSurfaceCreateFlagsKHR;
>
> struct wine_vk_surface
>  {
> - Window window;
> - HWND hwnd;
> VkSurfaceKHR surface; /* native surface */
>
> };
>
> @@ -179,8 +179,8 @@ static void wine_vk_surface_destroy(VkInstance instance, struct wine_vk_surface
>      /* vkDestroySurfaceKHR must handle VK_NULL_HANDLE (0) for surface. */
>      pvkDestroySurfaceKHR(instance, surface->surface, NULL /* allocator */);
> - if (surface->window)
> -
>    XDestroyWindow(gdi_display, surface->window);
> -
>
> if (surface->hwnd)
> -
>    destroy_client_window(surface->hwnd);
>
> heap_free(surface);
>
> }
> @@ -246,6 +246,7 @@ static VkResult X11DRV_vkCreateWin32SurfaceKHR(VkInstance instance,
>      VkResult res;
>      VkXlibSurfaceCreateInfoKHR create_info_host;
>      struct wine_vk_surface *x11_surface;
> -
>
> Window window;
>
> TRACE("%p %p %p %p\n", instance, create_info, allocator, surface);
>
> @@ -263,8 +264,8 @@ static VkResult X11DRV_vkCreateWin32SurfaceKHR(VkInstance instance,
>      if (!x11_surface)
>          return VK_ERROR_OUT_OF_HOST_MEMORY;
> - x11_surface->window = create_client_window(create_info->hwnd, &default_visual);
> - if (!x11_surface->window)
> - window = create_client_window(create_info->hwnd, &default_visual);
> - if (!window)
> {
>     ERR("Failed to allocate client window for hwnd=%p\n", create_info->hwnd);
>
> @@ -273,11 +274,13 @@ static VkResult X11DRV_vkCreateWin32SurfaceKHR(VkInstance instance,
>          goto err;
>      }
> - x11_surface->hwnd = create_info->hwnd;
> - create_info_host.sType = VK_STRUCTURE_TYPE_XLIB_SURFACE_CREATE_INFO_KHR;
>    create_info_host.pNext = NULL;
>    create_info_host.flags = 0; /* reserved */
>    create_info_host.dpy = gdi_display;
> - create_info_host.window = x11_surface->window;
> -
>
> create_info_host.window = window;
>
> res = pvkCreateXlibSurfaceKHR(instance, &create_info_host, NULL /* allocator */, &x11_surface->surface);
> if (res != VK_SUCCESS)
>
> diff --git a/dlls/winex11.drv/window.c b/dlls/winex11.drv/window.c
> index 5fecd9a17b..f16964cdac 100644
> --- a/dlls/winex11.drv/window.c
> +++ b/dlls/winex11.drv/window.c
> @@ -1497,6 +1497,22 @@ Window create_client_window( HWND hwnd, const XVisualInfo *visual )
>  }
>
> +/**********************************************************************
> -
> -
> destroy_client_window
> - */
>
> +void destroy_client_window( HWND hwnd )
> +{
> - struct x11drv_win_data *data = get_win_data( hwnd );
> - if (data)
> - {
> -
>    XDeleteContext( data->display, data->client_window, winContext );
> -
>    XDestroyWindow( gdi_display, data->client_window );
> -
>    data->client_window = 0;
> -
>    release_win_data( data );
> - }
>
> +}
> -
> -
>
> /**********************************************************************
> -
> create_whole_window
> -
>
> diff --git a/dlls/winex11.drv/x11drv.h b/dlls/winex11.drv/x11drv.h
> index 85a05a904a..a4df2b2fab 100644
> --- a/dlls/winex11.drv/x11drv.h
> +++ b/dlls/winex11.drv/x11drv.h
> @@ -594,6 +594,7 @@ extern void read_net_wm_states( Display *display, struct x11drv_win_data *data )
>  extern void update_net_wm_states( struct x11drv_win_data *data ) DECLSPEC_HIDDEN;
>  extern void make_window_embedded( struct x11drv_win_data *data ) DECLSPEC_HIDDEN;
>  extern Window create_client_window( HWND hwnd, const XVisualInfo *visual ) DECLSPEC_HIDDEN;
> +extern void destroy_client_window( HWND hwnd ) DECLSPEC_HIDDEN;
>  extern void set_window_visual( struct x11drv_win_data *data, const XVisualInfo *vis, BOOL use_alpha ) DECLSPEC_HIDDEN;
>  extern void change_systray_owner( Display *display, Window systray_window ) DECLSPEC_HIDDEN;
>  extern void update_systray_balloon_position(void) DECLSPEC_HIDDEN;
> -- 
> 2.17.1