Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WaylandBackend: prevent crash after closing window #1733

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

MithicSpirit
Copy link

@MithicSpirit MithicSpirit commented Jan 30, 2025

This addresses two related issues around closing windows in the Wayland backend.

  1. "Zombie windows":

    When the last gamescope client closes, the wayland backend tries to
    close the window until a new client appears. However, this is done in
    the destructor for the connector for the window, which is stored in
    a shared_ptr. This shared_ptr was held by both steamcompmgr (which
    correctly dropped it) and by the wayland backend, which would never drop
    it. This led to the first window being kept around as a "zombie window",
    which would show the last frame from the previous client, but otherwise
    never update (the backend would only hold a reference to the first
    window, and no subsequent ones).

    This fixes this issue by downgrading the shared_ptr held by the backend
    into a weak_ptr. Thus, the only owning reference to the backend at
    (almost) any given point is held by steamcompmgr, which allows the
    window to be correctly closed when steamcompmgr drops it. This also
    allows for the connector held by the wayland backend to be updated when
    a new window is created.

    HACK: This is NOT memory-safe, and will result in a segmentation fault
    if the connector held by the wayland backend is used (e.g., via
    ::GetCurrentConnector) after it was dropped by steamcompmgr. The code
    already included a TODO to remove the need for the backend to hold
    a reference to the connection, and this is still necessary.

  2. Crashing on window close:

    Whenever a window was truly closed (i.e., not left behind as a zombie
    window), gamescope would segfault due to calling
    IsSurfacePlane with null (from Wayland_Pointer_Leave, and maybe a few
    other places). This is addressed by having IsSurfacePlane short-circuit
    if it's passed null.

    HACK: I feel like IsSurfacePlane shouldn't ever be called with a null
    pointer, but this is the easiest way to solve this for now, and the code
    needs refactoring anyway.

See also #1611 and #1456.

Whenever a window was closed*, gamescope would segfault due to calling
IsSurfacePlane with null (from Wayland_Pointer_Leave, and maybe a few
other places). This is addressed by having IsSurfacePlane short-circuit
if it's passed null.

HACK: I feel like IsSurfacePlane shouldn't ever be called with a null
pointer, but this is the easiest way to solve this for now, and the code
needs refactoring anyway.

*Due to the "zombie window" issue, windows aren't necessarily closed
when you think they should be. This will be addressed in the next
commit.
When the last gamescope client closes, the wayland backend tries to
close the window until a new client appears. However, this is done in
the destructor for the connector for the window, which is stored in
a shared_ptr. This shared_ptr was held by both steamcompmgr (which
correctly dropped it) and by the wayland backend, which would never drop
it. This led to the first window being kept around as a "zombie window",
which would show the last frame from the previous client, but otherwise
never update (the backend would only hold a reference to the first
window, and no subsequent ones).

This fixes this issue by downgrading the shared_ptr held by the backend
into a weak_ptr. Thus, the only owning reference to the backend at
(almost) any given point is held by steamcompmgr, which allows the
window to be correctly closed when steamcompmgr drops it. This also
allows for the connector held by the wayland backend to be updated when
a new window is created.

HACK: This is *NOT* memory-safe, and will result in a segmentation fault
if the connector held by the wayland backend is used (e.g., via
::GetCurrentConnector) after it was dropped by steamcompmgr. The code
already included a TODO to remove the need for the backend to hold
a reference to the connection, and this is still necessary.
@layercak3
Copy link

I tested it briefly yesterday and there was issues with the output resolution not being reset, and one time I saw the window show up without an app ID (I have server side decorations with the title bar configured to show the app ID next to the title). I think it should not be closed at least until I reserve some time to check more thoroughly.

@MithicSpirit
Copy link
Author

MithicSpirit commented Jan 30, 2025

I have that configured as well, but I haven't even been looking at that lol. I haven't tested changing resolutions either. For testing, I just run gamescope --backend wayland -- sh -c 'for _ in {1..10}; do vkcube; sleep 0.2; done' and see what happens as I close the windows in different ways (all in sway, so if someone can test it out on different compositors that would be nice).

ProjectSynchro added a commit to ProjectSynchro/copr-gamescope-git that referenced this pull request Feb 10, 2025
Pull this patch when ValveSoftware/gamescope#1733 is merged upstream.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants