This commit introduces logic for using a new X11 window for each
incoming transfer, rather than having a global window for each selection
source.
This eliminates a whole class of bugs involving multiple concurrent
incoming transfers.
For now, we retain the outgoing transfer queue, and the selection
source-specific windows to support it. Source-specific windows are no
longer used in the incoming path, and will be removed in a future PR.
Refs #1497.
Previously, Xwayland could restart, and we'd get events for transfers
pointing to the previous (now freed) xwm instance. This led to
use-after-free segfaults.
Closes#2565.
This will hopefully be fixed in the future by having separate windows
for each X11-to-Wayland transfer, but until then, let's avoid a
compositor crash.
Previously, `transfer->incr` was being cleared on the next selection.
However, if the next selection was *also* incremental, it's possible
that `xwm_handle_selection_property_notify` would route us to
`xwm_get_incr_chunk` instead of `xwm_selection_get_data`.
Apart from reducing duplication, this has the positive side-effect of
allowing all deallocs to use
`xwm_selection_transfer_destroy_property_reply`, as opposed to the
latter and a mix of ad-hoc `free`s.
Previously, if the Wayland client died before an incremental transfer
was complete, the logs would be spammed by "write error to target fd" as
wlroots entered some control flow wherein it'd continually try
scheduling further writes to the already-dead pipe.
This commit contains no behavioral changes, but introduces explicit
handling for draining the X11 selection in case of Wayland client death.
If `xwm_data_source_write` failed, it's failed permanently. In fact, a
failing `xwm_data_source_write` sets `transfer->property_reply` to
null as part of its error handling.
Instead of relying on an indirect check (whether
`transfer->property_reply` is still non-null), explicitly use the return
value from `xwm_data_source_write`.
The `fd` is marked `O_NONBLOCK`, so `write` will never spuriously return
`EINTR`. Therefore, `write` failing is permanent, and we can return 0 to
make the return value meaningful.
wlroots would log "Unhandled NET_WM_STATE property change" log
messages for atoms we know about. Simplify the code structure
and remove these extra messages.
Previously, wlr_xwm_selection_transfer.source_fd meant:
- the source of data in a Wayland -> X11 copy (good)
- the destination of data in a X11 -> Wayland copy (confusing)
This made reading through xwayland/selection/incoming.c difficult: in
many places, "source" actually means "destination".
Previously, any error would be masked by an internal isatty call:
24:31:48.174 [DEBUG] [wlr] [xwayland/selection/incoming.c:386] XCB_SELECTION_NOTIFY (selection=277, property=278, target=256)
24:31:48.174 [ERROR] [wlr] [xwayland/selection/incoming.c:30] write error to target fd: Inappropriate ioctl for device
In certain situations windows can have their input field set to false
but still expect to receive input focus by passively listening to key
presses via a parent window. The ICCCM specification outlines how focus
should be given to clients.
Further reading: https://tronche.com/gui/x/icccm/sec-4.html#s-4.1.7
Relates to #2604
Xwayland has its own special handling for signals like SIGSEGV/SIGABRT.
Instead of leaving the job to the OS, it tries to walk up the call stack
(badly, because a lot of information is missing), print the stack trace
to stdout, then exit(1). This is very annoying because it prevents
Xwayland crashes from being easily debugged.
Xwayland has a flag "-core" that aborts instead of exiting. This allows
the OS to generate a coredump. It's far from perfect but better than
nothing, I guess.
We already mostly did this, but there were a couple of branches
(`calloc` failures) where we'd bail without letting the other side know.
Refs swaywm/sway#4007. Likely not going to be a real improvement there
(if `calloc` fails you're already pretty screwed), but it does address a
theoretical possibility.
It seems that if we ever try to reply to a selection request after
another has been sent by the same requestor (we reply in FIFO order),
the requestor never reads from it, and we end up stalling forever on a
transfer that will never complete.
It appears that `XCB_SELECTION_REQUEST` has some sort of singleton
semantics, and new requests for the same selection are meant to replace
outstanding older ones. I couldn't find a reference for this, but
empirically this does seem to be the case.
Real (contrived) case where we don't currently do this, and things break:
* run fcitx
* run Slack
* wl-copy < <(base64 /opt/firefox/libxul.so) # or some other large file
* focus Slack (no need to paste)
fcitx will send in an `XCB_SELECTION_REQUEST`, and we'll start
processing it. Immediately after, Slack sends its own. fcitx hangs for a
long, long time. In the meantime, Slack retries and sends another
selection request. We now have two pending requests from Slack.
Eventually fcitx gives up (or it can be `pkill`'d), and we start
processing the first request Slack gave us (FIFO). Slack (Electron?)
isn't listening on the other end anymore, and this transfer never
completes. The X11 clipboard becomes unusable until Slack is killed.
After this patch, the clipboard is immediately usable again after fcitx
bails. Also added a bunch of debug-level logging that makes diagnosing
this sort of issue easier.
Refs swaywm/sway#4007.
When debugging Xwayland-related issues, a common first step in debugging
has been to ask the reporter to move their real Xwayland to
/usr/bin/Xwayland.bin, and create a shell script starting Xwayland with
extra arguments under the original /usr/bin/Xwayland location.
Introducing a `WLR_XWAYLAND` environment variable makes this less
invasive, by allowing the user to swap out Xwayland without resorting to
global system changes (or source patches).
Fixes#2425.
wlroots can only handle one outgoing transfer at a time, so it keeps a
list of pending selections. The head of the list is the currently-active
selection, and when that transfer completes and is destroyed, the next
one is started.
The trouble is when you have a transfer to some app that is misbehaving.
fcitx is one such application. With really large transfers, fcitx will
hang and never wake up again. So, you can end up with a transfer list
that looks like this:
| T1: started | T2: pending | T3: pending | T4: pending |
The file descriptor for transfer T1 is registered in libwayland's epoll
loop. The rest are waiting in wlroots' list.
As a user, you want your clipboard back, so you `pkill fcitx`. Now
Xwayland sends `XCB_DESTROY_NOTIFY` to let us know to give up. We clean
up T4 first.
Due to a bug in wlroots code, we register the (fd, transfer data
pointer) pair for T1 with libwayland *again*, despite it already being
registered. We do this 2 more times as we remove T3 and T2.
Finally, we remove T1 and `free` all the memory associated with it,
before `close`-ing its transfer file descriptor.
However, we still have 3 copies of T1's file descriptor left in the
epoll loop, since we erroneously added them as part of removing T2/3/4.
When we `close` the file descriptor as part of T1's teardown, we
actually cause the epoll loop to wake up the next time around, saying
"this file descriptor has activity!" (it was closed, so `read`-ing would
normally return 0 to let us know of EOF).
But instead of returning 0, it returns -1 with `EBADF`, because the file
descriptor has already been closed. And finally, as part of error-handling
this, we access the transfer pointer, which was `free`'d. And we crash.
This one was awful to track down, but calls to `wlr_log` with %m have
the errno masked by the `isatty` call in `log_stderr`. Switch them to
`wlr_log_errno` instead.
Cue quality "how can read(2) POSSIBLY be returning ENOTTY?" moments.
Certain clients require this property to be set for expected behavior.
Most notably, steam client CSD maximize button no longer worked
after unmaximizing once, unless the state was changed by another
method. The state is unset whenever another surface gains focus.
If Xwayland is restarted, the ready handler assumes there is no xwm instance.
This means all of xwm was leaked on Xwayland restart. This caused compositors
to consume all cpu resources, where time is spent dispatching. Now we destroy
xwm if we get an event mask containing WL_EVENT_HANGUP or WL_EVENT_ERROR.
The xwayland ready signals are used to do initial setup like starting xwm.
Discarding the signals means that the handler functions will not be called
in the case that Xwayland is restarted and thus, xwm managed clients fail.
Fixes #2174."
This fixes issues with (at least) dialogs in Jetbrains IDEs becoming
unclickable if they ever lost focus (ref. swaywm/sway#5373). Prior to
this change, since `xwm->focus_surface` would be set prior to
`xwm_surface_activate` being called, the latter would short-circuit
immediately and not notify the application of the focus change.
Tudor Brindus
Simon Ser
BrassyPanache
Chris Chamberlain
Isaac Freund
Dominik Honnef
Ilia Bozhinov
Rouven Czerwinski
Scott Moreau
Antonin Décimo
Tobias Langendorf
John Chadwick