Simplify Wayland cursor theme handling

Now themes are loaded once per scale and stored centrally not per
window. They are not unloaded till application shutdown. Since there
is unlikely to be more than two or three scales this is not a big waste
of resources. Since cursor lifetime is tied to theme lifetime and cursors
are stored per window, destroying a theme requires destroying all
cursors win all windows referring to that theme, which is too much work.

Should hopefully fix #2810

docs/changelog.rst

@@ -64,6 +64,9 @@ To update |kitty|, :doc:`follow the instructions <binary>`.
 - macOS: Make the window id of OS windows available in the ``WINDOWID``
   environment variable (:pull:`2877`)
 
+- Wayland: Fix a regression in 0.18.0 that could cause crashes related to mouse
+  cursors in some rare circumstances (:iss:`2810`)
+
 
 0.18.1 [2020-06-23]
 --------------------

glfw/wl_cursors.c

@@ -1,7 +1,5 @@
 // Future devs supporting whatever Wayland protocol stabilizes for cursor selection: see _themeAdd.
 
-#include "wl_cursors.h"
-
 #include "internal.h"
 
 #include <assert.h>
@@ -10,155 +8,58 @@
 #include <stdio.h>
 #include <stdlib.h>
 
-typedef struct {
+static int
-    struct wl_cursor_theme *theme;
+pixels_from_scale(int scale) {
-    int                     px;
+    static bool queried_env = false;
-    int                     refcount;
+    static int factor = 32;
-} _themeData;
+    if (!queried_env) {
-
+        const char *env = getenv("XCURSOR_SIZE");
-struct _wlCursorThemeManager {
+        if (env) {
-    size_t count;
+            const int retval = atoi(env);
-    /** Pointer to the head of an unsorted array of themes with no sentinel.
+            if (retval > 0 && retval < 2048) factor = 32;
-     *
-     * The lack of sort (and thus forcing a linear search) is intentional;
-     * in most cases, users are likely to have 1-2 different cursor sizes loaded.
-     * For those cases, we get no benefit from sorting and added constant overheads.
-     *
-     * Don't change this to a flexible array member because that complicates growing/shrinking.
-     */
-    _themeData *themes;
-};
-
-static void
-_themeInit(_themeData *dest, const char *name, int px) {
-    dest->px = px;
-    dest->refcount = 1;
-    if (_glfw.wl.shm) {
-        dest->theme = wl_cursor_theme_load(name, px, _glfw.wl.shm);
-        if(!dest->theme) {
-            _glfwInputError(GLFW_PLATFORM_ERROR,
-                            "Wayland: Unable to load cursor theme");
         }
-    } else {
+        queried_env = true;
-        dest->theme = NULL;
     }
+    return factor * scale;
 }
 
-static struct wl_cursor_theme*
-_themeAdd(int px, _wlCursorThemeManager *manager) {
-   ++manager->count;
-   _themeData *temp = realloc(manager->themes, sizeof(_themeData)*manager->count);
-   if (!temp) {
-       _glfwInputError(GLFW_OUT_OF_MEMORY,
-                       "OOM during cursor theme management.");
-       return NULL;
-   } else {
-       manager->themes = temp;
-       _themeInit(manager->themes + manager->count-1,
-                   getenv("XCURSOR_THEME"),
-                   px);
-       return manager->themes[manager->count-1].theme;
-   }
-}
 
-//WARNING: No input safety checks.
+struct wl_cursor_theme*
-static inline void _themeInc(_themeData
+glfw_wlc_theme_for_scale(int scale) {
-                          *theme) {
+    GLFWWLCursorThemes *t = &_glfw.wl.cursor_themes;
-    ++(theme->refcount);
+    for (size_t i = 0; i < t->count; i++) {
-}
+        if (t->themes[i].scale == scale) return t->themes[i].theme;
+    }
 
-// WARNING: No input safety checks.
+    if (t->count >= t->capacity) {
-// In particular, doesn't check if theme is actually managed by the manager.
+        t->themes = realloc(t->themes, sizeof(GLFWWLCursorTheme) * (t->count + 16));
-static void
+        if (!t->themes) {
-_themeDec(_themeData *theme, _wlCursorThemeManager *manager) {
+            _glfwInputError(GLFW_PLATFORM_ERROR, "Wayland: Out of memory allocating space for cursor themes");
-    if (--(theme->refcount) == 0) {
+            return NULL;
-        wl_cursor_theme_destroy(theme->theme);
-        if (--(manager->count) > 0) {
-            const _themeData *last_theme = (manager->themes)+(manager->count);
-            *theme = *last_theme;
-            _themeData *temp = realloc(manager->themes, (manager->count)*sizeof(_themeData));
-            // We're shrinking here, so it's not catastrophic if realloc fails.
-            if (temp) manager->themes = temp;
-        } else {
-            free(manager->themes);
-            manager->themes = NULL;
         }
+        t->capacity = t->count + 16;
     }
-}
+    struct wl_cursor_theme *ans = wl_cursor_theme_load(getenv("XCURSOR_THEME"), pixels_from_scale(scale), _glfw.wl.shm);
-
+    if (!ans) {
-static _wlCursorThemeManager _default = {0};
+        _glfwInputError(
-
+            GLFW_PLATFORM_ERROR, "Wayland: wl_cursor_theme_load failed at scale: %d pixels: %d",
-_wlCursorThemeManager*
+            scale, pixels_from_scale(scale)
-_wlCursorThemeManagerDefault() {
+        );
-    return &_default;
+        return NULL;
+    }
+    GLFWWLCursorTheme *theme = t->themes + t->count++;
+    theme->scale = scale;
+    theme->theme = ans;
+    return ans;
 }
 
 void
-_wlCursorThemeManagerDestroy(_wlCursorThemeManager *manager) {
+glfw_wlc_destroy(void) {
-    if (manager) {
+    GLFWWLCursorThemes *t = &_glfw.wl.cursor_themes;
-        for (size_t i = 0; i < manager->count; ++i) {
-            wl_cursor_theme_destroy(manager->themes[i].theme);
-        }
-        free(manager->themes);
-    }
-}
 
-static struct wl_cursor_theme*
+    for (size_t i = 0; i < t->count; i++) {
-_wlCursorThemeManagerGet(_wlCursorThemeManager *manager, int px) {
+        wl_cursor_theme_destroy(t->themes[i].theme);
-    _themeData *themedata = NULL;
-    for (size_t i = 0; i < manager->count; ++i) {
-        _themeData *temp = manager->themes+i;
-        if (temp->px == px) {
-            themedata = temp;
-            break;
-        }
     }
-    if (themedata != NULL) {
+    free(t->themes);
-        _themeInc(themedata);
+    t->themes = NULL; t->capacity = 0; t->count = 0;
-        return themedata->theme;
-    }
-    return _themeAdd(px, manager);
-}
-
-struct wl_cursor_theme*
-_wlCursorThemeManage(_wlCursorThemeManager *manager, struct wl_cursor_theme *theme, int px) {
-    //WARNING: Multiple returns.
-    if (manager == NULL) {
-        return NULL;
-    }
-    if (theme != NULL) {
-        // Search for the provided theme in the manager.
-        _themeData *themedata = NULL;
-        for (size_t i = 0; i < manager->count; ++i) {
-            _themeData *temp = manager->themes+i;
-            if (temp->theme == theme) {
-                themedata = temp;
-                break;
-            }
-        }
-        if (themedata != NULL) {
-            // Search succeeded. Check if we can avoid unnecessary operations.
-            if (themedata->px == px) return theme;
-            _themeDec(themedata, manager);
-        } else {
-            _glfwInputError(GLFW_PLATFORM_ERROR,
-                            "Wayland internal: managed theme isn't in the provided manager");
-            return theme;
-            //^ This is probably the sanest behavior for this situation: do nothing.
-        }
-    }
-    return px > 0 ? _wlCursorThemeManagerGet(manager, px) : NULL;
-}
-
-int
-_wlCursorPxFromScale(int scale) {
-    const char *envStr = getenv("XCURSOR_SIZE");
-    if(envStr != NULL) {
-        const int retval = atoi(envStr);
-        //^ atoi here is fine since 0 is an invalid value.
-        if(retval > 0 && retval <= INT_MAX/scale) {
-            return retval*scale;
-        }
-    }
-    return 32*scale;
 }

glfw/wl_cursors.h

@@ -2,29 +2,17 @@
 
 #include <wayland-cursor.h>
 
-typedef struct _wlCursorThemeManager _wlCursorThemeManager;
+typedef struct {
+    struct wl_cursor_theme *theme;
+    int scale;
+} GLFWWLCursorTheme;
 
-/** Returns a pointer to a wlCursorThemeManagerInstance.
- * Repeatedly calling this function will return the same instance.
- *
- * The retrieved instance must be destroyed with _wlCursorThemeManagerDestroy.
- */
-_wlCursorThemeManager* _wlCursorThemeManagerDefault(void);
 
-/** Set a wl_cursor_theme pointer variable to a pointer to a managed cursor theme.
+typedef struct {
- * Pass the desired px as the third argument.
+    GLFWWLCursorTheme *themes;
- * Returns a pointer to a managed theme, or NULL if the desired px is 0 or an error occurs.
+    size_t count, capacity;
- *
+} GLFWWLCursorThemes;
- * The passed theme pointer must either be NULL or a pointer to a theme managed by the passed manager.
- * The provided pointer may be invalidated if it's non-NULL.
- */
-struct wl_cursor_theme*
-_wlCursorThemeManage(_wlCursorThemeManager*, struct wl_cursor_theme*, int);
 
-void _wlCursorThemeManagerDestroy(_wlCursorThemeManager*);
 
-/** Helper method to determine the appropriate size in pixels for a given scale.
+struct wl_cursor_theme* glfw_wlc_theme_for_scale(int scale);
- *
+void glfw_wlc_destroy(void);
- * Reads XCURSOR_SIZE if it's set and is valid, else defaults to 32*scale.
- */
-int _wlCursorPxFromScale(int);

glfw/wl_init.c

@@ -148,12 +148,9 @@ static void setCursor(GLFWCursorShape shape, _GLFWwindow* window)
     struct wl_surface* surface = _glfw.wl.cursorSurface;
     const int scale = window->wl.scale;
 
-    window->wl.cursorTheme = _wlCursorThemeManage(
+    struct wl_cursor_theme *theme = glfw_wlc_theme_for_scale(scale);
-        _glfw.wl.cursorThemeManager,
+    if (!theme) return;
-        window->wl.cursorTheme,
+    cursor = _glfwLoadCursor(shape, theme);
-        _wlCursorPxFromScale(scale)
-    );
-    cursor = _glfwLoadCursor(shape, window->wl.cursorTheme);
     if (!cursor) return;
     // TODO: handle animated cursors too.
     image = cursor->images[0];
@@ -780,13 +777,14 @@ int _glfwPlatformInit(void)
 
     if (_glfw.wl.shm)
     {
-        _glfw.wl.cursorThemeManager = _wlCursorThemeManagerDefault();
         _glfw.wl.cursorSurface =
             wl_compositor_create_surface(_glfw.wl.compositor);
     }
     else
     {
-        _glfw.wl.cursorThemeManager = NULL;
+        _glfwInputError(GLFW_PLATFORM_ERROR,
+                        "Wayland: Failed to find Wayland SHM");
+        return false;
     }
 
     return true;
@@ -812,9 +810,7 @@ void _glfwPlatformTerminate(void)
 
     if (_glfw.wl.cursorSurface)
         wl_surface_destroy(_glfw.wl.cursorSurface);
-    if (_glfw.wl.cursorThemeManager) {
+    glfw_wlc_destroy();
-        _wlCursorThemeManagerDestroy(_glfw.wl.cursorThemeManager);
-    }
     if (_glfw.wl.subcompositor)
         wl_subcompositor_destroy(_glfw.wl.subcompositor);
     if (_glfw.wl.compositor)

glfw/wl_platform.h

@@ -132,7 +132,6 @@ typedef struct _GLFWwindowWayland
 
     _GLFWcursor*                currentCursor;
     double                      cursorPosX, cursorPosY;
-    struct wl_cursor_theme*     cursorTheme;
 
     char*                       title;
     char                        appId[256];
@@ -262,8 +261,7 @@ typedef struct _GLFWlibraryWayland
     size_t dataOffersCounter;
     _GLFWWaylandDataOffer dataOffers[8];
     char* primarySelectionString;
-
+    GLFWWLCursorThemes cursor_themes;
-    _wlCursorThemeManager*      cursorThemeManager;
 } _GLFWlibraryWayland;
 
 // Wayland-specific per-monitor data

glfw/wl_window.c

@@ -41,11 +41,6 @@
 #include <fcntl.h>
 #include <sys/mman.h>
 
-static void
-load_cursor_theme(_GLFWwindow* window) {
-    window->wl.cursorTheme = _wlCursorThemeManage(
-        _glfw.wl.cursorThemeManager, window->wl.cursorTheme, _wlCursorPxFromScale(window->wl.scale));
-}
 
 static void
 setCursorImage(_GLFWwindow* window)
@@ -63,8 +58,9 @@ setCursorImage(_GLFWwindow* window)
     } else
     {
         if (cursorWayland->scale != scale) {
-            if (!window->wl.cursorTheme) load_cursor_theme(window);
+            struct wl_cursor *newCursor = NULL;
-            struct wl_cursor *newCursor = _glfwLoadCursor(cursorWayland->shape, window->wl.cursorTheme);
+            struct wl_cursor_theme *theme = glfw_wlc_theme_for_scale(scale);
+            if (theme) newCursor = _glfwLoadCursor(cursorWayland->shape, theme);
             if (newCursor != NULL) {
                 cursorWayland->cursor = newCursor;
                 cursorWayland->scale = scale;
@@ -136,7 +132,6 @@ static bool checkScaleChange(_GLFWwindow* window)
     {
         window->wl.scale = scale;
         wl_surface_set_buffer_scale(window->wl.surface, scale);
-        load_cursor_theme(window);
         setCursorImage(window);
         return true;
     }
@@ -945,12 +940,6 @@ int _glfwPlatformCreateWindow(_GLFWwindow* window,
 
 void _glfwPlatformDestroyWindow(_GLFWwindow* window)
 {
-    if (window->wl.cursorTheme) {
-        _wlCursorThemeManage(_glfw.wl.cursorThemeManager,
-                             window->wl.cursorTheme,
-                             0);
-        window->wl.cursorTheme = NULL;
-    }
     if (window == _glfw.wl.pointerFocus)
     {
         _glfw.wl.pointerFocus = NULL;