fix orphaned pre-commit hook and missing returns after post_error

[infraflakes]

Description

The ext_session_lock_surface_v1 pre-commit hook added in GetLockSurface is never removed on destroy. After the destroyed callback resets last_acked to None, any commit to the WlSurface hits post_error and kills the client connection.

The fix is observed and comfirmed to be working with quickshell's lock module from Dank Material Shell, the connection dies on unlock. Breaking similar session lock clients.

Checklist

• I agree to smithay's Developer Certificate of Origin.

[YaLTeR]

The fix is observed and comfirmed to be working with quickshell's lock module from Dank Material Shell, the connection dies on unlock. Breaking similar session lock clients.

But I use DMS and it never died on unlock for me on any of my machines?

[infraflakes]

The fix is observed and comfirmed to be working with quickshell's lock module from Dank Material Shell, the connection dies on unlock. Breaking similar session lock clients.

But I use DMS and it never died on unlock for me on any of my machines?

That's weird, I'm currently using nixos-unstable and dms via NixOS module and screen locking dies on unlock. The same happened to dms imported via flake.

[YaLTeR]

Try on niri?

[infraflakes]

Try on niri?

I actually did and it didn't die, but I'm testing out on my wm. I tried to reference Niri's code but I couldn't resolve this. But I find that niri's commit handler has a comment acknowledging that orphaned lock surface commits can occur after unlock:

// This message can trigger for lock surfaces that had a commit right after we unlocked
        // the session, but that's ok, we don't need to handle them.
        trace!("commit on an unrecognized surface: {surface:?}, root: {root_surface:?}");

But the pre-commit hook runs before that handler, so the post_error would still fire. It appears that Niri also has this issue and this fix would be for every wayland compositor based on Smithay.

[YaLTeR]

But the pre-commit hook runs before that handler, so the post_error would still fire. It appears that Niri also has this issue

But it apparently doesn't?

[infraflakes]

But the pre-commit hook runs before that handler, so the post_error would still fire. It appears that Niri also has this issue

But it apparently doesn't?

I'm not sure if we're on the same page but what I meant is that the "issue" I refer to is about the pre-commit hook, not the shell die after unlock. And Niri's life cycle happens to prevent this, but I still think this is a bug. Please correct me if I'm wrong.

[Drakulix]

Weird that other compositors don't run into this, but the fix seems very much correct.

I don't see anything in the protocol, that explicitly forbits re-use of a wl_surface for a new ext-lock-surface, though I guess the following paragraph could be read this way:

On binding this interface the compositor will immediately send the first configure event. After making the ack_configure request in response to this event the client should attach and commit the first buffer. Committing the surface before acking the first configure is a protocol error. Committing the surface with a null buffer at any time is a protocol error.

If that is the conclusion, a better fix would be to not reset lack_acked on destroy. @YaLTeR any opinion on that?
At least I don't think there is anything unsafe about allowing surface re-use.

[YaLTeR]

I don't have the surrounding code context "paged in", which was why I was asking why niri had no issue (I don't have the niri handling "paged in" either).

I'd verify what sway does. If it allows ext-session-lock role reuse then we can do the same in Smithay. The fix should be consistent with others (layer-shell and xdg-shell) which should also allow role reuse.

[Drakulix]

I don't have the surrounding code context "paged in", which was why I was asking why niri had no issue (I don't have the niri handling "paged in" either).

Fair enough, I primarily pinged you, because you did introduce code for lock-surface reuse: #1817 (or more specifically e6d3633).

I'd verify what sway does. If it allows ext-session-lock role reuse then we can do the same in Smithay. The fix should be consistent with others (layer-shell and xdg-shell) which should also allow role reuse.

I don't think it does. The destroy logic doesn't seem to unset the role-object: https://gitlab.freedesktop.org/wlroots/wlroots/-/blob/master/types/wlr_session_lock_v1.c?ref_type=heads#L155
Which would be required in wlroots for a full-reset. (Note that the role-object is different from the role, which cannot be reassigned.)

However it will short-circuit the commit logic: https://gitlab.freedesktop.org/wlroots/wlroots/-/blob/master/types/wlr_session_lock_v1.c?ref_type=heads#L155

I think this PR currently would leave the surface in a state, where a client could destroy the lock-surface, do a NULL-commit and then re-use the wl_surface with get_lock_surface.

If we want to prevent that, I think we should simply add an AliveTracker to the ExtLockSurfaceUserData, remove all the destroy-handling logic and instead set the tracker to false and change the commit-hook to return early, if the surface isn't alive anymore.