Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wlr_* structs should be free'd when the wl_resource is #909

Open
emersion opened this issue Apr 25, 2018 · 6 comments

Comments

Projects
None yet
3 participants
@emersion
Copy link
Member

commented Apr 25, 2018

Rationale: it shouldn't be possible to destroy a wlr_* struct without destroying its resource.

Right now, we generally have a wlr_thing_destroy that destroys the wlr_thing without destroying the resource. Then, we have a wl_resource destroy handler which calls wlr_thing_destroy. This is an issue since calling wlr_thing_destroy won't free the resource (ie. leak memory, and more importantly allow the client to send requests to it and make the compositor crash).

We can't call wl_resource_destroy from wlr_thing_destroy because we can't call wl_resource_destroy from the resource destroy handler. We also still want to destroy the wlr_thing if the client disconnects.

So instead we should make wlr_thing_destroy just call wl_resource_destroy (if we want to keep this function at all), and the resource destroy handler do the actual cleaning.

TODO:

  • Core Wayland protocol
  • gamma control
  • idle inhibit
  • idle
  • input inhibitor
  • layer shell
  • linux dmabuf
  • primary selection
  • screenshooter will be replaced
  • server decoration will be replaced
  • xdg output
  • zxdg-shell v6, xdg-shell stable

@emersion emersion added the proposal label Apr 25, 2018

@ddevault

This comment has been minimized.

Copy link
Member

commented Apr 26, 2018

So instead we should make wlr_thing_destroy just call wl_resource_destroy (if we want to keep this function at all), and the resource destroy handler do the actual cleaning.

I've tried to do this before and ran into Problems

@emersion

This comment has been minimized.

Copy link
Member Author

commented Apr 26, 2018

I've tried to do this before and ran into Problems

Allowing clients to crash the compositor is a Big Problem. I guess I'll try to do this on one interface, try to reproduce your Problems, and if I can't go on.

I'll have a look at Weston's strategy to deal with this, not sure it's applicable to wlroots though.

@Ongy

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2018

Do you remember which problems? I've done it this way in my tablet-v2 branch, and didn't run into any problems yet.

But on that one most (all) structs with resources are hidden from the compositor, and it only interacts with "multiplexer" structs, that have lists of client bindings (kinda like wlr_seat)

@ddevault

This comment has been minimized.

Copy link
Member

commented Apr 26, 2018

I'm afraid I don't remember the exact semantics, but if you use this approach you should test it very carefully and try to hit every edge case.

@emersion

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2018

Okay, I now remember the reason why this doesn't always work. Wayland has the concept of "inert resources". For instance when a surface which also happens to be a subsurface is destroyed by the client, the subsurface becomes inert.

Being inert means that all requests except the destructor are ignored. Being inert also means being a pain in the ass.

So this needs to be done for resources that can become inert:

  1. When the resource becomes inert, destroy our wlr_* struct (removes listeners, releases memory). Set the resource user_data to NULL. Do not destroy the resource.
  2. For each request made to a resource that can be inert: add a NULL check, ignore the request if the resource is inert.
  3. When the client calls the destructor request on the resource: wl_resource_destroy(resource).
  4. When the resource is destroyed, if the resource isn't inert (user_data NULL check), destroy our wlr_* struct.
@emersion

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2018

I forgot to mention that inert semantics can be even more annoying, for instance see the last comment of this patch about wl_seat: https://patchwork.freedesktop.org/patch/43356/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.