[PATCH 0/1] MR6438: winewayland: Delay setting user driver functions.

[Aida Jonikienė (＠DodoGTA)]

This fixes segfaults and explorer.exe being stuck when this driver
fails to load.

[PATCH 1/1] winewayland: Delay setting user driver functions.

[Aida Jonikienė]

From: Aida Jonikienė

This fixes segfaults and explorer.exe being stuck when this driver
fails to load.
---
 dlls/winewayland.drv/wayland.c         | 20 ++++++++++++++++++
 dlls/winewayland.drv/waylanddrv_main.c | 29 +-------------------------
 2 files changed, 21 insertions(+), 28 deletions(-)

diff --git a/dlls/winewayland.drv/wayland.c b/dlls/winewayland.drv/wayland.c
index 35dbcb5f0ef..2ba1d50cb69 100644
--- a/dlls/winewayland.drv/wayland.c
+++ b/dlls/winewayland.drv/wayland.c
@@ -32,6 +32,25 @@

WINE_DEFAULT_DEBUG_CHANNEL(waylanddrv);

+static const struct user_driver_funcs waylanddrv_funcs =
+{
+    .pClipCursor = WAYLAND_ClipCursor,
+    .pDesktopWindowProc = WAYLAND_DesktopWindowProc,
+    .pDestroyWindow = WAYLAND_DestroyWindow,
+    .pKbdLayerDescriptor = WAYLAND_KbdLayerDescriptor,
+    .pReleaseKbdTables = WAYLAND_ReleaseKbdTables,
+    .pSetCursor = WAYLAND_SetCursor,
+    .pSetWindowText = WAYLAND_SetWindowText,
+    .pSysCommand = WAYLAND_SysCommand,
+    .pUpdateDisplayDevices = WAYLAND_UpdateDisplayDevices,
+    .pWindowMessage = WAYLAND_WindowMessage,
+    .pWindowPosChanged = WAYLAND_WindowPosChanged,
+    .pWindowPosChanging = WAYLAND_WindowPosChanging,
+    .pCreateWindowSurface = WAYLAND_CreateWindowSurface,
+    .pVulkanInit = WAYLAND_VulkanInit,
+    .pwine_get_wgl_driver = WAYLAND_wine_get_wgl_driver,
+};
+
 struct wayland process_wayland =
 {
     .seat.mutex = PTHREAD_MUTEX_INITIALIZER,
@@ -285,6 +304,7 @@ BOOL wayland_process_init(void)
         return FALSE;
     }

+    __wine_set_user_driver(&waylanddrv_funcs, WINE_GDI_DRIVER_VERSION);
     process_wayland.initialized = TRUE;

return TRUE;
diff --git a/dlls/winewayland.drv/waylanddrv_main.c b/dlls/winewayland.drv/waylanddrv_main.c
index 47c1299dd01..369086fda2c 100644
--- a/dlls/winewayland.drv/waylanddrv_main.c
+++ b/dlls/winewayland.drv/waylanddrv_main.c
@@ -33,25 +33,6 @@

char *process_name = NULL;

-static const struct user_driver_funcs waylanddrv_funcs =
-{
-    .pClipCursor = WAYLAND_ClipCursor,
-    .pDesktopWindowProc = WAYLAND_DesktopWindowProc,
-    .pDestroyWindow = WAYLAND_DestroyWindow,
-    .pKbdLayerDescriptor = WAYLAND_KbdLayerDescriptor,
-    .pReleaseKbdTables = WAYLAND_ReleaseKbdTables,
-    .pSetCursor = WAYLAND_SetCursor,
-    .pSetWindowText = WAYLAND_SetWindowText,
-    .pSysCommand = WAYLAND_SysCommand,
-    .pUpdateDisplayDevices = WAYLAND_UpdateDisplayDevices,
-    .pWindowMessage = WAYLAND_WindowMessage,
-    .pWindowPosChanged = WAYLAND_WindowPosChanged,
-    .pWindowPosChanging = WAYLAND_WindowPosChanging,
-    .pCreateWindowSurface = WAYLAND_CreateWindowSurface,
-    .pVulkanInit = WAYLAND_VulkanInit,
-    .pwine_get_wgl_driver = WAYLAND_wine_get_wgl_driver,
-};
-
 static void wayland_init_process_name(void)
 {
     WCHAR *p, *appname;
@@ -82,19 +63,11 @@ static void wayland_init_process_name(void)

static NTSTATUS waylanddrv_unix_init(void *arg)
 {
-    /* Set the user driver functions now so that they are available during
-     * our initialization. We clear them on error. */
-    __wine_set_user_driver(&waylanddrv_funcs, WINE_GDI_DRIVER_VERSION);
-
     wayland_init_process_name();

-    if (!wayland_process_init()) goto err;
+    if (!wayland_process_init()) return STATUS_UNSUCCESSFUL;

return 0;
-
-err:
-    __wine_set_user_driver(NULL, WINE_GDI_DRIVER_VERSION);
-    return STATUS_UNSUCCESSFUL;
 }

static NTSTATUS waylanddrv_unix_read_events(void *arg)

[Rémi Bernon (＠rbernon)]

Rémi Bernon (@rbernon) commented about dlls/winewayland.drv/wayland.c:
>  /* We need two roundtrips. One to get and bind globals, one to handle all
>   * initial events produced from registering the globals. */
>  wl_display_roundtrip_queue(process_wayland.wl_display, process_wayland.wl_event_queue);
>  wl_display_roundtrip_queue(process_wayland.wl_display, process_wayland.wl_event_queue);

The issue, is that as far as I understand (which may not be correct), this will trigger the display device initialization, which eventually calls the `output_done` handler which calls back into win32u through `wayland_output_done` > `maybe_init_display_devices`.

It may look, or actually be, harmless for now, but I think it's asking for trouble later, and drivers shouldn't do anything without their callbacks in place. You will need to better describe how and why this segfaults, and try to find a different solution.

[Aida Jonikienė (＠DodoGTA)]

On Tue Sep  3 16:12:27 2024 +0000, Rémi Bernon wrote:
> The issue, is that as far as I understand (which may not be correct),
> this will trigger the display device initialization, which eventually
> calls the `output_done` handler which calls back into win32u through
> `wayland_output_done` > `maybe_init_display_devices`.
> It may look, or actually be, harmless for now, but I think it's asking
> for trouble later, and drivers shouldn't do anything without their
> callbacks in place. You will need to better describe how and why this
> segfaults, and try to find a different solution.

Right now the current `__wine_set_user_driver()` behavior is to not change user driver entrypoints if they've been changed from the lazy load ones (if I'm reading the function code correctly); this means the Wayland driver entrypoints are always being used (even if the driver loading fails) and this causes weird behavior like `planes = 0` FIXME being printed for some bitmap function and libelf segfaulting

Should I change the behavior of `__wine_set_user_driver(NULL)` to always change the entrypoints (even if the entrypoints have been modified from the lazy load ones)? Right now none of the Wine graphics drivers except winewayland use this function in that way