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

Mon Nov 23 22:39:32 PST 2015

On Tue, Nov 24, 2015 at 03:11:26PM +1000, Peter Hutterer wrote:
> On Tue, Nov 24, 2015 at 11:44:54AM +0800, Jonas Ådahl wrote:
> > 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.
> 
> ok, this is something we should get into the protocol then, as a defined
> term. A <glossary> tag would not be amiss here :)
> 
> > > 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.
> 
> I'm aware of that, but the important bit here is that *it spells it out*.
> without this blurb, handling of capability events is left open for
> interpretation. with this blurb, you get exactly two options for each case
> and one is listed as "don't rely on it" (but we can't remove it for client
> breakage).

I see your point, but don't we have these two options just because we
looked at two implementations only? Anyway, if we're to spell out these
two possibilities, I think we should make it more clear that
object-may-wake-up-again case really is an unreliable implementation
detail.

It might be a matter of taste, but I consider writing things like below
makes it more clear that reusable pointer objects are unreliable and
should not be used:

	When the pointer capability is added, a client may create
	wl_pointer object using the wl_seat.get_pointer request. This
	object will receive pointer events until the capability is
	removed in the future.

	When the pointer capability is removed, a client should destroy
	the wl_pointer objects associated with the seat where the
	capability was removed. The client cannot on rely ever receiving
	any pointer events on existing wl_pointer events at this time.

	Whether an existing wl_pointer will start emitting events when
	the pointer capability is added to the corresponding seat is an
	implementation detail and should not be relied on.

> 
> 90% of writing specs is spelling out what may happen and what must not
> happen so that there is no room for interpretation and guesswork.
> 
> > 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.
> 
> That's what the above says - "may send pointer events in the future". We can
> be more expressive about this though and explicitly say this is only for
> backwards-compatibility and new clients or clients binding a version greater
> than X must implement the new 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?
> 
> if we had a clean slate, yeah. But how many compositors do we already have?
> How many clients out there rely on the current weston behaviour? Do you want
> to break those?

I expect most if not all clients to destroy/create on demand, because
all examples has been doing so, but this of course is not something we
can rely on, and I guess its a pipe dream to have a "clean" fix to this.

Jonas

> 
> Cheers,
>    Peter
> 
> > 
> > > +
> > > +	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>
> > > 
> > > 
> > > 
> > > 