[PATCH] shell: resolve one random endless loop issue

[Ander Conselvan de Oliveira]

On 07/16/2012 10:14 AM, juan.j.zhao at linux.intel.com wrote:
> From: Juan Zhao <juan.j.zhao at linux.intel.com>
>
> When we're launching applications, we meet weston random unresponsive issue.
> Regarding the backtrace, it is caused by set_busy_cursor->shell_grab_start->
>   wl_pointer_set_focus->wl_signal_emit->handle_pointer_focus->set_busy_cursor.

With this information I was able to reproduce the bug here. My steps were:

   - run simple-egl and kill -STOP it, so it becomes unresponsive;
   - move the pointer over the simple-egl surface;
   - kill -STOP desktop-shell;
   - move the pointer out of the simple-egl surface and then back in.

The root cause of the problem is that the grab surface set by 
desktop-shell is a shell surface and the shell code inside weston is 
sending ping events to it. Since desktop shell didn't respond to the 
ping, weston tries to set the busy cursor for that surface and that 
leads to set focus to the grab surface, that is unresponsive.

I managed to fix the problem by making ping_handler() not ping the 
grab_surface. It works fine except it uncovered a bug in toytoolkit.

If a client looses focus while the process is stopped, it may end up 
sending a set cursor request while it does not have the pointer focus, 
in which case the request fails. But it will still request a frame 
callback on the cursor surface, and since the set cursor request failed, 
this callback will never be called because the cursor surface isn't 
mapped. The following calls to input_set_pointer_image() will not send a 
new set cursor request because of the pending frame callback, so it will 
stop setting the cursor all together.

I'll send the fixes for both issues shortly.

> The busy cursor will be set by ping_timeout_handler, no need to set it again
>   in handler_pointer_focus when the client is unresponsive. Or, the compositor
>   will be lead to endless loop isse.

The reason for setting the busy cursor on handle_pointer_focus() is that 
the cursor might move away from the unresponsive surface and later move 
back into it. When the cursor moves away, the busy grab will be 
terminated so it is necessary to initiate it again when the focus 
returns to an unresponsive surface.


Cheers,
Ander


> backtrace:
> ' #101672 wl_pointer_set_focus (pointer=0x8215a0c, surface=0x824c518,
>   sx=0, sy=0) at wayland-server.c:822
> ' #101673 0xb694e038 in shell_grab_start (grab=0x824b370,
>   interface=<value optimized out>, shsurf=0x824c738,
>   pointer=0x8215a0c,
>   cursor=DESKTOP_SHELL_CURSOR_BUSY) at shell.c:272
> ' #101674 0xb694e098 in set_busy_cursor (shsurf=0x824c738,
>   pointer=0x8215a0c) at shell.c:865
> ' #101675 0xb7fb3b31 in wl_signal_emit (pointer=0x8215a0c,
>   surface=0x824c518, sx=0, sy=0) at wayland-server.h:166
> ' #101676 wl_pointer_set_focus (pointer=0x8215a0c, surface=0x824c518,
>   sx=0, sy=0) at wayland-server.c:822
> ' #101677 0xb694e038 in shell_grab_start (grab=0x824b370,
>   interface=<value optimized out>, shsurf=0x824c738,
>   pointer=0x8215a0c,
>   cursor=DESKTOP_SHELL_CURSOR_BUSY) at shell.c:272
> ' #101678 0xb694e098 in set_busy_cursor (shsurf=0x824c738,
>   pointer=0x8215a0c) at shell.c:865
> ' #101679 0xb7fb3b31 in wl_signal_emit (pointer=0x8215a0c,
>   surface=0x824c518, sx=0, sy=0) at wayland-server.h:166
> ' #101680 wl_pointer_set_focus (pointer=0x8215a0c, surface=0x824c518,
>   sx=0, sy=0) at wayland-server.c:822
> ' #101681 0xb694e038 in shell_grab_start (grab=0x824b370,
>   interface=<value optimized out>, shsurf=0x824c738,
>   pointer=0x8215a0c,
>   cursor=DESKTOP_SHELL_CURSOR_BUSY) at shell.c:272
> ' #101682 0xb694e098 in set_busy_cursor (shsurf=0x824c738,
>   pointer=0x8215a0c) at shell.c:865
> ' #101683 0xb7fb3b31 in wl_signal_emit (pointer=0x8215a0c,
>   surface=0x824c518, sx=0, sy=0) at wayland-server.h:166
> ' #101684 wl_pointer_set_focus (pointer=0x8215a0c, surface=0x824c518,
>   sx=0, sy=0) at wayland-server.c:822
> ' #101685 0xb694e038 in shell_grab_start (grab=0x824b370,
>   interface=<value optimized out>, shsurf=0x824c738,
>   pointer=0x8215a0c,
>   cursor=DESKTOP_SHELL_CURSOR_BUSY) at shell.c:272
> ' #101686 0xb694e098 in set_busy_cursor (shsurf=0x824c738,
>   pointer=0x8215a0c) at shell.c:865
> ' #101687 0xb7fb3b31 in wl_signal_emit (pointer=0x8215a0c,
>   surface=0x824c518, sx=0, sy=0) at wayland-server.h:166
> ' #101688 wl_pointer_set_focus (pointer=0x8215a0c, surface=0x824c518,
>   sx=0, sy=0) at wayland-server.c:822
> ' #101689 0xb694e038 in shell_grab_start (grab=0x824b370,
>   interface=<value optimized out>, shsurf=0x824c738,
>   pointer=0x8215a0c,
>   cursor=DESKTOP_SHELL_CURSOR_BUSY) at shell.c:272
> ' #101690 0xb694e098 in set_busy_cursor (shsurf=0x824c738,
>   pointer=0x8215a0c) at shell.c:865
>
> Signed-off-by: Juan Zhao <juan.j.zhao at linux.intel.com>
> ---
>   src/shell.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/src/shell.c b/src/shell.c
> index d0492d9..83452fe 100644
> --- a/src/shell.c
> +++ b/src/shell.c
> @@ -954,9 +954,7 @@ handle_pointer_focus(struct wl_listener *listener, void *data)
>   	compositor = surface->compositor;
>   	shsurf = get_shell_surface(surface);
>
> -	if (shsurf && shsurf->unresponsive) {
> -		set_busy_cursor(shsurf, pointer);
> -	} else {
> +	if (!(shsurf && shsurf->unresponsive)) {
>   		serial = wl_display_next_serial(compositor->wl_display);
>   		ping_handler(surface, serial);
>   	}
>