[PATCH] Destroy resources when destroying input and output

[Jason Ekstrand]

On Fri, May 9, 2014 at 1:37 AM, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> Looking at it in general, there is one more fun complication.
>
> If the inert object has requests in its interface, that create new
> objects, the server cannot just ignore those requests. I think the
> server will need to actually create new objects if requested, but
> make those inert too. Otherwise there would again be a mismatch
> between the server and the client on which objects exist.

I was hoping that this wasn't actually going to be a problem, but on
wl_seat it is.  The server can destroy a seat while a client is calling
wl_seat.get_pointer for instance.  Then, yes, we do have to create an
intert object.

On Fri, May 9, 2014 at 9:02 AM, Pekka Paalanen <ppaalanen at gmail.com> wrote:

> On Fri, 09 May 2014 14:33:58 +0100
> Neil Roberts <neil at linux.intel.com> wrote:
>
> > Perhaps we should consider applying the patch anyway even though it's
> > not ideal. Currently if a client uses a dead output in a request such as
> > xdg_surface.set_output Weston will end up with a weston_output pointer
> > that points to freed memory. This could cause the compositor to crash.
>
> True, it's very bad now.
>
> > That is worse than the alternative provided by this patch which is to
> > make the client abort. The clients know about the output being destroyed
> > via the wl_registry.global_remove event so in practice they would only
> > hit the problem in the unlikely event that they used the output in a
> > request in the short time between the output being unplugged and
> > noticing the removal event.
>
> That is also true, but if it is fixed improperly now, there is a very
> good chance we just forget about the problem, and never fix it for
> real. Especially when it becomes very hard to trigger.
>
> At least make sure we have an open bug report about it, please.
>

I agree.  This is bad and we need to make sure it gets fixed properly.


>
> > In the longer term I was thinking maybe it would be good to handle the
> > inert resource idea within libwayland-server. We could add a function
> > like wl_resource_zombify() which would mark the resource as a zombie and
> > call the destroy handlers. From the compositor's perspective it can then
> > act as if the resource has been destroyed. We could detect zombie
> > resources being used within the request marshalling code and ignore the
> > request. If the request creates new resource we can internally create
> > new zombie resources too and Weston would never need to know about it. I
> > am planning to experiment with this approach now.
>
> Hmm... will be interesting to see, if that works out. It does feel like
> quite a lot of magic in libwayland-server, while also making life a lot
> easier.
>

Most of the magic there is in allowing resources with no handler in
libwayland-server.  The patch would be about 4 lines.  Right now,
client-side wl_proxy objects are allowed to have a NULL implementation and
there's no problem;  server-side, this is not currently allowed.  If we
allowed this ten Neil's wl_resource_zombify would only have to set the
destructor, and implementation to NULL.  For that matter, we could just do
that explicitly in weston and not add API for it.

The big chunk of magic is in handling new_id requests.  We would need to
create zombie wl_resource's for each new_id argument on a call to a
zombified wl_resource.  Now that Pekka mentioned it, I'm not sure that
we're handling those correctly client-side if there's no implementation set.

--Jason Ekstrand
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20140509/d09cf349/attachment.html>