Raise the correct protocol error on a wl_surface role conflict#5005
Raise the correct protocol error on a wl_surface role conflict#5005robert-ancell wants to merge 4 commits into
Conversation
Assigning a second role to a wl_surface previously threw a generic
std::runtime_error from WlSurface::set_role, which surfaces to the client
as an internal error/disconnect rather than the protocol-specific error
the Wayland specs require.
Add a WlSurface::has_role() accessor and check it at each request that
assigns a role, raising the error mandated by the relevant protocol before
the role object is constructed:
- wl_subcompositor.get_subsurface -> wl_subcompositor.bad_surface
- xdg_wm_base.get_xdg_surface -> xdg_wm_base.role
- zwlr_layer_shell_v1.get_layer_surface -> zwlr_layer_shell_v1.role
- wl_pointer.set_cursor -> wl_pointer.role
- wl_data_device.start_drag (icon) -> wl_data_device.role
The xdg check is placed at get_xdg_surface to match the spec wording
("illegal to create an xdg_surface for a wl_surface which already has an
assigned role"). Re-using the current cursor surface to update its hotspot
remains allowed, as required by the spec. The generic throw in set_role is
retained as a defensive backstop.
This behaviour is covered by WLCS.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR improves Wayland protocol compliance in Mir’s Wayland frontend by ensuring wl_surface role conflicts raise the protocol-mandated errors (instead of surfacing as generic internal errors to clients).
Changes:
- Add
WlSurface::has_role()to detect whether a wl_surface already has an assigned role. - Pre-check role assignment requests across multiple protocols (xdg-shell, subsurfaces, layer-shell, pointer cursor, and DnD icon) and throw the corresponding
ProtocolErrorearly.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/server/frontend_wayland/xdg_shell_stable.cpp | Reject get_xdg_surface when the wl_surface already has a role, using xdg_wm_base.role. |
| src/server/frontend_wayland/wl_surface.h | Add has_role() accessor to support protocol-level role conflict checks. |
| src/server/frontend_wayland/wl_subcompositor.cpp | Reject get_subsurface when the child surface already has a role, using wl_subcompositor.bad_surface. |
| src/server/frontend_wayland/wl_pointer.cpp | Reject set_cursor when assigning cursor role to a surface that already has a role. |
| src/server/frontend_wayland/wl_data_device.cpp | Reject start_drag with an icon surface that already has a role, using wl_data_device.role. |
| src/server/frontend_wayland/layer_shell_v1.cpp | Reject get_layer_surface when the wl_surface already has a role, using zwlr_layer_shell_v1.role. |
| // The surface is becoming a cursor for the first time (for this pointer). | ||
| // Re-using the current cursor surface is allowed and handled above, but a | ||
| // surface with any other role must raise a protocol error. | ||
| if (wl_surface->has_role()) | ||
| { | ||
| throw mw::ProtocolError{ | ||
| resource, | ||
| Error::role, | ||
| "Surface already has a role"}; | ||
| } |
There was a problem hiding this comment.
This is already noted in the comment - the if branch above is already handling the case where it is already a cursor.
There was a problem hiding this comment.
The branch above isn't actually handling the case where the newly-applied surface already has a cursor role; it's handling the case where the newly-applied surface is the same surface as the currently applied one.
Clients could create multiple WlPointer instances and set the same surface as a pointer on each; this is meant to work, but would fail.
(Also, we incorretly unset the pointer role when a surface is no longer being used as a cursor, meaning a client could set a surface as a cursor, unset that surface, and then (invalidly!) map that surface as an xdg_toplevel)
…bject The bad_surface and bad_parent errors are defined on the wl_subcompositor interface, so they must be posted on the wl_subcompositor object. They were posted on the new wl_subsurface object instead, whose interface defines only bad_surface (and not bad_parent), so the client would interpret the error code against the wrong interface. Raise all three errors on the wl_subcompositor resource instead. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Failing in WLCS test - will need checking. |
|
Oh, now I understand. The failure was: Which is because the test now works. The solution is to remove this from |
RAOF
left a comment
RAOF
left a comment
There was a problem hiding this comment.
This is not quite the right set of checks — from the wl_surface protocol:
A surface can have only one role at a time. Initially a wl_surface does not have a role. Once a wl_surface is given a role, it is set permanently for the whole lifetime of the wl_surface object. Giving the current role again is allowed, unless explicitly forbidden by the relevant interface specification.
...
...For instance, if a wl_subsurface object is destroyed, the wl_surface it was created for will be unmapped and forget its position and z-order. It is allowed to create a wl_subsurface for the same wl_surface again, but it is not allowed to use the wl_surface as a cursor (cursor is a different role than sub-surface, and role switching is not allowed).
| // The surface is becoming a cursor for the first time (for this pointer). | ||
| // Re-using the current cursor surface is allowed and handled above, but a | ||
| // surface with any other role must raise a protocol error. | ||
| if (wl_surface->has_role()) | ||
| { | ||
| throw mw::ProtocolError{ | ||
| resource, | ||
| Error::role, | ||
| "Surface already has a role"}; | ||
| } |
There was a problem hiding this comment.
The branch above isn't actually handling the case where the newly-applied surface already has a cursor role; it's handling the case where the newly-applied surface is the same surface as the currently applied one.
Clients could create multiple WlPointer instances and set the same surface as a pointer on each; this is meant to work, but would fail.
(Also, we incorretly unset the pointer role when a surface is no longer being used as a cursor, meaning a client could set a surface as a cursor, unset that surface, and then (invalidly!) map that surface as an xdg_toplevel)
| /// Whether the surface currently has a role assigned (e.g. subsurface, cursor, | ||
| /// drag-and-drop icon, or a window role such as xdg_toplevel). Used to raise the | ||
| /// appropriate protocol error when a client tries to assign a second role. | ||
| bool has_role() const { return role != &null_role; } |
There was a problem hiding this comment.
Because roles are sticky, this might want to return the current role so that we can check if they're assigning a different role, or re-assigning back to the same roll.
Assigning a second role to a wl_surface previously threw a generic std::runtime_error from WlSurface::set_role, which surfaces to the client as an internal error/disconnect rather than the protocol-specific error the Wayland specs require.
Add a WlSurface::has_role() accessor and check it at each request that assigns a role, raising the error mandated by the relevant protocol before the role object is constructed:
The xdg check is placed at get_xdg_surface to match the spec wording ("illegal to create an xdg_surface for a wl_surface which already has an assigned role"). Re-using the current cursor surface to update its hotspot remains allowed, as required by the spec. The generic throw in set_role is retained as a defensive backstop.
This behaviour is covered by WLCS.