Wlroots: Use one X11 window per data transfer

[emersion]

Right now we have a queue, because I didn't know it was possible to use multiple windows. We can actually do multiple data transfers in parallel. This will be faster and will simplify the code.

Just posting this here as a reminder because it's been a while I wanted to do this but I still haven't got the time to.

[wmww]

Instead of multiple windows, might it be possible and slightly more efficient to use multiple property atoms on the same window? _WL_SELECTION_0, _WL_SELECTION_1, etc?

[emersion]

I don't think that's how KDE does it, and I'm not sure it's more efficient. In any case, it probably doesn't matter much.

[Xyene]

I think there's one IMO significant upside to having one window per transfer, rather than one property on the same window: error handling.

Say there's an ongoing Wayland → X11 incremental transfer. The X11 client is waiting for an empty property to be written, but the Wayland client dies after sending some incremental chunks. That empty property will never come, and the X11 client isn't aware that the Wayland client died -- so it's in a "stuck" state.

A compositor _could_ generate a "synthetic" empty property to end the transfer if it detects the Wayland client hung up, but to the X11 side this would look like the transfer successfully completed (even though the clipboard may now contain half-transferred garbage).

On the other hand, if we destroy a transfer-specific window, this should be as close to what happens in the "real" X11 case... the X11 client would already have proper handling for it, it's a clear error condition upon which the X11 client could discard its selection, and we don't have to do extra work on the compositor end.

[Xyene]

Now that X11 → Wayland is ~taken care of, here's a possible implementation for the Wayland → X11 path. It's a bit more tricky than X11 → Wayland, so here's a textual description of what we could do, without getting bogged down in diffs.

Some setup:

• In struct wlr_xwm_selection, on top of struct wl_list incoming, we now need a struct wlr_xwm_selection_transfer *wl_offer_to_x11.
• wl_offer_to_x11.wl_client_fd should always be -1, since there's no Wayland client on the other side (yet).
• wl_offer_to_x11 is NULL if a Wayland client doesn't own the clipboard.
• On handle_seat_set{_primary,}_selection and seat_handle_set_drag, allocate a new wl_offer_to_x11, and advertise wl_offer_to_x11.window as the SelectionOwner.
  
  
  
  • If the Wayland client loses ownership / selection becomes null, we destroy the wl_offer_to_x11.
  
  

For accepting SelectionRequest on a struct wlr_xwm_selection selection:

• If wl_offer_to_x11 matches the window in the SelectionRequest:
  
  
  
  • Reply to the X11 client.
  
  
  • Accept the offer from the Wayland client, and populate wl_offer_to_x11.wl_client_fd.
  
  
  • Link selection.wl_offer_to_x11 into selection.incoming.
  
  
  • Set selection.wl_offer_to_x11 to NULL.
  
  
  • Broadcast that wl_offer_to_x11.window is no longer the SelectionOwner.¹
  
  
  • Allocate a new wl_offer_to_x11 and advertise it as the SelectionOwner.
  
  
• If transfer.window for any transfer in selection.outgoing matches SelectionRequest.window:
  
  
  
  • The situation described in https://github.com/swaywm/wlroots/pull/2428 has happened.
  
  
  • Unlink transfer from selection.outgoing, and destroy it (including transfer.window).
  
  
  • Nack the SelectionRequest. ²
  
  

Error handling:

• If the Wayland client hangs up, destroying the transfer (and X11 associated window) is sufficient.

---

1. OK, this part is kind of sketchy. One can imagine an adversarial client that:

   • Requests a large (incremental) selection.
   • Is halfway through reading it when it receives the xfixes "SelectionOwner changed" notification.
   • Decides to request from the new owner instead.

   Such a client would never make forward progress: it would only ever complete if it managed to complete the transfer by the time it got the owner-changed notification.

   Under https://www.x.org/releases/X11R7.6/doc/xorg-docs/specs/ICCCM/icccm.html#responsibilities_of_the_selection_owner, ICCCM says:

   If an owner loses ownership while it has a transfer in progress (that is, before it receives notification that the requestor has received all the data), it must continue to service the ongoing transfer until it is complete.

   So, this scheme should be kosher from the point of view of how the selection owner should behave.

   I assume — but haven't been able to find formally specified — that an X11 client must also continue to drain the transfer it requested even if it received an owner-changed notification in the meantime. Otherwise, the requestor would have forced a leak upon the owner.

   I'm not sure how much we care about this corner case, or if such an adversarial X11 client actually exists in widespread use. I'm open to alternative schemes that avoid this issue, but haven't been able to conjure one up myself.

2. I don't think we can do better than this, because re-initializing the transfer requires hanging up on the Wayland client's offer and requesting it again (we can't seek backwards on the fd)... but the same offer cannot be accepted twice, and the Wayland client would have moved on.

   The X11 client can SelectionRequest on selection.wl_offer_to_x11 (the new SelectionOwner) to get the clipboard from the current owning Wayland client. There's no reason (I think?) that an X11 client would be sending SelectionRequest to a window other than the current SelectionOwner, outside of network races.

   If the X11 client doesn't try again, this would manifest as a paste operation failing to produce output on the X11 side. I think as long as we make sure to log when we do this, we can easily rule this out when users file tickets like "copying into X11 window produces no input".

[psychon]

Broadcast that wl_offer_to_x11.window is no longer the SelectionOwner.
Allocate a new wl_offer_to_x11 and advertise it as the SelectionOwner.

Your footnote says why you think X11 clients should survive that, but... why is this even needed? Can't you just use the same X11 window for multiple transfers? The actual transfer should happen on a window that "some X11 client" created, not your selection owner window. Thus, there shouldn't be any conflicts with just continuing to use the same X11 window.

Or am I missing something?

(I would expect that X11 clipboard managers request the selection content whenever the selection changes (I never looked at one, this is just a guess). Also, just requesting the list of available MIME targets counts as a selection transfer, so you'd be creating lots and lots of windows. That surely causes some kind of races down the road.)

[emersion]

Can't you just use the same X11 window for multiple transfers?

We can't use the same X11 window and the same X11 property for multiple transfers at the same time. Creating a window isn't really worse than creating a property.

[psychon]

@emersion For the wayland -> X11 direction, the window is just used to set it as the selection owner, e.g. "this window owns the PRIMARY selection". For actual selection transfers, some X11 client has to create its own window and then send a request to the selection owner saying "dear selection owner, I created a window XYZ; please use the FOOBAR property of that window for a selection transfer of type WHATEVER". (This last bit is what xcb_convert_selection() is for; it makes the X11 server look up the selection owner and send it a selection request.)

Thus, the selection owner window is not used for multiple transfers at the same time. It is not even used for a single transfer.

The above made me notice:

Unlink transfer from selection.outgoing, and destroy it (including transfer.window).

You should not destroy transfer.window since you did not create it. Destroying other programs windows is a good way to make them get a BadWindow error, which Xlib usually handles by exiting the program.

Edit: In other news, in X11 windows are used for lots and lots of stuff. In this case, the selection "owner/sender" and "receiver" both create a window just to do some copy&paste, but neither of these windows are visible. If you have a large hammer, everything looks like a nail. ;-)

[Xyene]

You should not destroy transfer.window since you did not create it.

transfer.window is _always_ created and owned by wlroots as a proxy to the X11 selection, so it's always safe to destroy it. (And in fact, we must, otherwise we leak a window.) It's not a reference to the requestor window... destroying _that_ would be bad.

Edit: I suppose renaming it to transfer.proxy_window would make sense to be explicit; I was using the current wlroots naming in my description, and I can see how that could be confusing not having spent a lot of time staring at that code :)

Can't you just use the same X11 window for multiple transfers?

Yes, but then as far as I can tell we'd be left with no way of signaling to the X11 client in case the Wayland side dies. You're right that the transfer target is on the requestor's window, but I _presume_ that a robust X11 client has to listen for whether the sender dies, right? Otherwise, it could get stuck during an incremental transfer, since the sender hasn't sent an empty prop, and won't ever.

Aside: the ICCCM spec says:

Defining a new atom consumes resources in the server that are not released until the server reinitializes. Thus, reducing the need for newly minted atoms is an important goal for the use of the selection atoms.

I'm not sure if this is still relevant in present times, but might be worth keeping in mind when considering a single-window _WL_SELECTION_${autoincremented id} scheme. (This is not relevant in the Wayland → X11 case, just bringing it up since it was mentioned earlier.)

[psychon]

but I presume that a robust X11 client has to listen for whether the sender dies,

I'm not serious, but: Hahaha, how much time did you spent with X11 so far? "robust" and "X11" in the same sentence! :rofl:

On a more serious note, let's look at the definition of INCR transfers:
https://tronche.com/gui/x/icccm/sec-2.html#s-2.7.2

This is about 15 lines of text, 9 of them in bullet points. This just explains the "happy path" for how a transfer works. No word is lost about "what could go wrong". There is no defined protocol for aborting a transfer.

So, how is this dealt with in practice?
I picked some random piece of code: Qt.

Google claims that the following is a relative up to date copy of the source:

https://github.com/qt/qtbase/blob/658b93386eea34398de3ddacab436601e1ab5bbb/src/plugins/platforms/xcb/qxcbclipboard.cpp

I don't know this code, but it seems that QXcbClipboard::getSelection() is the relevant function (at the end of the file). It uses clipboardReadProperty() to read some property. If it is of type INCR, it calls clipboardReadIncrementalProperty() to do the transfer. This function has a loop for (;;) { that waits (blockingly!) for the next part of the transfer. The loop only reacts to property notify events for its window. The only way out are:

• Finishing the transfer via an empty property
• waitForClipboardEvent() failing (returning false) (this is complicated, but the "main way" to get out seems to be timeout-based: const int QXcbClipboard::clipboard_timeout = 5000; (this is in milliseconds)
• clipboardReadProperty() failing (returning false) (this function does some complicated stuff just to get the value of some property, it fails if the property does not exist)

Nothing in here cares about some window being destroyed.

I once wrote some non-blocking / callback based selection transfer code (unless you ask, I won't post details) and this code certainly does not do any kind of error handling. If a transfer hangs, it just hangs and we "leak" a window. So far, no one complained and if I were to invent my own error handling here, it would likely be buggy as well. I guess Qt's timeout based scheme (5 seconds without progress -> abort) is the most robust thing you can hope for.

Sorry, but I still think you should not destroy your selection owner window. If anything, that is just one more chance to trigger bugs in X11 clients, but doesn't really save you any trouble.

I'm not sure if this is still relevant in present times, but might be worth keeping in mind when considering a single-window _WL_SELECTION_${autoincremented id} scheme. (This is not relevant in the Wayland → X11 case, just bringing it up since it was mentioned earlier.)

In case you want to go that way and want to save atoms:
Just go 1, 2, 3, ... etc and invent your own atoms (as a number). The protocol does not care what the atom is, so you could just use xcb_get_atom_name() to check if that number refers to a valid atom and then just use it. All the numbers up to 68 are predefined atoms and are guaranteed to exist, so you can do at least 68 transfers in parallel with this scheme.

Downside: When debugging this, someone will wonder "why does it use PRIMARY for the transfer?" (PRIMARY always is atom 1).

[Xyene]

"robust" and "X11" in the same sentence! 🤣

😭

There is no defined protocol for aborting a transfer.

Yes, ICCCM is frustratingly lacking in anything but the happy path. As a substitute, on principle it'd be good to have a Wayland client look and behave (from an X11 client's point of view) as much as a normal X11 client — even if it's all a proxy through the compositor. If a Wayland client dies holding a selection, it would be good to get an associated DestroyNotify on the X11 side.

I picked some random piece of code: Qt.

Thanks for pulling up a real example.

Based on my priors with Qt's Wayland clipboard I'm not sure it's the example I would've picked to exemplify "robustness", but it does get your point across. I think the amount we should care should be proportional to how many apps actually use this behavior.

Nothing in here cares about some window being destroyed.

Agreed. Personally, I'm paranoid enough that if I were writing X11 code I'd listen for DestroyNotify events and abort my transfer if the window destroyed was the selection owner. Qt's 5-second abort is also good, and catches a superset of hang causes that listening to DestroyNotify does.

That said, since an X11 client _could_ be implementing this behavior (and not a 5-second abort) I'd like on principle — and when possible — to have wlroots emulate it the best it can.

So far, no one complained

Well, I don't doubt that. I really don't expect the cases we're talking about to ~ever pop up in practice.

Sorry, but I still think you should not destroy your selection owner window.

I'm more tempted to agree now than previously. I think having one proxy X11 window per Wayland transfer may be superfluous. I do still think that we should at least have one proxy X11 window per Wayland client, and have the window be refcounted. So:

• When the Wayland client is set as the selection owner, wlroots creates a proxy X11 window for it, and publicizes it as the SelectionOwner.
• Multiple X11 clients may request a transfer from this proxy window. Each X11 transfer will result in a new fd from the Wayland client.
• When the Wayland client is no longer the selection owner and closes its last outstanding fd to an ongoing transfer, destroy the proxy window. (The fds all get closed in case a Wayland client dies, so that case needs no special handling.)

@psychon — do you spot any obvious flaws with this approach?

@emersion — do you remember why outgoing.c only starts a single outgoing transfer at any time? Reading through the code, I don't see any obvious property conflicts in starting all transfers concurrently.

[emersion]

I remember doing some tests with multiple transfers at the same time. IIRC I just decided to go with the queue approach because it fixed some client, either Chromium or Firefox. Trying to do it differently didn't work.

[emersion]

Ref'counting windows sounds like complexity for no real benefit, IMHO.

[Xyene]

I remember doing some tests with multiple transfers at the same time. IIRC I just decided to go with the queue approach because it fixed some client, either Chromium or Firefox. Trying to do it differently didn't work.

Ack. From the description, perhaps the underlying issue was https://github.com/swaywm/wlroots/pull/2428? Multiple outgoing transfers to distinct X11 clients shouldn't be an issue, but for the same client, new selection requests must take precedence over previous ones.

As a first step I can try removing the queuing but keeping something spiritually similar to https://github.com/swaywm/wlroots/pull/2428 in, and see if things continue to work.

Ref'counting windows sounds like complexity for no real benefit, IMHO.

I think it's necessary if we want to "properly" emit DestroyNotify events if the Wayland client dies, but if the simple solution of "one proxy window per selection, forever" happens to work all the time, I'm more than happy to not have to write code for this until we run into a real X11 app that needs this behavior.

[psychon]

Based on my priors with Qt's Wayland clipboard I'm not sure it's the example I would've picked to exemplify "robustness", b

Okay... well, GTK/GDK... I got lost in its code, but I do not see any obvious error handling. The start of a transfer seems to be here:
https://github.com/GNOME/gtk/blob/949f22b8b2778c1f3e83b1e47d904fcad6a5b913/gdk/x11/gdkselectioninputstream-x11.c#L510-L546
The only "error handling" that I could find just truncates the transfer and pretends that everything was transfered:
https://github.com/GNOME/gtk/blob/949f22b8b2778c1f3e83b1e47d904fcad6a5b913/gdk/x11/gdkselectioninputstream-x11.c#L410-L416
(The error case is reached if XGetWindowProperty() fails)

(There might be a "higher level" in GTK that provides a synchronous API on top of this async one, but I do not really feel like digging.)

@psychon — do you spot any obvious flaws with this approach?

Not really. However, I still don't think that the lifetime of the selection owner window is so closely tied to the transfer. IMO ongoing transfers should be able to continue, but I am not really sure and this is more a "gut feeling".

[Xyene]

Okay... well, GTK/GDK...

Sorry, I didn't mean to come off as discounting your example — if Qt doesn't implement special behavior, that's significant. Thanks for looking into GTK though; that reinforces the idea that we shouldn't do anything fancy with one X11 window per outgoing transfer.

On a personal level, I'm surprised things actually work OK in practice given how loose this protocol is with errors, but that's neither here nor there :)

However, I still don't think that the lifetime of the selection owner window is so closely tied to the transfer

Sure, let's try this for starters. I've put up https://github.com/swaywm/wlroots/pull/2707 that is a simple patch on top of https://github.com/swaywm/wlroots/pull/2701 that stops queuing outgoing transfers.

[psychon]

Sorry, I didn't mean to come off as discounting your example

You didn't come off this way. It was just "okay, so Qt was causing problems, so let's take a look at some other 'large' code base".

Also: Thank you for excusing even though it was (IMHO) not necessary. This is definitely a lot better than the other way around and not the standard behaviour "on the internet".

I'm surprised things actually work OK in practice given how loose this protocol is with errors,

Off-topic, but: I once looked into Java's source code to figure out some weirdness. What I found is: It only does what ICCCM requires it to do to when run under KWin. A comment in the code indicates that this is to work around a bug in KWin. If it did not detect KWin, it required things that were not guaranteed by ICCCM.
(This rant is from the top of my head, so I am not sure that it really was KWin; this also was not about selection transfers, but about when a WM has to send synthetic configure events, so it really is off-topic. I guess my point is: With X11, you are not even "safe" when you do what the spec requires you to do. Some "large player" can safely ignore the spec and everyone else has to follow.)

[emersion]

I think this can be closed now. Thanks for your work @Xyene, and thanks for the feedback @psychon!