Add serial to data control to avoid race condition with clipboard managers

[davidedmundson]

A typically clipboard manager works as follows:

• On a new clipboard, we copy any text
• If the clipboard becomes empty (typically because a client died), we
  paste our previously copied text

Our race happens as follows:

first copy:

• client creates a wl_data_offer
• compositors sees it forwards it clipboard manager which copies the
  text...

second copy:

• client deletes its old wl_data_offer before creating a new one

• compositor forwards this update to clipboard manager

• clipboard manager knows the clipboard is empty and starts its
  operation to prevent an empty clipboard

• client creates a new wl_data_offer and calls set_selection

• clipboard manager creates a new wlr_data_offer (with the old
  clipboard text) and calls set_selection

The compositor can get these last two in any order, and we end up
replacing our new clipboard content with out-of-date previous clipboard
contents.

This patch adds a serial number that can be used when a set_selection is
used in repsponse to a selection event.

[zzag]

It looks like you forgot to bump the version of the zwlr_data_control_device_v1 interface. ;-)

[emersion]

The commit message (and PR title) seems broken. Copy-pasta gone wrong? :P

[emersion]

Bikeshedding: maybe we could find a better name? Would swap_selection make sense, since it replaces the selection with the provided serial? Or set_selection_with_serial?

[davidedmundson]

Generally speaking in my use case I'm trying to go from being empty to having content. So "swap" seems slightly off.

set_selection_with_serial is a very literal approach but that works for me.

[davidedmundson]

Throwing out another alternative - just so we consider all the options. We could add the response serial to the data control source. It would be an optional thing to send there and avoid having two requests here.

[emersion]

That sounds like a good idea! Sources cannot be re-used anyways.

[emersion]

@davidedmundson, would you be willing to update the implementations if I update the protocol here? Would be nice to move forward with this PR. :)

[davidedmundson]

would you be willing to update the implementations if I update the protocol here? Would be nice to move forward with this PR. :)

I can update this too. Apologies for the delay. It's now in my immediate TODO queue.

[emersion]

Apart from @zzag's comments, looks good! Will wait for a compositor and client impl before merging this.

[davidedmundson]

Will wait for a compositor and client impl before merging this.

Ack, I was hoping to get this approved in principle. Then write the code, then merge simultaneously (providing we don't find anything)

[davidedmundson]

There is now an implementation in our library which passes a unit tests made to recreate this issue.

[emersion]

Cool! Can you add a link to the server and client implementations?

[davidedmundson]

server https://invent.kde.org/plasma/kwayland-server/-/merge_requests/75
client https://invent.kde.org/plasma/plasma-workspace/-/merge_requests/232/

(these are currently outdated)

[bugaevc]

Hi @davidedmundson!

It's great that you're trying to work on protocol improvements for fixing this race. I've been thinking about issues like the one you're running into, and more, for quite a while. (Only thing I don't understand is how come, if this has been pending for weeks, @emersion told me just yesterday that "There's no need for any new protocol extension/update" & "the KDE clipboad manager folks don't need anything else".)

Adding some sort of serial would sure help fix some races, but unfortunately there are more/deeper issues here. Please read my previous thoughts on the matter (in this discussion and here). I've previously proposed an API like this:

    <request name="set_backup_source" since="3">
      <description summary="set a backup selection source">
        This request asks the compositor to set the given source as the *backup* source
        for the given selection offer that is coming from another client.

        If and when the source backing the offer abruptly goes away while being the active
        selection, the compositor should atomically replace the selection with the provided
        backup source, and send out the new wl_data_device.offer events to clients describing
        this backup source. The client can then expect to receive
        zwlr_data_control_source_v1.send and zwlr_data_control_source_v1.cancelled events
        as usual.

        If, on the other hand, the selection is later explicitly set to another source
        (or to nil), the compositor should cancel the provided backup source by means of an
        zwlr_data_control_source_v1.cancelled event.
      </description>
      <arg name="backup_source" type="object" interface="zwlr_data_control_source_v1" />
      <arg name="offer" type="object" interface="zwlr_data_control_offer_v1" />
    </request>

Note that an API like this would fix both the races / atomicity concerns (the destroyed source gets atomically replaced with the backup source by the compositor, without round-tripping to the clipboard manager, and without other clients ever seeing the intermediary selection(nil) state), and the password manager vs dying client problem, because the compositor would only use the backup source in the latter case, and cancel it immediately in the former case.

How does that sound? Naming, description, and everything else is up for debate/bikeshedding, of course.

P.S. I would appreciate if I was cc'd on discussions about this & related topics in the future 🙂

[emersion]

Please open a new issue for this, so that this PR's discussion stays readable.

(Only thing I don't understand is how come, if this has been pending for weeks, @emersion told me just yesterday that "There's no need for any new protocol extension/update" & "the KDE clipboad manager folks don't need anything else".)

I meant, there's no need for any of the extra complexity you've suggested, and this PR was already opened for quite a while.

[emersion]

wlr-protocols has migrated to gitlab.freedesktop.org. This pull request has been moved to:

https://gitlab.freedesktop.org/wlroots/wlr-protocols/-/merge_requests/94