[PATCH weston] xwm: Check whether the seat is NULL sometimes in weston_wm_handle_button

[Boyan Ding]

On Mon, 2014-09-01 at 12:14 +0300, Pekka Paalanen wrote:
> On Fri, 29 Aug 2014 22:10:32 +0800
> Boyan Ding <stu_dby at 126.com> wrote:
> 
> > Under some certain circumstances, pointer button may have been released
> > when frame is still being resized/moved. When this happens, the picked
> > seat is NULL and it will segfault when moving/resizing surfaces. Check
> > whether the seat is NULL and ignore move/resize in that case.
> > 
> > Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=82827
> > Signed-off-by: Boyan Ding <stu_dby at 126.com>
> > ---
> >  xwayland/window-manager.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/xwayland/window-manager.c b/xwayland/window-manager.c
> > index a216b76..f633324 100644
> > --- a/xwayland/window-manager.c
> > +++ b/xwayland/window-manager.c
> > @@ -1648,12 +1648,14 @@ weston_wm_handle_button(struct weston_wm *wm, xcb_generic_event_t *event)
> >  		weston_wm_window_schedule_repaint(window);
> >  
> >  	if (frame_status(window->frame) & FRAME_STATUS_MOVE) {
> > -		shell_interface->move(window->shsurf, seat);
> > +		if (seat != NULL)
> > +			shell_interface->move(window->shsurf, seat);
> >  		frame_status_clear(window->frame, FRAME_STATUS_MOVE);
> >  	}
> >  
> >  	if (frame_status(window->frame) & FRAME_STATUS_RESIZE) {
> > -		shell_interface->resize(window->shsurf, seat, location);
> > +		if (seat != NULL)
> > +			shell_interface->resize(window->shsurf, seat, location);
> >  		frame_status_clear(window->frame, FRAME_STATUS_RESIZE);
> >  	}
> >  
> 
> Hi,
> 
> do you know if this condition is something that should be silently
> ignored like in your patch, or should we at least print an error that
> something unexpected is happening and being papered over?
> 
> Looking at how the seat is found:
> 
> static struct weston_seat *
> weston_wm_pick_seat_for_window(struct weston_wm_window *window)
> {
> 	struct weston_wm *wm = window->wm;
> 	struct weston_seat *seat, *s;
> 
> 	seat = NULL;
> 	wl_list_for_each(s, &wm->server->compositor->seat_list, link) {
> 		if (s->pointer != NULL &&
> 		    s->pointer->focus == window->view &&
> 		    s->pointer->button_count > 0 &&
> 		    (seat == NULL ||
> 		     s->pointer->grab_serial -
> 		     seat->pointer->grab_serial < (1 << 30)))
> 			seat = s;
> 	}
> 
> 	return seat;
> }
> 
> and that gets called as a response to an XCB input event via
> weston_wm_handle_button...
> 
> The function will return NULL if there are no buttons pressed, even if
> the pointer is focused on the window. Does that make sense in general?
> Does it not cause every last-button-up event to hit seat==NULL?
> So why don't we see this problem more often?
> 
> Could there be a problem in the shared frame code, maybe it makes
> assumptions that won't work with X11?

At least I think weston_wm_pick_seat_for_window makes sense where it is
used. If no button is down, things like resizing can be safely ignored.

And I don't think it has anything to do with X11 since input handling
here (and in Xwayland) is pure wayland.

I guess (it's only a guess) maybe it's caused by some race conditions in
frame and the event handling here?

Thanks,
Boyan Ding

> 
> If no-one knows (i.e. no-one replies assuring one way or the other), I
> can merge this patch if someone at least confirms it fixes an issue.
> 
> 
> Thanks,
> pq
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel