[PATCH weston v2] xwm: use always a valid 'primary view' for an X window

[Derek Foreman]

On 01/02/15 08:18 AM, Giulio Camuffo wrote:
> The xwm gets a primary view for a X window using the get_primary_view
> vfunc of the shell_interface struct. Storing it is dangerous though because
> it doesn't listen for its destruction so it may end up using the old stored
> view pointer after that view was freed, or after the primary view for that
> window was changed to another one.
> Fetch the primary view just before using it every time and try to not
> abuse this 'primary view' concept which may map badly to some shells:
> iterate over all the views instead when it makes sense.
> ---

Good commit log, makes it obvious that we should be doing this...

> v2: check window->shsurf
> 
>  xwayland/window-manager.c | 39 +++++++++++++++++++++------------------
>  1 file changed, 21 insertions(+), 18 deletions(-)
> 
> diff --git a/xwayland/window-manager.c b/xwayland/window-manager.c
> index 07c2ef3..be56ffc 100644
> --- a/xwayland/window-manager.c
> +++ b/xwayland/window-manager.c
> @@ -124,7 +124,6 @@ struct weston_wm_window {
>  	uint32_t surface_id;
>  	struct weston_surface *surface;
>  	struct shell_surface *shsurf;
> -	struct weston_view *view;
>  	struct wl_listener surface_destroy_listener;
>  	struct wl_event_source *repaint_source;
>  	struct wl_event_source *configure_source;
> @@ -750,18 +749,27 @@ weston_wm_window_transform(struct wl_listener *listener, void *data)
>  	struct weston_wm_window *window = get_wm_window(surface);
>  	struct weston_wm *wm =
>  		container_of(listener, struct weston_wm, transform_listener);
> +	struct weston_view *view;
> +	struct weston_shell_interface *shell_interface =
> +		&wm->server->compositor->shell_interface;
>  	uint32_t mask, values[2];
>  
> -	if (!window || !wm)
> +	if (!window || !wm || !window->shsurf)
>  		return;
>  
> -	if (!window->view || !weston_view_is_mapped(window->view))
> +	if (!shell_interface->get_primary_view)
>
>  		return;
>  
> -	if (window->x != window->view->geometry.x ||
> -	    window->y != window->view->geometry.y) {
> -		values[0] = window->view->geometry.x;
> -		values[1] = window->view->geometry.y;
> +	view = shell_interface->get_primary_view(shell_interface->shell,
> +	                                         window->shsurf);
> +
> +	if (!view || !weston_view_is_mapped(view))
> +		return;
> +
> +	if (window->x != view->geometry.x ||
> +	    window->y != view->geometry.y) {
> +		values[0] = view->geometry.x;
> +		values[1] = view->geometry.y;
>  		mask = XCB_CONFIG_WINDOW_X | XCB_CONFIG_WINDOW_Y;
>  
>  		xcb_configure_window(wm->conn, window->frame_id, mask, values);
> @@ -972,7 +980,6 @@ weston_wm_handle_unmap_notify(struct weston_wm *wm, xcb_generic_event_t *event)
>  		wl_list_remove(&window->surface_destroy_listener.link);
>  	window->surface = NULL;
>  	window->shsurf = NULL;
> -	window->view = NULL;
>  
>  	weston_wm_window_set_wm_state(window, ICCCM_WITHDRAWN_STATE);
>  	weston_wm_window_set_virtual_desktop(window, -1);
> @@ -992,6 +999,7 @@ weston_wm_window_draw_decoration(void *data)
>  	struct weston_shell_interface *shell_interface =
>  		&wm->server->compositor->shell_interface;
>  	uint32_t flags = 0;
> +	struct weston_view *view;
>  
>  	weston_wm_window_read_properties(window);
>  
> @@ -1033,8 +1041,8 @@ weston_wm_window_draw_decoration(void *data)
>  						  window->width + 2,
>  						  window->height + 2);
>  		}
> -		if (window->view)
> -			weston_view_geometry_dirty(window->view);
> +		wl_list_for_each(view, &window->surface->views, surface_link)
> +			weston_view_geometry_dirty(view);

Is this an unrelated functional change?  Previously we only dirtied the
primary view, now we dirty all views?

>  
>  		pixman_region32_fini(&window->surface->pending.input);
>  
> @@ -1060,6 +1068,7 @@ static void
>  weston_wm_window_schedule_repaint(struct weston_wm_window *window)
>  {
>  	struct weston_wm *wm = window->wm;
> +	struct weston_view *view;
>  	int width, height;
>  
>  	if (window->frame_id == XCB_WINDOW_NONE) {
> @@ -1072,8 +1081,8 @@ weston_wm_window_schedule_repaint(struct weston_wm_window *window)
>  				pixman_region32_init_rect(&window->surface->pending.opaque, 0, 0,
>  							  width, height);
>  			}
> -			if (window->view)
> -				weston_view_geometry_dirty(window->view);
> +			wl_list_for_each(view, &window->surface->views, surface_link)
> +				weston_view_geometry_dirty(view);

Same as above...

It seems like these two hunks are actually a separate bug fix?  Or is
this because some shells might not have a get_primary_view?  Either way,
it's a functional change from before?

I'd like to see this as two patches, the one that's removing the view
caching is already
Reviewed-by: Derek Foreman <derekf at osg.samsung.com>

(CC me and I'll review the other quickly, I just want to see a commit
log and understand the change)

>  		}
>  		return;
>  	}
> @@ -1430,7 +1439,6 @@ surface_destroy(struct wl_listener *listener, void *data)
>  	 * Don't try to use it later. */
>  	window->shsurf = NULL;
>  	window->surface = NULL;
> -	window->view = NULL;
>  }
>  
>  static void
> @@ -2409,9 +2417,6 @@ xserver_map_shell_surface(struct weston_wm_window *window,
>  	if (!shell_interface->create_shell_surface)
>  		return;
>  
> -	if (!shell_interface->get_primary_view)
> -		return;
> -
>  	if (window->surface->configure) {
>  		weston_log("warning, unexpected in %s: "
>  			   "surface's configure hook is already set.\n",
> @@ -2423,8 +2428,6 @@ xserver_map_shell_surface(struct weston_wm_window *window,
>  		shell_interface->create_shell_surface(shell_interface->shell,
>  						      window->surface,
>  						      &shell_client);
> -	window->view = shell_interface->get_primary_view(shell_interface->shell,
> -							 window->shsurf);
>  
>  	if (window->name)
>  		shell_interface->set_title(window->shsurf, window->name);
>