[PATCH weston 02/17] xdg-shell: Require proper object tree destruction

[Jonas Ådahl]

On Thu, Apr 30, 2015 at 04:54:07PM +0300, Pekka Paalanen wrote:
> On Tue,  7 Apr 2015 17:01:17 +0800
> Jonas Ådahl <jadahl at gmail.com> wrote:
> 
> > Require all child objects to be destroyed before the parent. In other
> > words, all popups and surfaces created by one xdg_shell instance needs
> > to be destroyed before the xdg_shell object, otherwise a protocol error
> > is raised.
> > 
> > Signed-off-by: Jonas Ådahl <jadahl at gmail.com>
> > ---
> >  desktop-shell/shell.c  | 26 ++++++++++++++++++++++++++
> >  protocol/xdg-shell.xml |  6 +++---
> >  2 files changed, 29 insertions(+), 3 deletions(-)
> > 
> > diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
> > index 826692f..778c120 100644
> > --- a/desktop-shell/shell.c
> > +++ b/desktop-shell/shell.c
> > @@ -107,6 +107,7 @@ struct shell_surface {
> >  	struct wl_resource *resource;
> >  	struct wl_signal destroy_signal;
> >  	struct shell_client *owner;
> > +	struct wl_resource *owner_resource;
> >  
> >  	struct weston_surface *surface;
> >  	struct weston_view *view;
> > @@ -239,6 +240,7 @@ struct shell_client {
> >  	struct wl_event_source *ping_timer;
> >  	uint32_t ping_serial;
> >  	int unresponsive;
> > +	struct wl_list surface_list;
> >  };
> >  
> >  static struct desktop_shell *
> > @@ -3554,6 +3556,7 @@ shell_destroy_shell_surface(struct wl_resource *resource)
> >  
> >  	if (!wl_list_empty(&shsurf->popup.grab_link))
> >  		remove_popup_grab(shsurf);
> > +	wl_list_remove(wl_resource_get_link(shsurf->resource));
> >  	shsurf->resource = NULL;
> >  }
> >  
> > @@ -3725,6 +3728,7 @@ shell_get_shell_surface(struct wl_client *client,
> >  	wl_resource_set_implementation(shsurf->resource,
> >  				       &shell_surface_implementation,
> >  				       shsurf, shell_destroy_shell_surface);
> > +	wl_list_init(wl_resource_get_link(shsurf->resource));
> >  }
> >  
> >  static bool
> > @@ -3988,6 +3992,21 @@ static void
> >  xdg_shell_destroy(struct wl_client *client,
> >  		  struct wl_resource *resource)
> >  {
> > +	struct shell_client *sc = wl_resource_get_user_data(resource);
> > +	struct wl_resource *shsurf_resource;
> > +	struct shell_surface *shsurf;
> > +
> > +	wl_resource_for_each(shsurf_resource, &sc->surface_list) {
> > +		shsurf = wl_resource_get_user_data(shsurf_resource);
> > +		if (shsurf->owner_resource == resource) {
> > +			wl_resource_post_error(
> > +				resource,
> > +				XDG_SHELL_ERROR_DEFUNCT_SURFACES,
> > +				"not all child surface objects destroyed");
> > +			return;
> > +		}
> > +	}
> > +
> >  	wl_resource_destroy(resource);
> >  }
> >  
> > @@ -4056,6 +4075,9 @@ xdg_get_xdg_surface(struct wl_client *client,
> >  	wl_resource_set_implementation(shsurf->resource,
> >  				       &xdg_surface_implementation,
> >  				       shsurf, shell_destroy_shell_surface);
> > +	shsurf->owner_resource = resource;
> > +	wl_list_insert(&sc->surface_list,
> > +		       wl_resource_get_link(shsurf->resource));
> >  }
> >  
> >  static bool
> > @@ -4181,6 +4203,9 @@ xdg_get_xdg_popup(struct wl_client *client,
> >  	wl_resource_set_implementation(shsurf->resource,
> >  				       &xdg_popup_implementation,
> >  				       shsurf, shell_destroy_shell_surface);
> > +	shsurf->owner_resource = resource;
> > +	wl_list_insert(&sc->surface_list,
> > +		       wl_resource_get_link(shsurf->resource));
> >  }
> >  
> >  static void
> > @@ -5798,6 +5823,7 @@ shell_client_create(struct wl_client *client, struct desktop_shell *shell,
> >  	sc->shell = shell;
> >  	sc->destroy_listener.notify = handle_shell_client_destroy;
> >  	wl_client_add_destroy_listener(client, &sc->destroy_listener);
> > +	wl_list_init(&sc->surface_list);
> >  
> >  	return sc;
> >  }
> > diff --git a/protocol/xdg-shell.xml b/protocol/xdg-shell.xml
> > index d1b47d1..09ce019 100644
> > --- a/protocol/xdg-shell.xml
> > +++ b/protocol/xdg-shell.xml
> > @@ -49,6 +49,7 @@
> >  
> >      <enum name="error">
> >        <entry name="role" value="0" summary="given wl_surface has another role"/>
> > +      <entry name="defunct_surfaces" value="1" summary="xdg_shell was destroyed before children"/>
> >      </enum>
> >  
> >      <request name="destroy" type="destructor">
> > @@ -56,9 +57,8 @@
> >          Destroy this xdg_shell object.
> >  
> >          Destroying a bound xdg_shell object while there are surfaces
> > -        still alive with roles from this interface is illegal and will
> > -        result in a protocol error. Make sure to destroy all surfaces
> > -        before destroying this object.
> > +        still alive created by this xdg_shell object instance is illegal
> > +        and will result in a protocol error.
> >        </description>
> >      </request>
> >  
> 
> Acked-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>

Thanks Bryce and Pekka and everyone else for the reviews and acks.

I went a head and landed this one as well as
xdg-shell: Specify fullscreen size-mismatch handling
xdg-shell: Specify the meaning of 0x0 window geometry in configure
xdg-shell: Some xdg_popup clarifications
xdg-shell: Some minor clarifications
xdg-shell: Fix a couple of typos
xdg-shell: Document that xdg_surface.set_window_geometry needs a commit
xdg-shell: Require a buffer and a wl_surface.commit for mapping a window
xdg-shell: Move xdg_shell.get_xdg_popup errors to xdg_shell
with the various Acks and RBs. The ones than need extra RBs are sent as
replies to some relevant mail in reply thread of the original patch.

Please have a look whenever time is available.


Jonas

> 
> FYI, I do not think this patch is reason to bump the experimental
> version. If you can build, the protocol is still compatible. An error
> code being unknown or not does not really change anything.
> 
> 
> Thanks,
> pq