Lifetime of wl_seat objects (was Re: [PATCH wayland-protocols 1/2] Introduce wp_relative_pointer) interface

[Peter Hutterer]

On Mon, Nov 23, 2015 at 12:01:01PM -0600, Derek Foreman wrote:
> On 23/11/15 12:32 AM, Jonas Ådahl wrote:
> > On Mon, Nov 23, 2015 at 02:42:09PM +1000, Peter Hutterer wrote:
> >> On Fri, Nov 20, 2015 at 02:59:50PM +0800, Jonas Ådahl wrote:
> >> [...]
> >>>>> +    <request name="destroy" type="destructor">
> >>>>> +      <description summary="release the relative pointer object"/>
> >>>>> +    </request>
> >>>>> +
> >>>>
> >>>> Should we add something about pairing this with wl_seat capabilities here?
> >>>> Just looking at this protocol doesn't make it clear what happens if the
> >>>> compositor drops the wl_pointer. Right now it's implicit, when the
> >>>> capability drops you need to destroy the wl_pointer and the
> >>>> wl_relative_pointer; spelling this out is useful.
> >>>
> >>> It's not even spelled out what happens to a wl_pointer when the seat
> >>> looses the pointer capability. For example right now a client running on
> >>> weston can ignore any capability lost event and just keep whatever
> >>> objects it ever created and they'll work again. So, I think we should
> >>> first make it clear there, then follow up here and do the same.
> >>
> >> urgh. so I guess we need to encode this in the protocol then, a
> >> wl_pointer/keyboard/touch object once obtained is re-used in the seat when
> >> the capability comes back. it's terrible IMO, but if we've been shipping
> >> this behaviour there isn't much about this we can do now without breaking
> >> existing clients. I don't mind writing the documentation patches, but I'd
> >> like to have general consensus about it.
> 
> I guess it's worth pointing out that old clients aren't capable of doing
> the right thing here, since we've only very recently added
> wl_pointer_release() as opposed to wl_pointer_destroy().  :)
> 
> > It's a bit of a confusing mess, but IIRC this is the different cases
> > that are implemented in mutter now:
> 
> I think you mean "weston" here, not mutter?
> 
> > 1. get object before first capability -> object not created (protocol
> > violation)
> > 
> > 2. get object after first capability -> object created and valid for all
> > eternity
> > 
> > 3. get object after capability is lost -> object will eventually work
> > whenever the capability reappears
> 
> To the casual observer this looks dumb - but it's to prevent the race when:
> 
> server says seat has pointer caps
> client says gimme pointer
> device is unplugged before server receives request
> (at this point if the objects aren't kept around forever the client gets
> killed)
>
> > mutter on the other hand does it slightly different. Case 1 is the same,
> > but for case 2, the objects are forever defunct after the capability is
> > lost. Case 3 works the same as case 1, i.e. the object is not created
> > (protocol violation).
> 
> Does mutter kill apps when this race triggers? :(

the spec here is terribly ambiguous. "This request only takes effect if the
seat has the pointer capability." The problem here is that "Takes effect"
isn't defined. are you supposed to get an error? is "protocol violation"
defined?

creating an inert object fixes the race condition, but we cannot encode this
in the protocol - older compositors would break newer clients that rely on
this. we still need to fail in case 1.

(Also, I'm using "inert" as object is alive but does not send events. If
that's not the current definition please let me know)

> > I'm not sure this is a good enough reason for doing the "capability lost
> > -> defunct object", but some way or another we need to fix the spec and
> > the implementations.
> >
> > Specifying it to create inert objects and "inerting" objects when a
> > capability is lost will probably for the most part work, since all the
> > example code has always destroyed and recreated objects as their
> > corresponding capabilities disappear and reappear, but if there is some
> > wild client that depends objects not going dark we'd break that.
> 
> I think David Fort was working on patches to actually do this...
> 
> > Specifying it to having the objects always be valid (but only certain
> > effect, such as updating the pointer sprite, when applicable), clients
> > that want to rely on this will only reliably work on new enough
> > compositor versions, meaning to work on the current wl_seat inteface
> > version, they'd still need to rely on the old undefined behaviour, to
> > work across versions.
> 
> I like this less because if we let clients hang onto these input
> resources forever then a seat can never go away.  Not a big deal for a
> default single seat weston configuration, but for the rdp backend this
> gets ugly (since a connection is a seat), it really wants to delete
> seats when clients disconnect.

you still know whether a client has a reference to the object, right? so you
can destroy everything once there are no references left. Requires the
client to call release on everything and behave properly, but that's not
something we can avoid.

> I suppose it would be possible to make the RDP backend re-use a single
> seat for multiple client logins. :(
> 
> > Both these are not very optimal, and I think option 1 (create inert
> > objects/inertify existing objects), while slightly ruthless, seems most
> > sane, since it's what most if not all clients already expects. What we
> > can't do, however, is to write the spec according to the weston
> > implementation without making new clients depending on those semantics
> > from not working on older versions of other compositors.
> 
> My vote is also for option 1.
> 
> I don't know how we dig out of this hole though.
 
> How bad is it to have the wl_seat version dictate behavior, and any
> version before the seat.*_release pointers existed needs to provide
> pointers forever, any version newer can inert them or provide them as inert?

bad. in X we had cases where a client would run multiple versions of the
same protocol (the toolkit would support XI2.0, a toolkit plugin XI2.2). I
don't know whether that's likely to happen in wayland, but generally you
cannot define the behaviour based on versions.

below is a first attempt at providing some more details here. it's iffy and
covers both cases out of necessity but provides a bit of leeway for the
compositor, for clients with seat versions < 3 you can create permanently
valid objects that just activate/deactivate. everything else can just use
its local lifetime. will be a mess of conditions in the implementation
though.


diff --git a/protocol/wayland.xml b/protocol/wayland.xml
index f9e6d76..bb1fec7 100644
--- a/protocol/wayland.xml
+++ b/protocol/wayland.xml
@@ -1350,6 +1350,28 @@
 	This is emitted whenever a seat gains or loses the pointer,
 	keyboard or touch capabilities.  The argument is a capability
 	enum containing the complete set of capabilities this seat has.
+
+	When the pointer capability is added:
+	- if the client does not have a wl_pointer object, it may create one
+	  with get_pointer. This object will receive pointer events
+	  until the capability is removed in the future.
+	- if the client has a valid wl_pointer object on this seat from an
+	  earlier get_pointer request, that object may send pointer events
+	  in the future. This behavior is implementation-dependent, clients
+	  must not rely on this behavior.
+
+	When the pointer capability is removed:
+	- if the client has a wl_pointer object, that object ceases to send
+	  events. The client should call wl_pointer.release on the object
+	  to release any associated resources.
+	- if the client does not have a wl_pointer object, it may create one
+	  with get_pointer. This object will not send events immediately.
+	  The object may send events in the future when the pointer
+	  capability is added. This behavior is implementation-dependent,
+	  clients must not rely on this behavior.
+
+	The above behavior also applies to wl_keyboard and wl_touch with the
+	keyboard and touch capabilities, respectively.
       </description>
       <arg name="capabilities" type="uint" enum="capability"/>
     </event>
@@ -1360,7 +1382,9 @@
 	for this seat.
 
 	This request only takes effect if the seat has the pointer
-	capability.
+	capability, or has had the pointer capability in the past.
+	It is a protocol violation to issue this request on a seat that has
+	never had the pointer capability.
       </description>
       <arg name="id" type="new_id" interface="wl_pointer"/>
     </request>
@@ -1371,7 +1395,9 @@
 	for this seat.
 
 	This request only takes effect if the seat has the keyboard
-	capability.
+	capability, or has had the keyboard capability in the past.
+	It is a protocol violation to issue this request on a seat that has
+	never had the keyboard capability.
       </description>
       <arg name="id" type="new_id" interface="wl_keyboard"/>
     </request>
@@ -1382,7 +1408,9 @@
 	for this seat.
 
 	This request only takes effect if the seat has the touch
-	capability.
+	capability, or has had the touch capability in the past.
+	It is a protocol violation to issue this request on a seat that has
+	never had the touch capability.
       </description>
       <arg name="id" type="new_id" interface="wl_touch"/>
     </request>