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

[Jonas Ådahl]

On Tue, Nov 24, 2015 at 10:21:45AM +1000, Peter Hutterer wrote:
> 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?

The "protocol violation" violation is not done by the client, but by the
server, which ignores the object creation. The client is not doing
anything wrong.

In practice, the project violation would result in the client being
killed if it called any request (such as wl_pointer.release) on the
object it has no idea was not created, since the server will have no
idea what server side object the object id corresponds to.


> 
> 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)

More or less. An "inert" object also listens to requests and creates new
objects on requests that are specified to do so.

> 
> > > 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.

Meh. This just spells out what weston does, and then asks clients not to
rely on it. Spelling it out that it might work I suspect will just
confuse users.

Can't we just say its undefined behaviour and that objects are not
expected to start working again, and spell out what behaviour is
expected from clients? If we don't want to break possibly existing
clients working on weston, then weston's implementation can be the old
"objects suddenly work again" behaviour.

At the same time, even not breaking clients relying on this undefined
behaviour is dangerous IMO, since clients may start rely on it without
knowing it. Isn't it actually better to break those clients, so that we
can get same behaviour across compositors?


Jonas

> +
> +	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>
> 
> 
> 
>